Open |current_url| in Chrome when |previous_url| is empty

Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try

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

[Yusuke Sato, 2016-08-23 22:27:04]

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

[commit-bot: I haz the power, 2016-08-23 22:28:12]

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-08-23 22:28:50]

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

[Yusuke Sato, 2016-08-23 22:28:51]

ptal

[commit-bot: I haz the power, 2016-08-23 23:25:48]

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

[commit-bot: I haz the power, 2016-08-23 23:25:49]

Dry run: This issue passed the CQ dry run.

[djacobo_, 2016-08-23 23:32:42]

lgtm

[Ben Kwa, 2016-08-24 14:24:48]

Do we understand all the scenarios under which the referrer may not be set, and
are we okay with all those scenarios never popping up a disambig?  For example,
would installing a referrer blocker cause this code to always block the
disambig?

[djacobo_, 2016-09-02 02:23:11]

On 2016/08/24 14:24:48, Ben Kwa wrote:
> Do we understand all the scenarios under which the referrer may not be set,
and
> are we okay with all those scenarios never popping up a disambig?  For
example,
> would installing a referrer blocker cause this code to always block the
> disambig?

I am not sure of all the scenarios, but I found one pretty common that would
break the intent picker flow, so please let's re-think our solution for this bug
:)

[djacobo_, 2016-09-20 01:53:52]

On 2016/09/02 02:23:11, djacobo wrote:
> On 2016/08/24 14:24:48, Ben Kwa wrote:
> > Do we understand all the scenarios under which the referrer may not be set,
> and
> > are we okay with all those scenarios never popping up a disambig?  For
> example,
> > would installing a referrer blocker cause this code to always block the
> > disambig?
> 
> I am not sure of all the scenarios, but I found one pretty common that would
> break the intent picker flow, so please let's re-think our solution for this
bug
> :)

As per our discussion offline, I am OK with this solution :)

[Yusuke Sato, 2016-09-20 20:12:36]

Description was changed from

==========
Open |current_url| in Chrome when |previous_url| is empty

BUG=640393
TEST=see repro steps in b/31043273
==========

to

==========
Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try
==========

[Yusuke Sato, 2016-09-20 20:16:08]

Sorry for the delay - I was too busy with other P1s. Replies inlined:

> I found one pretty common that would break the intent picker flow

David and I confirmed this was not true.

>  Do we understand all the scenarios under which the referrer may not be set,
and are we okay with all those scenarios never popping up a disambig?

Yes and yes. The referrer becomes empty if 1) the navigation is from https to
http (to avoid leaking the secure URL), or 2) the navigation is from a site
where sending a referrer is explicitly disabled, or 3) a Chrome extension for
removing referrers is installed.

1) is done in Referrer::SanitizeForRequest() in
content/public/common/referrer.cc (see |is_downgrade| variable.) 2) is typically
done via the site's <meta> element, the link's (<a> element's) attribute, etc.
b/31043273 is caused by 2).

> For example, would installing a referrer blocker cause this code to always
block the disambig?

This is an example of 3).

In all cases, I think not showing the picker is acceptable because false
positives (showing the picker when we shouldn't) are much more harmful for the
user than false negatives (not showing the picker when we should). The user can
usually work around false negatives by right-clicking the link or launching the
app manually, but the user doesn't always have a way to work around false
positives. False positives sometimes completely break users' work flow and
b/31043273 is a good example. I prefer to be on the safer side.

[Yusuke Sato, 2016-09-20 20:18:16]

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

[commit-bot: I haz the power, 2016-09-20 20:18:43]

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-20 20:19:50]

Added unit tests for ShouldOverrideUrlLoading() as the function is getting more
complicated. Please take another look.

[Ben Kwa, 2016-09-20 21:28:30]

LGTM with a couple of nits.

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

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:12: // A
navigation within the same domain shouldn't be overwridden.
nit: "overridden"

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:26: // If either
of two paramters is empty, the fucntion should return false.
nit: "function"

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:34: // A
navigation not within the same domain can be.
suggest: "can be overridden"

[Yusuke Sato, 2016-09-20 22:15:00]

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

[commit-bot: I haz the power, 2016-09-20 22:15:58]

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-20 22:16:08]

yusukes@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Yusuke Sato, 2016-09-20 22:16:09]

Thanks Ben.

+xiyuan, chromeos/ OWNER.

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

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:12: // A
navigation within the same domain shouldn't be overwridden.
On 2016/09/20 21:28:30, Ben Kwa wrote:
> nit: "overridden"

Done.

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:26: // If either
of two paramters is empty, the fucntion should return false.
On 2016/09/20 21:28:30, Ben Kwa wrote:
> nit: "function"

Done.

https://codereview.chromium.org/2277533002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle_unittest.cc:34: // A
navigation not within the same domain can be.
On 2016/09/20 21:28:30, Ben Kwa wrote:
> suggest: "can be overridden"

Done.

[xiyuan, 2016-09-20 22:27:59]

lgtm

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

https://codereview.chromium.org/2277533002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:77: static bool
ShouldOverrideUrlLoading(const GURL& previous_url,
nit: Move this to cc in an anonymous namespace ?

[Yusuke Sato, 2016-09-20 22:47:56]

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

https://codereview.chromium.org/2277533002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:265: // When the
navigation is initiated via e.g. JavaScript code, the referrer
Slightly modified the comment too.

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

https://codereview.chromium.org/2277533002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:77: static bool
ShouldOverrideUrlLoading(const GURL& previous_url,
On 2016/09/20 22:27:58, xiyuan wrote:
> nit: Move this to cc in an anonymous namespace ?

That's better, thanks. Done.

[Yusuke Sato, 2016-09-20 22:48:01]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-09-20 22:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from djacobo@google.com,
kenobi@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2277533002/#ps60001
(title: "Address Xiyuan's comment")

[commit-bot: I haz the power, 2016-09-20 22:48:45]

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-20 23:56:43]

Description was changed from

==========
Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try
==========

to

==========
Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try
==========

[commit-bot: I haz the power, 2016-09-20 23:56:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-20 23:58:55]

Description was changed from

==========
Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try
==========

to

==========
Open |current_url| in Chrome when |previous_url| is empty

to prevent the disambig window from being shown when it shouldn't be.

BUG=640393
TEST=see repro steps in b/31043273
TEST=try

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

[commit-bot: I haz the power, 2016-09-20 23:58:57]

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