[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web Manifest

[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest scope.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*

Review-Url: https://codereview.chromium.org/2636103002
Cr-Commit-Position: refs/heads/master@{#444619}
Committed: https://chromium.googlesource.com/chromium/src/+/b9096f1be81426d51cefb1c2d839f5df9ae92192

[pkotwicz, 2017-01-17 02:02:58]

Description was changed from

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest origin.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this is
not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*
==========

to

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest origin.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*
==========

[pkotwicz, 2017-01-17 02:02:58]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2017-01-17 02:03:19]

Dominick, can you please take a look? This is somewhat high priority because
this is for M57

[dominickn, 2017-01-17 02:51:14]

On 2017/01/17 02:03:19, pkotwicz wrote:
> Dominick, can you please take a look? This is somewhat high priority because
> this is for M57

I will get to this ASAP. I'm buried triaging security bugs right now but I
should have it reviewed by the end of my day here.

[dominickn, 2017-01-17 05:23:55]

Mostly minor comments, the main one being that some tests got lost. Once you've
addressed them I can lg straight away.

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
(right):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java:155:
public static boolean isUrlWithinScope(String url, String scopeUrl) {
In a follow-up, can you see if WebappRegistry#getWebappDataStorageForUrl can use
this method as well?

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappBrowserControlsDelegate.java
(right):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappBrowserControlsDelegate.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Nit: 2017

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappBrowserControlsDelegate.java:58:
return !UrlUtilities.sameDomainOrHost(info.uri().toString(), url, true);
Side issue: news.google.com and finance.google.com are different origins, but
this method would return that they are the sameDomainOrHost.

This looks consistent with what webapps do right now, but it really should be
based on origins. Can you update the CL description to not use the word
"origin"? I'll file a bug to chase this up.

"The header is shown whenever the WebAPK is on a page outside of the Web
Manifest scope."

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java
(left):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java:49:
// Show browser controls for non secure URLs.
You should probably keep all of these non-secure URL tests.

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java
(right):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java:53:
// registries
Nit: fix the comment indentation

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java:79:
String scopeUrl, String url, int securityLevel, Type type) {
Can you call this variable webappStartUrlOrScopeUrl? It's not the scopeUrl for
non-WebAPKs, so that confused me when looking at line 86. Add a comment that
says when type == Type.WEBAPP, this is the startUrl, otherwise it's the
scopeUrl.

[pkotwicz, 2017-01-17 19:15:24]

Description was changed from

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest origin.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*
==========

to

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest scope.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*
==========

[pkotwicz, 2017-01-17 22:32:19]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2017-01-17 22:41:40]

Dominick, can you please take another look? I think that I have addressed all of
your comments

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
(right):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java:155:
public static boolean isUrlWithinScope(String url, String scopeUrl) {
Sure.

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java
(left):

https://codereview.chromium.org/2636103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java:49:
// Show browser controls for non secure URLs.
Ok I have added the tests back

This test tests three inputs to shouldShowBrowserControls():
- URL and Scope
- ConnectionSecurityLevel

I understand that it is important to test that the header is shown for
ConnectionSecurityLevel.SECURITY_WARNING in scenarios where the header would not
be shown at a lower security level.

I don't understand why it is important to test that the header is shown for
ConnectionSecurityLevel.SECURITY_WARNING in scenarios where the header would be
also shown at a lower security level.

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java
(right):

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappVisibilityTest.java:65:
// Show browser controls for non secure URLs.
I changed the order of the tests in this section so that the "useful" tests are
first.

I also added a test for ConnectionSecurityLevel.SECURITY_DANGEROUS for an URL
which would hide the header for WebAPKs

[dominickn, 2017-01-17 22:49:07]

lgtm, thanks!

[pkotwicz, 2017-01-18 01:26:01]

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

[pkotwicz, 2017-01-18 01:26:01]

mariakhomenko@ for OWNERS for the code outside of webapps/

[Maria, 2017-01-18 04:07:24]

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
(right):

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java:156:
if (url.startsWith(scopeUrl)) return true;
Slightly concerned about the safety of this check. E.g. if scope is google.com
and url is google.comevil.domain then this won't work. Not sure if such inputs
are possible here. May be safer to just call the native impl always.

[pkotwicz, 2017-01-18 16:52:21]

Maria, can you please take another look?

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
(right):

https://codereview.chromium.org/2636103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java:156:
if (url.startsWith(scopeUrl)) return true;
You are right. I have removed this if statement.

Thank you very much for catching this bug!

[Maria, 2017-01-18 17:11:08]

lgtm

https://codereview.chromium.org/2636103002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/util/UrlUtilitiesTest.java
(right):

https://codereview.chromium.org/2636103002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/util/UrlUtilitiesTest.java:155:
assertFalse(UrlUtilities.isUrlWithinScope("http://awesome.example.com",
"http://example.com"));
long line?

[pkotwicz, 2017-01-18 19:55:29]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-01-18 19:55:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2636103002/#ps80001
(title: "Merge branch 'master' into minibar")

[commit-bot: I haz the power, 2017-01-18 19:56:20]

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-01-18 21:49:02]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2017-01-19 02:16:06]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-01-19 02:16:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2636103002/#ps100001
(title: "Merge branch 'master' into minibar")

[commit-bot: I haz the power, 2017-01-19 02:16:38]

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-01-19 03:09:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484792166757560,
"parent_rev": "d31e43ce6af3cea0654218a0d4db7188addb71d3", "commit_rev":
"b9096f1be81426d51cefb1c2d839f5df9ae92192"}

[commit-bot: I haz the power, 2017-01-19 03:10:15]

Description was changed from

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest scope.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*
==========

to

==========
[WebAPKs] Show minibar when WebAPK navigates outside scope specified in Web
Manifest

This CL changes the behavior when a user taps a link in the WebAPK which
navigates out of the scope specified by the Web Manifest. Currently, the
navigation creates a new Chrome tab. The new behavior is to stay within the
WebAPK but to show a header with the page origin. The header is shown whenever
the WebAPK is on a page outside of the Web Manifest scope.

In the near future we will launch a Chrome custom tab https://crbug.com/624236
when a WebAPK navigates outside of the Web Manifest scope. Unfortunately this
is not possible for Chrome 57.

BUG=680167
TEST=ExternalNavigationHandlerTest.*, WebappVisibilityTest.*

Review-Url: https://codereview.chromium.org/2636103002
Cr-Commit-Position: refs/heads/master@{#444619}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9096f1be81426d51cefb1c2d839...
==========

[commit-bot: I haz the power, 2017-01-19 03:10:16]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b9096f1be81426d51cefb1c2d839...