[webkit-reviews] review granted: [Bug 202622] [Clipboard API] Introduce bindings for the async Clipboard API : [Attachment 380339] Adjust WK1 test expectations.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 202622: [Clipboard API] Introduce bindings for the async Clipboard API
https://bugs.webkit.org/show_bug.cgi?id=202622

Attachment 380339: Adjust WK1 test expectations.

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 380339
  --> https://bugs.webkit.org/attachment.cgi?id=380339
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.