Capture http to https navigations w/ intent picker

Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.

Committed: https://crrev.com/21236422b54f032ac6a7dcd42d4c8768129f82ae
Cr-Commit-Position: refs/heads/master@{#426610}

[djacobo_, 2016-10-19 23:25:02]

The CQ bit was checked by djacobo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 23:25:51]

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

[djacobo_, 2016-10-19 23:30:53]

Description was changed from

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.
==========

to

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.
==========

[djacobo_, 2016-10-19 23:30:53]

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

[commit-bot: I haz the power, 2016-10-20 00:41:05]

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

[commit-bot: I haz the power, 2016-10-20 00:41:06]

Dry run: This issue passed the CQ dry run.

[djacobo_, 2016-10-20 08:12:59]

PTAL

[Yusuke Sato, 2016-10-20 15:20:53]

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

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:129: // committed URL. We
don't use GetVisibleURL() since it may contains a still
nit: contain?

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:132:
referrer_url.is_empty() ? last_committed_url : referrer_url;
qq: We still need the is_empty() check because there are cases where
referrer_url is not empty but last_committed_url is empty, correct? When does
that happen? Probably a new tab or window is created for the navigation (e.g.
with <a href=... target="_blank">)?

[djacobo_, 2016-10-20 18:36:51]

The CQ bit was checked by djacobo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 18:37:41]

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

[djacobo_, 2016-10-20 18:38:14]

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

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:129: // committed URL. We
don't use GetVisibleURL() since it may contains a still
On 2016/10/20 15:20:53, Yusuke Sato wrote:
> nit: contain?

Done.

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:132:
referrer_url.is_empty() ? last_committed_url : referrer_url;
On 2016/10/20 15:20:52, Yusuke Sato wrote:
> qq: We still need the is_empty() check because there are cases where
> referrer_url is not empty but last_committed_url is empty, correct? When does
> that happen? Probably a new tab or window is created for the navigation (e.g.
> with <a href=... target="_blank">)?

Correct, the navigation started with target="_blank" will have no past entries
for a new tab, so last_committed_url can be empty whereas referrer preserves a
value.

[Yusuke Sato, 2016-10-20 19:32:05]

lgtm with a small request.

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

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:132:
referrer_url.is_empty() ? last_committed_url : referrer_url;
On 2016/10/20 18:38:14, djacobo wrote:
> On 2016/10/20 15:20:52, Yusuke Sato wrote:
> > qq: We still need the is_empty() check because there are cases where
> > referrer_url is not empty but last_committed_url is empty, correct? When
does
> > that happen? Probably a new tab or window is created for the navigation
(e.g.
> > with <a href=... target="_blank">)?
> 
> Correct, the navigation started with target="_blank" will have no past entries
> for a new tab, so last_committed_url can be empty whereas referrer preserves a
> value.

Can you mention this in the code comment?

[commit-bot: I haz the power, 2016-10-20 19:35:15]

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

[commit-bot: I haz the power, 2016-10-20 19:35:16]

Dry run: This issue passed the CQ dry run.

[djacobo_, 2016-10-20 20:06:03]

The CQ bit was checked by djacobo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 20:06:43]

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

[djacobo_, 2016-10-20 20:07:31]

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

https://codereview.chromium.org/2436923002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:132:
referrer_url.is_empty() ? last_committed_url : referrer_url;
On 2016/10/20 19:32:05, Yusuke Sato wrote:
> On 2016/10/20 18:38:14, djacobo wrote:
> > On 2016/10/20 15:20:52, Yusuke Sato wrote:
> > > qq: We still need the is_empty() check because there are cases where
> > > referrer_url is not empty but last_committed_url is empty, correct? When
> does
> > > that happen? Probably a new tab or window is created for the navigation
> (e.g.
> > > with <a href=... target="_blank">)?
> > 
> > Correct, the navigation started with target="_blank" will have no past
entries
> > for a new tab, so last_committed_url can be empty whereas referrer preserves
a
> > value.
> 
> Can you mention this in the code comment?

Done.

[Yusuke Sato, 2016-10-20 20:10:26]

lgtm, thanks!

[commit-bot: I haz the power, 2016-10-20 21:16:18]

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

[commit-bot: I haz the power, 2016-10-20 21:16:18]

Dry run: This issue passed the CQ dry run.

[djacobo_, 2016-10-20 21:17:08]

The CQ bit was checked by djacobo@google.com

[commit-bot: I haz the power, 2016-10-20 21:17: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-10-20 21:49:37]

Description was changed from

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.
==========

to

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.
==========

[commit-bot: I haz the power, 2016-10-20 21:49:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:23:14]

Description was changed from

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.
==========

to

==========
Capture http to https navigations w/ intent picker

For navigations from http to https we cannot rely on the Referrer's url
since it is cleaned due to the sanitization proccess. For this case we
will make use of GetLastCommittedURL(), the main purpose of this
information is to decide whether or not to register
ArcNavigationThrottle as a candidate to handle a given navigation.

BUG=654941
TEST=look at the bug description for repro.

Committed: https://crrev.com/21236422b54f032ac6a7dcd42d4c8768129f82ae
Cr-Commit-Position: refs/heads/master@{#426610}
==========

[commit-bot: I haz the power, 2016-10-21 13:23:15]

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