Initial stub version of Async Clipboard API

Initial version of Async Clipboard API

Includes basic support for plaintext read/write.

See https://w3c.github.io/clipboard-apis/

BUG=677564
Review-Url: https://codereview.chromium.org/2695593006
Cr-Commit-Position: refs/heads/master@{#485121}
Committed: https://chromium.googlesource.com/chromium/src/+/ac319a7ed300e2d8b7de3e6dcdfce2acf3363122

[garykac, 2017-02-15 00:44:22]

Description was changed from

==========
Initial stub version of Async Clipboard API

See https://w3c.github.io/clipboard-apis/

BUG=677564
==========

to

==========
Initial stub version of Async Clipboard API

Only supports plaintext as a proof-of-concept. I'd like to get feedback on the
overall structure before going further.

Also, I'd like to add tests before going any further, but I'm not sure how those
should be set up.  Can you point me to docs or an example cl that shows how best
to test this?

See https://w3c.github.io/clipboard-apis/

BUG=677564
==========

[garykac, 2017-02-15 00:44:22]

garykac@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2017-02-21 11:07:22]

https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
has some suggestions for how to create a new top-level directory in
web-platform-tests. Upstream first or as part of this CL, either is OK. Since
it'll probably include an idlharness.js test and not just be an empty directory,
maybe as part of this CL is better.

As for the code itself, is there someone you're working with that would be a
better reviewer. I'll skim the IDL files a bit though.

[foolip, 2017-02-21 11:13:18]

When it comes to the tests, it turns out there's already an empty
https://github.com/w3c/web-platform-tests/tree/master/clipboard that you can
start adding to. Can you also add yourself to the OWNERS file in
web-platform-tests so that you notice changes from other vendors?

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/clipboard/Clipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:5: [SecureContext]
Can you include a link to
https://w3c.github.io/clipboard-apis/#clipboard-interface above here?

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:7: [CallWith=ScriptState]
Promise read();
Can you include the <DataTransfer> and similar here too? It doesn't change the
generated code at all I think, but would show up as a difference between spec
and IDL that someone would have to fix later.

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:8: [CallWith=ScriptState]
Promise readText();
This and writeText() isn't in the spec, should it be?

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/NavigatorClipboard.idl:9:
[CallWith=ScriptState, SecureContext] readonly attribute Clipboard clipboard;
Can you also add [SameObject] here and in the spec?

[garykac, 2017-05-12 21:15:00]

garykac@chromium.org changed reviewers:
+ dcheng@chromium.org

[garykac, 2017-05-12 21:16:39]

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/clipboard/Clipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:5: [SecureContext]
On 2017/02/21 11:13:18, foolip_UTC9_OOO wrote:
> Can you include a link to
> https://w3c.github.io/clipboard-apis/#clipboard-interface above here?

Done.

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:7: [CallWith=ScriptState]
Promise read();
On 2017/02/21 11:13:18, foolip_UTC9_OOO wrote:
> Can you include the <DataTransfer> and similar here too? It doesn't change the
> generated code at all I think, but would show up as a difference between spec
> and IDL that someone would have to fix later.

Done.

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/clipboard/Clipboard.idl:8: [CallWith=ScriptState]
Promise readText();
On 2017/02/21 11:13:18, foolip wrote:
> This and writeText() isn't in the spec, should it be?

Done.

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/NavigatorClipboard.idl:9:
[CallWith=ScriptState, SecureContext] readonly attribute Clipboard clipboard;
On 2017/02/21 11:13:18, foolip wrote:
> Can you also add [SameObject] here and in the spec?

Done.

[dcheng, 2017-05-13 22:18:32]

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:25:
m_buffer(WebClipboard::kBufferStandard) {}
Out of curiosity, do you see this being used for non-clipboard purposes in the
future as well?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:103:
m_scriptPromiseResolver.Clear();
As far as I can tell, we don't check that the m_scriptPromiseResolver is
non-null before using it in the handle* callbacks; will this crash if the
context is detached?

Also, I'm wondering if we need to do this at all: promises should already
observe the context and detach themselves if needed; is this to try to avoid
unnecessary work?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.h (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:18: class CORE_EXPORT
ClipboardAsync final
My main question about this is stylistic: this seems like helpers for
implementing Clipboard -- any reason to expose this publicly? Or perhaps name
this like ClipboardPromise, and instead of having schedule* methods, change them
to createForRead(), createForReadText(), etc?

I think it's easy for a reader to get the wrong impression that there's one
ClipboardAsync object, and you can call it multiple times with scheduleFoo()
methods.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:27: static
ClipboardAsync* create(ScriptState*);
Nit: to follow the updated style guide, these shoulw all be UpperCased, since
they aren't being consumed by the IDL.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:53:
WebClipboard::Buffer m_buffer;
Similarly, this should be buffer_

[foolip, 2017-05-15 15:13:59]

On 2017/02/21 11:07:22, foolip wrote:
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> has some suggestions for how to create a new top-level directory in
> web-platform-tests. Upstream first or as part of this CL, either is OK. Since
> it'll probably include an idlharness.js test and not just be an empty
directory,
> maybe as part of this CL is better.
> 
> As for the code itself, is there someone you're working with that would be a
> better reviewer. I'll skim the IDL files a bit though.

Can you add an entry for clipboard/ in W3CImportExpectations? The tests won't
get deleted if you don't (that file lists directories to skip) but it will look
like the tests have no Blink-side owner.

[foolip, 2017-05-15 15:33:28]

I've reviewed the tests and the IDL files. dcheng@, can you review everything
else?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:3:
<title>Async Clipboard basic tests</title>
Can you put this test under wpt/clipboard-apis/, to match the shortname of
https://w3c.github.io/clipboard-apis/?

Also, can you name it navigator-clipboard-basics.html or something similar to
avoid the number in the name? It's not uncommon in wpt, but also not very
helpful unless tests for the same API grow so big that they must be split to run
fast enough.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:8:
assert_not_equals(navigator.clipboard, undefined);
Can you also assert_true(navigator.clipboard instanceof Clipboard) and
assert_equals(navigator.clipboard, navigator.clipboard)?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:9: },
"Verify navigator.clipboard exists");
Nit: the leading "Verify " of all descriptions can be removed.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:15: },
"Verify navigator.clipboard.write(DataTransfer) succeeds");
Can you add a test for things that should not succeed, in particular
navigator.clipboard.write(null)? and navigator.clipboard.write("string")?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:22: return
navigator.clipboard.read();
Also assert some things about the value with which the promise resolves. It
should be a DataTransfer instance, etc.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:26: return
navigator.clipboard.readText();
Ditto.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:13:
cb.write(dt).then(function() {
The spec says "Handle access permissions. Reject promise if not allowed". Does
that mean that a user gesture check is planned?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:15:
t.step(function(){assert_equals(data.items.length, 1);});
None of the explicit t.step should be required. Instead, wrap each callback
function in t.step_func, e.g.:

cb.write(dt).then(t.step_func(() => {
  cb.read().then(t.step_func(data => { .... }));
});

It could well be that using promise_test simplifies the test by avoiding the
t.ste_func bits, but async_test is fine as long as the indentation doesn't get
out of hand.

With those changes, you should be able to write a bare
assert_equals(data.items.length, 1) here.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:18:
t.done();
This'll be the final and only t.done(). It doesn't matter, but when you prefer
you can combine it with the last step with t.step_func_done(...) and still have
asserts in the dot-dot-dot bit.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:21:
t.step(function(){assert_false(true, "Read fail");});
If you stick with async_test you can use t.unreached_func("Read fail") here, and
with promise_test a promise rejection is automatically a test failure.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:30:
Note: This is a manual test because it writes/reads to the shared system
Can you file bugs on web-platform-tests with type:untestable that describes what
testing infrastructure would be needed to test this in an automated fashion
across browsers?

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-text-manual.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-text-manual.html:11:
cb.readText().then(function(data) {
Can you also test what happens with writeText+read and write+readText?

Comments about t.step_func etc. also apply here.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.idl:9:
[CallWith=ScriptState, SecureContext, SameObject]
Given that you have SecureContext here, it should be necessary to name the wpt
tests foo.https.html. If the pass anyway, something is strange.

[foolip, 2017-05-15 15:42:08]

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:8:
assert_not_equals(navigator.clipboard, undefined);
On 2017/05/15 15:33:27, foolip wrote:
> Can you also assert_true(navigator.clipboard instanceof Clipboard) and
> assert_equals(navigator.clipboard, navigator.clipboard)?

Actually, the existing line could be turned into an idlharness.js test. Can you
create interfaces/clipboard-apis.idl and write an idlharness.js test?
https://github.com/w3c/web-platform-tests/pull/5903 is one I did recently.

[garykac, 2017-05-17 23:51:15]

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:3:
<title>Async Clipboard basic tests</title>
On 2017/05/15 15:33:27, foolip wrote:
> Can you put this test under wpt/clipboard-apis/, to match the shortname of
> https://w3c.github.io/clipboard-apis/
> 
> Also, can you name it navigator-clipboard-basics.html or something similar to
> avoid the number in the name? It's not uncommon in wpt, but also not very
> helpful unless tests for the same API grow so big that they must be split to
run
> fast enough.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:8:
assert_not_equals(navigator.clipboard, undefined);
On 2017/05/15 15:33:27, foolip wrote:
> Can you also assert_true(navigator.clipboard instanceof Clipboard) and
> assert_equals(navigator.clipboard, navigator.clipboard)?

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:8:
assert_not_equals(navigator.clipboard, undefined);
On 2017/05/15 15:42:08, foolip wrote:
> On 2017/05/15 15:33:27, foolip wrote:
> > Can you also assert_true(navigator.clipboard instanceof Clipboard) and
> > assert_equals(navigator.clipboard, navigator.clipboard)?
> 
> Actually, the existing line could be turned into an idlharness.js test. Can
you
> create interfaces/clipboard-apis.idl and write an idlharness.js test?
> https://github.com/w3c/web-platform-tests/pull/5903 is one I did recently.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:9: },
"Verify navigator.clipboard exists");
On 2017/05/15 15:33:27, foolip wrote:
> Nit: the leading "Verify " of all descriptions can be removed.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:15: },
"Verify navigator.clipboard.write(DataTransfer) succeeds");
On 2017/05/15 15:33:27, foolip wrote:
> Can you add a test for things that should not succeed, in particular
> navigator.clipboard.write(null)? and navigator.clipboard.write("string")?

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:22: return
navigator.clipboard.read();
On 2017/05/15 15:33:27, foolip wrote:
> Also assert some things about the value with which the promise resolves. It
> should be a DataTransfer instance, etc.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-001.html:26: return
navigator.clipboard.readText();
On 2017/05/15 15:33:26, foolip wrote:
> Ditto.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:13:
cb.write(dt).then(function() {
On 2017/05/15 15:33:27, foolip wrote:
> The spec says "Handle access permissions. Reject promise if not allowed". Does
> that mean that a user gesture check is planned?

We have a clipboard Permission[1] that is not yet implemented

[1] https://w3c.github.io/permissions/#clipboard

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:15:
t.step(function(){assert_equals(data.items.length, 1);});
On 2017/05/15 15:33:27, foolip wrote:
> None of the explicit t.step should be required. Instead, wrap each callback
> function in t.step_func, e.g.:
> 
> cb.write(dt).then(t.step_func(() => {
>   cb.read().then(t.step_func(data => { .... }));
> });
> 
> It could well be that using promise_test simplifies the test by avoiding the
> t.ste_func bits, but async_test is fine as long as the indentation doesn't get
> out of hand.
> 
> With those changes, you should be able to write a bare
> assert_equals(data.items.length, 1) here.

Done. Thanks! That looks nicer.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:18:
t.done();
On 2017/05/15 15:33:27, foolip wrote:
> This'll be the final and only t.done(). It doesn't matter, but when you prefer
> you can combine it with the last step with t.step_func_done(...) and still
have
> asserts in the dot-dot-dot bit.

Acknowledged.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:21:
t.step(function(){assert_false(true, "Read fail");});
On 2017/05/15 15:33:27, foolip wrote:
> If you stick with async_test you can use t.unreached_func("Read fail") here,
and
> with promise_test a promise rejection is automatically a test failure.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-dt-text-manual.html:30:
Note: This is a manual test because it writes/reads to the shared system
On 2017/05/15 15:33:27, foolip wrote:
> Can you file bugs on web-platform-tests with type:untestable that describes
what
> testing infrastructure would be needed to test this in an automated fashion
> across browsers?

Filed: https://github.com/w3c/web-platform-tests/issues/5970

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-text-manual.html
(right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard/async-rw-text-manual.html:11:
cb.readText().then(function(data) {
On 2017/05/15 15:33:27, foolip wrote:
> Can you also test what happens with writeText+read and write+readText?
> 
> Comments about t.step_func etc. also apply here.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:25:
m_buffer(WebClipboard::kBufferStandard) {}
On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> Out of curiosity, do you see this being used for non-clipboard purposes in the
> future as well?

Probably not. So I removed CORE_EXPORT from the .h file.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:103:
m_scriptPromiseResolver.Clear();
On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> As far as I can tell, we don't check that the m_scriptPromiseResolver is
> non-null before using it in the handle* callbacks; will this crash if the
> context is detached?

AIUI, the resolver sticks around until garbage collected and you can use
GetExecutionContext if you need to verify the context.

Resolving/rejecting the promise doesn't care about the ExecutionContext.

> Also, I'm wondering if we need to do this at all: promises should already
> observe the context and detach themselves if needed; is this to try to avoid
> unnecessary work?

This is the pattern found elsewhere in the code with ScriptPromiseResolvers. For
example:
* WebKit/Source/modules/mediastream/MediaDevicesRequest.cpp
* WebKit/Source/modules/payments/PaymentRequest.cpp

I don't know enough about the plumbing here to know if it is unnecessary, but I
assume that it's being done for a good reason.

But I'm happy to remove it if it's not needed.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.h (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:18: class CORE_EXPORT
ClipboardAsync final
On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> My main question about this is stylistic: this seems like helpers for
> implementing Clipboard -- any reason to expose this publicly? Or perhaps name
> this like ClipboardPromise, and instead of having schedule* methods, change
them
> to createForRead(), createForReadText(), etc?
> 
> I think it's easy for a reader to get the wrong impression that there's one
> ClipboardAsync object, and you can call it multiple times with scheduleFoo()
> methods.

Re: The name. That's a good point that I hadn't considered. Renamed to
ClipboardPromise and changed the create*() methods.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:27: static
ClipboardAsync* create(ScriptState*);
On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> Nit: to follow the updated style guide, these shoulw all be UpperCased, since
> they aren't being consumed by the IDL.

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.h:53:
WebClipboard::Buffer m_buffer;
On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> Similarly, this should be buffer_

Done.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.idl (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.idl:9:
[CallWith=ScriptState, SecureContext, SameObject]
On 2017/05/15 15:33:27, foolip wrote:
> Given that you have SecureContext here, it should be necessary to name the wpt
> tests foo.https.html. If the pass anyway, something is strange.

Interesting. I was running the tests from the w3c/github repo by running ./serve
and navigating (in my local dev build) to http://localhost:8000/clipboard/.

And they all passed.

I just now ran them using:
  blink/tools/run_layout_tests.sh external/wpt/clipboard
and I see the expected failures ("disallowed" or "undefined").

Renaming the files to .https.html fixes the failures.

Using blink/tools/run_layout_tests.sh also ran all the manual tests. Is there a
clever naming convention to prevent that? Adding -manual doesn't seem to do it
(although it does work when running the github version).

OTOH, the manual tests succeeded in blink when I ran them locally, but I don't
know if that'll be true on the bots (it won't be if they run in parallel).

[garykac, 2017-05-17 23:52:10]

Description was changed from

==========
Initial stub version of Async Clipboard API

Only supports plaintext as a proof-of-concept. I'd like to get feedback on the
overall structure before going further.

Also, I'd like to add tests before going any further, but I'm not sure how those
should be set up.  Can you point me to docs or an example cl that shows how best
to test this?

See https://w3c.github.io/clipboard-apis/

BUG=677564
==========

to

==========
Initial version of Async Clipboard API

Includes basic support for plaintext read/write.

See https://w3c.github.io/clipboard-apis/

BUG=677564
==========

[foolip, 2017-05-24 11:47:13]

I'll be OOO for a while, if necessary please ask another reviewer.

[garykac, 2017-06-06 18:32:58]

Ping?

[dcheng, 2017-06-06 23:33:38]

Sorry I some drafts but forgot to publish them.

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:103:
m_scriptPromiseResolver.Clear();
On 2017/05/17 23:51:14, garykac wrote:
> On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> > As far as I can tell, we don't check that the m_scriptPromiseResolver is
> > non-null before using it in the handle* callbacks; will this crash if the
> > context is detached?
> 
> AIUI, the resolver sticks around until garbage collected and you can use
> GetExecutionContext if you need to verify the context.

Right--but if we call clear() here, Member<ScriptPromiseResolver> gets set to
null. So line 99 would crash if it executed on a detached context, right?

> 
> Resolving/rejecting the promise doesn't care about the ExecutionContext.
> 
> > Also, I'm wondering if we need to do this at all: promises should already
> > observe the context and detach themselves if needed; is this to try to avoid
> > unnecessary work?
> 
> This is the pattern found elsewhere in the code with ScriptPromiseResolvers.
For
> example:
> * WebKit/Source/modules/mediastream/MediaDevicesRequest.cpp
> * WebKit/Source/modules/payments/PaymentRequest.cpp
> 
> I don't know enough about the plumbing here to know if it is unnecessary, but
I
> assume that it's being done for a good reason.
> 
> But I'm happy to remove it if it's not needed.

Reading the code suggests that trying to resolve or reject a promise for a
detached context will automatically become a no-op already:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard-apis/async-write-dttext-read-dttext-manual.https.html
(right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard-apis/async-write-dttext-read-dttext-manual.https.html:28:
Note: This is a manual test because it writes/reads to the shared system
I'm not sure this statement is true in Chrome, fwiw (based on our discussion)

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/Clipboard.h (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/Clipboard.h:35: //
ContextLifecycleObserver
Nit: remove this comment, the requirement to override it actually comes from
EventTarget

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:18: ScriptPromise
ClipboardPromise::CreateForRead(ScriptState* scriptState) {
Nit: script_state, etc since we know use Chromium/Google style

(Same for Clipboard.cpp and NavigatorClipboard.cpp)

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:93:
objectItem->GetType() == "text/plain") {
Nit: please use kMimeTypeTextPlain from ClipboardMimeTypes.h.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp:22: if
(!supplement->m_clipboard) {
Nit: let's just have NavigatorClipboard construct it internally rather than
creating it on demand here.

[garykac, 2017-06-09 20:39:00]

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp (right):

https://codereview.chromium.org/2695593006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardAsync.cpp:103:
m_scriptPromiseResolver.Clear();
On 2017/06/06 23:33:38, dcheng wrote:
> On 2017/05/17 23:51:14, garykac wrote:
> > On 2017/05/13 22:18:32, dcheng (in AEST) wrote:
> > > As far as I can tell, we don't check that the m_scriptPromiseResolver is
> > > non-null before using it in the handle* callbacks; will this crash if the
> > > context is detached?
> > 
> > AIUI, the resolver sticks around until garbage collected and you can use
> > GetExecutionContext if you need to verify the context.
> 
> Right--but if we call clear() here, Member<ScriptPromiseResolver> gets set to
> null. So line 99 would crash if it executed on a detached context, right?
> 
> > 
> > Resolving/rejecting the promise doesn't care about the ExecutionContext.
> > 
> > > Also, I'm wondering if we need to do this at all: promises should already
> > > observe the context and detach themselves if needed; is this to try to
avoid
> > > unnecessary work?
> > 
> > This is the pattern found elsewhere in the code with ScriptPromiseResolvers.
> For
> > example:
> > * WebKit/Source/modules/mediastream/MediaDevicesRequest.cpp
> > * WebKit/Source/modules/payments/PaymentRequest.cpp
> > 
> > I don't know enough about the plumbing here to know if it is unnecessary,
but
> I
> > assume that it's being done for a good reason.
> > 
> > But I'm happy to remove it if it's not needed.
> 
> Reading the code suggests that trying to resolve or reject a promise for a
> detached context will automatically become a no-op already:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

OK. I've removed the entire ContextDestroyed method since it doesn't appear to
be necessary.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/clipboard-apis/async-write-dttext-read-dttext-manual.https.html
(right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/clipboard-apis/async-write-dttext-read-dttext-manual.https.html:28:
Note: This is a manual test because it writes/reads to the shared system
On 2017/06/06 23:33:38, dcheng wrote:
> I'm not sure this statement is true in Chrome, fwiw (based on our discussion)

It's not true when Chrome runs its copy of the tests, but these tests live in
WPT where it is true.

This comment is to explain the motivation for the "-manual" suffix so that no
one removes it.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/Clipboard.h (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/Clipboard.h:35: //
ContextLifecycleObserver
On 2017/06/06 23:33:38, dcheng wrote:
> Nit: remove this comment, the requirement to override it actually comes from
> EventTarget

Done.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:18: ScriptPromise
ClipboardPromise::CreateForRead(ScriptState* scriptState) {
On 2017/06/06 23:33:38, dcheng wrote:
> Nit: script_state, etc since we know use Chromium/Google style
> 
> (Same for Clipboard.cpp and NavigatorClipboard.cpp)

Done.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:93:
objectItem->GetType() == "text/plain") {
On 2017/06/06 23:33:38, dcheng wrote:
> Nit: please use kMimeTypeTextPlain from ClipboardMimeTypes.h.

Done.

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp (right):

https://codereview.chromium.org/2695593006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp:22: if
(!supplement->m_clipboard) {
On 2017/06/06 23:33:38, dcheng wrote:
> Nit: let's just have NavigatorClipboard construct it internally rather than
> creating it on demand here.

Done.

[garykac, 2017-06-14 17:54:18]

ping.

[dcheng, 2017-06-14 18:56:22]

LGTM with comments addressed.

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp (right):

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:69: String
plainText = Platform::Current()->Clipboard()->ReadPlainText(buffer_);
Nit: plain_text

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:73:
DataTransferAccessPolicy access =
Nit: the compiler should be able to optimize this, but considering hinting it by
marking these as const here.

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp (right):

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp:35: : nullptr);
Btw, can we combine Clipboard and NavigatorClipboard? It doesn't seem like other
supplements (such as NavigatorPlugins) split it into two.

[garykac, 2017-06-29 01:37:26]

foolip@, could you take a look at RuntimeEnabledFeatures.json5?

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp (right):

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:69: String
plainText = Platform::Current()->Clipboard()->ReadPlainText(buffer_);
On 2017/06/14 18:56:22, dcheng wrote:
> Nit: plain_text

Done.

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/clipboard/ClipboardPromise.cpp:73:
DataTransferAccessPolicy access =
On 2017/06/14 18:56:22, dcheng wrote:
> Nit: the compiler should be able to optimize this, but considering hinting it
by
> marking these as const here.

Done.

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp (right):

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp:35: : nullptr);
On 2017/06/14 18:56:22, dcheng (OOO Jun 25 - Jul 1) wrote:
> Btw, can we combine Clipboard and NavigatorClipboard? It doesn't seem like
other
> supplements (such as NavigatorPlugins) split it into two.

We have 2 separate IDL files (Navigator supplement and the actual clipboard).
While we could combine them, I believe that it is cleaner to leave them
separate.

Let's follow up on this when you get back from vacation and I can do a follow-up
cl if necessary.

[dcheng1, 2017-06-29 05:08:16]

dcheng@google.com changed reviewers:
+ dcheng@google.com

[dcheng1, 2017-06-29 05:08:17]

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp (right):

https://codereview.chromium.org/2695593006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/NavigatorClipboard.cpp:35: : nullptr);
On 2017/06/29 01:37:26, garykac wrote:
> On 2017/06/14 18:56:22, dcheng (OOO Jun 25 - Jul 1) wrote:
> > Btw, can we combine Clipboard and NavigatorClipboard? It doesn't seem like
> other
> > supplements (such as NavigatorPlugins) split it into two.
> 
> We have 2 separate IDL files (Navigator supplement and the actual clipboard).
> While we could combine them, I believe that it is cleaner to leave them
> separate.
> 
> Let's follow up on this when you get back from vacation and I can do a
follow-up
> cl if necessary.

OK, that's a good reason =)

[foolip, 2017-06-30 11:11:33]

RuntimeEnabledFeatures.json5 lgtm

[garykac, 2017-06-30 20:41:27]

The CQ bit was checked by garykac@chromium.org

[garykac, 2017-06-30 20:41:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593006/#ps100001
(title: "Fix names (review comments)")

[commit-bot: I haz the power, 2017-06-30 20:41:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 21:37:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-30 21:37:13]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[garykac, 2017-07-06 00:43:42]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2017-07-06 00:44:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 00:47:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 00:47:04]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[garykac, 2017-07-06 21:20:10]

The CQ bit was checked by garykac@chromium.org

[garykac, 2017-07-06 21:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593006/#ps120001
(title: "Sync/merge")

[commit-bot: I haz the power, 2017-07-06 21:20:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 22:23:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 22:23:54]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dcheng, 2017-07-06 23:42:18]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-07-06 23:42:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 01:25:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 01:25:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[garykac, 2017-07-07 17:56:59]

The CQ bit was checked by garykac@chromium.org

[garykac, 2017-07-07 17:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593006/#ps140001
(title: "Update Blink layout tests")

[commit-bot: I haz the power, 2017-07-07 17:57:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 20:01:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 20:01:16]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[garykac, 2017-07-07 20:20:29]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2017-07-07 20:21:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 22:31:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 22:31:32]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[garykac, 2017-07-07 22:50:50]

The CQ bit was checked by garykac@chromium.org

[garykac, 2017-07-07 22:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2695593006/#ps160001
(title: "Expected layout tests results for mac")

[commit-bot: I haz the power, 2017-07-07 22:51:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-08 00:53:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499467850439810,
"parent_rev": "cbd458915f1a65c631f3a8e09e9f625ed935b927", "commit_rev":
"ac319a7ed300e2d8b7de3e6dcdfce2acf3363122"}

[commit-bot: I haz the power, 2017-07-08 00:53:32]

Description was changed from

==========
Initial version of Async Clipboard API

Includes basic support for plaintext read/write.

See https://w3c.github.io/clipboard-apis/

BUG=677564
==========

to

==========
Initial version of Async Clipboard API

Includes basic support for plaintext read/write.

See https://w3c.github.io/clipboard-apis/

BUG=677564

Review-Url: https://codereview.chromium.org/2695593006
Cr-Commit-Position: refs/heads/master@{#485121}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac319a7ed300e2d8b7de3e6dcdfc...
==========

[commit-bot: I haz the power, 2017-07-08 00:53:34]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ac319a7ed300e2d8b7de3e6dcdfc...