mojo: Create a basic clipboard.

mojo: Create a basic clipboard.

This creates a basic clipboard interface and uses it from
html_viewer. This is a minimal implementation and does not actually
interact with the system clipboard.

BUG=411039
First Committed: https://crrev.com/757286d8a2c778fe4622890140c9b9d2afd21063

Committed: https://crrev.com/b69fbca034e3c2181e0a12cf02baec4de526bf25
Cr-Commit-Position: refs/heads/master@{#295182}

[Elliot Glaysher, 2014-09-11 19:10:03]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2014-09-11 19:10:04]

Finally ready for review

[sky, 2014-09-11 19:52:38]

sky@chromium.org changed reviewers:
+ darin@chromium.org

[sky, 2014-09-11 19:52:39]

+darin as hew knows the blink side

https://codereview.chromium.org/562483002/diff/320001/mojo/mojo_services.gypi
File mojo/mojo_services.gypi (right):

https://codereview.chromium.org/562483002/diff/320001/mojo/mojo_services.gypi...
mojo/mojo_services.gypi:48: '../skia/skia.gyp:skia',
skia? Is skia really necessary? You don't have it listed in the gn file.

https://codereview.chromium.org/562483002/diff/320001/mojo/services/clipboard...
File mojo/services/clipboard/clipboard_standalone_impl.h (right):

https://codereview.chromium.org/562483002/diff/320001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.h:64: int
sequence_number_[kNumClipboards];
type should match that of mojom.

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
Does sequence really need to be uint64?
I'm not sure I get the value of the sequence number. It isn't checked any where.
What is it for?

https://codereview.chromium.org/562483002/diff/320001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/562483002/diff/320001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:44: "mojo_clipboard_unittests",
General flow is add to fyi bot first, let it back for a day and make sure it
isn't flakey then add it everywhere.

[dcheng, 2014-09-11 20:06:20]

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

[dcheng, 2014-09-11 20:06:22]

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
On 2014/09/11 19:52:39, sky wrote:
> Does sequence really need to be uint64?
> I'm not sure I get the value of the sequence number. It isn't checked any
where.
> What is it for?

This is used inside Blink to prevent a malicious script from repeatedly polling
event.clipboardData after the contents change.

[sky, 2014-09-11 20:15:15]

Does it need to be in mojo then? Could it be isolated in blink? If we
really need it then we need better documentation as to what it is.

On Thu, Sep 11, 2014 at 1:06 PM,  <dcheng@chromium.org> wrote:
>
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
> File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):
>
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
> mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
> GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
> On 2014/09/11 19:52:39, sky wrote:
>>
>> Does sequence really need to be uint64?
>> I'm not sure I get the value of the sequence number. It isn't checked
>
> any where.
>>
>> What is it for?
>
>
> This is used inside Blink to prevent a malicious script from repeatedly
> polling event.clipboardData after the contents change.
>
> https://codereview.chromium.org/562483002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-09-11 20:26:22]

On 2014/09/11 20:15:15, sky wrote:
> Does it need to be in mojo then? Could it be isolated in blink? If we
> really need it then we need better documentation as to what it is.
> 
> On Thu, Sep 11, 2014 at 1:06 PM,  <mailto:dcheng@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
> > File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):
> >
> >
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
> > mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
> > GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
> > On 2014/09/11 19:52:39, sky wrote:
> >>
> >> Does sequence really need to be uint64?
> >> I'm not sure I get the value of the sequence number. It isn't checked
> >
> > any where.
> >>
> >> What is it for?
> >
> >
> > This is used inside Blink to prevent a malicious script from repeatedly
> > polling event.clipboardData after the contents change.
> >
> > https://codereview.chromium.org/562483002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

It's tricky because we'd have to add a ClipboardObserver interface that Blink
could use to listen for clipboard changes. Unfortunately, this have to be
implemented using polling on Mac. As a result, I chose to implement this using
sequence numbers instead.

[sky, 2014-09-11 21:03:29]

The long term plan is to make many of these interfaces used by third
party developers. If we can avoid putting functionality specific to
blink into them, that would be great!

On Thu, Sep 11, 2014 at 1:26 PM,  <dcheng@chromium.org> wrote:
> On 2014/09/11 20:15:15, sky wrote:
>>
>> Does it need to be in mojo then? Could it be isolated in blink? If we
>> really need it then we need better documentation as to what it is.
>
>
>> On Thu, Sep 11, 2014 at 1:06 PM,  <mailto:dcheng@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
>>
>> > File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
>>
>> > mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
>> > GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
>> > On 2014/09/11 19:52:39, sky wrote:
>> >>
>> >> Does sequence really need to be uint64?
>> >> I'm not sure I get the value of the sequence number. It isn't checked
>> >
>> > any where.
>> >>
>> >> What is it for?
>> >
>> >
>> > This is used inside Blink to prevent a malicious script from repeatedly
>> > polling event.clipboardData after the contents change.
>> >
>> > https://codereview.chromium.org/562483002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> It's tricky because we'd have to add a ClipboardObserver interface that
> Blink
> could use to listen for clipboard changes. Unfortunately, this have to be
> implemented using polling on Mac. As a result, I chose to implement this
> using
> sequence numbers instead.
>
> https://codereview.chromium.org/562483002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2014-09-11 21:42:28]

ptal

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/320001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:19:
GetSequenceNumber(Type clipboard_type) => (uint64 sequence);
On 2014/09/11 20:06:21, dcheng (OOO) wrote:
> On 2014/09/11 19:52:39, sky wrote:
> > Does sequence really need to be uint64?
> > I'm not sure I get the value of the sequence number. It isn't checked any
> where.
> > What is it for?
> 
> This is used inside Blink to prevent a malicious script from repeatedly
polling
> event.clipboardData after the contents change.

I've added to this comment that sequence numbers are part of the clipboard
interface provided by all major operating systems, that they're monotonically
increasing, and that they increase on write.

[sky, 2014-09-11 22:11:43]

https://codereview.chromium.org/562483002/diff/340001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/340001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:20: // changes, and is
provided by all major operating systems (Win, Linux, Mac).
nit: no need to say provided by all major OSs here.

https://codereview.chromium.org/562483002/diff/340001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:28: ReadPlainText(Type
clipboard_type) => (string text);
Seems like the return type should be nullable. In so far as if plain text isn't
on the clipboard then shouldn't null be sent back? This applies to all other
ones.

https://codereview.chromium.org/562483002/diff/340001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:29: ReadHTML(Type
clipboard_type) =>
Can html and plain text be expressed as a mime type so that there is only one
read function?

https://codereview.chromium.org/562483002/diff/340001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:42:
QueueWritePlainText(Type clipboard_type, string text);
Two staged commit seems best done in the client. Could the interface be SetData
that takes a list of mime-type and data pairs?

[Elliot Glaysher, 2014-09-12 21:46:26]

I've redesigned the API around simple mime type key/value pairs, and have made
writing a one method interface.

[sky, 2014-09-15 15:28:20]

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:11: string data;
Is there a reason you want a string here rather than an array? There has been
talk of forcing string types to be valid utf8.

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:34:
GetAvailableFormatMimeTypes(Type clipboard_types) => (string[] types);
What does 'format' mean here? How about GetAvailableMimeTypes.

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:42: ReadMIMEType(Type
clipboard_type, string mime_type) => (string? data);
ReadMimeType (you use Mime above, not MIME).

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:46:
WriteClipboardData(Type clipboard_type, MimeTypePair[] data);
Is there ever a need to clear the clipboard? I'm specifically wondering if we
should allow data to be null?

[darin (slow to review), 2014-09-15 16:28:24]

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/562483002/diff/380001/mojo/services/public/in...
mojo/services/public/interfaces/clipboard/clipboard.mojom:28: // changes, and is
provided by Windoes, Linux, and Mac.
nit: Windoes -> Windows

Perhaps there should be a comment here indicating how this may be used?

[dcheng, 2014-09-15 21:37:20]

How do you plan on handling cases where there might be metadata associated with
a mime type? For example, URLs might have a title associated with them.

[Elliot Glaysher, 2014-09-15 21:58:33]

(sky, darin ptal)

On 2014/09/15 21:37:20, dcheng (OOO) wrote:
> How do you plan on handling cases where there might be metadata associated
with
> a mime type? For example, URLs might have a title associated with them.

I don't plan on handling metadata, so to speak.

I talked to Ben, and he told me to treat this as a redesign with no legacy
platform baggage since mojo is meant to be an API that we not only consume, but
ship to other people. The chrome clipboard implementation has a lot places where
we export something like WriteBookmark() and then have a platform specific
implementation of that. Instead, I assume we'd export a general interface, and
then do fixup in per-platform clipboard implementations. ie. standardize
internally on text/x-moz-url format (which describes the file format for a url
with a title), and convert during write objects on the one platform which
doesn't support that mime type.

Does that make sense?

[sky, 2014-09-15 23:16:52]

https://codereview.chromium.org/562483002/diff/460001/mojo/common/common_type...
File mojo/common/common_type_converters.cc (right):

https://codereview.chromium.org/562483002/diff/460001/mojo/common/common_type...
mojo/common/common_type_converters.cc:56: for (size_t i = 0; i < input.size();
++i)
Seems like there should be a better way to do these conversions. I'm hoping
Darin will suggest something.

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
File mojo/services/clipboard/clipboard_standalone_impl.cc (right):

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:16: virtual
~ClipboardData() {}
nit: no virtual.

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:46: // Storage for all
normal, one byte-string datatypes. Some datatypes also
This comment seems out of date.

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:73:
clipboard_state_[clipboard_type]->GetMimeTypes());
Do we guarantee enum values start at 0?

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:84:
callback.Run(mojo::Array<uint8_t>::From(mime_data).Pass());
Should this supply NULL so that the caller knows the type wasn't available?
Otherwise there is no way to distiguish empty and not available.

[Elliot Glaysher, 2014-09-15 23:30:15]

https://codereview.chromium.org/562483002/diff/460001/mojo/common/common_type...
File mojo/common/common_type_converters.cc (right):

https://codereview.chromium.org/562483002/diff/460001/mojo/common/common_type...
mojo/common/common_type_converters.cc:56: for (size_t i = 0; i < input.size();
++i)
On 2014/09/15 23:16:51, sky wrote:
> Seems like there should be a better way to do these conversions. I'm hoping
> Darin will suggest something.

I agree. But attempts at writing straight memcpys and using things like the
string(memory, size) ctors caused segmentation faults because I was apparently
accessing unallocated memory.

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
File mojo/services/clipboard/clipboard_standalone_impl.cc (right):

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:73:
clipboard_state_[clipboard_type]->GetMimeTypes());
On 2014/09/15 23:16:52, sky wrote:
> Do we guarantee enum values start at 0?

I believe we do, but just to make it explicit, I've hard coded the numbers in
the mojom file.

https://codereview.chromium.org/562483002/diff/460001/mojo/services/clipboard...
mojo/services/clipboard/clipboard_standalone_impl.cc:84:
callback.Run(mojo::Array<uint8_t>::From(mime_data).Pass());
On 2014/09/15 23:16:52, sky wrote:
> Should this supply NULL so that the caller knows the type wasn't available?
> Otherwise there is no way to distiguish empty and not available.

I thought we do supply NULL later on
("callback.Run(mojo::Array<uint8_t>().Pass());"), since empty arrays are NULL.

(Note: I don't like that decision since it makes things really confusing, but
that's what the interface is. Same with mojo::String.)

[sky, 2014-09-15 23:39:15]

LGTM - but wait for Darin. I'm hopeful there is a more efficient way to do the
memory copying you're after.

[dcheng, 2014-09-15 23:49:28]

On 2014/09/15 at 21:58:33, erg wrote:
> (sky, darin ptal)
> 
> On 2014/09/15 21:37:20, dcheng (OOO) wrote:
> > How do you plan on handling cases where there might be metadata associated
with
> > a mime type? For example, URLs might have a title associated with them.
> 
> I don't plan on handling metadata, so to speak.
> 
> I talked to Ben, and he told me to treat this as a redesign with no legacy
platform baggage since mojo is meant to be an API that we not only consume, but
ship to other people. The chrome clipboard implementation has a lot places where
we export something like WriteBookmark() and then have a platform specific
implementation of that. Instead, I assume we'd export a general interface, and
then do fixup in per-platform clipboard implementations. ie. standardize
internally on text/x-moz-url format (which describes the file format for a url
with a title), and convert during write objects on the one platform which
doesn't support that mime type.
> 
> Does that make sense?

If we can make it work, that sounds a lot better than the mess we currently
have.

[Elliot Glaysher, 2014-09-16 18:14:15]

Note: I'm CQing this now that I've resolved the issue with converters and
getting the memcpy() right.

[Elliot Glaysher, 2014-09-16 18:15:12]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-09-16 18:17:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562483002/520001

[commit-bot: I haz the power, 2014-09-16 18:36:06]

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

[commit-bot: I haz the power, 2014-09-16 18:36:08]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56639)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Elliot Glaysher, 2014-09-16 18:58:26]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-09-16 19:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562483002/540001

[commit-bot: I haz the power, 2014-09-16 21:27:44]

Committed patchset #29 (id:540001) as 3a04207e55635bea3a6895721e1f59479d69e99c

[commit-bot: I haz the power, 2014-09-16 21:28:29]

Patchset 29 (id:??) landed as
https://crrev.com/757286d8a2c778fe4622890140c9b9d2afd21063
Cr-Commit-Position: refs/heads/master@{#295143}

[Ken Rockot(use gerrit already), 2014-09-16 21:53:25]

A revert of this CL (patchset #29 id:540001) has been created in
https://codereview.chromium.org/579503003/ by rockot@chromium.org.

The reason for reverting is: Breaking GN builders.

http://build.chromium.org/p/chromium.linux/buildstatus?builder=Android%20GN&n...

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/12120/s...
.

[jamesr, 2014-09-16 22:13:05]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-09-16 22:13:06]

https://codereview.chromium.org/562483002/diff/540001/mojo/services/clipboard...
File mojo/services/clipboard/BUILD.gn (right):

https://codereview.chromium.org/562483002/diff/540001/mojo/services/clipboard...
mojo/services/clipboard/BUILD.gn:5: import("//mojo/system.gni")
delete this line

https://codereview.chromium.org/562483002/diff/540001/mojo/services/clipboard...
mojo/services/clipboard/BUILD.gn:13:
"//mojo/services/public/interfaces/clipboard",
add

  "//mojo/system:for_component",

https://codereview.chromium.org/562483002/diff/540001/mojo/services/clipboard...
mojo/services/clipboard/BUILD.gn:15: ] + mojo_system_for_component
delete this

[Elliot Glaysher, 2014-09-16 22:27:02]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-09-16 22:28:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562483002/580001

[commit-bot: I haz the power, 2014-09-17 00:09:06]

Committed patchset #31 (id:580001) as edd966581e929fbe72f2ff2f7a46d1c370a3d11e

[commit-bot: I haz the power, 2014-09-17 00:09:46]

Patchset 31 (id:??) landed as
https://crrev.com/b69fbca034e3c2181e0a12cf02baec4de526bf25
Cr-Commit-Position: refs/heads/master@{#295182}