[Attachment Support] Provide the `src` of an attachment to the UI delegate when an...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Jan 2018 18:46:04 +0000 (18:46 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Jan 2018 18:46:04 +0000 (18:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181638
<rdar://problem/36508702>

Reviewed by Dan Bernstein.

Source/WebCore:

Adjust the `didInsertAttachment` codepath to additionally propagate the attachment element's `src`.
Additionally, fix an issue with insertion and removal client notifications wherein the client can receive
insertion calls without corresponding removal calls, or vice versa. This is an existing issue, but matters more
now because we actually need to access the attachment element for its `src` when propagating changes to the
client. See below for details.

Test: WKAttachmentTests.AttachmentUpdatesWhenInsertingRichMarkup

* dom/Document.h:
(WebCore::Document::attachmentElementsByIdentifier const):
* editing/Editor.cpp:
(WebCore::Editor::notifyClientOfAttachmentUpdates):
* page/EditorClient.h:
(WebCore::EditorClient::didInsertAttachment):
* page/Frame.cpp:
(WebCore::Frame::setDocument):

When a Frame's document changes, inform the client that the attachments in the previous document are going away.
For each attachment currently connected to the document, we have either (1) already informed the client that it
was inserted, or (2) the attachment is pending an insertion call to the client. If (1) is the case, then we'll
tell the client that the attachment is removed, which will balance out the earlier insertion call. If (2) is the
case, then we'll remove the previously inserted attachment identifier from the set of attachment identifiers
pending insertion, and the client won't be informed of insertions or removals.

Source/WebKit:

Add a `source` parameter to the `didInsertAttachment` codepath for notifying WebKit2 clients when attachment
elements are inserted into the document.

* UIProcess/API/Cocoa/WKUIDelegatePrivate.h:
* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _didInsertAttachment:withSource:]):
(-[WKWebView _didInsertAttachment:]): Deleted.
* UIProcess/API/Cocoa/WKWebViewInternal.h:
* UIProcess/Cocoa/PageClientImplCocoa.h:
* UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::didInsertAttachment):
* UIProcess/PageClient.h:
(WebKit::PageClient::didInsertAttachment):
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didInsertAttachment):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* WebProcess/WebCoreSupport/WebEditorClient.cpp:
(WebKit::WebEditorClient::didInsertAttachment):
* WebProcess/WebCoreSupport/WebEditorClient.h:

Tools:

Tweak an existing API test to check that the `src` of an attachment element inserted via script matches the
`source` provided to the UI delegate via -[WKUIDelegate _webView:didInsertAttachment:withSource:].

* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(-[AttachmentUpdateObserver init]):
(-[AttachmentUpdateObserver sourceForIdentifier:]):
(-[AttachmentUpdateObserver _webView:didInsertAttachment:withSource:]):
(TestWebKitAPI::ObserveAttachmentUpdatesForScope::expectSourceForIdentifier):
(TestWebKitAPI::TEST):
(-[AttachmentUpdateObserver _webView:didInsertAttachment:]): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226977 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.h
Source/WebCore/editing/Editor.cpp
Source/WebCore/page/EditorClient.h
Source/WebCore/page/Frame.cpp
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.h
Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm
Source/WebKit/UIProcess/PageClient.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebPageProxy.messages.in
Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp
Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm

index 6a3001f..aa15e87 100644 (file)
@@ -1,3 +1,35 @@
+2018-01-16  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Provide the `src` of an attachment to the UI delegate when an attachment is inserted
+        https://bugs.webkit.org/show_bug.cgi?id=181638
+        <rdar://problem/36508702>
+
+        Reviewed by Dan Bernstein.
+
+        Adjust the `didInsertAttachment` codepath to additionally propagate the attachment element's `src`.
+        Additionally, fix an issue with insertion and removal client notifications wherein the client can receive
+        insertion calls without corresponding removal calls, or vice versa. This is an existing issue, but matters more
+        now because we actually need to access the attachment element for its `src` when propagating changes to the
+        client. See below for details.
+
+        Test: WKAttachmentTests.AttachmentUpdatesWhenInsertingRichMarkup
+
+        * dom/Document.h:
+        (WebCore::Document::attachmentElementsByIdentifier const):
+        * editing/Editor.cpp:
+        (WebCore::Editor::notifyClientOfAttachmentUpdates):
+        * page/EditorClient.h:
+        (WebCore::EditorClient::didInsertAttachment):
+        * page/Frame.cpp:
+        (WebCore::Frame::setDocument):
+
+        When a Frame's document changes, inform the client that the attachments in the previous document are going away.
+        For each attachment currently connected to the document, we have either (1) already informed the client that it
+        was inserted, or (2) the attachment is pending an insertion call to the client. If (1) is the case, then we'll
+        tell the client that the attachment is removed, which will balance out the earlier insertion call. If (2) is the
+        case, then we'll remove the previously inserted attachment identifier from the set of attachment identifiers
+        pending insertion, and the client won't be informed of insertions or removals.
+
 2018-01-16  Antoine Quint  <graouts@apple.com>
 
         Use traits for animation timing functions
index fb36972..955f41c 100644 (file)
@@ -1394,6 +1394,7 @@ public:
     void didInsertAttachmentElement(HTMLAttachmentElement&);
     void didRemoveAttachmentElement(HTMLAttachmentElement&);
     WEBCORE_EXPORT RefPtr<HTMLAttachmentElement> attachmentForIdentifier(const String& identifier) const;
+    const HashMap<String, Ref<HTMLAttachmentElement>>& attachmentElementsByIdentifier() const { return m_attachmentIdentifierToElementMap; }
 #endif
 
 #if ENABLE(SERVICE_WORKER)
index 96ec1e9..31eeae3 100644 (file)
@@ -3770,15 +3770,24 @@ void Editor::didRemoveAttachmentElement(HTMLAttachmentElement& attachment)
 
 void Editor::notifyClientOfAttachmentUpdates()
 {
-    if (auto* editorClient = client()) {
-        for (auto& identifier : m_removedAttachmentIdentifiers)
-            editorClient->didRemoveAttachment(identifier);
-        for (auto& identifier : m_insertedAttachmentIdentifiers)
-            editorClient->didInsertAttachment(identifier);
-    }
+    auto removedAttachmentIdentifiers = WTFMove(m_removedAttachmentIdentifiers);
+    auto insertedAttachmentIdentifiers = WTFMove(m_insertedAttachmentIdentifiers);
+    if (!client())
+        return;
 
-    m_removedAttachmentIdentifiers.clear();
-    m_insertedAttachmentIdentifiers.clear();
+    for (auto& identifier : removedAttachmentIdentifiers)
+        client()->didRemoveAttachment(identifier);
+
+    auto* document = m_frame.document();
+    if (!document)
+        return;
+
+    for (auto& identifier : insertedAttachmentIdentifiers) {
+        if (auto attachment = document->attachmentForIdentifier(identifier))
+            client()->didInsertAttachment(identifier, attachment->attributeWithoutSynchronization(HTMLNames::srcAttr));
+        else
+            ASSERT_NOT_REACHED();
+    }
 }
 
 void Editor::insertAttachment(const String& identifier, const AttachmentDisplayOptions& options, const String& filename, const String& filepath, std::optional<String> contentType)
index e02cc09..1f51a92 100644 (file)
@@ -73,7 +73,7 @@ public:
     virtual bool shouldMoveRangeAfterDelete(Range*, Range*) = 0;
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    virtual void didInsertAttachment(const String&) { }
+    virtual void didInsertAttachment(const String& /* identifier */, const String& /* source */) { }
     virtual void didRemoveAttachment(const String&) { }
 #endif
 
index e2e1310..c8afcb6 100644 (file)
@@ -284,6 +284,13 @@ void Frame::setDocument(RefPtr<Document>&& newDocument)
         m_loader->client().dispatchDidChangeMainDocument();
     }
 
+#if ENABLE(ATTACHMENT_ELEMENT)
+    if (m_doc) {
+        for (auto& attachment : m_doc->attachmentElementsByIdentifier().values())
+            editor().didRemoveAttachmentElement(attachment);
+    }
+#endif
+
     if (m_doc && m_doc->pageCacheState() != Document::InPageCache)
         m_doc->prepareForDestruction();
 
@@ -296,6 +303,13 @@ void Frame::setDocument(RefPtr<Document>&& newDocument)
     if (newDocument)
         newDocument->didBecomeCurrentDocumentInFrame();
 
+#if ENABLE(ATTACHMENT_ELEMENT)
+    if (m_doc) {
+        for (auto& attachment : m_doc->attachmentElementsByIdentifier().values())
+            editor().didInsertAttachmentElement(attachment);
+    }
+#endif
+
     InspectorInstrumentation::frameDocumentUpdated(*this);
 
     m_documentIsBeingReplaced = false;
index ebb0acb..4481a01 100644 (file)
@@ -1,3 +1,32 @@
+2018-01-16  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Provide the `src` of an attachment to the UI delegate when an attachment is inserted
+        https://bugs.webkit.org/show_bug.cgi?id=181638
+        <rdar://problem/36508702>
+
+        Reviewed by Dan Bernstein.
+
+        Add a `source` parameter to the `didInsertAttachment` codepath for notifying WebKit2 clients when attachment
+        elements are inserted into the document.
+
+        * UIProcess/API/Cocoa/WKUIDelegatePrivate.h:
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _didInsertAttachment:withSource:]):
+        (-[WKWebView _didInsertAttachment:]): Deleted.
+        * UIProcess/API/Cocoa/WKWebViewInternal.h:
+        * UIProcess/Cocoa/PageClientImplCocoa.h:
+        * UIProcess/Cocoa/PageClientImplCocoa.mm:
+        (WebKit::PageClientImplCocoa::didInsertAttachment):
+        * UIProcess/PageClient.h:
+        (WebKit::PageClient::didInsertAttachment):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didInsertAttachment):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+        * WebProcess/WebCoreSupport/WebEditorClient.cpp:
+        (WebKit::WebEditorClient::didInsertAttachment):
+        * WebProcess/WebCoreSupport/WebEditorClient.h:
+
 2018-01-15  Youenn Fablet  <youenn@apple.com>
 
         ASSERTION FAILED: m_ptr under WebKit::CacheStorage::Caches::writeRecord
index 3b2e8cb..7752fcd 100644 (file)
@@ -112,6 +112,7 @@ struct UIEdgeInsets;
 
 - (void)_webView:(WKWebView *)webView didRemoveAttachment:(_WKAttachment *)attachment WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 - (void)_webView:(WKWebView *)webView didInsertAttachment:(_WKAttachment *)attachment WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
+- (void)_webView:(WKWebView *)webView didInsertAttachment:(_WKAttachment *)attachment withSource:(NSString *)source WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 
 #if TARGET_OS_IPHONE
 - (BOOL)_webView:(WKWebView *)webView shouldIncludeAppLinkActionsForElement:(_WKActivatedElementInfo *)element WK_API_AVAILABLE(ios(9.0));
index 47ae4cb..0f0dcba 100644 (file)
@@ -1216,10 +1216,12 @@ static NSDictionary *dictionaryRepresentationForEditorState(const WebKit::Editor
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 
-- (void)_didInsertAttachment:(NSString *)identifier
+- (void)_didInsertAttachment:(NSString *)identifier withSource:(NSString *)source
 {
     id <WKUIDelegatePrivate> uiDelegate = (id <WKUIDelegatePrivate>)self.UIDelegate;
-    if ([uiDelegate respondsToSelector:@selector(_webView:didInsertAttachment:)])
+    if ([uiDelegate respondsToSelector:@selector(_webView:didInsertAttachment:withSource:)])
+        [uiDelegate _webView:self didInsertAttachment:[wrapper(API::Attachment::create(identifier, *_page).leakRef()) autorelease] withSource:source];
+    else if ([uiDelegate respondsToSelector:@selector(_webView:didInsertAttachment:)])
         [uiDelegate _webView:self didInsertAttachment:[wrapper(API::Attachment::create(identifier, *_page).leakRef()) autorelease]];
 }
 
index d19f776..db7513d 100644 (file)
@@ -149,7 +149,7 @@ struct PrintInfo;
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 - (void)_didRemoveAttachment:(NSString *)identifier;
-- (void)_didInsertAttachment:(NSString *)identifier;
+- (void)_didInsertAttachment:(NSString *)identifier withSource:(NSString *)source;
 #endif
 
 - (WKPageRef)_pageForTesting;
index ffc2343..b821c54 100644 (file)
@@ -39,7 +39,7 @@ public:
     void isPlayingAudioDidChange() final;
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    void didInsertAttachment(const String& identifier) final;
+    void didInsertAttachment(const String& identifier, const String& source) final;
     void didRemoveAttachment(const String& identifier) final;
 #endif
 
index 2d862c4..837ec1a 100644 (file)
@@ -46,10 +46,10 @@ void PageClientImplCocoa::isPlayingAudioDidChange()
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 
-void PageClientImplCocoa::didInsertAttachment(const String& identifier)
+void PageClientImplCocoa::didInsertAttachment(const String& identifier, const String& source)
 {
 #if WK_API_ENABLED
-    [m_webView _didInsertAttachment:identifier];
+    [m_webView _didInsertAttachment:identifier withSource:source];
 #else
     UNUSED_PARAM(identifier);
 #endif
index c51f105..550821f 100644 (file)
@@ -380,7 +380,7 @@ public:
 #endif
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    virtual void didInsertAttachment(const String& identifier) { }
+    virtual void didInsertAttachment(const String& identifier, const String& source) { }
     virtual void didRemoveAttachment(const String& identifier) { }
 #endif
 
index 070c8f4..7b76983 100644 (file)
@@ -7264,9 +7264,9 @@ void WebPageProxy::setAttachmentDataAndContentType(const String& identifier, Sha
     m_process->send(Messages::WebPage::SetAttachmentDataAndContentType(identifier, IPC::SharedBufferDataReference { &data }, WTFMove(newContentType), WTFMove(newFilename), callbackID), m_pageID);
 }
 
-void WebPageProxy::didInsertAttachment(const String& identifier)
+void WebPageProxy::didInsertAttachment(const String& identifier, const String& source)
 {
-    m_pageClient.didInsertAttachment(identifier);
+    m_pageClient.didInsertAttachment(identifier, source);
 }
 
 void WebPageProxy::didRemoveAttachment(const String& identifier)
index a9cbeda..ce8a33c 100644 (file)
@@ -1693,7 +1693,7 @@ private:
     void stopAllURLSchemeTasks();
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    void didInsertAttachment(const String& identifier);
+    void didInsertAttachment(const String& identifier, const String& source);
     void didRemoveAttachment(const String& identifier);
 #endif
 
index b0949bd..8ff0a80 100644 (file)
@@ -510,7 +510,7 @@ messages -> WebPageProxy {
 #endif
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    DidInsertAttachment(String identifier)
+    DidInsertAttachment(String identifier, String source)
     DidRemoveAttachment(String identifier)
 #endif
 }
index 5393df2..6c69d09 100644 (file)
@@ -159,9 +159,9 @@ bool WebEditorClient::shouldApplyStyle(StyleProperties* style, Range* range)
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 
-void WebEditorClient::didInsertAttachment(const String& identifier)
+void WebEditorClient::didInsertAttachment(const String& identifier, const String& source)
 {
-    m_page->send(Messages::WebPageProxy::DidInsertAttachment(identifier));
+    m_page->send(Messages::WebPageProxy::DidInsertAttachment(identifier, source));
 }
 
 void WebEditorClient::didRemoveAttachment(const String& identifier)
index a8a61b0..fde710a 100644 (file)
@@ -60,7 +60,7 @@ private:
     bool shouldMoveRangeAfterDelete(WebCore::Range*, WebCore::Range*) final;
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    void didInsertAttachment(const String& identifier) final;
+    void didInsertAttachment(const String& identifier, const String& source) final;
     void didRemoveAttachment(const String& identifier) final;
 #endif
 
index a6abab5..e0fbfc2 100644 (file)
@@ -1,3 +1,22 @@
+2018-01-16  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Provide the `src` of an attachment to the UI delegate when an attachment is inserted
+        https://bugs.webkit.org/show_bug.cgi?id=181638
+        <rdar://problem/36508702>
+
+        Reviewed by Dan Bernstein.
+
+        Tweak an existing API test to check that the `src` of an attachment element inserted via script matches the
+        `source` provided to the UI delegate via -[WKUIDelegate _webView:didInsertAttachment:withSource:].
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+        (-[AttachmentUpdateObserver init]):
+        (-[AttachmentUpdateObserver sourceForIdentifier:]):
+        (-[AttachmentUpdateObserver _webView:didInsertAttachment:withSource:]):
+        (TestWebKitAPI::ObserveAttachmentUpdatesForScope::expectSourceForIdentifier):
+        (TestWebKitAPI::TEST):
+        (-[AttachmentUpdateObserver _webView:didInsertAttachment:]): Deleted.
+
 2018-01-16  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed. Fix GTK unit tests execution in the bots after r226967.
index d175639..b87a60b 100644 (file)
@@ -50,6 +50,7 @@
 @implementation AttachmentUpdateObserver {
     RetainPtr<NSMutableArray<_WKAttachment *>> _inserted;
     RetainPtr<NSMutableArray<_WKAttachment *>> _removed;
+    RetainPtr<NSMutableDictionary<NSString *, NSString *>> _identifierToSourceMap;
 }
 
 - (instancetype)init
@@ -57,6 +58,7 @@
     if (self = [super init]) {
         _inserted = adoptNS([[NSMutableArray alloc] init]);
         _removed = adoptNS([[NSMutableArray alloc] init]);
+        _identifierToSourceMap = adoptNS([[NSMutableDictionary alloc] init]);
     }
     return self;
 }
     return _removed.get();
 }
 
-- (void)_webView:(WKWebView *)webView didInsertAttachment:(_WKAttachment *)attachment
+- (NSString *)sourceForIdentifier:(NSString *)identifier
+{
+    return [_identifierToSourceMap objectForKey:identifier];
+}
+
+- (void)_webView:(WKWebView *)webView didInsertAttachment:(_WKAttachment *)attachment withSource:(NSString *)source
 {
     [_inserted addObject:attachment];
+    if (source)
+        [_identifierToSourceMap setObject:source forKey:attachment.uniqueIdentifier];
 }
 
 - (void)_webView:(WKWebView *)webView didRemoveAttachment:(_WKAttachment *)attachment
@@ -115,6 +124,15 @@ public:
         EXPECT_TRUE(insertedAttachmentsMatch);
     }
 
+    void expectSourceForIdentifier(NSString *expectedSource, NSString *identifier)
+    {
+        NSString *observedSource = [observer() sourceForIdentifier:identifier];
+        BOOL success = observedSource == expectedSource || [observedSource isEqualToString:expectedSource];
+        EXPECT_TRUE(success);
+        if (!success)
+            NSLog(@"Expected source: %@ but observed: %@", expectedSource, observedSource);
+    }
+
 private:
     RetainPtr<AttachmentUpdateObserver> m_observer;
     RetainPtr<TestWKWebView> m_webView;
@@ -606,9 +624,10 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingRichMarkup)
     RetainPtr<_WKAttachment> attachment;
     {
         ObserveAttachmentUpdatesForScope observer(webView.get());
-        [webView _synchronouslyExecuteEditCommand:@"InsertHTML" argument:@"<div><strong><attachment title='a'></attachment></strong></div>"];
+        [webView _synchronouslyExecuteEditCommand:@"InsertHTML" argument:@"<div><strong><attachment src='cid:123-4567' title='a'></attachment></strong></div>"];
         attachment = observer.observer().inserted[0];
         observer.expectAttachmentUpdates(@[ ], @[attachment.get()]);
+        observer.expectSourceForIdentifier(@"cid:123-4567", [attachment uniqueIdentifier]);
     }
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);