Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for

Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for
the same host.

Since there's no referrer when going from HTTPS to HTTP,
ExternalNavigationHandler incorrectly determines that an intent chooser should
be shown even when navigating to a link within the same host.

This uses WebContents' getLastCommittedUrl() to provide a fallback and avoid
showing an intent chooser in this case.

Adds ChromeBrowserTestRule as a JUnit rule so that AccountManagerHelper is
initialized properly, since some recent changes caused tests that rely on its
initialization to fail unless now explicitly initialized.

BUG=702089
Review-Url: https://codereview.chromium.org/2885983002
Cr-Commit-Position: refs/heads/master@{#472984}
Committed: https://chromium.googlesource.com/chromium/src/+/344a463dc6925a89b86baabffa306cf569303e01

[troyhildebrandt, 2017-05-16 23:28:40]

Description was changed from

==========
Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for
the same host.

Since there's no referrer when going from HTTPS to HTTP,
ExternalNavigationHandler incorrectly determines that an intent chooser should
be shown even when navigating to a link within the same host.

This uses WebContents' getLastCommittedUrl() to provide a fallback and avoid
showing an intent chooser in this case.

Adds ChromeBrowserTestRule as a JUnit rule so that AccountManagerHelper is
initialized properly, since some recent changes caused tests that rely on its
initialization to fail unless now explicitly initialized.

BUG=702089
==========

to

==========
Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for
the same host.

Since there's no referrer when going from HTTPS to HTTP,
ExternalNavigationHandler incorrectly determines that an intent chooser should
be shown even when navigating to a link within the same host.

This uses WebContents' getLastCommittedUrl() to provide a fallback and avoid
showing an intent chooser in this case.

Adds ChromeBrowserTestRule as a JUnit rule so that AccountManagerHelper is
initialized properly, since some recent changes caused tests that rely on its
initialization to fail unless now explicitly initialized.

BUG=702089
==========

[troyhildebrandt, 2017-05-16 23:28:40]

thildebr@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Maria, 2017-05-17 00:29:19]

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:560:
if (tab == null || tab.getWebContents() == null) return false;
Can you add a TODO to this method to see if we can replace what we have below
with the call to getLastCommittedUrl() or if this returns something subtly
different.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:601:
return mTab.getWebContents().getLastCommittedUrl();
You should do null checks on tab and web contents

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
: mDelegate.getPreviousUrl();
did you run git cl format on this CL, the formatting looks a bit odd to me.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
: mDelegate.getPreviousUrl();
I wonder if we should use previous URL by default and fallback to referrer.
Using the previous URL has the advantage of a full URL which may affect the
resolution of the previous intent.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ChromeBrowserTestRule.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ChromeBrowserTestRule.java:30:
public class ChromeBrowserTestRule implements TestRule {
Seems like this ought to inherit from NativeLibraryTestRule or at the very least
utilize NativeLibraryTestCommon for shared code.

[troyhildebrandt, 2017-05-17 20:19:10]

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:560:
if (tab == null || tab.getWebContents() == null) return false;
On 2017/05/17 00:29:18, Maria wrote:
> Can you add a TODO to this method to see if we can replace what we have below
> with the call to getLastCommittedUrl() or if this returns something subtly
> different.

Done.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:601:
return mTab.getWebContents().getLastCommittedUrl();
On 2017/05/17 00:29:18, Maria wrote:
> You should do null checks on tab and web contents

Done.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
: mDelegate.getPreviousUrl();
On 2017/05/17 00:29:18, Maria wrote:
> did you run git cl format on this CL, the formatting looks a bit odd to me.

Done.

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
: mDelegate.getPreviousUrl();
On 2017/05/17 00:29:18, Maria wrote:
> I wonder if we should use previous URL by default and fallback to referrer.
> Using the previous URL has the advantage of a full URL which may affect the
> resolution of the previous intent.

I've switched it, but is there any case where we'd have a referrer but no
previous URL?

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ChromeBrowserTestRule.java
(right):

https://codereview.chromium.org/2885983002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ChromeBrowserTestRule.java:30:
public class ChromeBrowserTestRule implements TestRule {
On 2017/05/17 00:29:18, Maria wrote:
> Seems like this ought to inherit from NativeLibraryTestRule or at the very
least
> utilize NativeLibraryTestCommon for shared code.

Done.

[troyhildebrandt, 2017-05-18 18:33:17]

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

[commit-bot: I haz the power, 2017-05-18 18:34:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maria, 2017-05-18 20:43:55]

lgtm w/ nits

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
? mDelegate.getPreviousUrl()
cache previous result

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java
(right):

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java:48:
@RunWith(ChromeJUnit4ClassRunner.class)
move back to base?

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java:127:
public void setUp() throws Exception {
does this still need throws Exception if tearDown doesn't?

[troyhildebrandt, 2017-05-18 21:03:35]

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:433:
? mDelegate.getPreviousUrl()
On 2017/05/18 20:43:55, Maria wrote:
> cache previous result

Done.

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java
(right):

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java:48:
@RunWith(ChromeJUnit4ClassRunner.class)
On 2017/05/18 20:43:55, Maria wrote:
> move back to base?

Done.

https://codereview.chromium.org/2885983002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java:127:
public void setUp() throws Exception {
On 2017/05/18 20:43:55, Maria wrote:
> does this still need throws Exception if tearDown doesn't?

Nope, changed.

[troyhildebrandt, 2017-05-18 21:03:53]

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

[commit-bot: I haz the power, 2017-05-18 21:05:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[troyhildebrandt, 2017-05-18 21:13:07]

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

[commit-bot: I haz the power, 2017-05-18 21:14:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[troyhildebrandt, 2017-05-18 21:36:47]

The CQ bit was unchecked by thildebr@chromium.org

[troyhildebrandt, 2017-05-18 21:36:57]

The CQ bit was checked by thildebr@chromium.org

[troyhildebrandt, 2017-05-18 21:36:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2885983002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2017-05-18 21:38:04]

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, 2017-05-19 00:08:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495143417862570,
"parent_rev": "6ea9ff3e27ed759a26158a6e71fd3d9ea7789aeb", "commit_rev":
"344a463dc6925a89b86baabffa306cf569303e01"}

[commit-bot: I haz the power, 2017-05-19 00:09:08]

Description was changed from

==========
Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for
the same host.

Since there's no referrer when going from HTTPS to HTTP,
ExternalNavigationHandler incorrectly determines that an intent chooser should
be shown even when navigating to a link within the same host.

This uses WebContents' getLastCommittedUrl() to provide a fallback and avoid
showing an intent chooser in this case.

Adds ChromeBrowserTestRule as a JUnit rule so that AccountManagerHelper is
initialized properly, since some recent changes caused tests that rely on its
initialization to fail unless now explicitly initialized.

BUG=702089
==========

to

==========
Fixes intent chooser being shown incorrectly when going from HTTPS to HTTP for
the same host.

Since there's no referrer when going from HTTPS to HTTP,
ExternalNavigationHandler incorrectly determines that an intent chooser should
be shown even when navigating to a link within the same host.

This uses WebContents' getLastCommittedUrl() to provide a fallback and avoid
showing an intent chooser in this case.

Adds ChromeBrowserTestRule as a JUnit rule so that AccountManagerHelper is
initialized properly, since some recent changes caused tests that rely on its
initialization to fail unless now explicitly initialized.

BUG=702089

Review-Url: https://codereview.chromium.org/2885983002
Cr-Commit-Position: refs/heads/master@{#472984}
Committed:
https://chromium.googlesource.com/chromium/src/+/344a463dc6925a89b86baabffa30...
==========

[commit-bot: I haz the power, 2017-05-19 00:09:09]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/344a463dc6925a89b86baabffa30...