Readd the clipboard service and get it compiling again.

Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460
Committed: https://crrev.com/703e3fa71990ce557d52eda86e720f18bda81140
Cr-Commit-Position: refs/heads/master@{#394931}

[Elliot Glaysher, 2016-05-13 22:54:18]

Description was changed from

==========
Readd the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified.

BUG=
==========

to

==========
Readd the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and does the minimal changes to make it compile again.
(Mainly: dealing with the //services/ move and the switch away from
apptests.)

BUG=581460
==========

[Elliot Glaysher, 2016-05-13 22:54:18]

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

[Elliot Glaysher, 2016-05-13 22:54:26]

Description was changed from

==========
Readd the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and does the minimal changes to make it compile again.
(Mainly: dealing with the //services/ move and the switch away from
apptests.)

BUG=581460
==========

to

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and does the minimal changes to make it compile again.
(Mainly: dealing with the //services/ move and the switch away from
apptests.)

BUG=581460
==========

[dcheng, 2016-05-13 23:17:51]

https://codereview.chromium.org/1978763003/diff/20001/components/clipboard/pu...
File components/clipboard/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/1978763003/diff/20001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
This needs a security OWNERS file.

[Elliot Glaysher, 2016-05-13 23:34:50]

https://codereview.chromium.org/1978763003/diff/20001/components/clipboard/pu...
File components/clipboard/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/1978763003/diff/20001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/05/13 23:17:50, dcheng wrote:
> This needs a security OWNERS file.

Done.

[sky, 2016-05-14 00:01:12]

https://codereview.chromium.org/1978763003/diff/40001/components/clipboard/pu...
File components/clipboard/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/1978763003/diff/40001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1978763003/diff/40001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:7: interface Clipboard {
It seems like only code that has UI should be interacting with the clipboard. I
tend to think this should be part of mus.

https://codereview.chromium.org/1978763003/diff/40001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:29:
GetAvailableMimeTypes(Type clipboard_types) => (array<string> types);
I would expect the read functions to take a sequence number. Otherwise how does
the caller know what sequence the data corresponds to?

https://codereview.chromium.org/1978763003/diff/40001/components/clipboard/pu...
components/clipboard/public/interfaces/clipboard.mojom:42:
WriteClipboardData(Type clipboard_type, map<string, array<uint8>>? data);
I think this should callback with the actual sequence number, otherwise the
client won't necessarily know what number if got assigned.

[Elliot Glaysher, 2016-05-18 22:25:22]

Description was changed from

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and does the minimal changes to make it compile again.
(Mainly: dealing with the //services/ move and the switch away from
apptests.)

BUG=581460
==========

to

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460
==========

[Elliot Glaysher, 2016-05-18 22:25:58]

Redid patch from a sort of revert into a minimal integration with mus, along
with your requested interface changes.

[sky, 2016-05-19 15:33:34]

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

[sky, 2016-05-19 15:33:46]

+dcheng for mojom security review

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
File components/mus/clipboard/clipboard_impl.h (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
components/mus/clipboard/clipboard_impl.h:54: uint64_t
sequence_number_[kNumClipboards];
Did you consider putting the sequence number in the clipboard state? Seems like
the sequence number is inherently associated with the clipboard state, so that
it makes sense to be part of clipboard state.

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
File components/mus/clipboard/clipboard_unittest.cc (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
components/mus/clipboard/clipboard_unittest.cc:31: class ClipboardAppTest :
public shell::test::ShellTest {
If these tests are going to end up writing to the real system clipboard they
will need to be in interactive ui tests.

https://codereview.chromium.org/1978763003/diff/160001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:24: [Sync]
Your earlier patch didn't have these sync. Why the change?

[dcheng, 2016-05-19 17:42:53]

It looks like this doesn't interact at all with the system clipboard at all atm.
Is that the long-term plan?

[Elliot Glaysher, 2016-05-19 18:22:00]

On 2016/05/19 17:42:53, dcheng wrote:
> It looks like this doesn't interact at all with the system clipboard at all
atm.
> Is that the long-term plan?

(Terse because I lost a wall of text explaining this to backspace as navigate.)

Long term. Currently, we're porting ash onto mus for chromeos. We need a
multiprocess clipboard interface because different parts of chrome run in
different processes now. Ash's internal clipboard was the system clipboard on
chromeos (and testing linux implementations); never interacting with X11/etc.
Next year, when we start porting desktop chrome to mus, we'll need to have an
implementation of mus.mojom.Clipboard which speaks to the native clipboard.

[Elliot Glaysher, 2016-05-19 18:26:48]

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
File components/mus/clipboard/clipboard_impl.h (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
components/mus/clipboard/clipboard_impl.h:54: uint64_t
sequence_number_[kNumClipboards];
On 2016/05/19 15:33:46, sky wrote:
> Did you consider putting the sequence number in the clipboard state? Seems
like
> the sequence number is inherently associated with the clipboard state, so that
> it makes sense to be part of clipboard state.

Done.

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
File components/mus/clipboard/clipboard_unittest.cc (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/clipboa...
components/mus/clipboard/clipboard_unittest.cc:31: class ClipboardAppTest :
public shell::test::ShellTest {
On 2016/05/19 15:33:46, sky wrote:
> If these tests are going to end up writing to the real system clipboard they
> will need to be in interactive ui tests.

Noted, but I don't think this is going to happen this year.

https://codereview.chromium.org/1978763003/diff/160001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/1978763003/diff/160001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:24: [Sync]
On 2016/05/19 15:33:46, sky wrote:
> Your earlier patch didn't have these sync. Why the change?

The previous patch manually spun up RunLoops and waited on them. I thought the
new testing hotness was to make your methods sync and then not spin up nested
message loops or abuse WaitForIncomingMessage().

[sky, 2016-05-19 19:37:47]

LGTM

[dcheng, 2016-05-19 20:02:17]

mojom lgtm

[Elliot Glaysher, 2016-05-19 20:04:35]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-05-19 20:05:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978763003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978763003/180001

[commit-bot: I haz the power, 2016-05-19 22:12:14]

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

[commit-bot: I haz the power, 2016-05-19 22:12:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2016-05-19 22:14:46]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-05-19 22:15:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978763003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978763003/180001

[commit-bot: I haz the power, 2016-05-20 00:38:09]

Description was changed from

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460
==========

to

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460
==========

[commit-bot: I haz the power, 2016-05-20 00:38:10]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-20 00:40:17]

Description was changed from

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460
==========

to

==========
Revive the clipboard service and get it compiling again.

I want to have a base to start on for the clipboard stuff in chrome,
even if it needs to be heavily modified. This patch takes the old
code, and integrates it as a service in the mus app.

BUG=581460

Committed: https://crrev.com/703e3fa71990ce557d52eda86e720f18bda81140
Cr-Commit-Position: refs/heads/master@{#394931}
==========

[commit-bot: I haz the power, 2016-05-20 00:40:19]

Patchset 10 (id:??) landed as
https://crrev.com/703e3fa71990ce557d52eda86e720f18bda81140
Cr-Commit-Position: refs/heads/master@{#394931}