Bug 108396

Summary: WebKit2: provide new bundle APIs to allow bundle clients to be notified of pasteboard access.
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebKit APIAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, buildbot, dglazkov, gyuyoung.kim, japhet, mifenton, peter+ews, philn, rakuco, rniwa, rwlbuis, sam, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez, yong.li.webkit
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
ap: review+, ap: commit-queue-
Patch to test other platform builds
webkit-ews: commit-queue-
one more try at making something that builds buildbot: commit-queue-

Description Enrica Casucci 2013-01-30 15:54:36 PST
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.
Comment 1 Enrica Casucci 2013-01-30 15:54:50 PST
<rdar://problem/12920461>
Comment 2 Enrica Casucci 2013-01-30 16:27:35 PST
Created attachment 185616 [details]
Patch
Comment 3 WebKit Review Bot 2013-01-30 16:36:02 PST
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.
Comment 4 Alexey Proskuryakov 2013-01-30 16:51:31 PST
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.
Comment 5 Build Bot 2013-01-30 16:58:19 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16218791
Comment 6 Enrica Casucci 2013-01-30 17:06:32 PST
(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.
Comment 7 Early Warning System Bot 2013-01-30 17:10:43 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16202982
Comment 8 WebKit Review Bot 2013-01-30 17:22:25 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16252029
Comment 9 Peter Beverloo (cr-android ews) 2013-01-30 17:55:59 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16249051
Comment 10 Build Bot 2013-01-30 19:21:03 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16218874
Comment 11 EFL EWS Bot 2013-01-30 20:35:00 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16251134
Comment 12 Early Warning System Bot 2013-01-30 21:20:39 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16249165
Comment 13 Build Bot 2013-01-30 23:19:20 PST
Comment on attachment 185616 [details]
Patch

Attachment 185616 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16280002
Comment 14 WebKit Review Bot 2013-01-31 01:27:47 PST
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
Comment 15 Enrica Casucci 2013-01-31 11:18:44 PST
Created attachment 185825 [details]
Patch to test other platform builds
Comment 16 Sam Weinig 2013-01-31 11:21:45 PST
(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>.
Comment 17 WebKit Review Bot 2013-01-31 11:22:13 PST
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.
Comment 18 Early Warning System Bot 2013-01-31 11:28:22 PST
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
Comment 19 Build Bot 2013-01-31 11:36:12 PST
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
Comment 20 Enrica Casucci 2013-01-31 11:42:27 PST
Created attachment 185829 [details]
one more try at making something that builds
Comment 21 WebKit Review Bot 2013-01-31 11:45:58 PST
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.
Comment 22 WebKit Review Bot 2013-01-31 12:02:52 PST
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
Comment 23 Build Bot 2013-01-31 12:06:04 PST
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
Comment 24 Peter Beverloo (cr-android ews) 2013-01-31 12:11:17 PST
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
Comment 25 EFL EWS Bot 2013-01-31 12:33:44 PST
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
Comment 26 Enrica Casucci 2013-01-31 13:16:54 PST
Committed revision 141473.