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.
<rdar://problem/12920461>
Created attachment 185616 [details] Patch
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 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 on attachment 185616 [details] Patch Attachment 185616 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16218791
(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 on attachment 185616 [details] Patch Attachment 185616 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16202982
Comment on attachment 185616 [details] Patch Attachment 185616 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16252029
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 on attachment 185616 [details] Patch Attachment 185616 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16218874
Comment on attachment 185616 [details] Patch Attachment 185616 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16251134
Comment on attachment 185616 [details] Patch Attachment 185616 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16249165
Comment on attachment 185616 [details] Patch Attachment 185616 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16280002
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
Created attachment 185825 [details] Patch to test other platform builds
(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>.
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 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 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
Created attachment 185829 [details] one more try at making something that builds
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 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 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 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 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
Committed revision 141473.