arc: Revamp the ArcBridgeService interface

arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots

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

[Luis Héctor Chávez, 2016-07-08 16:35:14]

Description was changed from

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=None
TEST=trybots
==========

to

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots
==========

[Luis Héctor Chávez, 2016-07-08 16:41:49]

lhchavez@chromium.org changed reviewers:
+ afakhry@chromium.org, georgesak@chromium.org, hidehiko@chromium.org,
stevenjb@chromium.org

[Luis Héctor Chávez, 2016-07-08 16:41:50]

PTAL

This manages to remove almost 500 lines of boilerplate in components/arc and is
needed to make testing ARC-related components easier (will be done in another
change).

Outside of components/arc, this is just a mechanical refactor.

hidehiko *arc/*
afakhry: chrome/browser/task_management/providers
georgesak: chrome/browser/memory
stevenjb: chrome/browser

[stevenjb, 2016-07-08 16:56:21]

c/b RS LGTM

[afakhry, 2016-07-08 17:10:52]

c/b/task_management lgtm.

[hidehiko, 2016-07-11 05:24:54]

Good refactoring :-).

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:153: typedef
base::hash_map<base::ProcessHandle, int> ProcessScoreMap;  // NOLINT
Clarification: For what ... ? > NOLINT.

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:240:
bridge_service->app()->AddObserver(this);
To keep the consistency from the prev version, this line seems to need be
located between L237-238?

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:202: return
arc::LaunchApp(context, kSettingsAppId, true);  // landscape_layout
nit: "// landscape_layout" looks to comment for the line. maybe

return arc::LaunchApp(context, kSettingsAppId, /* landscape_layout= */ true);

or

constexpr bool kUseLandscapeLayout = true;
return arc::LaunchApp(context, kSettingsAppId, kUseLandscapeLayout);

etc.?

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:225: return (new
LaunchAppWithoutSize(context, app_id, landscape_layout))
nit: unnecessary parent?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:83: template <typename T>
Could you comment that T needs to be a mojo interface?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:84: class InstanceObserver {
InstanceObserver and InstanceHolder looks enough big. So, could you move this
out from this class?

Optional: Introducing a new file may be another option?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:93: virtual void OnInstanceClosed(T*) {}
Could you get rid of T*?

I'm guessing that it is supposed to inherit two ore more InstanceObserver
interface for a class, e.g.

class A : public InstanceObserver<T1>, public InstanceObserver<T2> ... {
};

etc. Though, in that case, I think it looks better to split the class into two
or more.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:96: virtual ~InstanceObserver() {}
nit: s/{}/= default/

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:108: T* instance() const { return raw_ptr_;
}
To access the raw pointer of T, there are two ways:

accessing InstanceHolder::instance(), or kee pthe ptr passed via
InstanceObserver::OnInstanceReady().
Actually, currently those are mixed in use. Which is the preferable way?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:135: if
(observer_list_.might_have_observers()) {
FOR_EACH_OBSERVER(InstanceObserver<T>, observer_list_, OnInstanceClosed());

should work?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:159: if
(observer_list_.might_have_observers()) {
Ditto for FOR_EACH_OBSERVER.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:193: base::WeakPtrFactory<InstanceHolder<T>>
weak_factory_;
Could you comment that this needs to be the last member, in order to cancel all
inflight callback with weak_ptr generated by this factory before destructing any
of other members?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:263: void
OnAppInstanceReady(mojom::AppInstancePtr app_ptr) override;
Clarification: Not necessary to do in this CL (moreover I think it is better to
do it in a separate CL,... but), WDYT about moving these implementation to the
ABSImpl?

If you agree, could you move some part of InstanceHolder implementation to
another new (maybe template) class in ABSImpl file, specifically,
CloseChannel(), OnInstanceReady(), and OnVersionReady()?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:24:
arc_bridge_service()->audio()->RemoveObserver(this);
nit: As you're here, could you move this to after L27?
(unregister callback in the reverse order of the registration sounds a good
manner).

https://codereview.chromium.org/2133503002/diff/80001/components/arc/crash_co...
File components/arc/crash_collector/arc_crash_collector_bridge.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/crash_co...
components/arc/crash_collector/arc_crash_collector_bridge.cc:18: const char
kCrashReporterPath[] = "/sbin/crash_reporter";
nit: if the below empty line is removed, should we remove the one above, too,
for consistency?

https://codereview.chromium.org/2133503002/diff/80001/components/arc/metrics/...
File components/arc/metrics/arc_metrics_service.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.cc:126: if (!binding_.is_bound()) {
(This is off topic so I'm ok to address this separately, but...),
Should this need to wait for MetricsInstance's initialization, too?
metrics()->instance() can be nullptr, here?

[hidehiko, 2016-07-11 06:09:32]

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:263: void
OnAppInstanceReady(mojom::AppInstancePtr app_ptr) override;
On 2016/07/11 05:24:53, hidehiko wrote:
> Clarification: Not necessary to do in this CL (moreover I think it is better
to
> do it in a separate CL,... but), WDYT about moving these implementation to the
> ABSImpl?
> 
> If you agree, could you move some part of InstanceHolder implementation to
> another new (maybe template) class in ABSImpl file, specifically,
> CloseChannel(), OnInstanceReady(), and OnVersionReady()?

Oh, your CL following this one actually did the ArcHost moving. So, splitting
InstanceHost is just my request.

[Luis Héctor Chávez, 2016-07-11 17:13:34]

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:153: typedef
base::hash_map<base::ProcessHandle, int> ProcessScoreMap;  // NOLINT
On 2016/07/11 05:24:53, hidehiko wrote:
> Clarification: For what ... ? > NOLINT.

`git cl lint` flags this as std::hash_map. I'll try to fix that in another
change.

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:240:
bridge_service->app()->AddObserver(this);
On 2016/07/11 05:24:53, hidehiko wrote:
> To keep the consistency from the prev version, this line seems to need be
> located between L237-238?

L237-238 should be together to be consistent with the previous version. L240 can
be either before or after that block and I don't have a preference, so I'll put
it before.

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:202: return
arc::LaunchApp(context, kSettingsAppId, true);  // landscape_layout
On 2016/07/11 05:24:53, hidehiko wrote:
> nit: "// landscape_layout" looks to comment for the line. maybe
> 
> return arc::LaunchApp(context, kSettingsAppId, /* landscape_layout= */ true);
> 
> or
> 
> constexpr bool kUseLandscapeLayout = true;
> return arc::LaunchApp(context, kSettingsAppId, kUseLandscapeLayout);
> 
> etc.?

The latter looks nicer.

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:225: return (new
LaunchAppWithoutSize(context, app_id, landscape_layout))
On 2016/07/11 05:24:53, hidehiko wrote:
> nit: unnecessary parent?

it's necessary, unfortunately.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:83: template <typename T>
On 2016/07/11 05:24:53, hidehiko wrote:
> Could you comment that T needs to be a mojo interface?

Done.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:84: class InstanceObserver {
On 2016/07/11 05:24:53, hidehiko wrote:
> InstanceObserver and InstanceHolder looks enough big. So, could you move this
> out from this class?
> 
> Optional: Introducing a new file may be another option?

Done.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:93: virtual void OnInstanceClosed(T*) {}
On 2016/07/11 05:24:53, hidehiko wrote:
> Could you get rid of T*?
> 
> I'm guessing that it is supposed to inherit two ore more InstanceObserver
> interface for a class, e.g.
> 
> class A : public InstanceObserver<T1>, public InstanceObserver<T2> ... {
> };
> 
> etc. Though, in that case, I think it looks better to split the class into two
> or more.

I can't :/ The metrics service tries needs to wait for both the metrics and
process instances.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:96: virtual ~InstanceObserver() {}
On 2016/07/11 05:24:53, hidehiko wrote:
> nit: s/{}/= default/

Done.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:108: T* instance() const { return raw_ptr_;
}
On 2016/07/11 05:24:53, hidehiko wrote:
> To access the raw pointer of T, there are two ways:
> 
> accessing InstanceHolder::instance(), or kee pthe ptr passed via
> InstanceObserver::OnInstanceReady().
> Actually, currently those are mixed in use. Which is the preferable way?

I'd prefer to not keep the ptr since it is more error-prone (since the user
needs to be diligent to reset the ptr to nullptr in the OnInstanceClosed()).
Having said that, keeping the pointer does save some boilerplate :/

Let's discuss this offline to see if it is possible to add this to the best
practices doc.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:135: if
(observer_list_.might_have_observers()) {
On 2016/07/11 05:24:53, hidehiko wrote:
> FOR_EACH_OBSERVER(InstanceObserver<T>, observer_list_, OnInstanceClosed());
> 
> should work?

It doesn't since it needs the 'typename' since InstanceObserver<T> cannot be
resolved to a type otherwise. I can try to fix the FOR_EACH_OBSERVER, but that
should be done in a different change.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:159: if
(observer_list_.might_have_observers()) {
On 2016/07/11 05:24:53, hidehiko wrote:
> Ditto for FOR_EACH_OBSERVER.

Acknowledged.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:193: base::WeakPtrFactory<InstanceHolder<T>>
weak_factory_;
On 2016/07/11 05:24:53, hidehiko wrote:
> Could you comment that this needs to be the last member, in order to cancel
all
> inflight callback with weak_ptr generated by this factory before destructing
any
> of other members?

Sure

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:263: void
OnAppInstanceReady(mojom::AppInstancePtr app_ptr) override;
On 2016/07/11 06:09:32, hidehiko wrote:
> On 2016/07/11 05:24:53, hidehiko wrote:
> > Clarification: Not necessary to do in this CL (moreover I think it is better
> to
> > do it in a separate CL,... but), WDYT about moving these implementation to
the
> > ABSImpl?
> > 
> > If you agree, could you move some part of InstanceHolder implementation to
> > another new (maybe template) class in ABSImpl file, specifically,
> > CloseChannel(), OnInstanceReady(), and OnVersionReady()?
> 
> Oh, your CL following this one actually did the ArcHost moving. So, splitting
> InstanceHost is just my request.

Acknowledged.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:24:
arc_bridge_service()->audio()->RemoveObserver(this);
On 2016/07/11 05:24:53, hidehiko wrote:
> nit: As you're here, could you move this to after L27?
> (unregister callback in the reverse order of the registration sounds a good
> manner).

Done.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/crash_co...
File components/arc/crash_collector/arc_crash_collector_bridge.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/crash_co...
components/arc/crash_collector/arc_crash_collector_bridge.cc:18: const char
kCrashReporterPath[] = "/sbin/crash_reporter";
On 2016/07/11 05:24:53, hidehiko wrote:
> nit: if the below empty line is removed, should we remove the one above, too,
> for consistency?

Sure. I just ran a replacement script + `git cl format`, don't know why
clang-format did this.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/metrics/...
File components/arc/metrics/arc_metrics_service.cc (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.cc:126: if (!binding_.is_bound()) {
On 2016/07/11 05:24:53, hidehiko wrote:
> (This is off topic so I'm ok to address this separately, but...),
> Should this need to wait for MetricsInstance's initialization, too?
> metrics()->instance() can be nullptr, here?

OnArcStartTimeRetrieved already waits for MetricInstance since it can only be
called from OnInstanceReady(MetricsInstance). Having said that, it can go away
between that function is called and this one, so just adding a null-check before
this should be ok.

[Luis Héctor Chávez, 2016-07-11 20:10:44]

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (right):

https://codereview.chromium.org/2133503002/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.h:153: typedef
base::hash_map<base::ProcessHandle, int> ProcessScoreMap;  // NOLINT
On 2016/07/11 17:13:33, Luis Héctor Chávez wrote:
> On 2016/07/11 05:24:53, hidehiko wrote:
> > Clarification: For what ... ? > NOLINT.
> 
> `git cl lint` flags this as std::hash_map. I'll try to fix that in another
> change.

Filed crbug.com/627202. I have a fix ready and will apply it as soon as I can
figure out how to send it for review and have this rolled.

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2133503002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:135: if
(observer_list_.might_have_observers()) {
On 2016/07/11 17:13:34, Luis Héctor Chávez wrote:
> On 2016/07/11 05:24:53, hidehiko wrote:
> > FOR_EACH_OBSERVER(InstanceObserver<T>, observer_list_, OnInstanceClosed());
> > 
> > should work?
> 
> It doesn't since it needs the 'typename' since InstanceObserver<T> cannot be
> resolved to a type otherwise. I can try to fix the FOR_EACH_OBSERVER, but that
> should be done in a different change.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=627209 and will
follow-up separately. Will amend this change if the fix lands first.

[hidehiko, 2016-07-12 16:25:14]

LGTM with comments.

https://codereview.chromium.org/2133503002/diff/180001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2133503002/diff/180001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:24: constexpr bool
kUseLandscapeLayout = true;
optional: as this is constexpr, and used only in LaunchAndroidSettingsApp, you
can safely move this to the function, like:

LaunchAndroidSettingsApp(...) {
  constexpr bool kUseLnadscapeLayout = true;
  return arc::LaunchApp(..., kUseLandscapeLayout);
}

Up to you.

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
components/arc/instance_holder.h:30: virtual void OnInstanceReady(T* instance,
uint32_t version) {}
I prefer to standardize the way to access T*.
If the preferred way is via InstanceHolder::instance() (and version()), then it
looks better to avoid having args here, because it provides alternative way
unnecessarily.

The only drawback should be compile errors, IIUC. It can be resolved by
introducing tiny adapter class.
Also, as offline-chatted, if you'd like to embed type info, you can into class's
template args, which is known as "phantom type" technique.

As we chat offline, to move forward, could you add TODO to drop args?

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
components/arc/instance_holder.h:34: virtual void OnInstanceClosed(T*) {}
Ditto. Could you add TODO to drop T*?

[Luis Héctor Chávez, 2016-07-12 18:00:24]

https://codereview.chromium.org/2133503002/diff/180001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/2133503002/diff/180001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:24: constexpr bool
kUseLandscapeLayout = true;
On 2016/07/12 16:25:14, hidehiko wrote:
> optional: as this is constexpr, and used only in LaunchAndroidSettingsApp, you
> can safely move this to the function, like:
> 
> LaunchAndroidSettingsApp(...) {
>   constexpr bool kUseLnadscapeLayout = true;
>   return arc::LaunchApp(..., kUseLandscapeLayout);
> }
> 
> Up to you.

Looks cleaner, so done.

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
components/arc/instance_holder.h:30: virtual void OnInstanceReady(T* instance,
uint32_t version) {}
On 2016/07/12 16:25:14, hidehiko wrote:
> I prefer to standardize the way to access T*.
> If the preferred way is via InstanceHolder::instance() (and version()), then
it
> looks better to avoid having args here, because it provides alternative way
> unnecessarily.
> 
> The only drawback should be compile errors, IIUC. It can be resolved by
> introducing tiny adapter class.
> Also, as offline-chatted, if you'd like to embed type info, you can into
class's
> template args, which is known as "phantom type" technique.
> 
> As we chat offline, to move forward, could you add TODO to drop args?

Args were dropped.

https://codereview.chromium.org/2133503002/diff/180001/components/arc/instanc...
components/arc/instance_holder.h:34: virtual void OnInstanceClosed(T*) {}
On 2016/07/12 16:25:14, hidehiko wrote:
> Ditto. Could you add TODO to drop T*?

Dropped.

[hidehiko, 2016-07-12 18:13:04]

LGTM, assuming all tests pass. Thank you!

[Georges Khalil, 2016-07-12 18:29:33]

lgtm % 1 question.

https://codereview.chromium.org/2133503002/diff/210001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (right):

https://codereview.chromium.org/2133503002/diff/210001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_delegate_chromeos.h:153: typedef
base::hash_map<base::ProcessHandle, int> ProcessScoreMap;  // NOLINT
nit: Curious why this is needed?

[Luis Héctor Chávez, 2016-07-12 19:51:31]

https://codereview.chromium.org/2133503002/diff/210001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_delegate_chromeos.h (right):

https://codereview.chromium.org/2133503002/diff/210001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_delegate_chromeos.h:153: typedef
base::hash_map<base::ProcessHandle, int> ProcessScoreMap;  // NOLINT
On 2016/07/12 18:29:33, Georges Khalil wrote:
> nit: Curious why this is needed?

It's a bug in `git cl lint`: crbug.com/627202

Since it will take some time to land and roll, I'll remove this with a followup
change.

[Luis Héctor Chávez, 2016-07-12 21:01:33]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-07-12 21:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
afakhry@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2133503002/#ps210001
(title: "Moved a constexpr closer to where it's used")

[commit-bot: I haz the power, 2016-07-12 21:03: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, 2016-07-13 02:30:04]

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

[commit-bot: I haz the power, 2016-07-13 02:30:07]

Failed to apply patch for chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:234
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc' with
conflicts.
  U chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

Patch:       chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
Index: chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
diff --git a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
index
bcbc88c270abcdb51bce53a2b19b0eefe62dc959..aab930faba5f675d6b22d978fdb084440c55a0d7
100644
--- a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
+++ b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
@@ -108,9 +108,9 @@ bool InstallIconFromFileThread(const std::string& app_id,
 
   base::CreateDirectory(icon_path.DirName());
 
-  int wrote = base::WriteFile(icon_path,
-                              reinterpret_cast<const char*>(&content_png[0]),
-                              content_png.size());
+  int wrote =
+      base::WriteFile(icon_path, reinterpret_cast<const
char*>(&content_png[0]),
+                      content_png.size());
   if (wrote != static_cast<int>(content_png.size())) {
     VLOG(2) << "Failed to write ARC icon file: " << icon_path.MaybeAsASCII()
             << ".";
@@ -234,14 +234,18 @@ ArcAppListPrefs::ArcAppListPrefs(const base::FilePath&
base_path,
   arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get();
   DCHECK(bridge_service);
 
+  bridge_service->app()->AddObserver(this);
   bridge_service->AddObserver(this);
-  OnStateChanged(bridge_service->state());
+  if (bridge_service->state() != arc::ArcBridgeService::State::READY)
+    OnBridgeStopped();
 }
 
 ArcAppListPrefs::~ArcAppListPrefs() {
   arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get();
-  if (bridge_service)
+  if (bridge_service) {
     bridge_service->RemoveObserver(this);
+    bridge_service->app()->RemoveObserver(this);
+  }
 
   arc::ArcAuthService* auth_service = arc::ArcAuthService::Get();
   if (auth_service)
@@ -257,35 +261,34 @@ base::FilePath ArcAppListPrefs::GetIconPath(
     ui::ScaleFactor scale_factor) const {
   const base::FilePath app_path = GetAppPath(app_id);
   switch (scale_factor) {
-  case ui::SCALE_FACTOR_100P:
-    return app_path.AppendASCII("icon_100p.png");
-  case ui::SCALE_FACTOR_125P:
-    return app_path.AppendASCII("icon_125p.png");
-  case ui::SCALE_FACTOR_133P:
-    return app_path.AppendASCII("icon_133p.png");
-  case ui::SCALE_FACTOR_140P:
-    return app_path.AppendASCII("icon_140p.png");
-  case ui::SCALE_FACTOR_150P:
-    return app_path.AppendASCII("icon_150p.png");
-  case ui::SCALE_FACTOR_180P:
-    return app_path.AppendASCII("icon_180p.png");
-  case ui::SCALE_FACTOR_200P:
-    return app_path.AppendASCII("icon_200p.png");
-  case ui::SCALE_FACTOR_250P:
-    return app_path.AppendASCII("icon_250p.png");
-  case ui::SCALE_FACTOR_300P:
-    return app_path.AppendASCII("icon_300p.png");
-  default:
-    NOTREACHED();
-    return base::FilePath();
+    case ui::SCALE_FACTOR_100P:
+      return app_path.AppendASCII("icon_100p.png");
+    case ui::SCALE_FACTOR_125P:
+      return app_path.AppendASCII("icon_125p.png");
+    case ui::SCALE_FACTOR_133P:
+      return app_path.AppendASCII("icon_133p.png");
+    case ui::SCALE_FACTOR_140P:
+      return app_path.AppendASCII("icon_140p.png");
+    case ui::SCALE_FACTOR_150P:
+      return app_path.AppendASCII("icon_150p.png");
+    case ui::SCALE_FACTOR_180P:
+      return app_path.AppendASCII("icon_180p.png");
+    case ui::SCALE_FACTOR_200P:
+      return app_path.AppendASCII("icon_200p.png");
+    case ui::SCALE_FACTOR_250P:
+      return app_path.AppendASCII("icon_250p.png");
+    case ui::SCALE_FACTOR_300P:
+      return app_path.AppendASCII("icon_300p.png");
+    default:
+      NOTREACHED();
+      return base::FilePath();
   }
 }
 
 void ArcAppListPrefs::RequestIcon(const std::string& app_id,
                                   ui::ScaleFactor scale_factor) {
   if (!IsRegistered(app_id)) {
-    VLOG(2) << "Request to load icon for non-registered app: "
-            <<  app_id << ".";
+    VLOG(2) << "Request to load icon for non-registered app: " << app_id <<
".";
     return;
   }
 
@@ -301,7 +304,7 @@ void ArcAppListPrefs::RequestIcon(const std::string& app_id,
     NOTREACHED();
     return;
   }
-  arc::mojom::AppInstance* app_instance = bridge_service->app_instance();
+  arc::mojom::AppInstance* app_instance = bridge_service->app()->instance();
   if (!app_instance) {
     // AppInstance should be ready since we have app_id in ready_apps_.
     NOTREACHED();
@@ -310,7 +313,7 @@ void ArcAppListPrefs::RequestIcon(const std::string& app_id,
 
   std::unique_ptr<AppInfo> app_info = GetApp(app_id);
   if (!app_info) {
-    VLOG(2) << "Failed to get app info: " <<  app_id << ".";
+    VLOG(2) << "Failed to get app info: " << app_id << ".";
     return;
   }
 
@@ -344,9 +347,9 @@ void ArcAppListPrefs::SetNotificationsEnabled(const
std::string& app_id,
   // In case app is not ready, defer this request.
   if (!ready_apps_.count(app_id)) {
     SetNotificationsEnabledDeferred(prefs_).Put(app_id, enabled);
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnNotificationsEnabledChanged(
-                          app_info->package_name, enabled));
+    FOR_EACH_OBSERVER(
+        Observer, observer_list_,
+        OnNotificationsEnabledChanged(app_info->package_name, enabled));
     return;
   }
 
@@ -356,14 +359,14 @@ void ArcAppListPrefs::SetNotificationsEnabled(const
std::string& app_id,
     return;
   }
 
-  arc::mojom::AppInstance* app_instance = bridge_service->app_instance();
+  arc::mojom::AppInstance* app_instance = bridge_service->app()->instance();
   if (!app_instance) {
     // AppInstance should be ready since we have app_id in ready_apps_.
     NOTREACHED();
     return;
   }
 
-  if (bridge_service->app_version() < kSetNotificationsEnabledMinVersion) {
+  if (bridge_service->app()->version() < kSetNotificationsEnabledMinVersion) {
     VLOG(2) << "app version is too small to set notifications enabled.";
     return;
   }
@@ -424,8 +427,8 @@ std::vector<std::string>
ArcAppListPrefs::GetAppIdsNoArcEnabledCheck() const {
 
   // crx_file::id_util is de-facto utility for id generation.
   const base::DictionaryValue* apps = prefs_->GetDictionary(prefs::kArcApps);
-  for (base::DictionaryValue::Iterator app_id(*apps);
-       !app_id.IsAtEnd(); app_id.Advance()) {
+  for (base::DictionaryValue::Iterator app_id(*apps); !app_id.IsAtEnd();
+       app_id.Advance()) {
     if (!crx_file::id_util::IdIsValid(app_id.key()))
       continue;
 
@@ -523,8 +526,7 @@ void ArcAppListPrefs::DisableAllApps() {
   std::set<std::string> old_ready_apps;
   old_ready_apps.swap(ready_apps_);
   for (auto& app_id : old_ready_apps) {
-    FOR_EACH_OBSERVER(Observer,
-                      observer_list_,
+    FOR_EACH_OBSERVER(Observer, observer_list_,
                       OnAppReadyChanged(app_id, false));
   }
 }
@@ -541,8 +543,7 @@ void ArcAppListPrefs::NotifyRegisteredApps() {
       NOTREACHED();
       continue;
     }
-    FOR_EACH_OBSERVER(Observer,
-                      observer_list_,
+    FOR_EACH_OBSERVER(Observer, observer_list_,
                       OnAppRegistered(app_id, *app_info));
   }
 
@@ -562,18 +563,17 @@ void ArcAppListPrefs::OnOptInEnabled(bool enabled) {
     RemoveAllApps();
 }
 
-void ArcAppListPrefs::OnStateChanged(arc::ArcBridgeService::State state) {
-  if (state != arc::ArcBridgeService::State::READY)
-    DisableAllApps();
+void ArcAppListPrefs::OnBridgeStopped() {
+  DisableAllApps();
 }
 
-void ArcAppListPrefs::OnAppInstanceReady() {
+void ArcAppListPrefs::OnInstanceReady() {
   arc::ArcBridgeService* bridge_service = arc::ArcBridgeService::Get();
   if (!bridge_service) {
     NOTREACHED();
     return;
   }
-  arc::mojom::AppInstance* app_instance = bridge_service->app_instance();
+  arc::mojom::AppInstance* app_instance = bridge_service->app()->instance();
   if (!app_instance) {
     VLOG(2) << "Request to refresh app list when bridge service is not ready.";
     return;
@@ -583,7 +583,7 @@ void ArcAppListPrefs::OnAppInstanceReady() {
   app_instance->RefreshAppList();
 }
 
-void ArcAppListPrefs::OnAppInstanceClosed() {
+void ArcAppListPrefs::OnInstanceClosed() {
   ready_apps_.clear();
 }
 
@@ -626,8 +626,7 @@ void ArcAppListPrefs::AddAppAndShortcut(
     ready_apps_.insert(app_id);
 
   if (was_registered) {
-    FOR_EACH_OBSERVER(Observer,
-                      observer_list_,
+    FOR_EACH_OBSERVER(Observer, observer_list_,
                       OnAppReadyChanged(app_id, true));
   } else {
     AppInfo app_info(name, package_name, activity, intent_uri,
icon_resource_id,
@@ -674,14 +673,11 @@ void ArcAppListPrefs::RemoveApp(const std::string& app_id)
{
   // app_id may be released by observers, get the path first.
   const base::FilePath app_path = GetAppPath(app_id);
 
-  FOR_EACH_OBSERVER(Observer,
-                    observer_list_,
-                    OnAppRemoved(app_id));
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnAppRemoved(app_id));
 
   // Remove local data on file system.
   content::BrowserThread::GetBlockingPool()->PostTask(
-      FROM_HERE,
-      base::Bind(&DeleteAppFolderFromFileThread, app_path));
+      FROM_HERE, base::Bind(&DeleteAppFolderFromFileThread, app_path));
 }
 
 void ArcAppListPrefs::OnAppListRefreshed(
@@ -755,8 +751,8 @@ void
ArcAppListPrefs::OnInstallShortcut(arc::mojom::ShortcutInfoPtr shortcut) {
 void ArcAppListPrefs::OnPackageRemoved(const mojo::String& package_name) {
   const base::DictionaryValue* apps = prefs_->GetDictionary(prefs::kArcApps);
   std::vector<std::string> apps_to_remove;
-  for (base::DictionaryValue::Iterator app_it(*apps);…
(message too large)

[commit-bot: I haz the power, 2016-07-13 02:31:28]

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

[Luis Héctor Chávez, 2016-07-13 02:43:18]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-07-13 02:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, afakhry@chromium.org, stevenjb@chromium.org,
georgesak@chromium.org
Link to the patchset: https://codereview.chromium.org/2133503002/#ps230001
(title: "More rebasing")

[commit-bot: I haz the power, 2016-07-13 02:45:07]

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, 2016-07-13 04:43:14]

Description was changed from

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots
==========

to

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots
==========

[commit-bot: I haz the power, 2016-07-13 04:43:16]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2016-07-13 04:43:33]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 04:46:01]

Description was changed from

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots
==========

to

==========
arc: Revamp the ArcBridgeService interface

Adding a new host/instance pair required a lot of boilerplate. It also
made it very hard to properly do fakes for tests. This change adds two
more interfaces: InterfaceHolder<T> and InterfaceObserver<T>, so that
ArcBridgeService can stop having hundreds of methods to deal with
instance lifecycles.

BUG=626695
TEST=trybots

Committed: https://crrev.com/de2de961f2851a852febfb7223b0f27df5e6afb9
Cr-Commit-Position: refs/heads/master@{#405025}
==========

[commit-bot: I haz the power, 2016-07-13 04:46:03]

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