arc: Notify ARC instance failures via callbacks.

arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.

Committed: https://crrev.com/0c9dabff67eaefb515e9bc39299e33811915ffb4
Cr-Commit-Position: refs/heads/master@{#405241}

[Shuhei Takahashi, 2016-07-08 10:07:19]

Description was changed from

==========
arc: Add callbacks to observe ARC boot failures.

BUG=chromium:625923
TEST=TODO
==========

to

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
==========

[Shuhei Takahashi, 2016-07-08 10:12:24]

Description was changed from

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
==========

to

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually checked ARC boots.
==========

[Shuhei Takahashi, 2016-07-08 10:12:36]

Description was changed from

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually checked ARC boots.
==========

to

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

[Shuhei Takahashi, 2016-07-08 10:17:06]

Description was changed from

==========
arc: Add a callback to observe ARC boot failures.

This callback is used to show notifications when ARC boot fails.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

to

==========
arc: Add a callback to observe ARC instance failures.

Two new callbacks are added to observers:
- ArcBridgeBootstrap::Observer::OnAborting
- ArcBridgeService::Observer::OnBridgeAborting

By observing those callbacks, callers of the bridge can detect
boot failures and crashes of the ARC instance.

I am planning to use them soon to show notifications on those
events; see the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

[Shuhei Takahashi, 2016-07-08 10:17:26]

nya@chromium.org changed reviewers:
+ hidehiko@chromium.org, lhchavez@chromium.org

[Shuhei Takahashi, 2016-07-08 10:17:27]

lhchavez, hidehiko: PTAL

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

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.h:27: // Called when a connection with ARC
instance has been established.
nit: when the connection

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.h:42: virtual void OnStopped() = 0;
Do you have an example of an instance where OnAborting() would need to be called
more than once before calling OnStopped()? Is there any way we can avoid that?

I think I'd prefer to have only OnStopped() with a StopReason parameter (where
it can be BOOT_FAILURE, CRASH, or CLEAN_TERMINATION). At least that way the
opt-in flow can have a slightly simpler implementation.

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:66: // ARC instance boot failed for unknown
errors.
nit: it's better to say that this is just any error that happens during the
setup before the instance is actually attempted to be started.

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:80: // Called whenever the bridge got ready.
nit: the bridge is ready.

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:89: // Called whenever the bridge stopped.
nit: the bridge is stopped.

[hidehiko, 2016-07-11 05:40:08]

My initial scan (minus what Luis said).

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service_unittest.cc:153:
ASSERT_EQ(ArcBridgeService::AbortReason::CRASH, abort_reason_);
Could you add GENERIC_BOOT_FAILURE case, too?

[Shuhei Takahashi, 2016-07-11 08:25:19]

PTAL

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.h:27: // Called when a connection with ARC
instance has been established.
On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> nit: when the connection

Done.

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.h:42: virtual void OnStopped() = 0;
On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> Do you have an example of an instance where OnAborting() would need to be
called
> more than once before calling OnStopped()? Is there any way we can avoid that?
> 
> I think I'd prefer to have only OnStopped() with a StopReason parameter (where
> it can be BOOT_FAILURE, CRASH, or CLEAN_TERMINATION). At least that way the
> opt-in flow can have a slightly simpler implementation.

It is possible ArcBridgeBootstrap encounters multiple errors. For example, both
ArcBridgeBootstrapImpl::AcceptInstanceConnection() failure and ARC container
crash can happen at the same time.

Anyway, I agree adding StopReason parameter to OnStopped() is easier to use from
callers. I've updated the patch to add a OnStopped()/OnBridgeStopped()
parameter, instead of adding a new observer method.

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:66: // ARC instance boot failed for unknown
errors.
On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> nit: it's better to say that this is just any error that happens during the
> setup before the instance is actually attempted to be started.

Thanks, I've updated the comment. Does it look good to you?

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:80: // Called whenever the bridge got ready.
On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> nit: the bridge is ready.

Done.

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:89: // Called whenever the bridge stopped.
On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> nit: the bridge is stopped.

Done.

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service_unittest.cc:153:
ASSERT_EQ(ArcBridgeService::AbortReason::CRASH, abort_reason_);
On 2016/07/11 05:40:08, hidehiko wrote:
> Could you add GENERIC_BOOT_FAILURE case, too?

Done.

[Luis Héctor Chávez, 2016-07-11 22:28:50]

Just nits at this point, so deferring to hidehiko@ to avoid further
cross-timezone roundtrips.

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

https://codereview.chromium.org/2133653002/diff/60001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:66: // ARC instance boot failed for unknown
errors.
On 2016/07/11 08:25:19, Shuhei Takahashi wrote:
> On 2016/07/08 18:01:02, Luis Héctor Chávez wrote:
> > nit: it's better to say that this is just any error that happens during the
> > setup before the instance is actually attempted to be started.
> 
> Thanks, I've updated the comment. Does it look good to you?

It does, thanks!

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

https://codereview.chromium.org/2133653002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:67: NO_ERROR,
nit: I'd rename this to SHUTDOWN (and the description should be something along
the lines of "Shutdown was called and the instance was cleanly stopped"), since
"no error" is not a reason, but shutdown is.

https://codereview.chromium.org/2133653002/diff/100001/components/arc/test/fa...
File components/arc/test/fake_arc_bridge_instance.h (right):

https://codereview.chromium.org/2133653002/diff/100001/components/arc/test/fa...
components/arc/test/fake_arc_bridge_instance.h:45: void
SimulateCrash(ArcBridgeService::StopReason reason);
nit: I would rename this to Stop(), since it doesn't necessarily simulate a
crash. Also, the delegate method should be OnStopped.

[Shuhei Takahashi, 2016-07-12 06:13:57]

hidehiko: PTAL

Thanks for reviewing, Luis.

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

https://codereview.chromium.org/2133653002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:67: NO_ERROR,
On 2016/07/11 22:28:50, Luis Héctor Chávez wrote:
> nit: I'd rename this to SHUTDOWN (and the description should be something
along
> the lines of "Shutdown was called and the instance was cleanly stopped"),
since
> "no error" is not a reason, but shutdown is.

Sure, done.

https://codereview.chromium.org/2133653002/diff/100001/components/arc/test/fa...
File components/arc/test/fake_arc_bridge_instance.h (right):

https://codereview.chromium.org/2133653002/diff/100001/components/arc/test/fa...
components/arc/test/fake_arc_bridge_instance.h:45: void
SimulateCrash(ArcBridgeService::StopReason reason);
On 2016/07/11 22:28:50, Luis Héctor Chávez wrote:
> nit: I would rename this to Stop(), since it doesn't necessarily simulate a
> crash. Also, the delegate method should be OnStopped.

Done.

[hidehiko, 2016-07-12 17:01:49]

LGTM with minor comment. Sorry for the delay.

Please update your commit message, before sending it to CQ.

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

https://codereview.chromium.org/2133653002/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.h:23: using StopReason =
ArcBridgeService::StopReason;
This looks a part of Delegate interface?
(I.e. move into delegate?)

[Shuhei Takahashi, 2016-07-13 04:12:47]

Description was changed from

==========
arc: Add a callback to observe ARC instance failures.

Two new callbacks are added to observers:
- ArcBridgeBootstrap::Observer::OnAborting
- ArcBridgeService::Observer::OnBridgeAborting

By observing those callbacks, callers of the bridge can detect
boot failures and crashes of the ARC instance.

I am planning to use them soon to show notifications on those
events; see the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

to

==========
arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

[Shuhei Takahashi, 2016-07-13 04:49:38]

hidehiko: PTAL

Thanks for reviewing. Could you take another look for sanity check?

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

https://codereview.chromium.org/2133653002/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.h:23: using StopReason =
ArcBridgeService::StopReason;
On 2016/07/12 17:01:49, hidehiko wrote:
> This looks a part of Delegate interface?
> (I.e. move into delegate?)

As per offline chat, I removed this alias and used ArcBridgeService::StopReason
everywhere.

[hidehiko, 2016-07-13 04:55:20]

Still LGTM.

https://codereview.chromium.org/2133653002/diff/140001/components/arc/test/fa...
File components/arc/test/fake_arc_bridge_instance.h (right):

https://codereview.chromium.org/2133653002/diff/140001/components/arc/test/fa...
components/arc/test/fake_arc_bridge_instance.h:21: virtual void
OnStopped(StopReason reason) = 0;
Optional: to standardize other code in this CL,
ArcBridgeService::StopReason here, too?

[Shuhei Takahashi, 2016-07-13 05:01:27]

Thanks!

https://codereview.chromium.org/2133653002/diff/140001/components/arc/test/fa...
File components/arc/test/fake_arc_bridge_instance.h (right):

https://codereview.chromium.org/2133653002/diff/140001/components/arc/test/fa...
components/arc/test/fake_arc_bridge_instance.h:21: virtual void
OnStopped(StopReason reason) = 0;
On 2016/07/13 04:55:20, hidehiko wrote:
> Optional: to standardize other code in this CL,
> ArcBridgeService::StopReason here, too?

Oops, you're right. Thanks for catching.

[Shuhei Takahashi, 2016-07-13 05:02:01]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2016-07-13 05:02:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2133653002/#ps160001
(title: "Address hidehiko's comment.")

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

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 05:04:29]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Shuhei Takahashi, 2016-07-13 06:45:19]

nya@chromium.org changed reviewers:
+ msw@chromium.org

[Shuhei Takahashi, 2016-07-13 06:45:19]

msw: PTAL for chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

Rebased to tot, and now I need to touch
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc.

[msw, 2016-07-13 16:45:00]

rubber stamp lgtm

[Shuhei Takahashi, 2016-07-13 17:16:43]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2016-07-13 17:16:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2133653002/#ps180001
(title: "Rebased to master.")

[commit-bot: I haz the power, 2016-07-13 17:17:12]

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 19:54:40]

Description was changed from

==========
arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

to

==========
arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

[commit-bot: I haz the power, 2016-07-13 19:54:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-13 19:55:56]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 19:57:48]

Description was changed from

==========
arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.
==========

to

==========
arc: Notify ARC instance failures via callbacks.

StopReason is passed to following observer methods:
- ArcBridgeBootstrap::Observer::OnStopped
- ArcBridgeService::Observer::OnBridgeStopped

Those callbacks can now tell the reason the bridge stopped.

I am planning to use this soon to show notifications on failures.
See the bug entry for more details.

BUG=chromium:625923
TEST=components_unittests --gtest_filter=ArcBridgeTest.*
TEST=Manually verified that ARC boots.

Committed: https://crrev.com/0c9dabff67eaefb515e9bc39299e33811915ffb4
Cr-Commit-Position: refs/heads/master@{#405241}
==========

[commit-bot: I haz the power, 2016-07-13 19:57:50]

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