Consolidate 4 GetIntentHelper functions

Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons

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

[Yusuke Sato, 2016-09-14 18:01:28]

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

[commit-bot: I haz the power, 2016-09-14 18:02:13]

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-09-14 18:17:33]

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

[commit-bot: I haz the power, 2016-09-14 18:17:34]

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

[Yusuke Sato, 2016-09-14 18:27:50]

Description was changed from

==========
wip: Consolidate 3 GetIntentHelper functions

and move it to arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try
==========

to

==========
Consolidate 3 GetIntentHelper functions

and move it to arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

[Yusuke Sato, 2016-09-14 19:57:39]

Description was changed from

==========
Consolidate 3 GetIntentHelper functions

and move it to arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

to

==========
Consolidate 4 GetIntentHelper functions

and move it to arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

[Yusuke Sato, 2016-09-14 20:01:24]

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

[commit-bot: I haz the power, 2016-09-14 20:02:41]

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

[Yusuke Sato, 2016-09-14 20:08:45]

Description was changed from

==========
Consolidate 4 GetIntentHelper functions

and move it to arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

to

==========
Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

[Yusuke Sato, 2016-09-14 20:17:51]

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

[Yusuke Sato, 2016-09-14 20:17:52]

PTAL

[Luis Héctor Chávez, 2016-09-14 20:28:49]

nice :D

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:137: *out_error_code =
GetResult::FAILED_ARC_NOT_SUPPORTED;
Shouldn't this be FAILED_ARC_NOT_READY? Unless ArcBridgeService::IsEnabled(...)
returns false.

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:81: static
mojom::IntentHelperInstance* GetIntentHelperInstance(
I wonder if it's better to provide two versions of this function to avoid having
to pass the magic nullptr as second parameter. Of course the second version will
just pass nullptr to the first one :P

[Yusuke Sato, 2016-09-14 21:02:25]

PTAL

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:137: *out_error_code =
GetResult::FAILED_ARC_NOT_SUPPORTED;
On 2016/09/14 20:28:49, Luis Héctor Chávez wrote:
> Shouldn't this be FAILED_ARC_NOT_READY? Unless
ArcBridgeService::IsEnabled(...)
> returns false.

Good point, fixed. Done.

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:81: static
mojom::IntentHelperInstance* GetIntentHelperInstance(
On 2016/09/14 20:28:49, Luis Héctor Chávez wrote:
> I wonder if it's better to provide two versions of this function to avoid
having
> to pass the magic nullptr as second parameter. Of course the second version
will
> just pass nullptr to the first one :P

I didn't do this as function overloading is discouraged (to some extent) in
Chromium, but well, in this case having two versions is probably okay. Done.

[Yusuke Sato, 2016-09-14 21:02:36]

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

[commit-bot: I haz the power, 2016-09-14 21:03:45]

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

[Luis Héctor Chávez, 2016-09-14 21:09:12]

lgtm with two optional nits.

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:81: static
mojom::IntentHelperInstance* GetIntentHelperInstance(
On 2016/09/14 21:02:25, Yusuke Sato wrote:
> On 2016/09/14 20:28:49, Luis Héctor Chávez wrote:
> > I wonder if it's better to provide two versions of this function to avoid
> having
> > to pass the magic nullptr as second parameter. Of course the second version
> will
> > just pass nullptr to the first one :P
> 
> I didn't do this as function overloading is discouraged (to some extent) in
> Chromium, but well, in this case having two versions is probably okay. Done.

You can add a "WithErrorCode" suffix if you want to avoid overloading and make
the intention clearer. Up to you.

https://codereview.chromium.org/2337193005/diff/60001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2337193005/diff/60001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:134: if
(!ArcBridgeService::GetEnabled(base::CommandLine::ForCurrentProcess())) {
Maybe only do this if |bridge_service| is nullptr. That's a much cheaper check.

[Yusuke Sato, 2016-09-14 21:20:58]

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

[commit-bot: I haz the power, 2016-09-14 21:21:20]

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

[Yusuke Sato, 2016-09-14 21:21:49]

kinaba@ PTAL

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/2337193005/diff/40001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:81: static
mojom::IntentHelperInstance* GetIntentHelperInstance(
On 2016/09/14 21:09:12, Luis Héctor Chávez wrote:
> On 2016/09/14 21:02:25, Yusuke Sato wrote:
> > On 2016/09/14 20:28:49, Luis Héctor Chávez wrote:
> > > I wonder if it's better to provide two versions of this function to avoid
> > having
> > > to pass the magic nullptr as second parameter. Of course the second
version
> > will
> > > just pass nullptr to the first one :P
> > 
> > I didn't do this as function overloading is discouraged (to some extent) in
> > Chromium, but well, in this case having two versions is probably okay. Done.
> 
> You can add a "WithErrorCode" suffix if you want to avoid overloading and make
> the intention clearer. Up to you.

Done.

https://codereview.chromium.org/2337193005/diff/60001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/2337193005/diff/60001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.cc:134: if
(!ArcBridgeService::GetEnabled(base::CommandLine::ForCurrentProcess())) {
On 2016/09/14 21:09:12, Luis Héctor Chávez wrote:
> Maybe only do this if |bridge_service| is nullptr. That's a much cheaper
check.

Done.

[commit-bot: I haz the power, 2016-09-14 22:00:47]

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

[commit-bot: I haz the power, 2016-09-14 22:00:48]

Dry run: This issue passed the CQ dry run.

[kinaba, 2016-09-14 22:29:52]

lgtm

[Yusuke Sato, 2016-09-14 22:45:16]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-09-14 22:45:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2337193005/#ps80001
(title: "Address comments2")

[commit-bot: I haz the power, 2016-09-14 22:45: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-09-14 22:52:09]

Description was changed from

==========
Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

to

==========
Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

[commit-bot: I haz the power, 2016-09-14 22:52:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-14 22:54:02]

Description was changed from

==========
Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons
==========

to

==========
Consolidate 4 GetIntentHelper functions

and have it in arc::ArcIntentHelperBridge.

BUG=b:29079617, b:31406784
TEST=try, ARC still loads activity icons

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

[commit-bot: I haz the power, 2016-09-14 22:54:03]

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