Bug 108396 – WebKit2: provide new bundle APIs to allow bundle clients to be notified of pasteboard access.

• Component: WebKit API
• Status: RESOLVED FIXED

[Enrica Casucci]

We need to add a mechanism for WebKit2 clients to be notified when the WebProcess is getting ready to add content to the pasteboard and potentially provide new pasteboard formats to add together with the standard WebKit supported types.

[Enrica Casucci]

<rdar://problem/12920461>

[Enrica Casucci]

Created attachment 185616 [details]
Patch

[WebKit Review Bot]

Attachment 185616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/Editor.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/EditorClient.h', u'Source/WebCore/platform/mac/PasteboardMac.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.h', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/API/c/WKSharedAPICast.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.h', u'Source/WebKit2/Shared/APIClientTraits.cpp', u'Source/WebKit2/Shared/APIClientTraits.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/WebArchive.cpp', u'Source/WebKit2/Shared/WebArchive.h', u'Source/WebKit2/Shared/WebArchiveResource.cpp', u'Source/WebKit2/Shared/WebArchiveResource.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications.mm', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications_Bundle.cpp', u'Tools/TestWebKitAPI/Tests/WebKit2/execCopy.html']" exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:322:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 185616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185616&action=review

I have a number of comments which seem straightforward to address, r=me.

> Source/WebCore/ChangeLog:15
> +        (WebCore::Editor::cut): Added calls to didWriteSelectionToPasteboard and
> +        willWriteSelectionToPasteboard.

I don't think that there is a call to didWriteSelectionToPasteboard() in the final version of the patch.

> Source/WebCore/page/EditorClient.h:95
> +    virtual void willWriteSelectionToPasteboard(PassRefPtr<Range>) = 0;
>      virtual void didWriteSelectionToPasteboard() = 0;
> +    virtual void getClientPasteboardDataForRange(PassRefPtr<Range>, Vector<String>& pasteboardTypes, Vector<RefPtr<SharedBuffer> >& pasteboardData) = 0;

I don't see why passing Range ownership from WebCore to WebKit is desirable here. Semantically, this looks like it should be taking a plain pointer.

> Source/WebKit2/Shared/WebArchive.cpp:68
> +    Vector<PassRefPtr<ArchiveResource> > coreArchiveResources;

I'm surprised that Vector<PassRefPtr> works. I don't understand what it means to have such a vector though - can you use a regular Vector<RefPtr>?

> Source/WebKit2/Shared/WebArchive.cpp:75
> +    Vector<PassRefPtr<LegacyWebArchive> > coreSubframeLegacyWebArchives;

Ditto.

> Source/WebKit2/Shared/WebArchiveResource.cpp:95
> +WebCore::ArchiveResource* WebArchiveResource::coreArchiveResource()

This file has "using namespace WebCore", so it should not need WebCore prefixes.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp:131
> +void InjectedBundlePageEditorClient::willWriteToPasteboard(WebPage* page, WebCore::Range* range)

This file has using namespace WebCore, so it should not need WebCore prefixes.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp:139
> +void InjectedBundlePageEditorClient::getPasteboardDataForRange(WebPage* page, WebCore::Range* range, Vector<String>& pasteboardTypes, Vector<RefPtr<WebCore::SharedBuffer> >& pasteboardData)

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp:168
> +                RefPtr<WebCore::SharedBuffer> buffer = WebCore::SharedBuffer::create(item->bytes(), item->size());

Ditto.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:320
> +void WebEditorClient::willWriteSelectionToPasteboard(PassRefPtr<WebCore::Range>)
> +{
> +}
> +
> +void WebEditorClient::getClientPasteboardDataForRange(PassRefPtr<WebCore::Range>, Vector<String>& pasteboardTypes, Vector<RefPtr<WebCore::SharedBuffer> >& pasteboardData)
> +{
> +}

I would appreciate a comment here, explaining the status of implementation.

Actually, I suspect that this is breaking other platforms. Can the functions just have an empty default implementation in WebCore?

> Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications_Bundle.cpp:74
> +
> +    

Two empty lines here.

[Build Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16218791

[Enrica Casucci]

(In reply to comment #4)
> (From update of attachment 185616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185616&action=review
> 
> I have a number of comments which seem straightforward to address, r=me.
> 
> > Source/WebCore/ChangeLog:15
> > +        (WebCore::Editor::cut): Added calls to didWriteSelectionToPasteboard and
> > +        willWriteSelectionToPasteboard.
> 
> I don't think that there is a call to didWriteSelectionToPasteboard() in the final version of the patch.
Will fix it.
> 
> > Source/WebCore/page/EditorClient.h:95
> > +    virtual void willWriteSelectionToPasteboard(PassRefPtr<Range>) = 0;
> >      virtual void didWriteSelectionToPasteboard() = 0;
> > +    virtual void getClientPasteboardDataForRange(PassRefPtr<Range>, Vector<String>& pasteboardTypes, Vector<RefPtr<SharedBuffer> >& pasteboardData) = 0;
> 
> I don't see why passing Range ownership from WebCore to WebKit is desirable here. Semantically, this looks like it should be taking a plain pointer.
You're correct. I'll change it to Range*.
> 
> > Source/WebKit2/Shared/WebArchive.cpp:68
> > +    Vector<PassRefPtr<ArchiveResource> > coreArchiveResources;
> 
> I'm surprised that Vector<PassRefPtr> works. I don't understand what it means to have such a vector though - can you use a regular Vector<RefPtr>?
> 
> > Source/WebKit2/Shared/WebArchive.cpp:75
> > +    Vector<PassRefPtr<LegacyWebArchive> > coreSubframeLegacyWebArchives;
> 
> Ditto.

I'll let Sam comment on these.


> I would appreciate a comment here, explaining the status of implementation.
Sure.

> 
> Actually, I suspect that this is breaking other platforms. Can the functions just have an empty default implementation in WebCore?
I don't believe it is possible. I'll fix the EditorClient implementations for the other platforms.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications_Bundle.cpp:74
> > +
> > +    
> 
> Two empty lines here.

ok.
Thanks for the review.
I will make sure the patch builds on all platforms before landing.

[Early Warning System Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16202982

[WebKit Review Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16252029

[Peter Beverloo (cr-android ews)]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16249051

[Build Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16218874

[EFL EWS Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16251134

[Early Warning System Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16249165

[Build Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16280002

[WebKit Review Bot]

Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16270078

[Enrica Casucci]

Created attachment 185825 [details]
Patch to test other platform builds

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 185616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185616&action=review
> 
> > Source/WebKit2/Shared/WebArchive.cpp:68
> > +    Vector<PassRefPtr<ArchiveResource> > coreArchiveResources;
> 
> I'm surprised that Vector<PassRefPtr> works. I don't understand what it means to have such a vector though - can you use a regular Vector<RefPtr>?
> 
> > Source/WebKit2/Shared/WebArchive.cpp:75
> > +    Vector<PassRefPtr<LegacyWebArchive> > coreSubframeLegacyWebArchives;
> 
> Ditto.
> 


This weirdness is necessary due to the very weird API of LegacyWebArchive, which takes a Vector<PassRefPtr>.

[WebKit Review Bot]

Attachment 185825 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/Editor.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/EditorClient.h', u'Source/WebCore/platform/mac/PasteboardMac.mm', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp', u'Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/EditorClientImpl.cpp', u'Source/WebKit/chromium/src/EditorClientImpl.h', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/EditorClientEfl.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.h', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/EditorClientQt.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebEditorClient.h', u'Source/WebKit/wince/ChangeLog', u'Source/WebKit/wince/WebCoreSupport/EditorClientWinCE.cpp', u'Source/WebKit/wince/WebCoreSupport/EditorClientWinCE.h', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp', u'Source/WebKit/wx/WebKitSupport/EditorClientWx.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/API/c/WKSharedAPICast.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.h', u'Source/WebKit2/Shared/APIClientTraits.cpp', u'Source/WebKit2/Shared/APIClientTraits.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/WebArchive.cpp', u'Source/WebKit2/Shared/WebArchive.h', u'Source/WebKit2/Shared/WebArchiveResource.cpp', u'Source/WebKit2/Shared/WebArchiveResource.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications.mm', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications_Bundle.cpp', u'Tools/TestWebKitAPI/Tests/WebKit2/execCopy.html']" exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:320:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 185825 [details]
Patch to test other platform builds

Attachment 185825 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16270310

[Build Bot]

Comment on attachment 185825 [details]
Patch to test other platform builds

Attachment 185825 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16251483

[Enrica Casucci]

Created attachment 185829 [details]
one more try at making something that builds

[WebKit Review Bot]

Attachment 185829 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/Editor.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/EditorClient.h', u'Source/WebCore/platform/mac/PasteboardMac.mm', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp', u'Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/EditorClientImpl.cpp', u'Source/WebKit/chromium/src/EditorClientImpl.h', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/EditorClientEfl.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.h', u'Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/EditorClientQt.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebEditorClient.h', u'Source/WebKit/wince/ChangeLog', u'Source/WebKit/wince/WebCoreSupport/EditorClientWinCE.cpp', u'Source/WebKit/wince/WebCoreSupport/EditorClientWinCE.h', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp', u'Source/WebKit/wx/WebKitSupport/EditorClientWx.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/API/c/WKSharedAPICast.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchive.h', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.cpp', u'Source/WebKit2/Shared/API/c/mac/WKWebArchiveResource.h', u'Source/WebKit2/Shared/APIClientTraits.cpp', u'Source/WebKit2/Shared/APIClientTraits.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/WebArchive.cpp', u'Source/WebKit2/Shared/WebArchive.h', u'Source/WebKit2/Shared/WebArchiveResource.cpp', u'Source/WebKit2/Shared/WebArchiveResource.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications.mm', u'Tools/TestWebKitAPI/Tests/WebKit2/PasteboardNotifications_Bundle.cpp', u'Tools/TestWebKitAPI/Tests/WebKit2/execCopy.html']" exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:320:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 185825 [details]
Patch to test other platform builds

Attachment 185825 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16252544

[Build Bot]

Comment on attachment 185829 [details]
one more try at making something that builds

Attachment 185829 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16280336

[Peter Beverloo (cr-android ews)]

Comment on attachment 185825 [details]
Patch to test other platform builds

Attachment 185825 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16267318

[EFL EWS Bot]

Comment on attachment 185825 [details]
Patch to test other platform builds

Attachment 185825 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16268369

[Enrica Casucci]

Committed revision 141473.