Summary: | WebKit2: provide new bundle APIs to allow bundle clients to be notified of pasteboard access. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||
Component: | WebKit API | Assignee: | 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
Enrica Casucci
2013-01-30 15:54:36 PST
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. |