Remove explicit singletonness of ArcBridgeService part 3.

Remove explicit singletonness of ArcBridgeService part 3.

This CL replaces ArcBridgeService::Get() by
ArcServiceManager::Get()->arc_bridge_service() conceptually.
For error handling, now ArcServiceManager::Get() can return
nullptr, if not available.

BUG=657687
BUG=b/31079732
TEST=Ran bots.

Committed: https://crrev.com/4573cdea03df32f06dbf9d0357a17ea3327cde24
Cr-Commit-Position: refs/heads/master@{#437219}

[hidehiko, 2016-12-06 10:24:21]

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

[commit-bot: I haz the power, 2016-12-06 10:24:42]

Dry run: 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-06 11:02:56]

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

[commit-bot: I haz the power, 2016-12-06 11:02:57]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-06 14:43:10]

hidehiko@chromium.org changed reviewers:
+ afakhry@chromium.org, bartfab@chromium.org, dmazzoni@chromium.org,
georgesak@chromium.org, lhchavez@chromium.org, msw@chromium.org,
xiyuan@chromium.org

[hidehiko, 2016-12-06 14:43:11]

PTAL.

lhchavez@: c/b/c/arc, components/arc
bartfab@: c/b/policy, c/b/c/policy
georgesak@: c/b/memory
dmazzoni@: c/b/speech
afakhry@: c/b/task_manager
xiyuan@: c/b/ui/app_list
msw@: c/b/ui/views

[xiyuan, 2016-12-06 17:36:08]

+khmel fyi

c/b/ui/app_list lgtm

[msw, 2016-12-06 17:49:32]

c/b/ui/views lgtm with nits

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right):

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:20: #include
"components/arc/arc_bridge_service.h"
nit: remove if not needed

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:40: // A valid
|arc_app_list_prefs_| is needed for the Arc bridge service and
nit: update mention of Arc bridge service, if it's no longer relevant here.

[afakhry, 2016-12-06 18:31:37]

c/b/task_manager lgtm

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

https://codereview.chromium.org/2557513004/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2557513004/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.cc:148: auto*
intent_helper =
nit: maybe intent_helper_holder? instance_holder?

[hidehiko, 2016-12-06 19:11:29]

Thank you for review. PTAL.

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
File chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc (right):

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:20: #include
"components/arc/arc_bridge_service.h"
On 2016/12/06 17:49:32, msw wrote:
> nit: remove if not needed

Done.

https://codereview.chromium.org/2557513004/diff/1/chrome/browser/ui/views/arc...
chrome/browser/ui/views/arc_app_dialog_view_browsertest.cc:40: // A valid
|arc_app_list_prefs_| is needed for the Arc bridge service and
On 2016/12/06 17:49:32, msw wrote:
> nit: update mention of Arc bridge service, if it's no longer relevant here.

Hmm... Good catch. Actually neither ArcBridgeService nor ArcSessionManager uses
ArcAppListPrefs directly...
I've just removed the confusing comment. We may need to revisit here, later,
though. Let me keep the code L42-L46 as is, in order to focus on the
ArcBridgeService::Get() removal in this CL.

https://codereview.chromium.org/2557513004/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2557513004/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.cc:148: auto*
intent_helper =
On 2016/12/06 18:55:00, Luis Héctor Chávez wrote:
> nit: maybe intent_helper_holder? instance_holder?

Thank you for suggestion. Replaced by intent_helper_holder.

[Luis Héctor Chávez, 2016-12-06 19:29:32]

lgtm with a minor comment I'm unsure how to resolve.

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

https://codereview.chromium.org/2557513004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:258: // A reference to
ArcServiceManager is kept here so that it would crash the
I'm not sure what reference this comment is talking about. Wouldn't it be better
to do something along the lines of

arc::ArcsessionManager* arc_session_manager = arc::ArcSessionManager::Get();
if (!arc_session_manager)
  return;
DCHECK(arc::ArcServiceManager::Get());
arc_session_manager->RemoveObserver(this);
app_instance_holder_->RemoveObserver(this);

?

[Georges Khalil, 2016-12-06 20:56:46]

chrome/browser/memory/ LGTM

[bartfab (slow), 2016-12-06 21:15:39]

policy lgtm

[dmazzoni, 2016-12-07 06:34:10]

lgtm for chrome/browser/speech/tts_chromeos.cc

[hidehiko, 2016-12-08 08:35:10]

Thank you for review.

I mis-operated to upload PS3 mixing rebase and change to address Luis's comment,
so diff PS2 and 3 includes both.

Submitting.

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

https://codereview.chromium.org/2557513004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:258: // A reference to
ArcServiceManager is kept here so that it would crash the
On 2016/12/06 19:29:32, Luis Héctor Chávez wrote:
> I'm not sure what reference this comment is talking about. Wouldn't it be
better
> to do something along the lines of
> 
> arc::ArcsessionManager* arc_session_manager = arc::ArcSessionManager::Get();
> if (!arc_session_manager)
>   return;
> DCHECK(arc::ArcServiceManager::Get());
> arc_session_manager->RemoveObserver(this);
> app_instance_holder_->RemoveObserver(this);
> 
> ?

Done.

Note: the guard by arc::ArcServiceManager::Get() is to ensure
|app_instance_holder_| is not a dangling pointer here.

Also, this RemoveObserver() won't run in the real usage because;
- ArcAppListPrefs() is a BrowserContextKeyedService
- ArcSessionManager / ArcServiceManager are destruct before the
BrowserContextKeyedService instances.

We should fix the situation, but due to complicated dependencies and code
structure, we have to change some parts of ARC services as
BrowserContextKeyedService, which is yet another big refactoring. So let me keep
this as is for now, considering this is not the regression.

[hidehiko, 2016-12-08 08:35:24]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-08 08:35:24]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
msw@chromium.org, afakhry@chromium.org, bartfab@chromium.org,
lhchavez@chromium.org, georgesak@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2557513004/#ps40001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-08 08:35:36]

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-08 09:15:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481186124103000,
"parent_rev": "695b5844fb11748b9dca0e9d782b0e9f69f9d342", "commit_rev":
"8e3bad307d8b852de643860e89b38d279fa9e8fd"}

[commit-bot: I haz the power, 2016-12-08 09:16:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-08 09:18:18]

Description was changed from

==========
Remove explicit singletonness of ArcBridgeService part 3.

This CL replaces ArcBridgeService::Get() by
ArcServiceManager::Get()->arc_bridge_service() conceptually.
For error handling, now ArcServiceManager::Get() can return
nullptr, if not available.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

to

==========
Remove explicit singletonness of ArcBridgeService part 3.

This CL replaces ArcBridgeService::Get() by
ArcServiceManager::Get()->arc_bridge_service() conceptually.
For error handling, now ArcServiceManager::Get() can return
nullptr, if not available.

BUG=657687
BUG=b/31079732
TEST=Ran bots.

Committed: https://crrev.com/4573cdea03df32f06dbf9d0357a17ea3327cde24
Cr-Commit-Position: refs/heads/master@{#437219}
==========

[commit-bot: I haz the power, 2016-12-08 09:18:19]

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