Bug 165276 - [Cocoa] Expose InjectedBundlePageEditorClient via the Objective-C bundle SPI
Summary: [Cocoa] Expose InjectedBundlePageEditorClient via the Objective-C bundle SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-01 14:15 PST by mitz
Modified: 2016-12-05 03:52 PST (History)
7 users (show)

See Also:


Attachments
Add WKWebProcessPlugInEditingDelegate (51.07 KB, patch)
2016-12-03 10:41 PST, mitz
no flags Details | Formatted Diff | Diff
Add WKWebProcessPlugInEditingDelegate (83.99 KB, patch)
2016-12-03 14:25 PST, mitz
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (783.61 KB, application/zip)
2016-12-03 15:44 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2016-12-01 14:15:14 PST
Allow a WKWebProcessPlugInBrowserContextController delegate to provide pasteboard data for a selected range and get notified when it’s written to the pasteboard.
Comment 1 mitz 2016-12-01 14:17:50 PST
<rdar://problem/29467040>
Comment 2 mitz 2016-12-03 10:41:38 PST
Created attachment 296056 [details]
Add WKWebProcessPlugInEditingDelegate
Comment 3 Darin Adler 2016-12-03 11:34:34 PST
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 4 mitz 2016-12-03 14:22:19 PST
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.
Comment 5 mitz 2016-12-03 14:25:37 PST
Created attachment 296070 [details]
Add WKWebProcessPlugInEditingDelegate
Comment 6 WebKit Commit Bot 2016-12-03 14:27:15 PST
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.
Comment 7 Darin Adler 2016-12-03 14:44:49 PST
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);
Comment 8 mitz 2016-12-03 14:45:48 PST
(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.
Comment 9 Darin Adler 2016-12-03 14:46:57 PST
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.
Comment 10 mitz 2016-12-03 14:49:25 PST
(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.
Comment 11 Darin Adler 2016-12-03 15:15:59 PST
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<>&.
Comment 12 Darin Adler 2016-12-03 15:18:55 PST
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.
Comment 13 Build Bot 2016-12-03 15:44:31 PST
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
Comment 14 Build Bot 2016-12-03 15:44:34 PST
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
Comment 15 mitz 2016-12-03 15:50:24 PST
Committed <https://trac.webkit.org/r209307>.
Comment 16 mitz 2016-12-03 16:12:28 PST
Fixed the build in <https://trac.webkit.org/r209308>.
Comment 17 Csaba Osztrogonác 2016-12-05 03:51:00 PST
(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.
Comment 18 Csaba Osztrogonác 2016-12-05 03:52:06 PST
buildfix landed in https://trac.webkit.org/changeset/209316