WebView: change default shouldOverrideUrlLoading

WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=testNullContentsClientOpenAboutUrlInWebView

Review-Url: https://codereview.chromium.org/2780403002
Cr-Commit-Position: refs/heads/master@{#461966}
Committed: https://chromium.googlesource.com/chromium/src/+/570a5854119d55591e3a02ac8a471a07039bf467

[Nate Fischer, 2017-03-30 02:10:57]

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

[Nate Fischer, 2017-03-30 02:11:19]

ntfschr@chromium.org changed reviewers:
+ boliu@chromium.org

[Nate Fischer, 2017-03-30 02:11:39]

PTAL

[commit-bot: I haz the power, 2017-03-30 02:11:46]

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, 2017-03-30 03:06:56]

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

[commit-bot: I haz the power, 2017-03-30 03:06:57]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[boliu, 2017-03-30 16:59:37]

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:182:
if (url.startsWith("about:")) {
no random constants, there are url_constants.h in constant, and this feels like
a bit late, we should avoid inserting the throttle, or intercepting the
navigation altogether rather than doing this here.

also should catch chrome:

[sgurun-gerrit only, 2017-03-30 18:42:54]

sgurun@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2017-03-30 18:42:55]

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:178:
return false;
do not change the behavior on redirect.

[Nate Fischer, 2017-03-30 18:55:51]

Please see my comments on the bug as well

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:178:
return false;
On 2017/03/30 at 18:42:54, sgurun wrote:
> do not change the behavior on redirect.

No change in behavior. This only changes behavior if it's *not* a redirect and
*not* a user-initiated navigation (e.g. JavaScript-initiated navigation).

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:182:
if (url.startsWith("about:")) {
On 2017/03/30 at 16:59:37, boliu wrote:
> no random constants, there are url_constants.h in constant, and this feels
like a bit late, we should avoid inserting the throttle, or intercepting the
navigation altogether rather than doing this here.
> 
> also should catch chrome:

 - Where are the constants for Java? A quick code search can't find them
https://cs.chromium.org/search/?q=%22about:%22+lang:java+-f:chrome+-f:test+-f...
 - I thought the issue was the default shouldOverrideUrlLoading had the wrong
behavior. This sounds like the right place to me, since it's specific to the
no-webview-client-set case.
 - I'll mention this on the bug, but chrome:// URLs never get this far, so it
would be redundant to catch them here. My apologies for forgetting to mention
that.

[sgurun-gerrit only, 2017-03-30 18:57:58]

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:178:
return false;
On 2017/03/30 18:55:51, Nate Fischer wrote:
> On 2017/03/30 at 18:42:54, sgurun wrote:
> > do not change the behavior on redirect.
> 
> No change in behavior. This only changes behavior if it's *not* a redirect and
> *not* a user-initiated navigation (e.g. JavaScript-initiated navigation).

agreed, my mistake.

[boliu, 2017-03-31 16:50:26]

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/2780403002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:182:
if (url.startsWith("about:")) {
On 2017/03/30 18:55:51, Nate Fischer wrote:
> On 2017/03/30 at 16:59:37, boliu wrote:
> > no random constants, there are url_constants.h in constant, and this feels
> like a bit late, we should avoid inserting the throttle, or intercepting the
> navigation altogether rather than doing this here.
> > 
> > also should catch chrome:
> 
>  - Where are the constants for Java? A quick code search can't find them
>
https://cs.chromium.org/search/?q=%22about:%22+lang:java+-f:chrome+-f:test+-f...
>  - I thought the issue was the default shouldOverrideUrlLoading had the wrong
> behavior. This sounds like the right place to me, since it's specific to the
> no-webview-client-set case.
>  - I'll mention this on the bug, but chrome:// URLs never get this far, so it
> would be redundant to catch them here. My apologies for forgetting to mention
> that.

you can move the generic parts of
chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java into
content, and reuse that

[Nate Fischer, 2017-03-31 20:39:37]

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

[commit-bot: I haz the power, 2017-03-31 20:40:15]

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, 2017-03-31 21:39:03]

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

[commit-bot: I haz the power, 2017-03-31 21:39:05]

Dry run: This issue passed the CQ dry run.

[Nate Fischer, 2017-04-01 01:03:03]

Description was changed from

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading():

 * The WebView will always navigate to 'about:' URLs, since these are
   internal.
 * The WebView will also navigate when initiated without gesture or
   redirect. It previously ignored these navigations.

BUG=655149
TEST=Manually tested with a custom app
==========

to

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading():

 * The WebView will always navigate to 'about:' URLs, since these are
   internal.

BUG=655149
TEST=Manually tested with a custom app
==========

[Nate Fischer, 2017-04-01 01:11:02]

Description was changed from

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading():

 * The WebView will always navigate to 'about:' URLs, since these are
   internal.

BUG=655149
TEST=Manually tested with a custom app
==========

to

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=Manually tested with a custom app
==========

[Nate Fischer, 2017-04-01 01:12:13]

PTAL. This no longer touches the behavior for non-gesture-initiated navigation.
CL description has been updated to describe the new behavior.

[boliu, 2017-04-01 04:16:26]

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java
(right):

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:5:
package org.chromium.content.browser;
put this in content_public.common

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:10:
public class AboutUrlConstants {
just UrlConstants, it's likely to add more things later. and rename the
constants as appropriate

final and make a private constructor so this is cannot be instantiated

[Nate Fischer, 2017-04-03 18:04:29]

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

[commit-bot: I haz the power, 2017-04-03 18:05:25]

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

[Nate Fischer, 2017-04-03 18:24:09]

boliu@ PTAL

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java
(right):

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:5:
package org.chromium.content.browser;
On 2017/04/01 at 04:16:25, boliu wrote:
> put this in content_public.common

done

https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:10:
public class AboutUrlConstants {
On 2017/04/01 at 04:16:25, boliu wrote:
> just UrlConstants, it's likely to add more things later. and rename the
constants as appropriate
> 
> final and make a private constructor so this is cannot be instantiated

Done. This requires that I use fully-qualified names though, which may impair
readability. Any suggestions, or is this really what we want?

[boliu, 2017-04-03 18:36:09]

On 2017/04/03 18:24:09, Nate Fischer wrote:
> boliu@ PTAL
> 
>
https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java
> (right):
> 
>
https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:5:
> package org.chromium.content.browser;
> On 2017/04/01 at 04:16:25, boliu wrote:
> > put this in content_public.common
> 
> done
> 
>
https://codereview.chromium.org/2780403002/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/AboutUrlConstants.java:10:
> public class AboutUrlConstants {
> On 2017/04/01 at 04:16:25, boliu wrote:
> > just UrlConstants, it's likely to add more things later. and rename the
> constants as appropriate
> > 
> > final and make a private constructor so this is cannot be instantiated
> 
> Done. This requires that I use fully-qualified names though, which may impair
> readability. Any suggestions, or is this really what we want?

Ok, name the new thing ContentUrlConstants to avoid the name clash? Probably
should rename chrome's one ChromeUrlConstants in a future CL as well..

[Nate Fischer, 2017-04-03 19:06:40]

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

[commit-bot: I haz the power, 2017-04-03 19:07:48]

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

[Nate Fischer, 2017-04-03 19:24:35]

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

[Nate Fischer, 2017-04-03 19:24:36]

boliu@ This should address all your comments. I'll work on adding a javatest for
this.

mariakhomenko@, can you please look at the changes in chrome/android?

[boliu, 2017-04-03 19:32:52]

lgtm

[commit-bot: I haz the power, 2017-04-03 20:36:26]

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

[commit-bot: I haz the power, 2017-04-03 20:36:27]

Dry run: This issue passed the CQ dry run.

[Maria, 2017-04-03 21:17:40]

On 2017/04/03 19:32:52, boliu wrote:
> lgtm

lgtm w/ caveats

Please don't rename UrlConstants into ChromeUrlConstants -- I feel that's not
really meaningful, just as the current split made between ContentUrlConstants
and UrlConstants isn't really meaningful from a design point of view. E.g.
"chrome:" is a content URL part, but "http:" is not? I am fine with having a
common place to stick constants used both by Chrome and Webview (perhaps you
should add a comment to that effect on the Content class). I can imagine long
term all non-Chrome-feature things migrating to content and then the rename
might make sense (e.g. Content one being called UrlConstants and Chrome getting
a suffix, but we're not there yet.

[Nate Fischer, 2017-04-04 00:44:33]

On 2017/04/03 at 21:17:40, mariakhomenko wrote:
> On 2017/04/03 19:32:52, boliu wrote:
> > lgtm
> 
> lgtm w/ caveats
> 
> Please don't rename UrlConstants into ChromeUrlConstants -- I feel that's not
really meaningful, just as the current split made between ContentUrlConstants
and UrlConstants isn't really meaningful from a design point of view. E.g.
"chrome:" is a content URL part, but "http:" is not? I am fine with having a
common place to stick constants used both by Chrome and Webview (perhaps you
should add a comment to that effect on the Content class). I can imagine long
term all non-Chrome-feature things migrating to content and then the rename
might make sense (e.g. Content one being called UrlConstants and Chrome getting
a suffix, but we're not there yet.

Ack. I expanded the comment in ContentUrlConstants.

boliu@ can you take a look at the added test? This also changes another test to
use CallbackHelper instead of polling.

[Nate Fischer, 2017-04-04 01:18:35]

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

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

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, 2017-04-04 02:30:12]

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

[commit-bot: I haz the power, 2017-04-04 02:30:13]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-04-04 15:25:43]

lgtm % suggestion

https://codereview.chromium.org/2780403002/diff/80001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
(right):

https://codereview.chromium.org/2780403002/diff/80001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:60:
private static class TestDefaultContentsClient extends NullContentsClient {
can use TestAwContentsClient and only override hasWebViewClient?

[Nate Fischer, 2017-04-04 17:21:43]

https://codereview.chromium.org/2780403002/diff/80001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
(right):

https://codereview.chromium.org/2780403002/diff/80001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:60:
private static class TestDefaultContentsClient extends NullContentsClient {
On 2017/04/04 at 15:25:43, boliu wrote:
> can use TestAwContentsClient and only override hasWebViewClient?

Good suggestion, I'll do that.

[Nate Fischer, 2017-04-04 17:22:03]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-04-04 17:22:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2780403002/#ps100001
(title: "Extend TestAwContentsClient instead of NullContentsClient")

[commit-bot: I haz the power, 2017-04-04 17:22:25]

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-04-04 17:36:57]

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

[commit-bot: I haz the power, 2017-04-04 17:36:58]

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

[Nate Fischer, 2017-04-05 00:57:39]

Description was changed from

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=Manually tested with a custom app
==========

to

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=testNullContentsClientOpenAboutUrlInWebView
==========

[Nate Fischer, 2017-04-05 00:57:49]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-04-05 00:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2780403002/#ps120001
(title: "Squash commits")

[commit-bot: I haz the power, 2017-04-05 00:58:51]

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-04-05 02:12:37]

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

[commit-bot: I haz the power, 2017-04-05 02:12:38]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Nate Fischer, 2017-04-05 03:19:48]

The CQ bit was checked by ntfschr@chromium.org

[commit-bot: I haz the power, 2017-04-05 03:20:10]

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-04-05 03:25:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491362388737750,
"parent_rev": "7afabff17431ee27e1dbe217849b7ecfc78c258e", "commit_rev":
"570a5854119d55591e3a02ac8a471a07039bf467"}

[commit-bot: I haz the power, 2017-04-05 03:26:36]

Description was changed from

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=testNullContentsClientOpenAboutUrlInWebView
==========

to

==========
WebView: change default shouldOverrideUrlLoading

This CL changes the default shouldOverrideUrlLoading() for 'about:'
URLs. WebView will always navigate to these internally, instead of
creating intents if no WebViewClient is set.

This also refactors Java URL constants for about* URLs into the
content layer.

BUG=655149
TEST=testNullContentsClientOpenAboutUrlInWebView

Review-Url: https://codereview.chromium.org/2780403002
Cr-Commit-Position: refs/heads/master@{#461966}
Committed:
https://chromium.googlesource.com/chromium/src/+/570a5854119d55591e3a02ac8a47...
==========

[commit-bot: I haz the power, 2017-04-05 03:26:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/570a5854119d55591e3a02ac8a47...