Bug 165276 – [Cocoa] Expose InjectedBundlePageEditorClient via the Objective-C bundle SPI

• Component: WebKit2
• Status: RESOLVED FIXED

Allow a WKWebProcessPlugInBrowserContextController delegate to provide pasteboard data for a selected range and get notified when it’s written to the pasteboard.

<rdar://problem/29467040>

Created attachment 296056 [details]
Add WKWebProcessPlugInEditingDelegate

[Darin Adler]

Comment on attachment 296056 [details]
Add WKWebProcessPlugInEditingDelegate

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

Do we have a good testing strategy for this (and the other injected bundle clients)?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h:50
> +class InjectedBundlePageEditorClient : public API::Client<WKBundlePageEditorClientBase>, public API::InjectedBundle::EditorClient {

I suggest making this class final.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h:52
> +    explicit InjectedBundlePageEditorClient(const WKBundlePageEditorClientBase*);

I suggest we take a reference rather than a pointer.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageEditorClient.h:67
> +    bool shouldBeginEditing(WebPage*, WebCore::Range*) override;
> +    bool shouldEndEditing(WebPage*, WebCore::Range*) override;
> +    bool shouldInsertNode(WebPage*, WebCore::Node*, WebCore::Range* rangeToReplace, WebCore::EditorInsertAction) override;
> +    bool shouldInsertText(WebPage*, StringImpl*, WebCore::Range* rangeToReplace, WebCore::EditorInsertAction) override;
> +    bool shouldDeleteRange(WebPage*, WebCore::Range*) override;
> +    bool shouldChangeSelectedRange(WebPage*, WebCore::Range* fromRange, WebCore::Range* toRange, WebCore::EAffinity, bool stillSelecting) override;
> +    bool shouldApplyStyle(WebPage*, WebCore::CSSStyleDeclaration*, WebCore::Range*) override;
> +    void didBeginEditing(WebPage*, StringImpl* notificationName) override;
> +    void didEndEditing(WebPage*, StringImpl* notificationName) override;
> +    void didChange(WebPage*, StringImpl* notificationName) override;
> +    void didChangeSelection(WebPage*, StringImpl* notificationName) override;
> +    void willWriteToPasteboard(WebPage*, WebCore::Range*) override;
> +    void getPasteboardDataForRange(WebPage*, WebCore::Range*, Vector<String>& pasteboardTypes, Vector<RefPtr<WebCore::SharedBuffer>>& pasteboardData) override;
> +    void didWriteToPasteboard(WebPage*) override;

I suggest marking these all final and making them private as well.

> Source/WebKit2/WebProcess/InjectedBundle/API/APIInjectedBundleEditorClient.h:31
> +#include <WebCore/EditorInsertAction.h>
> +#include <WebCore/SharedBuffer.h>
> +#include <WebCore/TextAffinity.h>
> +#include <wtf/Forward.h>

Strange that we both WebCore/ and wtf/ here -- I would have thought we’d use JavaScriptCore/Forward.h and not depended directly on forwarding headers.

> Source/WebKit2/WebProcess/InjectedBundle/API/APIInjectedBundleEditorClient.h:64
> +    virtual bool shouldBeginEditing(WebKit::WebPage*, WebCore::Range*) { return false; }
> +    virtual bool shouldEndEditing(WebKit::WebPage*, WebCore::Range*) { return false; }
> +    virtual bool shouldInsertNode(WebKit::WebPage*, WebCore::Node*, WebCore::Range* rangeToReplace, WebCore::EditorInsertAction) { return false; }
> +    virtual bool shouldInsertText(WebKit::WebPage*, StringImpl*, WebCore::Range* rangeToReplace, WebCore::EditorInsertAction) { return false; }
> +    virtual bool shouldDeleteRange(WebKit::WebPage*, WebCore::Range*) { return false; }
> +    virtual bool shouldChangeSelectedRange(WebKit::WebPage*, WebCore::Range* fromRange, WebCore::Range* toRange, WebCore::EAffinity affinity, bool stillSelecting) { return false; }
> +    virtual bool shouldApplyStyle(WebKit::WebPage*, WebCore::CSSStyleDeclaration*, WebCore::Range*) { return false; }
> +    virtual void didBeginEditing(WebKit::WebPage*, StringImpl* notificationName) { }
> +    virtual void didEndEditing(WebKit::WebPage*, StringImpl* notificationName) { }
> +    virtual void didChange(WebKit::WebPage*, StringImpl* notificationName) { }
> +    virtual void didChangeSelection(WebKit::WebPage*, StringImpl* notificationName) { }
> +    virtual void willWriteToPasteboard(WebKit::WebPage*, WebCore::Range*) { }
> +    virtual void getPasteboardDataForRange(WebKit::WebPage*, WebCore::Range*, Vector<WTF::String>& pasteboardTypes, Vector<RefPtr<WebCore::SharedBuffer>>& pasteboardData) { }
> +    virtual void didWriteToPasteboard(WebKit::WebPage*) { }

Would be nice if these took references rather than pointers.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:580
> +    class EditorClient : public API::InjectedBundle::EditorClient {

Since the class is scoped inside this function, I could imagine giving it an even shorter name, "Client", or a more explicit and hence longer name "DelegateForwardingEditorClient".

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:588
> +            auto editingDelegate = m_controller->_editingDelegate.get();
> +            m_delegateMethods.willWriteToPasteboard = [editingDelegate respondsToSelector:@selector(_webProcessPlugInBrowserContextController:willWriteRangeToPasteboard:)];
> +            m_delegateMethods.getPasteboardDataForRange = [editingDelegate respondsToSelector:@selector(_webProcessPlugInBrowserContextController:pasteboardDataForRange:)];
> +            m_delegateMethods.didWriteToPasteboard = [editingDelegate respondsToSelector:@selector(_webProcessPlugInBrowserContextControllerDidWriteToPasteboard:)];

Would be neat to do this in a helper function so that we could use an initializer. Then we could make the data member const to make the pattern of use a little clearer. Or maybe the extra const would just be clutter since this is a local class.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:591
> +        virtual ~EditorClient() { }

This seems unnecessary. I think we’d get exactly the same code if we omitted it.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:593
> +        void willWriteToPasteboard(WebKit::WebPage*, WebCore::Range* range)

Make this const?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:601
> +        void getPasteboardDataForRange(WebPage*, Range* range, Vector<String>& pasteboardTypes, Vector<RefPtr<SharedBuffer>>& pasteboardData)

Make this const?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:606
> +            NSDictionary<NSString *, NSData *> *data = [m_controller->_editingDelegate.get() _webProcessPlugInBrowserContextController:m_controller pasteboardDataForRange:wrapper(*InjectedBundleRangeHandle::getOrCreate(range).get())];

Should we we use auto here to avoid writing out that type name? Or maybe just write it all out as a single giant line?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:607
> +            [data enumerateKeysAndObjectsUsingBlock:^(NSString *type, NSData *data, BOOL*) {

I was slightly confused by the use of the identifier "data" for both the NSDictionary and the NSData object.

Is this the most efficient idiom? I had the impression that the for loop would be efficient even with the need to call objectForKey: each time.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:613
> +        void didWriteToPasteboard(WebKit::WebPage*)

Make this const?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:622
> +        WKWebProcessPlugInBrowserContextController *m_controller;

Make this const?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:626
> +            bool willWriteToPasteboard : 1;
> +            bool getPasteboardDataForRange : 1;
> +            bool didWriteToPasteboard : 1;

Is it valuable to use bitfields for this instead of just bool?

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:627
> +        } m_delegateMethods;

Make this const?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:325
> +    void setInjectedBundleEditorClient(std::unique_ptr<API::InjectedBundle::EditorClient>);

We need to figure out whether to use std::unique_ptr&& or std::unique_ptr for move-only arguments. My attempt to research this by reading articles on the web yielded conflicting advice.

Comment on attachment 296056 [details]
Add WKWebProcessPlugInEditingDelegate

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

Thanks for the review! Going to post a new patch with the changes mentioned below and with an API test.

>> Source/WebKit2/WebProcess/InjectedBundle/API/APIInjectedBundleEditorClient.h:31
>> +#include <wtf/Forward.h>
> 
> Strange that we both WebCore/ and wtf/ here -- I would have thought we’d use JavaScriptCore/Forward.h and not depended directly on forwarding headers.

This comment confused me. In WebKit2 internals, this is how we include headers originating from WebCore and wtf.

>> Source/WebKit2/WebProcess/InjectedBundle/API/APIInjectedBundleEditorClient.h:64
>> +    virtual void didWriteToPasteboard(WebKit::WebPage*) { }
> 
> Would be nice if these took references rather than pointers.

I changed the WebPage pointers to references. Didn’t change the pointers that come as such from WebCore’s EditorClient interface, nor the notificationName StringImpl ones, although we can probably pass those as String.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:580
>> +    class EditorClient : public API::InjectedBundle::EditorClient {
> 
> Since the class is scoped inside this function, I could imagine giving it an even shorter name, "Client", or a more explicit and hence longer name "DelegateForwardingEditorClient".

Changed to “Client”.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:588
>> +            m_delegateMethods.didWriteToPasteboard = [editingDelegate respondsToSelector:@selector(_webProcessPlugInBrowserContextControllerDidWriteToPasteboard:)];
> 
> Would be neat to do this in a helper function so that we could use an initializer. Then we could make the data member const to make the pattern of use a little clearer. Or maybe the extra const would just be clutter since this is a local class.

Gave the struct an initializer.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:591
>> +        virtual ~EditorClient() { }
> 
> This seems unnecessary. I think we’d get exactly the same code if we omitted it.

Removed.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:593
>> +        void willWriteToPasteboard(WebKit::WebPage*, WebCore::Range* range)
> 
> Make this const?

Not sure what or how to make const here. But made this class final and these functions private and final.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:606
>> +            NSDictionary<NSString *, NSData *> *data = [m_controller->_editingDelegate.get() _webProcessPlugInBrowserContextController:m_controller pasteboardDataForRange:wrapper(*InjectedBundleRangeHandle::getOrCreate(range).get())];
> 
> Should we we use auto here to avoid writing out that type name? Or maybe just write it all out as a single giant line?

auto it is.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:607
>> +            [data enumerateKeysAndObjectsUsingBlock:^(NSString *type, NSData *data, BOOL*) {
> 
> I was slightly confused by the use of the identifier "data" for both the NSDictionary and the NSData object.
> 
> Is this the most efficient idiom? I had the impression that the for loop would be efficient even with the need to call objectForKey: each time.

Renamed the dictionary to dataByType and changed to use a for loop.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:626
>> +            bool didWriteToPasteboard : 1;
> 
> Is it valuable to use bitfields for this instead of just bool?

Changed to just bools.

>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:627
>> +        } m_delegateMethods;
> 
> Make this const?

Done.

Created attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:590:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:598:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:610:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:620:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 4 in 27 files


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

[Darin Adler]

Here’s the iOS simulator failure from EWS:

/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/BundleEditingDelegate.mm:78:29: error: use of undeclared identifier 'NSPasteboard'
    EXPECT_STREQ("hello", [[NSPasteboard generalPasteboard] stringForType:@"org.webkit.data"].UTF8String);

(In reply to comment #7)
> Here’s the iOS simulator failure from EWS:
> 
> /Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/
> BundleEditingDelegate.mm:78:29: error: use of undeclared identifier
> 'NSPasteboard'
>     EXPECT_STREQ("hello", [[NSPasteboard generalPasteboard]
> stringForType:@"org.webkit.data"].UTF8String);

Thanks! I’ll need to do something very similar with UIPasteboard when building for iOS.

[Darin Adler]

Comment on attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate

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

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:619
> +            DelegateMethods(RetainPtr<id <WKWebProcessPlugInEditingDelegate>> delegate)

Since this doesn’t retain, I think it should take a regular pointer. And looking above, it looks like we are passing a regular pointer. So we can omit the RetainPtr<> here.

(In reply to comment #9)
> Comment on attachment 296070 [details]
> Add WKWebProcessPlugInEditingDelegate
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296070&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:619
> > +            DelegateMethods(RetainPtr<id <WKWebProcessPlugInEditingDelegate>> delegate)
> 
> Since this doesn’t retain, I think it should take a regular pointer. And
> looking above, it looks like we are passing a regular pointer.

We are passing a RetainPtr, which is what WeakObjCPtr<>::get returns. Seems safer and nicer to pass that than to extract the raw pointer with another get() and reason about the lifetime of the RetainPtr.

[Darin Adler]

Comment on attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate

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

>>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:619
>>> +            DelegateMethods(RetainPtr<id <WKWebProcessPlugInEditingDelegate>> delegate)
>> 
>> Since this doesn’t retain, I think it should take a regular pointer. And looking above, it looks like we are passing a regular pointer. So we can omit the RetainPtr<> here.
> 
> We are passing a RetainPtr, which is what WeakObjCPtr<>::get returns. Seems safer and nicer to pass that than to extract the raw pointer with another get() and reason about the lifetime of the RetainPtr.

OK, I guess it’s just a taste thing. I personally would add another get() and use the raw pointer. There is a tiny retain pointer churn cost to not doing that.

You could avoid that cost without reasoning about RetainPtr lifetime by changing the argument type here to const RetainPtr<>&.

[Darin Adler]

Comment on attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate

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

>>>> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:619
>>>> +            DelegateMethods(RetainPtr<id <WKWebProcessPlugInEditingDelegate>> delegate)
>>> 
>>> Since this doesn’t retain, I think it should take a regular pointer. And looking above, it looks like we are passing a regular pointer. So we can omit the RetainPtr<> here.
>> 
>> We are passing a RetainPtr, which is what WeakObjCPtr<>::get returns. Seems safer and nicer to pass that than to extract the raw pointer with another get() and reason about the lifetime of the RetainPtr.
> 
> OK, I guess it’s just a taste thing. I personally would add another get() and use the raw pointer. There is a tiny retain pointer churn cost to not doing that.
> 
> You could avoid that cost without reasoning about RetainPtr lifetime by changing the argument type here to const RetainPtr<>&.

Hmm, I think I’m wrong and move semantics will save us the retain/release with the code just like this.

[Build Bot]

Comment on attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate

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

New failing tests:
media/modern-media-controls/media-controller/media-controller-resize.html

[Build Bot]

Created attachment 296077 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

Committed <https://trac.webkit.org/r209307>.

Fixed the build in <https://trac.webkit.org/r209308>.

[Csaba Osztrogonác]

(In reply to comment #15)
> Committed <https://trac.webkit.org/r209307>.

It broke the Apple Mac cmake build, WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInRangeHandle.mm should be added to the cmake build system too.

[Csaba Osztrogonác]

buildfix landed in https://trac.webkit.org/changeset/209316