arc-bridge: Split ArcBridgeService to common part and IPC based impl.

arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug

Committed: https://crrev.com/46050cbe5a7fe9d8a2d9c8dee89f7f87d48c0798
Cr-Commit-Position: refs/heads/master@{#361860}

[khmel1, 2015-11-25 12:37:47]

Description was changed from

==========
arc-bridge: Minimal support for ARC app launcher.

This CL comes along with:
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

to

==========
arc-bridge: Minimal support for ARC app launcher.

This CL comes along with:
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

[khmel1, 2015-11-25 12:37:47]

khmel@google.com changed reviewers:
+ hidehiko@chromium.org, satorux@chromium.org

[khmel1, 2015-11-25 12:38:44]

Description was changed from

==========
arc-bridge: Minimal support for ARC app launcher.

This CL comes along with:
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

to

==========
arc-bridge: Minimal support for ARC app launcher.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

[khmel1, 2015-11-25 12:43:00]

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

[khmel1, 2015-11-25 12:43:00]

Hi Hidehiko-san and Satoru-san,

This is splitting of https://codereview.chromium.org/1475563002 and includes
arc_bridge_service_impl with addressed comments.

PTAL

[Luis Héctor Chávez, 2015-11-25 21:55:42]

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

[Luis Héctor Chávez, 2015-11-25 21:55:42]

deferring to hidehiko, but LGTM.

[hidehiko, 2015-11-26 02:21:51]

Almost looks good.
I only have nitpick comments.

Could you update the title of this CL (and the first line of the commit
message), because this refactoring can be independent from app-launcher support
(although it is actually your changes' preparation)? It'd be nice to have a
title by which other Chromium contributors easily understand this is
refactoring.

https://codereview.chromium.org/1481523002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:392:
arc::ArcBridgeService::Create(
nit: std::move

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:26: DCHECK(!g_arc_bridge_service);
Move this to Create?

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:78: return scoped_ptr<ArcBridgeService>(
nit: make_scoped_ptr

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

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:14: namespace base {
nit: please merge namespace to just below one.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:85: ArcBridgeService();
Please move this to protected.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:88: // Create instance of |ArcBridgeService|
for normal use.
nit: Create*s*. (For function or class comment, third-person singular present
form is preferred).

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:145: base::ObserverList<Observer>
observer_list_;
Following three fields can and should be private?

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:153:
DISALLOW_COPY_AND_ASSIGN(ArcBridgeService);
private:

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.h (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.h:18: }
nit: // namespace base

[satorux1, 2015-11-26 02:27:16]

chrome/browser/chromeos/chrome_browser_main_chromeos.cc lgtm. I'll defer to
hidehiko for everything else. :)

[khmel1, 2015-11-26 04:37:04]

Description was changed from

==========
arc-bridge: Minimal support for ARC app launcher.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

to

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

[khmel1, 2015-11-26 05:46:43]

Thanks for reviewing and for prompt feedback.
Comments were addressed.
PTAL

https://codereview.chromium.org/1481523002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:392:
arc::ArcBridgeService::Create(
On 2015/11/26 02:21:51, hidehiko wrote:
> nit: std::move

Done, BTW why this is preferred?

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:26: DCHECK(!g_arc_bridge_service);
On 2015/11/26 02:21:51, hidehiko wrote:
> Move this to Create?

There is similar check in DTOR 'DCHECK(g_arc_bridge_service);'. Imho it is
better to leave as is for consistency. WDYT?

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:78: return scoped_ptr<ArcBridgeService>(
On 2015/11/26 02:21:51, hidehiko wrote:
> nit: make_scoped_ptr

Done, thanks, good to know.

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

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:14: namespace base {
On 2015/11/26 02:21:51, hidehiko wrote:
> nit: please merge namespace to just below one.

Done.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:85: ArcBridgeService();
On 2015/11/26 02:21:51, hidehiko wrote:
> Please move this to protected.

Done.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:88: // Create instance of |ArcBridgeService|
for normal use.
On 2015/11/26 02:21:51, hidehiko wrote:
> nit: Create*s*. (For function or class comment, third-person singular present
> form is preferred).

Sorry, copied from legacy code as template and did not notice.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:145: base::ObserverList<Observer>
observer_list_;
On 2015/11/26 02:21:51, hidehiko wrote:
> Following three fields can and should be private?

Done

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:153:
DISALLOW_COPY_AND_ASSIGN(ArcBridgeService);
On 2015/11/26 02:21:51, hidehiko wrote:
> private:

Done.

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.h (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.h:18: }
On 2015/11/26 02:21:51, hidehiko wrote:
> nit: // namespace base

Done.

[hidehiko, 2015-11-26 05:55:31]

LGTM.
Could you update "Subject" of this page, too?

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:26: DCHECK(!g_arc_bridge_service);
On 2015/11/26 05:46:43, khmel1 wrote:
> On 2015/11/26 02:21:51, hidehiko wrote:
> > Move this to Create?
> 
> There is similar check in DTOR 'DCHECK(g_arc_bridge_service);'. Imho it is
> better to leave as is for consistency. WDYT?

Ah, I overlooked. Ok, that's fine.
Could you change dtor's DCHECK to
"DCHECK(g_arc_bridge_service == this);"
while you're here?

[khmel1, 2015-11-26 06:11:16]

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1481523002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:26: DCHECK(!g_arc_bridge_service);
On 2015/11/26 05:55:31, hidehiko wrote:
> On 2015/11/26 05:46:43, khmel1 wrote:
> > On 2015/11/26 02:21:51, hidehiko wrote:
> > > Move this to Create?
> > 
> > There is similar check in DTOR 'DCHECK(g_arc_bridge_service);'. Imho it is
> > better to leave as is for consistency. WDYT?
> 
> Ah, I overlooked. Ok, that's fine.
> Could you change dtor's DCHECK to
> "DCHECK(g_arc_bridge_service == this);"
> while you're here?
> 
> 

Yes. also think this might be better

[khmel1, 2015-11-26 06:12:08]

The CQ bit was checked by khmel@google.com

[khmel1, 2015-11-26 06:12:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, satorux@chromium.org, hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/1481523002/#ps100001
(title: "Nit addressed")

[khmel1, 2015-11-26 06:12:13]

The CQ bit was unchecked by khmel@google.com

[hidehiko, 2015-11-26 06:12:27]

LGTM.
Thank you for quick update!

[khmel1, 2015-11-26 06:14:16]

The CQ bit was checked by khmel@google.com

[khmel1, 2015-11-26 06:14:17]

lgtm

[commit-bot: I haz the power, 2015-11-26 06:14:38]

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

[commit-bot: I haz the power, 2015-11-26 06:30:36]

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

[commit-bot: I haz the power, 2015-11-26 06:30:37]

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

[khmel1, 2015-11-26 08:38:54]

Description was changed from

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
==========

to

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug
==========

[khmel1, 2015-11-26 08:39:11]

The CQ bit was checked by khmel@google.com

[khmel1, 2015-11-26 08:39:12]

lgtm

Several bots failed. One reason it was not accepted to use std::move. I reverted
usage to Pass for scoped_ptr. Second I missed that unit_tests resides in
components_unittest.

Changes are trivial.

[khmel1, 2015-11-26 08:39:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, lhchavez@chromium.org, satorux@chromium.org
Link to the patchset: https://codereview.chromium.org/1481523002/#ps120001
(title: "discard using std::move. Update unittest")

[commit-bot: I haz the power, 2015-11-26 08:39:29]

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

[hidehiko, 2015-11-26 08:49:07]

On 2015/11/26 08:39:29, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1481523002/120001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1481523002/120001

You just can drop ".Pass()", so that it will be used as rvalue ref.

[khmel1, 2015-11-26 08:53:19]

On 2015/11/26 08:49:07, hidehiko wrote:
> On 2015/11/26 08:39:29, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1481523002/120001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1481523002/120001
> 
> You just can drop ".Pass()", so that it will be used as rvalue ref.

Thanks, if everything is correct with this CL will update this nit in next
review.

[hidehiko, 2015-11-26 08:53:59]

On 2015/11/26 08:53:19, khmel1 wrote:
> On 2015/11/26 08:49:07, hidehiko wrote:
> > On 2015/11/26 08:39:29, commit-bot: I haz the power wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/1481523002/120001
> > > View timeline at
> > >  https://chromium-cq-status.appspot.com/patch-timeline/1481523002/120001
> > 
> > You just can drop ".Pass()", so that it will be used as rvalue ref.
> 
> Thanks, if everything is correct with this CL will update this nit in next
> review.

SG. Thank you for quick fix!

[commit-bot: I haz the power, 2015-11-26 09:49:40]

Description was changed from

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug
==========

to

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug
==========

[commit-bot: I haz the power, 2015-11-26 09:49:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-26 09:50:42]

Description was changed from

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug
==========

to

==========
arc-bridge: Split ArcBridgeService to common part and IPC based impl.

This CL comes along with:
https://codereview.chromium.org/1475563002 (arc_bridge)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)

This refactores ArcBridgeService in order to be able to
implement FakeArcBridgeService.

BUG=558209
BUG=b/24545231
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
TEST=components_unittests Debug

Committed: https://crrev.com/46050cbe5a7fe9d8a2d9c8dee89f7f87d48c0798
Cr-Commit-Position: refs/heads/master@{#361860}
==========

[commit-bot: I haz the power, 2015-11-26 09:50:43]

Patchset 7 (id:??) landed as
https://crrev.com/46050cbe5a7fe9d8a2d9c8dee89f7f87d48c0798
Cr-Commit-Position: refs/heads/master@{#361860}