[Attachment Support] Remove _WKAttachments and notify the UI client upon mainframe...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2018 03:21:56 +0000 (03:21 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 22 Aug 2018 03:21:56 +0000 (03:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188715
<rdar://problem/43541790>

Reviewed by Tim Horton.

Source/WebCore:

Rename didInsertAttachment to didInsertAttachmentWithIdentifier. See WebKit ChangeLog for more detail.

Tests:  WKAttachmentTests.InvalidateAttachmentsAfterMainFrameNavigation
        WKAttachmentTests.InvalidateAttachmentsAfterWebProcessTermination

* editing/Editor.cpp:
(WebCore::Editor::notifyClientOfAttachmentUpdates):
* page/EditorClient.h:
(WebCore::EditorClient::didInsertAttachmentWithIdentifier):
(WebCore::EditorClient::didRemoveAttachmentWithIdentifier):
(WebCore::EditorClient::didInsertAttachment): Deleted.
(WebCore::EditorClient::didRemoveAttachment): Deleted.

Source/WebKit:

Adds logic for invalidating Attachment objects upon mainframe navigation, or upon web content process
termination. An invalid Attachment's wrapper may still be retained by a UI client; however, calls to -info and
other SPI methods will either return nil or immediately invoke their completion handlers with a nil result and
an NSError. See changes below for more detail.

This patch also takes some extra measures to avoid sending redundant or unexpected removal updates to the UI
client upon invalidating all Attachments.

See Tools changes for new API tests.

* UIProcess/API/APIAttachment.cpp:
(API::Attachment::invalidate):
* UIProcess/API/APIAttachment.h:
* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _didInsertAttachment:withSource:]):
(-[WKWebView _didRemoveAttachment:]):

Refactor these methods to take references to the API::Attachment that is being inserted or removed, rather than
take attachment identifiers. This allows us to keep logic for setting the InsertionState of Attachment objects
in WebPageProxy, rather than in platform code.

* UIProcess/API/Cocoa/WKWebViewInternal.h:
* UIProcess/API/Cocoa/_WKAttachment.h:
* UIProcess/API/Cocoa/_WKAttachment.mm:
(-[_WKAttachment info]):

If the attachment object is invalid, return nil.

(-[_WKAttachment setDisplayOptions:completion:]):
(-[_WKAttachment setFileWrapper:contentType:completion:]):

If the attachment object is invalid, immediately invoke the completion block with an error.

(-[_WKAttachment isConnected]):

Add a new readonly property for a client to easily determine whether a _WKAttachment is connected to the
document, without having to maintain the current set of connected attachment objects by observing insertion and
removal callbacks.

* UIProcess/Cocoa/PageClientImplCocoa.h:
* UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::didInsertAttachment):
(WebKit::PageClientImplCocoa::didRemoveAttachment):

Make these take API::Attachment&s rather than identifier strings.

* UIProcess/PageClient.h:
(WebKit::PageClient::didInsertAttachment):
(WebKit::PageClient::didRemoveAttachment):

Also make these take API::Attachment&s rather than identifier strings.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didCommitLoadForFrame):
(WebKit::WebPageProxy::resetStateAfterProcessExited):
(WebKit::WebPageProxy::invalidateAllAttachments):

Introduce a helper function that invalidates all Attachments and notifies the UI client if needed. This is
invoked upon mainframe navigation, and when the web content process terminates.

(WebKit::WebPageProxy::platformRegisterAttachment):
(WebKit::WebPageProxy::didInsertAttachmentWithIdentifier):
(WebKit::WebPageProxy::didRemoveAttachmentWithIdentifier):
(WebKit::WebPageProxy::didInsertAttachment):
(WebKit::WebPageProxy::didRemoveAttachment):

Keep track of whether we've notified the UI client that an Attachment has been inserted into the document. This
allows us to send removal updates to the UI client only for Attachments that are currently in the document, from
the perspective of the UI client, and allows us to avoid sending extra removal updates in
invalidateAllAttachments for Attachments that either have already been removed, or Attachments which we haven't
inserted yet.

* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* WebProcess/WebCoreSupport/WebEditorClient.cpp:
(WebKit::WebEditorClient::didInsertAttachmentWithIdentifier):
(WebKit::WebEditorClient::didRemoveAttachmentWithIdentifier):
(WebKit::WebEditorClient::didInsertAttachment): Deleted.
(WebKit::WebEditorClient::didRemoveAttachment): Deleted.
* WebProcess/WebCoreSupport/WebEditorClient.h:

Rename did{Insert|Remove}Attachment to did{Insert|Remove}AttachmentWithIdentifier.

Tools:

Adds API tests to exercises cases where (1) the UI client is notified of attachment removal upon mainframe
navigation, and (2) the UI client is notified of attachment removal upon web content process termination.

* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(TestWebKitAPI::ObserveAttachmentUpdatesForScope::expectAttachmentUpdates):
(TestWebKitAPI::TEST):

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

20 files changed:
Source/WebCore/ChangeLog
Source/WebCore/editing/Editor.cpp
Source/WebCore/page/EditorClient.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/APIAttachment.cpp
Source/WebKit/UIProcess/API/APIAttachment.h
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h
Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm
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 519f1ce..09dc96d 100644 (file)
@@ -1,3 +1,24 @@
+2018-08-21  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Remove _WKAttachments and notify the UI client upon mainframe navigation
+        https://bugs.webkit.org/show_bug.cgi?id=188715
+        <rdar://problem/43541790>
+
+        Reviewed by Tim Horton.
+
+        Rename didInsertAttachment to didInsertAttachmentWithIdentifier. See WebKit ChangeLog for more detail.
+
+        Tests:  WKAttachmentTests.InvalidateAttachmentsAfterMainFrameNavigation
+                WKAttachmentTests.InvalidateAttachmentsAfterWebProcessTermination
+
+        * editing/Editor.cpp:
+        (WebCore::Editor::notifyClientOfAttachmentUpdates):
+        * page/EditorClient.h:
+        (WebCore::EditorClient::didInsertAttachmentWithIdentifier):
+        (WebCore::EditorClient::didRemoveAttachmentWithIdentifier):
+        (WebCore::EditorClient::didInsertAttachment): Deleted.
+        (WebCore::EditorClient::didRemoveAttachment): Deleted.
+
 2018-08-21  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r235128.
index 514e25e..0700948 100644 (file)
@@ -3866,7 +3866,7 @@ void Editor::notifyClientOfAttachmentUpdates()
         return;
 
     for (auto& identifier : removedAttachmentIdentifiers)
-        client()->didRemoveAttachment(identifier);
+        client()->didRemoveAttachmentWithIdentifier(identifier);
 
     auto* document = m_frame.document();
     if (!document)
@@ -3874,7 +3874,7 @@ void Editor::notifyClientOfAttachmentUpdates()
 
     for (auto& identifier : insertedAttachmentIdentifiers) {
         if (auto attachment = document->attachmentForIdentifier(identifier))
-            client()->didInsertAttachment(identifier, attachment->attributeWithoutSynchronization(HTMLNames::srcAttr));
+            client()->didInsertAttachmentWithIdentifier(identifier, attachment->attributeWithoutSynchronization(HTMLNames::srcAttr));
         else
             ASSERT_NOT_REACHED();
     }
index 8ffd736..e9b1585 100644 (file)
@@ -76,8 +76,8 @@ public:
     virtual void registerAttachmentIdentifier(const String& /* identifier */, const String& /* contentType */, const String& /* preferredFileName */, Ref<SharedBuffer>&&) { }
     virtual void registerAttachmentIdentifier(const String& /* identifier */, const String& /* contentType */, const String& /* filePath */) { }
     virtual void cloneAttachmentData(const String& /* fromIdentifier */, const String& /* toIdentifier */) { }
-    virtual void didInsertAttachment(const String& /* identifier */, const String& /* source */) { }
-    virtual void didRemoveAttachment(const String&) { }
+    virtual void didInsertAttachmentWithIdentifier(const String& /* identifier */, const String& /* source */) { }
+    virtual void didRemoveAttachmentWithIdentifier(const String&) { }
     virtual bool supportsClientSideAttachmentData() const { return false; }
 #endif
 
index 8b06730..25cd024 100644 (file)
@@ -1,3 +1,94 @@
+2018-08-21  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Remove _WKAttachments and notify the UI client upon mainframe navigation
+        https://bugs.webkit.org/show_bug.cgi?id=188715
+        <rdar://problem/43541790>
+
+        Reviewed by Tim Horton.
+
+        Adds logic for invalidating Attachment objects upon mainframe navigation, or upon web content process
+        termination. An invalid Attachment's wrapper may still be retained by a UI client; however, calls to -info and
+        other SPI methods will either return nil or immediately invoke their completion handlers with a nil result and
+        an NSError. See changes below for more detail.
+
+        This patch also takes some extra measures to avoid sending redundant or unexpected removal updates to the UI
+        client upon invalidating all Attachments.
+
+        See Tools changes for new API tests.
+
+        * UIProcess/API/APIAttachment.cpp:
+        (API::Attachment::invalidate):
+        * UIProcess/API/APIAttachment.h:
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _didInsertAttachment:withSource:]):
+        (-[WKWebView _didRemoveAttachment:]):
+
+        Refactor these methods to take references to the API::Attachment that is being inserted or removed, rather than
+        take attachment identifiers. This allows us to keep logic for setting the InsertionState of Attachment objects
+        in WebPageProxy, rather than in platform code.
+
+        * UIProcess/API/Cocoa/WKWebViewInternal.h:
+        * UIProcess/API/Cocoa/_WKAttachment.h:
+        * UIProcess/API/Cocoa/_WKAttachment.mm:
+        (-[_WKAttachment info]):
+
+        If the attachment object is invalid, return nil.
+
+        (-[_WKAttachment setDisplayOptions:completion:]):
+        (-[_WKAttachment setFileWrapper:contentType:completion:]):
+
+        If the attachment object is invalid, immediately invoke the completion block with an error.
+
+        (-[_WKAttachment isConnected]):
+
+        Add a new readonly property for a client to easily determine whether a _WKAttachment is connected to the
+        document, without having to maintain the current set of connected attachment objects by observing insertion and
+        removal callbacks.
+
+        * UIProcess/Cocoa/PageClientImplCocoa.h:
+        * UIProcess/Cocoa/PageClientImplCocoa.mm:
+        (WebKit::PageClientImplCocoa::didInsertAttachment):
+        (WebKit::PageClientImplCocoa::didRemoveAttachment):
+
+        Make these take API::Attachment&s rather than identifier strings.
+
+        * UIProcess/PageClient.h:
+        (WebKit::PageClient::didInsertAttachment):
+        (WebKit::PageClient::didRemoveAttachment):
+
+        Also make these take API::Attachment&s rather than identifier strings.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didCommitLoadForFrame):
+        (WebKit::WebPageProxy::resetStateAfterProcessExited):
+        (WebKit::WebPageProxy::invalidateAllAttachments):
+
+        Introduce a helper function that invalidates all Attachments and notifies the UI client if needed. This is
+        invoked upon mainframe navigation, and when the web content process terminates.
+
+        (WebKit::WebPageProxy::platformRegisterAttachment):
+        (WebKit::WebPageProxy::didInsertAttachmentWithIdentifier):
+        (WebKit::WebPageProxy::didRemoveAttachmentWithIdentifier):
+        (WebKit::WebPageProxy::didInsertAttachment):
+        (WebKit::WebPageProxy::didRemoveAttachment):
+
+        Keep track of whether we've notified the UI client that an Attachment has been inserted into the document. This
+        allows us to send removal updates to the UI client only for Attachments that are currently in the document, from
+        the perspective of the UI client, and allows us to avoid sending extra removal updates in
+        invalidateAllAttachments for Attachments that either have already been removed, or Attachments which we haven't
+        inserted yet.
+
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+        * WebProcess/WebCoreSupport/WebEditorClient.cpp:
+        (WebKit::WebEditorClient::didInsertAttachmentWithIdentifier):
+        (WebKit::WebEditorClient::didRemoveAttachmentWithIdentifier):
+        (WebKit::WebEditorClient::didInsertAttachment): Deleted.
+        (WebKit::WebEditorClient::didRemoveAttachment): Deleted.
+        * WebProcess/WebCoreSupport/WebEditorClient.h:
+
+        Rename did{Insert|Remove}Attachment to did{Insert|Remove}AttachmentWithIdentifier.
+
 2018-08-21  Megan Gardner  <megan_gardner@apple.com>
 
         Change Selection modification to not snap the grabber when selecting above or below the selection anchor
index d15620f..6c2af52 100644 (file)
@@ -70,6 +70,17 @@ void Attachment::updateAttributes(uint64_t fileSize, const WTF::String& newConte
         callback(WebKit::CallbackBase::Error::OwnerWasInvalidated);
 }
 
+void Attachment::invalidate()
+{
+    m_identifier = { };
+    m_filePath = { };
+    m_contentType = { };
+    m_webPage.clear();
+#if PLATFORM(COCOA)
+    m_fileWrapper.clear();
+#endif
+}
+
 }
 
 #endif // ENABLE(ATTACHMENT_ELEMENT)
index dec4823..bf35f45 100644 (file)
@@ -52,10 +52,15 @@ public:
     static Ref<Attachment> create(const WTF::String& identifier, WebKit::WebPageProxy&);
     virtual ~Attachment();
 
+    enum class InsertionState : uint8_t { NotInserted, Inserted };
+
     const WTF::String& identifier() const { return m_identifier; }
     void setDisplayOptions(WebCore::AttachmentDisplayOptions, Function<void(WebKit::CallbackBase::Error)>&&);
     void updateAttributes(uint64_t fileSize, const WTF::String& newContentType, const WTF::String& newFilename, Function<void(WebKit::CallbackBase::Error)>&&);
 
+    void invalidate();
+    bool isValid() const { return !!m_webPage; }
+
 #if PLATFORM(COCOA)
     NSFileWrapper *fileWrapper() const { return m_fileWrapper.get(); }
     void setFileWrapper(NSFileWrapper *fileWrapper) { m_fileWrapper = fileWrapper; }
@@ -67,6 +72,9 @@ public:
     const WTF::String& contentType() const { return m_contentType; }
     void setContentType(const WTF::String& contentType) { m_contentType = contentType; }
 
+    InsertionState insertionState() const { return m_insertionState; }
+    void setInsertionState(InsertionState state) { m_insertionState = state; }
+
 private:
     explicit Attachment(const WTF::String& identifier, WebKit::WebPageProxy&);
 
@@ -77,6 +85,7 @@ private:
     WTF::String m_filePath;
     WTF::String m_contentType;
     WeakPtr<WebKit::WebPageProxy> m_webPage;
+    InsertionState m_insertionState { InsertionState::NotInserted };
 };
 
 } // namespace API
index f0b7a6e..3379b48 100644 (file)
@@ -1210,28 +1210,18 @@ static NSDictionary *dictionaryRepresentationForEditorState(const WebKit::Editor
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 
-- (void)_didInsertAttachment:(NSString *)identifier withSource:(NSString *)source
+- (void)_didInsertAttachment:(API::Attachment&)attachment withSource:(NSString *)source
 {
     id <WKUIDelegatePrivate> uiDelegate = (id <WKUIDelegatePrivate>)self.UIDelegate;
-    if (![uiDelegate respondsToSelector:@selector(_webView:didInsertAttachment:withSource:)])
-        return;
-
-    if (auto attachment = _page->attachmentForIdentifier(identifier))
-        [uiDelegate _webView:self didInsertAttachment:wrapper(*attachment) withSource:source];
-    else
-        ASSERT_NOT_REACHED();
+    if ([uiDelegate respondsToSelector:@selector(_webView:didInsertAttachment:withSource:)])
+        [uiDelegate _webView:self didInsertAttachment:wrapper(attachment) withSource:source];
 }
 
-- (void)_didRemoveAttachment:(NSString *)identifier
+- (void)_didRemoveAttachment:(API::Attachment&)attachment
 {
     id <WKUIDelegatePrivate> uiDelegate = (id <WKUIDelegatePrivate>)self.UIDelegate;
-    if (![uiDelegate respondsToSelector:@selector(_webView:didRemoveAttachment:)])
-        return;
-
-    if (auto attachment = _page->attachmentForIdentifier(identifier))
-        [uiDelegate _webView:self didRemoveAttachment:wrapper(*attachment)];
-    else
-        ASSERT_NOT_REACHED();
+    if ([uiDelegate respondsToSelector:@selector(_webView:didRemoveAttachment:)])
+        [uiDelegate _webView:self didRemoveAttachment:wrapper(attachment)];
 }
 
 #endif // ENABLE(ATTACHMENT_ELEMENT)
index 3a0541c..4f3abbc 100644 (file)
 
 typedef const struct OpaqueWKPage* WKPageRef;
 
+namespace API {
+class Attachment;
+}
+
 namespace WebKit {
 class ViewSnapshot;
 class WebPageProxy;
@@ -157,8 +161,8 @@ struct PrintInfo;
 #endif
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-- (void)_didRemoveAttachment:(NSString *)identifier;
-- (void)_didInsertAttachment:(NSString *)identifier withSource:(NSString *)source;
+- (void)_didRemoveAttachment:(API::Attachment&)attachment;
+- (void)_didInsertAttachment:(API::Attachment&)attachment withSource:(NSString *)source;
 #endif
 
 - (WKPageRef)_pageForTesting;
index f1441b4..a1f302c 100644 (file)
@@ -57,8 +57,9 @@ WK_CLASS_AVAILABLE(macosx(10.13.4), ios(11.3))
 - (void)setDisplayOptions:(_WKAttachmentDisplayOptions *)options completion:(void(^ _Nullable)(NSError * _Nullable))completionHandler;
 - (void)setFileWrapper:(NSFileWrapper *)fileWrapper contentType:(nullable NSString *)contentType completion:(void(^ _Nullable)(NSError * _Nullable))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 
-@property (nonatomic, readonly) _WKAttachmentInfo *info WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
+@property (nonatomic, readonly, nullable) _WKAttachmentInfo *info WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 @property (nonatomic, readonly) NSString *uniqueIdentifier;
+@property (nonatomic, readonly, getter=isConnected) BOOL connected WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 
 // Deprecated SPI.
 - (void)requestInfo:(void(^)(_WKAttachmentInfo * _Nullable, NSError * _Nullable))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("-info", macosx(WK_MAC_TBA, WK_MAC_TBA), ios(WK_IOS_TBA, WK_IOS_TBA));
index 696adbe..f495c71 100644 (file)
@@ -42,6 +42,9 @@
 
 using namespace WebKit;
 
+static const NSInteger UnspecifiedAttachmentErrorCode = 1;
+static const NSInteger InvalidAttachmentErrorCode = 2;
+
 @implementation _WKAttachmentDisplayOptions : NSObject
 
 - (WebCore::AttachmentDisplayOptions)coreDisplayOptions
@@ -146,6 +149,9 @@ static BOOL isDeclaredOrDynamicTypeIdentifier(NSString *type)
 
 - (_WKAttachmentInfo *)info
 {
+    if (!_attachment->isValid())
+        return nil;
+
     return [[[_WKAttachmentInfo alloc] initWithFileWrapper:_attachment->fileWrapper() filePath:_attachment->filePath() contentType:_attachment->contentType()] autorelease];
 }
 
@@ -156,6 +162,11 @@ static BOOL isDeclaredOrDynamicTypeIdentifier(NSString *type)
 
 - (void)setDisplayOptions:(_WKAttachmentDisplayOptions *)options completion:(void(^)(NSError *))completionHandler
 {
+    if (!_attachment->isValid()) {
+        completionHandler([NSError errorWithDomain:WKErrorDomain code:InvalidAttachmentErrorCode userInfo:nil]);
+        return;
+    }
+
     auto coreOptions = options ? options.coreDisplayOptions : WebCore::AttachmentDisplayOptions { };
     _attachment->setDisplayOptions(coreOptions, [capturedBlock = makeBlockPtr(completionHandler)] (CallbackBase::Error error) {
         if (!capturedBlock)
@@ -164,12 +175,17 @@ static BOOL isDeclaredOrDynamicTypeIdentifier(NSString *type)
         if (error == CallbackBase::Error::None)
             capturedBlock(nil);
         else
-            capturedBlock([NSError errorWithDomain:WKErrorDomain code:1 userInfo:nil]);
+            capturedBlock([NSError errorWithDomain:WKErrorDomain code:UnspecifiedAttachmentErrorCode userInfo:nil]);
     });
 }
 
 - (void)setFileWrapper:(NSFileWrapper *)fileWrapper contentType:(NSString *)contentType completion:(void (^)(NSError *))completionHandler
 {
+    if (!_attachment->isValid()) {
+        completionHandler([NSError errorWithDomain:WKErrorDomain code:InvalidAttachmentErrorCode userInfo:nil]);
+        return;
+    }
+
     auto fileSize = [fileWrapper.fileAttributes[NSFileSize] unsignedLongLongValue];
     if (!fileSize && fileWrapper.regularFile)
         fileSize = fileWrapper.regularFileContents.length;
@@ -187,7 +203,7 @@ static BOOL isDeclaredOrDynamicTypeIdentifier(NSString *type)
         if (error == CallbackBase::Error::None)
             capturedBlock(nil);
         else
-            capturedBlock([NSError errorWithDomain:WKErrorDomain code:1 userInfo:nil]);
+            capturedBlock([NSError errorWithDomain:WKErrorDomain code:UnspecifiedAttachmentErrorCode userInfo:nil]);
     });
 }
 
@@ -209,6 +225,11 @@ static BOOL isDeclaredOrDynamicTypeIdentifier(NSString *type)
     return [NSString stringWithFormat:@"<%@ %p id='%@'>", [self class], self, self.uniqueIdentifier];
 }
 
+- (BOOL)isConnected
+{
+    return _attachment->insertionState() == API::Attachment::InsertionState::Inserted;
+}
+
 @end
 
 #endif // WK_API_ENABLED
index b821c54..808d9e9 100644 (file)
 
 @class WKWebView;
 
+namespace API {
+class Attachment;
+}
+
 namespace WebKit {
 
 class PageClientImplCocoa : public PageClient {
@@ -39,8 +43,8 @@ public:
     void isPlayingAudioDidChange() final;
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    void didInsertAttachment(const String& identifier, const String& source) final;
-    void didRemoveAttachment(const String& identifier) final;
+    void didInsertAttachment(API::Attachment&, const String& source) final;
+    void didRemoveAttachment(API::Attachment&) final;
 #endif
 
 protected:
index d6e0e42..6afb935 100644 (file)
@@ -46,22 +46,22 @@ void PageClientImplCocoa::isPlayingAudioDidChange()
 
 #if ENABLE(ATTACHMENT_ELEMENT)
 
-void PageClientImplCocoa::didInsertAttachment(const String& identifier, const String& source)
+void PageClientImplCocoa::didInsertAttachment(API::Attachment& attachment, const String& source)
 {
 #if WK_API_ENABLED
-    [m_webView _didInsertAttachment:identifier withSource:source];
+    [m_webView _didInsertAttachment:attachment withSource:source];
 #else
-    UNUSED_PARAM(identifier);
+    UNUSED_PARAM(attachment);
     UNUSED_PARAM(source);
 #endif
 }
 
-void PageClientImplCocoa::didRemoveAttachment(const String& identifier)
+void PageClientImplCocoa::didRemoveAttachment(API::Attachment& attachment)
 {
 #if WK_API_ENABLED
-    [m_webView _didRemoveAttachment:identifier];
+    [m_webView _didRemoveAttachment:attachment];
 #else
-    UNUSED_PARAM(identifier);
+    UNUSED_PARAM(attachment);
 #endif
 }
 
index e2c691e..2683fc8 100644 (file)
@@ -52,6 +52,7 @@ OBJC_CLASS NSTextAlternatives;
 #endif
 
 namespace API {
+class Attachment;
 class HitTestResult;
 class Object;
 class OpenPanelParameters;
@@ -437,8 +438,8 @@ public:
 #endif
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    virtual void didInsertAttachment(const String& identifier, const String& source) { }
-    virtual void didRemoveAttachment(const String& identifier) { }
+    virtual void didInsertAttachment(API::Attachment&, const String& source) { }
+    virtual void didRemoveAttachment(API::Attachment&) { }
 #endif
 };
 
index 5a9531b..922342f 100644 (file)
@@ -3670,6 +3670,11 @@ void WebPageProxy::didCommitLoadForFrame(uint64_t frameID, uint64_t navigationID
             m_navigationClient->didCommitNavigation(*this, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get());
     } else
         m_loaderClient->didCommitLoadForFrame(*this, *frame, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get());
+
+#if ENABLE(ATTACHMENT_ELEMENT)
+    if (frame->isMainFrame())
+        invalidateAllAttachments();
+#endif
 }
 
 void WebPageProxy::didFinishDocumentLoadForFrame(uint64_t frameID, uint64_t navigationID, const UserData& userData)
@@ -6174,9 +6179,7 @@ void WebPageProxy::resetStateAfterProcessExited(ProcessTerminationReason termina
 #endif
 
 #if ENABLE(ATTACHMENT_ELEMENT)
-    for (auto identifier : m_attachmentIdentifierToAttachmentMap.keys())
-        m_pageClient.didRemoveAttachment(identifier);
-    m_attachmentIdentifierToAttachmentMap.clear();
+    invalidateAllAttachments();
 #endif
 
     if (terminationReason != ProcessTerminationReason::NavigationSwap) {
@@ -7818,9 +7821,19 @@ void WebPageProxy::cloneAttachmentData(const String& fromIdentifier, const Strin
     platformCloneAttachment(existingAttachment.releaseNonNull(), WTFMove(newAttachment));
 }
 
+void WebPageProxy::invalidateAllAttachments()
+{
+    for (auto& attachment : m_attachmentIdentifierToAttachmentMap.values()) {
+        if (attachment->insertionState() == API::Attachment::InsertionState::Inserted)
+            didRemoveAttachment(attachment.get());
+        attachment->invalidate();
+    }
+    m_attachmentIdentifierToAttachmentMap.clear();
+}
+
 #if !PLATFORM(COCOA)
 
-void WebPageProxy::platformRegisterAttachment(Ref<API::Attachment>&&, const String& preferredFileName, const IPC::DataReference&)
+void WebPageProxy::platformRegisterAttachment(Ref<API::Attachment>&&, const String&, const IPC::DataReference&)
 {
 }
 
@@ -7834,16 +7847,28 @@ void WebPageProxy::platformCloneAttachment(Ref<API::Attachment>&&, Ref<API::Atta
 
 #endif
 
-void WebPageProxy::didInsertAttachment(const String& identifier, const String& source)
+void WebPageProxy::didInsertAttachmentWithIdentifier(const String& identifier, const String& source)
+{
+    auto attachment = ensureAttachment(identifier);
+    didInsertAttachment(attachment.get(), source);
+}
+
+void WebPageProxy::didRemoveAttachmentWithIdentifier(const String& identifier)
+{
+    if (auto attachment = attachmentForIdentifier(identifier))
+        didRemoveAttachment(*attachment);
+}
+
+void WebPageProxy::didInsertAttachment(API::Attachment& attachment, const String& source)
 {
-    ensureAttachment(identifier);
-    m_pageClient.didInsertAttachment(identifier, source);
+    attachment.setInsertionState(API::Attachment::InsertionState::Inserted);
+    m_pageClient.didInsertAttachment(attachment, source);
 }
 
-void WebPageProxy::didRemoveAttachment(const String& identifier)
+void WebPageProxy::didRemoveAttachment(API::Attachment& attachment)
 {
-    ASSERT(attachmentForIdentifier(identifier));
-    m_pageClient.didRemoveAttachment(identifier);
+    attachment.setInsertionState(API::Attachment::InsertionState::NotInserted);
+    m_pageClient.didRemoveAttachment(attachment);
 }
 
 Ref<API::Attachment> WebPageProxy::ensureAttachment(const String& identifier)
index df42fda..40d21f2 100644 (file)
@@ -1809,9 +1809,12 @@ private:
     void platformRegisterAttachment(Ref<API::Attachment>&&, const String& filePath);
     void platformCloneAttachment(Ref<API::Attachment>&& fromAttachment, Ref<API::Attachment>&& toAttachment);
 
-    void didInsertAttachment(const String& identifier, const String& source);
-    void didRemoveAttachment(const String& identifier);
+    void didInsertAttachmentWithIdentifier(const String& identifier, const String& source);
+    void didRemoveAttachmentWithIdentifier(const String& identifier);
+    void didInsertAttachment(API::Attachment&, const String& source);
+    void didRemoveAttachment(API::Attachment&);
     Ref<API::Attachment> ensureAttachment(const String& identifier);
+    void invalidateAllAttachments();
 #endif
 
 #if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
index d2e3d1c..97048e9 100644 (file)
@@ -529,8 +529,8 @@ messages -> WebPageProxy {
     RegisterAttachmentIdentifierFromData(String identifier, String contentType, String preferredFileName, IPC::DataReference data)
     RegisterAttachmentIdentifierFromFilePath(String identifier, String contentType, String filePath)
     CloneAttachmentData(String fromIdentifier, String toIdentifier)
-    DidInsertAttachment(String identifier, String source)
-    DidRemoveAttachment(String identifier)
+    DidInsertAttachmentWithIdentifier(String identifier, String source)
+    DidRemoveAttachmentWithIdentifier(String identifier)
 #endif
 
 #if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
index 51d7789..f99803b 100644 (file)
@@ -175,14 +175,14 @@ void WebEditorClient::cloneAttachmentData(const String& fromIdentifier, const St
     m_page->send(Messages::WebPageProxy::CloneAttachmentData(fromIdentifier, toIdentifier));
 }
 
-void WebEditorClient::didInsertAttachment(const String& identifier, const String& source)
+void WebEditorClient::didInsertAttachmentWithIdentifier(const String& identifier, const String& source)
 {
-    m_page->send(Messages::WebPageProxy::DidInsertAttachment(identifier, source));
+    m_page->send(Messages::WebPageProxy::DidInsertAttachmentWithIdentifier(identifier, source));
 }
 
-void WebEditorClient::didRemoveAttachment(const String& identifier)
+void WebEditorClient::didRemoveAttachmentWithIdentifier(const String& identifier)
 {
-    m_page->send(Messages::WebPageProxy::DidRemoveAttachment(identifier));
+    m_page->send(Messages::WebPageProxy::DidRemoveAttachmentWithIdentifier(identifier));
 }
 
 #endif
index 6df441f..ef50e8a 100644 (file)
@@ -63,8 +63,8 @@ private:
     void registerAttachmentIdentifier(const String& identifier, const String& contentType, const String& preferredFileName, Ref<WebCore::SharedBuffer>&&) final;
     void registerAttachmentIdentifier(const String& identifier, const String& contentType, const String& filePath) final;
     void cloneAttachmentData(const String& fromIdentifier, const String& toIdentifier) final;
-    void didInsertAttachment(const String& identifier, const String& source) final;
-    void didRemoveAttachment(const String& identifier) final;
+    void didInsertAttachmentWithIdentifier(const String& identifier, const String& source) final;
+    void didRemoveAttachmentWithIdentifier(const String& identifier) final;
     bool supportsClientSideAttachmentData() const final { return true; }
 #endif
 
index 33b37fb..254b6c4 100644 (file)
@@ -1,3 +1,18 @@
+2018-08-21  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Attachment Support] Remove _WKAttachments and notify the UI client upon mainframe navigation
+        https://bugs.webkit.org/show_bug.cgi?id=188715
+        <rdar://problem/43541790>
+
+        Reviewed by Tim Horton.
+
+        Adds API tests to exercises cases where (1) the UI client is notified of attachment removal upon mainframe
+        navigation, and (2) the UI client is notified of attachment removal upon web content process termination.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+        (TestWebKitAPI::ObserveAttachmentUpdatesForScope::expectAttachmentUpdates):
+        (TestWebKitAPI::TEST):
+
 2018-08-21  Alex Christensen  <achristensen@webkit.org>
 
         Transition ResizeReversePaginatedWebView API test from WKPageLoaderClient to WKPageNavigationClient
index 953f5a6..89eea39 100644 (file)
@@ -27,6 +27,7 @@
 
 #import "DragAndDropSimulator.h"
 #import "PlatformUtilities.h"
+#import "TestNavigationDelegate.h"
 #import "TestWKWebView.h"
 #import "WKWebViewConfigurationExtras.h"
 #import <WebKit/WKPreferencesRefPrivate.h>
@@ -114,12 +115,12 @@ public:
 
     void expectAttachmentUpdates(NSArray<_WKAttachment *> *removed, NSArray<_WKAttachment *> *inserted)
     {
-        BOOL removedAttachmentsMatch = [observer().removed isEqual:removed];
+        BOOL removedAttachmentsMatch = [[NSSet setWithArray:observer().removed] isEqual:[NSSet setWithArray:removed]];
         if (!removedAttachmentsMatch)
             NSLog(@"Expected removed attachments: %@ to match %@.", observer().removed, removed);
         EXPECT_TRUE(removedAttachmentsMatch);
 
-        BOOL insertedAttachmentsMatch = [observer().inserted isEqual:inserted];
+        BOOL insertedAttachmentsMatch = [[NSSet setWithArray:observer().inserted] isEqual:[NSSet setWithArray:inserted]];
         if (!insertedAttachmentsMatch)
             NSLog(@"Expected inserted attachments: %@ to match %@.", observer().inserted, inserted);
         EXPECT_TRUE(insertedAttachmentsMatch);
@@ -1012,6 +1013,70 @@ TEST(WKAttachmentTests, InsertAttachmentUsingFileWrapperWithFilePath)
     [attachment expectRequestedDataToBe:testPDFData()];
 }
 
+TEST(WKAttachmentTests, InvalidateAttachmentsAfterMainFrameNavigation)
+{
+    auto webView = webViewForTestingAttachments();
+    RetainPtr<_WKAttachment> pdfAttachment;
+    RetainPtr<_WKAttachment> htmlAttachment;
+    {
+        ObserveAttachmentUpdatesForScope insertionObserver(webView.get());
+        pdfAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"attachment.pdf" contentType:@"application/pdf" data:testPDFData()];
+        htmlAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"index.html" contentType:@"text/html" data:testHTMLData()];
+        insertionObserver.expectAttachmentUpdates(@[ ], @[ pdfAttachment.get(), htmlAttachment.get() ]);
+        EXPECT_TRUE([pdfAttachment isConnected]);
+        EXPECT_TRUE([htmlAttachment isConnected]);
+    }
+
+    ObserveAttachmentUpdatesForScope removalObserver(webView.get());
+    [webView synchronouslyLoadTestPageNamed:@"simple"];
+    removalObserver.expectAttachmentUpdates(@[ pdfAttachment.get(), htmlAttachment.get() ], @[ ]);
+    EXPECT_FALSE([pdfAttachment isConnected]);
+    EXPECT_FALSE([htmlAttachment isConnected]);
+    [pdfAttachment expectRequestedDataToBe:nil];
+    [htmlAttachment expectRequestedDataToBe:nil];
+}
+
+TEST(WKAttachmentTests, InvalidateAttachmentsAfterWebProcessTermination)
+{
+    auto webView = webViewForTestingAttachments();
+    RetainPtr<_WKAttachment> pdfAttachment;
+    RetainPtr<_WKAttachment> htmlAttachment;
+    {
+        ObserveAttachmentUpdatesForScope insertionObserver(webView.get());
+        pdfAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"attachment.pdf" contentType:@"application/pdf" data:testPDFData()];
+        htmlAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"index.html" contentType:@"text/html" data:testHTMLData()];
+        insertionObserver.expectAttachmentUpdates(@[ ], @[ pdfAttachment.get(), htmlAttachment.get() ]);
+        EXPECT_TRUE([pdfAttachment isConnected]);
+        EXPECT_TRUE([htmlAttachment isConnected]);
+    }
+    {
+        ObserveAttachmentUpdatesForScope removalObserver(webView.get());
+        [webView stringByEvaluatingJavaScript:@"getSelection().collapseToEnd()"];
+        [webView _synchronouslyExecuteEditCommand:@"DeleteBackward" argument:nil];
+        removalObserver.expectAttachmentUpdates(@[ htmlAttachment.get() ], @[ ]);
+        EXPECT_TRUE([pdfAttachment isConnected]);
+        EXPECT_FALSE([htmlAttachment isConnected]);
+        [htmlAttachment expectRequestedDataToBe:testHTMLData()];
+    }
+
+    __block bool webProcessTerminated = false;
+    auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+    [navigationDelegate setWebContentProcessDidTerminate:^(WKWebView *) {
+        webProcessTerminated = true;
+    }];
+
+    ObserveAttachmentUpdatesForScope observer(webView.get());
+    [webView _killWebContentProcess];
+    TestWebKitAPI::Util::run(&webProcessTerminated);
+
+    observer.expectAttachmentUpdates(@[ pdfAttachment.get() ], @[ ]);
+    EXPECT_FALSE([pdfAttachment isConnected]);
+    EXPECT_FALSE([htmlAttachment isConnected]);
+    [pdfAttachment expectRequestedDataToBe:nil];
+    [htmlAttachment expectRequestedDataToBe:nil];
+}
+
 #pragma mark - Platform-specific tests
 
 #if PLATFORM(MAC)