Add more tests to arc_navigation_throttle.cc

Add more tests to arc_navigation_throttle.cc

Also, add GetAppIndex() to ArcNavigationThrottle and use it from
ArcExternalProtocolDialog to reduce code duplication.

BUG=None
TEST=try

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

[Yusuke Sato, 2016-10-24 18:07:09]

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

[commit-bot: I haz the power, 2016-10-24 18:08:07]

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-10-24 19:24:39]

yusukes@chromium.org changed reviewers:
+ djacobo@google.com, lhchavez@chromium.org

[Yusuke Sato, 2016-10-24 19:24:39]

PTAL

Since djacobo's refactoring for c/b/c/arc/ is mostly done now, I wanted to add
some more tests for the class.

[commit-bot: I haz the power, 2016-10-24 19:36:04]

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

[commit-bot: I haz the power, 2016-10-24 19:36:04]

Dry run: This issue passed the CQ dry run.

[djacobo_, 2016-10-24 20:22:14]

just a few typos, other than that lgtm

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:78: // If Chrome browser
was selected as the preferred app, we should't
nit: shouldn't*

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:142: // When
Chrome is the first |kMaxAppResults| elements, swap is unnecessary.
nit: When Chrome is within the...

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:151: // When
Chrome is the first |kMaxAppResults| elements, swap is unnecessary.
nit: same as line 136

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:157: // When
Chrome is the |kMaxAppResults|-th elements, swap is unnecessary.
nit: element*

[Luis Héctor Chávez, 2016-10-24 20:24:35]

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:64: !(handlers.size() ==
1 && ArcIntentHelperBridge::IsIntentHelperPackage(
nit: I think it's clearer after applying de Morgan's law:

return !handlers.empty() &&
       (handlers.size() > 1 ||
        !ArcIntentHelperBridge::IsIntentHelperPackage(...));

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:82: << url_for_logging;
isn't this missing |.spec()|? Since it's not a trivial function (there's at
least one potential mutex operation), I'm not sure if we want to pass the spec
directly into the function or keep the const-reference to GURL as parameter.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:97: if
(ArcIntentHelperBridge::IsIntentHelperPackage(
qq: is it possible that the intent helper package does not appear in the list at
all? (Based on the condition in L64, it seems like it).

IIUC if that happens, it will keep |chrome_app_index| as zero and therefore not
trigger L103, but it's probably better to clearly document this.

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

https://codereview.chromium.org/2443313002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.h:91: static constexpr
char kArcIntentHelperPackageName[] =
would this cause an ODR violation?

[Yusuke Sato, 2016-10-24 22:22:32]

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

[commit-bot: I haz the power, 2016-10-24 22:23:28]

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-10-24 22:26:17]

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

[commit-bot: I haz the power, 2016-10-24 22:26:35]

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-10-24 22:27:16]

PTAL

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:64: !(handlers.size() ==
1 && ArcIntentHelperBridge::IsIntentHelperPackage(
On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> nit: I think it's clearer after applying de Morgan's law:
> 
> return !handlers.empty() &&
>        (handlers.size() > 1 ||
>         !ArcIntentHelperBridge::IsIntentHelperPackage(...));

Done.

Applied yours and shuffled the order a little bit more to make the intention
clearer.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:78: // If Chrome browser
was selected as the preferred app, we should't
On 2016/10/24 20:22:14, djacobo wrote:
> nit: shouldn't*

Done.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:82: << url_for_logging;
On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> isn't this missing |.spec()|? Since it's not a trivial function (there's at
> least one potential mutex operation), I'm not sure if we want to pass the spec
> directly into the function or keep the const-reference to GURL as parameter.

I'm not sure if I understand your concern.. could you elaborate a bit more?

operator<< for url_for_logging just returns const string& (without any mutex
operation).

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:97: if
(ArcIntentHelperBridge::IsIntentHelperPackage(
On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> qq: is it possible that the intent helper package does not appear in the list
at
> all? (Based on the condition in L64, it seems like it).

On production, Chrome always appears actually. It's not my code but David's but
anyway, L64 is for detecting the case where |handlers| only have one element and
it is Chrome.

> IIUC if that happens, it will keep |chrome_app_index| as zero and therefore
not
> trigger L103, but it's probably better to clearly document this.

Having that said, documenting it is not a bad idea at all. Done with some
additional unit tests. Thanks for the suggestion.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:142: // When
Chrome is the first |kMaxAppResults| elements, swap is unnecessary.
On 2016/10/24 20:22:14, djacobo wrote:
> nit: When Chrome is within the...

Done.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:151: // When
Chrome is the first |kMaxAppResults| elements, swap is unnecessary.
On 2016/10/24 20:22:14, djacobo wrote:
> nit: same as line 136

Done.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:157: // When
Chrome is the |kMaxAppResults|-th elements, swap is unnecessary.
On 2016/10/24 20:22:14, djacobo wrote:
> nit: element*

Done.

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

https://codereview.chromium.org/2443313002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/arc_intent_helper_bridge.h:91: static constexpr
char kArcIntentHelperPackageName[] =
On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> would this cause an ODR violation?

does it? I thought if the variable is odr-used, a definition in .cc was required
(but I may be wrong.)

anyway, I've changed it to const. No class in Chromium seems to have static
constexpr char[], and introducing it here seems like a bad idea. Done.

[Luis Héctor Chávez, 2016-10-24 22:59:54]

lgtm with nits.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:82: << url_for_logging;
On 2016/10/24 22:27:16, Yusuke Sato wrote:
> On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> > isn't this missing |.spec()|? Since it's not a trivial function (there's at
> > least one potential mutex operation), I'm not sure if we want to pass the
spec
> > directly into the function or keep the const-reference to GURL as parameter.
> 
> I'm not sure if I understand your concern.. could you elaborate a bit more?
> 
> operator<< for url_for_logging just returns const string& (without any mutex
> operation).

This snippet of code used to log |url_for_logging.spec()|. We have two options:
pass in the |url_for_logging| directly or |url_for_logging.spec()|. The latter
looks cleaner, but its definition is not inlined:
https://cs.chromium.org/chromium/src/url/gurl.cc?q=GURL::spec()&sq=package:ch...

moreover, if it's empty it needs a mutex operation to evaluate the empty spec:
https://cs.chromium.org/chromium/src/url/gurl.cc?sq=package:chromium&dr=C&rcl...

so i'm not sure what option is better. After writing all this, it looks like
keeping the GURL is fine (but the spec() part still stands). Sorry for any
confusion.

https://codereview.chromium.org/2443313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: // Returns true if
|handlers| contain one or more apps.
Re: the assumption about production always containing Chrome. Can you please add
a note here about that?

[Yusuke Sato, 2016-10-24 23:24:31]

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

[Yusuke Sato, 2016-10-24 23:25:17]

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:82: << url_for_logging;
On 2016/10/24 22:59:54, Luis Héctor Chávez wrote:
> On 2016/10/24 22:27:16, Yusuke Sato wrote:
> > On 2016/10/24 20:24:34, Luis Héctor Chávez wrote:
> > > isn't this missing |.spec()|? Since it's not a trivial function (there's
at
> > > least one potential mutex operation), I'm not sure if we want to pass the
> spec
> > > directly into the function or keep the const-reference to GURL as
parameter.
> > 
> > I'm not sure if I understand your concern.. could you elaborate a bit more?
> > 
> > operator<< for url_for_logging just returns const string& (without any mutex
> > operation).
> 
> This snippet of code used to log |url_for_logging.spec()|. We have two
options:
> pass in the |url_for_logging| directly or |url_for_logging.spec()|. The latter
> looks cleaner, but its definition is not inlined:
>
https://cs.chromium.org/chromium/src/url/gurl.cc?q=GURL::spec()&sq=package:ch...
> 
> moreover, if it's empty it needs a mutex operation to evaluate the empty spec:
>
https://cs.chromium.org/chromium/src/url/gurl.cc?sq=package:chromium&dr=C&rcl...
> 
> so i'm not sure what option is better. After writing all this, it looks like
> keeping the GURL is fine (but the spec() part still stands). Sorry for any
> confusion.

Let me use url_for_logging (w/o .spec()) for simplicity.

https://codereview.chromium.org/2443313002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:220: <<
navigation_handle()->GetURL().spec();
Removed this .spec() call too to be consistent with L52 and L82.

https://codereview.chromium.org/2443313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (right):

https://codereview.chromium.org/2443313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:61: // Returns true if
|handlers| contain one or more apps.
On 2016/10/24 22:59:54, Luis Héctor Chávez wrote:
> Re: the assumption about production always containing Chrome. Can you please
add
> a note here about that?

Done.

[commit-bot: I haz the power, 2016-10-24 23:25:32]

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-10-25 00:34:12]

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

[commit-bot: I haz the power, 2016-10-25 00:34:13]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-10-25 00:38:08]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-10-25 00:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from djacobo@google.com,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2443313002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-10-25 00:38:56]

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-10-25 00:45:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 00:48:32]

Description was changed from

==========
Add more tests to arc_navigation_throttle.cc

Also, add GetAppIndex() to ArcNavigationThrottle and use it from
ArcExternalProtocolDialog to reduce code duplication.

BUG=None
TEST=try
==========

to

==========
Add more tests to arc_navigation_throttle.cc

Also, add GetAppIndex() to ArcNavigationThrottle and use it from
ArcExternalProtocolDialog to reduce code duplication.

BUG=None
TEST=try

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

[commit-bot: I haz the power, 2016-10-25 00:48:33]

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