[webkit-reviews] review granted: [Bug 108396] WebKit2: provide new bundle APIs to allow bundle clients to be notified of pasteboard access. : [Attachment 185616] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 108396: WebKit2: provide new bundle APIs to allow bundle clients to be
notified of pasteboard access.
https://bugs.webkit.org/show_bug.cgi?id=108396

Attachment 185616: Patch
https://bugs.webkit.org/attachment.cgi?id=185616&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.