Introduce ArcServiceManager::OnShutdown().

Introduce ArcServiceManager::OnShutdown().

For graceful shutdown, this CL introduces OnShutdown
observer callback.
Along with the change, EventRouter now properly calls RemoveObserver
in the callback.
ArcServiceManager::IsInitialized() is now no longer needed. Remove it.

BUG=672829
TEST=Ran bots.

Committed: https://crrev.com/772f3e537ed27631ea2350d9a26f470f0890a219
Cr-Commit-Position: refs/heads/master@{#439144}

[hidehiko, 2016-12-16 08:50:05]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 08:50:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2016-12-16 09:19:25]

hidehiko@chromium.org changed reviewers:
+ kinaba@chromium.org, lhchavez@chromium.org, yusukes@chromium.org

[hidehiko, 2016-12-16 09:19:26]

PTAL:
- yusukes@, lhchavez@: Overall design. components/arc OWNER.
- kinaba@: file_manager OWNER.

FYI:
- nya@, derat@: who may want to do similar thing.
- oka@: original author.

Thank you for your review in advance,
- hidehiko

[commit-bot: I haz the power, 2016-12-16 09:20:35]

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

[commit-bot: I haz the power, 2016-12-16 09:20:36]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2016-12-16 13:36:12]

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

[Shuhei Takahashi, 2016-12-16 13:36:13]

Drive-by.

https://codereview.chromium.org/2581953002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2581953002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:405:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
DCHECK_CURRENTLY_ON() is better because it can leave the log which thread it was
actually on on the failure.

Maybe you followed OnIntentFilteredUpdated(), but DCHECK_CURRENTLY_ON is used
everywhere except it. Strange...

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:62: static bool IsInitialized();
Please remove this declaration.

[kinaba, 2016-12-16 14:13:14]

lgtm

[Daniel Erat, 2016-12-16 16:05:43]

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

[Daniel Erat, 2016-12-16 16:05:44]

lgtm with a few comment nits

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:31: // Called when ArcServiceManager is
being shutdown. Observer implementation
nit: s/shutdown/shut down/ ("shut down" is the verb, "shutdown" is the noun)

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:32: // should call clean up ARC related
stuff here. One of the typical
nit: delete "call"?

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:33: // use case is calling
ArcServiceManager::RemoveObserver().
nit: s/case/cases/

[Luis Héctor Chávez, 2016-12-16 16:36:12]

lgtm!!!

[hidehiko, 2016-12-16 17:14:17]

Thank you for quick review!
nya@'s comments are drive-by and Yusuke is OOO, so let me send this to CQ, now.

https://codereview.chromium.org/2581953002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right):

https://codereview.chromium.org/2581953002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_manager/event_router.cc:405:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2016/12/16 13:36:13, Shuhei Takahashi wrote:
> DCHECK_CURRENTLY_ON() is better because it can leave the log which thread it
was
> actually on on the failure.
> 
> Maybe you followed OnIntentFilteredUpdated(), but DCHECK_CURRENTLY_ON is used
> everywhere except it. Strange...

Good catch. Done.

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:31: // Called when ArcServiceManager is
being shutdown. Observer implementation
On 2016/12/16 16:05:44, Daniel Erat wrote:
> nit: s/shutdown/shut down/ ("shut down" is the verb, "shutdown" is the noun)

Good to know about verb v.s. noun. Thank you. Fixed.

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:32: // should call clean up ARC related
stuff here. One of the typical
On 2016/12/16 16:05:44, Daniel Erat wrote:
> nit: delete "call"?

Done.

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:33: // use case is calling
ArcServiceManager::RemoveObserver().
On 2016/12/16 16:05:44, Daniel Erat wrote:
> nit: s/case/cases/

Done.

https://codereview.chromium.org/2581953002/diff/1/components/arc/arc_service_...
components/arc/arc_service_manager.h:62: static bool IsInitialized();
On 2016/12/16 13:36:13, Shuhei Takahashi wrote:
> Please remove this declaration.

Good catch. Done.

[hidehiko, 2016-12-16 17:14:26]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-16 17:14:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, derat@chromium.org, kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/2581953002/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-16 17:14:53]

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-12-16 17:52:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481908466401650,
"parent_rev": "ff441fd9aeb11c7eab3e72a94b59ae5c2290bec7", "commit_rev":
"c31b4c9d58eb5a51c6b6f388ddc8e2bb754c4e25"}

[commit-bot: I haz the power, 2016-12-16 17:53:16]

Description was changed from

==========
Introduce ArcServiceManager::OnShutdown().

For graceful shutdown, this CL introduces OnShutdown
observer callback.
Along with the change, EventRouter now properly calls RemoveObserver
in the callback.
ArcServiceManager::IsInitialized() is now no longer needed. Remove it.

BUG=672829
TEST=Ran bots.
==========

to

==========
Introduce ArcServiceManager::OnShutdown().

For graceful shutdown, this CL introduces OnShutdown
observer callback.
Along with the change, EventRouter now properly calls RemoveObserver
in the callback.
ArcServiceManager::IsInitialized() is now no longer needed. Remove it.

BUG=672829
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2581953002
==========

[commit-bot: I haz the power, 2016-12-16 17:53:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-16 17:55:58]

Description was changed from

==========
Introduce ArcServiceManager::OnShutdown().

For graceful shutdown, this CL introduces OnShutdown
observer callback.
Along with the change, EventRouter now properly calls RemoveObserver
in the callback.
ArcServiceManager::IsInitialized() is now no longer needed. Remove it.

BUG=672829
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2581953002
==========

to

==========
Introduce ArcServiceManager::OnShutdown().

For graceful shutdown, this CL introduces OnShutdown
observer callback.
Along with the change, EventRouter now properly calls RemoveObserver
in the callback.
ArcServiceManager::IsInitialized() is now no longer needed. Remove it.

BUG=672829
TEST=Ran bots.

Committed: https://crrev.com/772f3e537ed27631ea2350d9a26f470f0890a219
Cr-Commit-Position: refs/heads/master@{#439144}
==========

[commit-bot: I haz the power, 2016-12-16 17:55:59]

Patchset 2 (id:??) landed as
https://crrev.com/772f3e537ed27631ea2350d9a26f470f0890a219
Cr-Commit-Position: refs/heads/master@{#439144}

[Yusuke Sato, 2017-01-04 22:41:46]

lgtm thanks!!