arc-bridge: Move most methods to Mojo interfaces

arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests

Committed: https://crrev.com/3bdc0168bb4d055b647dd557d062432c9e835468
Cr-Commit-Position: refs/heads/master@{#366233}

[Luis Héctor Chávez, 2015-12-14 19:26:10]

Description was changed from

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
==========

to

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests
==========

[Luis Héctor Chávez, 2015-12-14 20:17:33]

lhchavez@chromium.org changed reviewers:
+ dcheng@chromium.org, hidehiko@chromium.org, reveman@chromium.org,
xiyuan@chromium.org

[Luis Héctor Chávez, 2015-12-14 20:17:33]

PTAL

reveman: components/arc/input
hidehiko: components/arc
dcheng: components/arc/common/*.mojom
xiyuan: chrome/browser

At a high level:

* Split ArcBridge into separate interfaces, each with its
  own readiness to better reflect the fact that the
  interfaces on the other side start at different times.
* Interface-specific Observers were removed, and the only
  remaining Observer handles readiness for all interfaces.
* Removed all applist-specific methods and logic from
  FakeArcBridgeService, moved it all to a FakeAppInstance.
* Ran `git cl lint` and `git cl format` on all files
  touched, which explains the 'unsigned short' to
  'uint16_t' changes.

[xiyuan, 2015-12-14 22:52:02]

chrome/browser/ui/app_list/* LGTM

https://codereview.chromium.org/1523643002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:49: class FakeAppInstance :
public arc::AppInstance {
nit: Think it would be better to put this into its own file, e.g.
components/arc/test/fake_app_instance.h.

[dcheng, 2015-12-16 06:26:16]

https://codereview.chromium.org/1523643002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:205:
mojo::Array<uint8_t>::From(png_data));
There's a TypeConverter to go between mojo::Array<uint8_t> and std::string, so
let's get rid of the std::vector intermediate.

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
File components/arc/input/arc_input_bridge_impl.cc (right):

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
components/arc/input/arc_input_bridge_impl.cc:72: if
(arc_bridge_service->input_instance() != nullptr) {
Nit: for consistency, write this as `if (arc_bridge_service->input_instance())`

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
File components/arc/input/arc_input_bridge_impl.h (right):

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
components/arc/input/arc_input_bridge_impl.h:72: uint16_t type,
Nit: stdint.h for these types

[Luis Héctor Chávez, 2015-12-16 17:07:22]

https://codereview.chromium.org/1523643002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:49: class FakeAppInstance :
public arc::AppInstance {
On 2015/12/14 22:52:02, xiyuan wrote:
> nit: Think it would be better to put this into its own file, e.g.
> components/arc/test/fake_app_instance.h.

Done.

https://codereview.chromium.org/1523643002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:205:
mojo::Array<uint8_t>::From(png_data));
On 2015/12/16 06:26:16, dcheng wrote:
> There's a TypeConverter to go between mojo::Array<uint8_t> and std::string, so
> let's get rid of the std::vector intermediate.

Done.

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
File components/arc/input/arc_input_bridge_impl.cc (right):

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
components/arc/input/arc_input_bridge_impl.cc:72: if
(arc_bridge_service->input_instance() != nullptr) {
On 2015/12/16 06:26:16, dcheng wrote:
> Nit: for consistency, write this as `if
(arc_bridge_service->input_instance())`

Done.

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
File components/arc/input/arc_input_bridge_impl.h (right):

https://codereview.chromium.org/1523643002/diff/60001/components/arc/input/ar...
components/arc/input/arc_input_bridge_impl.h:72: uint16_t type,
On 2015/12/16 06:26:16, dcheng wrote:
> Nit: stdint.h for these types

Done.

[xiyuan, 2015-12-16 17:17:51]

SLGTM

https://codereview.chromium.org/1523643002/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1523643002/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:6: #define
COMPONENTS_ARC_iTEST_FAKE_APP_INSTANCE_H_
nit: fix typo, iTEST-> TEST

[Luis Héctor Chávez, 2015-12-16 17:58:34]

lhchavez@chromium.org changed reviewers:
+ afakhry@chromium.org, elijahtaylor@chromium.org

[Luis Héctor Chávez, 2015-12-16 17:58:34]

PTAL

afakhry: chrome/browser/task_management/providers/arc/
elijahtaylor1: components/arc (hidehiko is on vacation).

https://codereview.chromium.org/1523643002/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.h (right):

https://codereview.chromium.org/1523643002/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.h:6: #define
COMPONENTS_ARC_iTEST_FAKE_APP_INSTANCE_H_
On 2015/12/16 17:17:51, xiyuan wrote:
> nit: fix typo, iTEST-> TEST

Done.

[hidehiko, 2015-12-16 18:34:03]

Oops, sorry for delaying. (Yes, I was actually on vacation).

https://codereview.chromium.org/1523643002/diff/120001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:156: // Mojo interfaces.
Should these be private (in impl), so getters should be virtual?
Or can we create setters for them with calling a corresponding Observer method?
(With this, you can merge some parts between the impl class and the Fake class,
I think).

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/app.mojom:9: SCALE_FACTOR_NONE = 0,
nit: maybe it's time to remove the common prefix as we do for other enums (even
they are just a copy from somewhere)?
I'm fine to do this in a separate CL.

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/app.mojom:44: // TODO(lhchavez): Migrate all
request/response messages to Mojo.
You've done already?

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/arc_bridge.mojom:15: OnAppInstanceReady@100(AppInstance
instance_ptr);
Why do you need the explicit order here? Comments?

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/notifications.mojom:62: // TODO(lhchavez): Migrate all
request/response messages to Mojo.
Ditto.

https://codereview.chromium.org/1523643002/diff/120001/components/arc/input/a...
File components/arc/input/arc_input_bridge_impl.cc (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/input/a...
components/arc/input/arc_input_bridge_impl.cc:75:
arc_bridge_service->AddObserver(this);
Should this be always registered for consistency?

[hidehiko, 2015-12-16 18:39:57]

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/process_list.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/process_list.mojom:87: interface ProcessListInstance {
One more nit (or bikeshed). As nya@ is trying to add "KillProcess" method here,
maybe ProcessInstance seems better (considering this does more than listing
processes)? WDYT?

[Luis Héctor Chávez, 2015-12-16 19:09:49]

https://codereview.chromium.org/1523643002/diff/120001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:156: // Mojo interfaces.
On 2015/12/16 18:34:03, hidehiko wrote:
> Should these be private (in impl), so getters should be virtual?
> Or can we create setters for them with calling a corresponding Observer
method?
> (With this, you can merge some parts between the impl class and the Fake
class,
> I think).

Actually I made ArcBridgeService implement ArcBridgeHost directly and get rid of
a lot of code duplication across this, the Impl and the Fake.

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/app.mojom:44: // TODO(lhchavez): Migrate all
request/response messages to Mojo.
On 2015/12/16 18:34:03, hidehiko wrote:
> You've done already?

What I want to be able to do is to change the methods to be

RefreshAppList() => (array<AppInfo> apps);
RequestAppIcon(string package, string activity, ScaleFactor scale_factor) =>
(array<uint8> icon_png_data);

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/arc_bridge.mojom:15: OnAppInstanceReady@100(AppInstance
instance_ptr);
On 2015/12/16 18:34:03, hidehiko wrote:
> Why do you need the explicit order here? Comments?

Several folks have been editing this file and falling into the pitfall that they
insert a method in the middle and then everything de-synchronizes. If we want to
keep this alphabetical, it's the only way to do it.

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/notifications.mojom:62: // TODO(lhchavez): Migrate all
request/response messages to Mojo.
On 2015/12/16 18:34:03, hidehiko wrote:
> Ditto.

SendNotificationEventToAndroid(string key, ArcNotificationEvent event) =>
(ArcNotificationData notification_data);

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
File components/arc/common/process_list.mojom (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/common/...
components/arc/common/process_list.mojom:87: interface ProcessListInstance {
On 2015/12/16 18:39:57, hidehiko wrote:
> One more nit (or bikeshed). As nya@ is trying to add "KillProcess" method
here,
> maybe ProcessInstance seems better (considering this does more than listing
> processes)? WDYT?

Sure. I wasn't sure of the intended scope of this interface, but Process sounds
better.

https://codereview.chromium.org/1523643002/diff/120001/components/arc/input/a...
File components/arc/input/arc_input_bridge_impl.cc (right):

https://codereview.chromium.org/1523643002/diff/120001/components/arc/input/a...
components/arc/input/arc_input_bridge_impl.cc:75:
arc_bridge_service->AddObserver(this);
On 2015/12/16 18:34:03, hidehiko wrote:
> Should this be always registered for consistency?

I don't see why not. Done.

[elijahtaylor1, 2015-12-16 19:19:52]

https://codereview.chromium.org/1523643002/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:237:
app_instance()->RefreshAppList();
was this a mistake before?

https://codereview.chromium.org/1523643002/diff/140001/components/arc/common/...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1523643002/diff/140001/components/arc/common/...
components/arc/common/notifications.mojom:65: Init(NotificationsHost host_ptr);
I don't see where this is called in other code, has the implementation not
landed?  Assuming you're coordinating, who will land first?

[hidehiko, 2015-12-16 19:20:16]

lgtm for components/arc.

[hidehiko, 2015-12-16 19:20:50]

and defer to Elijah.

[dcheng, 2015-12-17 07:35:55]

mojom lgtm

[Luis Héctor Chávez, 2015-12-17 22:38:03]

https://codereview.chromium.org/1523643002/diff/120001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/1523643002/diff/120001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:237:
app_instance()->RefreshAppList();
On 2015/12/16 19:19:52, elijahtaylor1 wrote:
> was this a mistake before?

It wasn't. The FakeArcBridgeService used to call this, but after the split it
now needs to be called explicitly.

https://codereview.chromium.org/1523643002/diff/140001/components/arc/common/...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1523643002/diff/140001/components/arc/common/...
components/arc/common/notifications.mojom:65: Init(NotificationsHost host_ptr);
On 2015/12/16 19:19:52, elijahtaylor1 wrote:
> I don't see where this is called in other code, has the implementation not
> landed?  Assuming you're coordinating, who will land first?

Probably yoshiki@.

[afakhry, 2015-12-18 00:51:06]

task_management/ lgtm

[elijahtaylor1, 2015-12-19 00:11:49]

lgtm

[Luis Héctor Chávez, 2015-12-19 00:39:32]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2015-12-19 00:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dcheng@chromium.org, afakhry@chromium.org, hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/1523643002/#ps160001
(title: "Rebased to ToT")

[commit-bot: I haz the power, 2015-12-19 00:40:35]

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

[commit-bot: I haz the power, 2015-12-19 00:46:15]

Description was changed from

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests
==========

to

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests
==========

[commit-bot: I haz the power, 2015-12-19 00:46:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-19 00:47:02]

Description was changed from

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests
==========

to

==========
arc-bridge: Move most methods to Mojo interfaces

This should make the ArcBridgeService surface area smaller by moving all
methods to separate Mojo interfaces. This also lets each individual
service handle its own lifetime and should simplify the Observers
significatively.

BUG=569446
TEST=unit_tests, components_unittests

Committed: https://crrev.com/3bdc0168bb4d055b647dd557d062432c9e835468
Cr-Commit-Position: refs/heads/master@{#366233}
==========

[commit-bot: I haz the power, 2015-12-19 00:47:03]

Patchset 9 (id:??) landed as
https://crrev.com/3bdc0168bb4d055b647dd557d062432c9e835468
Cr-Commit-Position: refs/heads/master@{#366233}

[lhc(google), 2015-12-19 15:15:25]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1534423002/ by lhchavez@google.com.

The reason for reverting is: Tests are flaky under Valgrind:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

This fixes the problem: https://codereview.chromium.org/1534413002/.