Bug 202622 – [Clipboard API] Introduce bindings for the async Clipboard API

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Work in progress towards implementing the async Clipboard API.

[Radar WebKit Bug Importer]

<rdar://problem/56038126>

[Wenson Hsieh]

Created attachment 380333 [details]
IDL files and API stubs.

[Wenson Hsieh]

I’m working on upstreaming the existing web platform tests as well.

[Wenson Hsieh]

Created attachment 380334 [details]
Fix CMake builds

[Wenson Hsieh]

Created attachment 380339 [details]
Adjust WK1 test expectations.

[Ryosuke Niwa]

Comment on attachment 380339 [details]
Adjust WK1 test expectations.

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

You can fix the GC issue in a separate patch but please be sure to file a bug before landing this so that we won't forget to fix it.

> Source/WebCore/ChangeLog:9
> +        Adds IDL support for the async clipboard API (with the exception of delayed item generation interfaces, which

Nit: Adds IDL. It's not like IDL is a feature to support.x

> Source/WebCore/ChangeLog:11
> +

Please add a link to the spec.

> Source/WebCore/ChangeLog:18
> +        * Modules/clipboard/Clipboard.cpp:

Can we use Modules/async-clipboard/ instead?
It's a bit confusing as is since we already have the support for sync clipboard API support.

> Source/WebCore/Modules/clipboard/Clipboard.h:58
> +    void refEventTarget() final { ref(); }
> +    void derefEventTarget() final { deref(); }

Why do we need these??

> Source/WebCore/Modules/clipboard/Clipboard.idl:32
> +    EnabledBySetting=AsyncClipboardAPI,
> +] interface Clipboard : EventTarget {

Probably need GenerateIsReachable=ReachableFromDOMWindow or something.
Otherwise, the wrapper of this object can be collected.

> Source/WebCore/Modules/clipboard/ClipboardItem.idl:40
> +    ImplementationLacksVTable
> +] interface ClipboardItem {

We need to start thinking about GC model of this object.
You'd probably need to make this item's Clipboard an opaque root if it's associated with one.
Alternatively, add GenerateIsReachable=ReachableFromDOMWindow
and make it return DOMWindow* when it's associated with Clipboard.

> Source/WebCore/Modules/clipboard/NavigatorClipboard.h:43
> +    static Clipboard* clipboard(Navigator&);
> +    Clipboard* clipboard();

Return RefPtr instead?

> LayoutTests/platform/ios-wk1/TestExpectations:19
> +# Async Clipboard API is not supported in WebKit1.
> +editing/clipboard [ Skip ]
> +

Can we add the support in a subsequent patch? It's not great to have discrepancy between WK1 and WK2 if we can avoid it.

[Wenson Hsieh]

Comment on attachment 380339 [details]
Adjust WK1 test expectations.

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

>> Source/WebCore/ChangeLog:9
>> +        Adds IDL support for the async clipboard API (with the exception of delayed item generation interfaces, which
> 
> Nit: Adds IDL. It's not like IDL is a feature to support.x

Fixed.

>> Source/WebCore/ChangeLog:11
>> +
> 
> Please add a link to the spec.

Added.

>> Source/WebCore/ChangeLog:18
>> +        * Modules/clipboard/Clipboard.cpp:
> 
> Can we use Modules/async-clipboard/ instead?
> It's a bit confusing as is since we already have the support for sync clipboard API support.

Sure! Renamed to async-clipboard. I also renamed the tests in the editing directory from clipboard to async-clipboard to match.

>> Source/WebCore/Modules/clipboard/Clipboard.h:58
>> +    void derefEventTarget() final { deref(); }
> 
> Why do we need these??

Clipboard is an EventTarget, and these are pure virtual methods on EventTarget. It looks like all other event target subclasses override refEventTarget() and derefEventTarget(), as well.

>> Source/WebCore/Modules/clipboard/Clipboard.idl:32
>> +] interface Clipboard : EventTarget {
> 
> Probably need GenerateIsReachable=ReachableFromDOMWindow or something.
> Otherwise, the wrapper of this object can be collected.

Good catch! I added `GenerateIsReachable=ReachableFromNavigator` here to ensure that the wrapper object is kept alive by the Navigator, and then added a new layout test to verify that the wrapper is kept alive between GC passes.

>> Source/WebCore/Modules/clipboard/ClipboardItem.idl:40
>> +] interface ClipboardItem {
> 
> We need to start thinking about GC model of this object.
> You'd probably need to make this item's Clipboard an opaque root if it's associated with one.
> Alternatively, add GenerateIsReachable=ReachableFromDOMWindow
> and make it return DOMWindow* when it's associated with Clipboard.

Good call. As we chatted about in person, I’ll make this reachable from navigator, with the intention of returning the Clipboard’s navigator if the item is associated with a Clipboard (and otherwise, return null).

Since all APIs are stubs at the moment and there’s no way to get items associated with a Clipboard yet, this will just return null for now, until the subsequent patch that adds support for Clipboard.read().

>> Source/WebCore/Modules/clipboard/NavigatorClipboard.h:43
>> +    Clipboard* clipboard();
> 
> Return RefPtr instead?

Done!

>> LayoutTests/platform/ios-wk1/TestExpectations:19
>> +
> 
> Can we add the support in a subsequent patch? It's not great to have discrepancy between WK1 and WK2 if we can avoid it.

Okay — I filed https://bugs.webkit.org/show_bug.cgi?id=202654 to track this effort, and referenced the bug in the test expectations.

[Wenson Hsieh]

Created attachment 380374 [details]
Incorporate review feedback + rebase on trunk

[Wenson Hsieh]

Created attachment 380376 [details]
Fix WPE build

[Wenson Hsieh]

Created attachment 380378 [details]
Try to fix GTK/WPE build again

[Wenson Hsieh]

Created attachment 380386 [details]
Try to fix GTK/WPE build (3)

[Wenson Hsieh]

Created attachment 380401 [details]
Rebase on trunk again

[WebKit Commit Bot]

Comment on attachment 380401 [details]
Rebase on trunk again

Clearing flags on attachment: 380401

Committed r250824: <https://trac.webkit.org/changeset/250824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.