WebView shell: automatically escape URLs

WebView shell: automatically escape URLs
    
Let WebView deal with weird URLs, rather than handling errors in the 
shell browser. WebView will automatically escape illegal characters
where appropriate, and display appropriate error pages for bad URLs.
                    
This was tested like so: 
                            
adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d URL 
                                    
where URL was:
                                            
https://example.com (should work normally)
example.com (should become http://example.com)
blah://example.com (should show WebView's ERR_UNKNOWN_URL_SCHEME page)
http://example.com/a^b (should become http://example.com/a%5Eb)
                                                                
BUG=685419
TEST=manual

Review-Url: https://codereview.chromium.org/2668273003
Cr-Commit-Position: refs/heads/master@{#447697}
Committed: https://chromium.googlesource.com/chromium/src/+/8d24b2d5f9d6e67d53f2aa8e532430f2dd096d59

[paulmiller, 2017-02-01 20:25:32]

Description was changed from

==========
WebView shell: automatically escape URLs

BUG=685419
==========

to

==========
WebView shell: automatically escape URLs

BUG=685419
==========

[paulmiller, 2017-02-01 20:25:32]

paulmiller@chromium.org changed reviewers:
+ timvolodine@chromium.org

[paulmiller, 2017-02-01 20:27:08]

On 2017/02/01 20:25:32, paulmiller wrote:
> mailto:paulmiller@chromium.org changed reviewers:
> + mailto:timvolodine@chromium.org

PTAL. I tested it with the URL from the bug:

adb shell am start -a "android.intent.action.VIEW" -d
"http://l.e.target.com/rts/go2.aspx?h=3959506\&tp=i-H55-7x-NTd-Su212k-1q-71RX1-1c-EA68-SsZ8hE-BWqbx&x=HN701201\|960248050799341171\|i-H55-7x-NTd-Su212k-1q-71RX1-1c-EA68-SsZ8hE-BWqbx"

(and also with no scheme, and with a different scheme).

[timvolodine, 2017-02-02 01:32:36]

nice, one comment below

https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
File
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java
(right):

https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:404:
if (Uri.parse(url).getScheme() == null) url = "http://" + url;
I think the original code also meant to catch things like incorrectly formed
schemes, does the new code catch this as well?

[paulmiller, 2017-02-02 01:43:20]

https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
File
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java
(right):

https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:404:
if (Uri.parse(url).getScheme() == null) url = "http://" + url;
On 2017/02/02 01:32:35, timvolodine wrote:
> I think the original code also meant to catch things like incorrectly formed
> schemes, does the new code catch this as well?

No, but I don't think we need to. If you give WebView a bad scheme, it'll give
you a "Webpage not available...net::ERR_UNKNOWN_URL_SCHEME" page. You can test
it like so:

adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d
"blah://example.com"

[timvolodine, 2017-02-02 01:56:08]

On 2017/02/02 01:43:20, paulmiller wrote:
>
https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
> File
>
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java
> (right):
> 
>
https://codereview.chromium.org/2668273003/diff/1/android_webview/tools/syste...
>
android_webview/tools/system_webview_shell/apk/src/org/chromium/webview_shell/WebViewBrowserActivity.java:404:
> if (Uri.parse(url).getScheme() == null) url = "http://" + url;
> On 2017/02/02 01:32:35, timvolodine wrote:
> > I think the original code also meant to catch things like incorrectly formed
> > schemes, does the new code catch this as well?
> 
> No, but I don't think we need to. If you give WebView a bad scheme, it'll give
> you a "Webpage not available...net::ERR_UNKNOWN_URL_SCHEME" page. You can test
> it like so:
> 
> adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d
> "blah://example.com"

okay thanks, lgtm.

perhaps would be good to add some more description (which is empty now) and
TEST=manual maybe with an explanation how to test (as you did in one of your
comments)

[paulmiller, 2017-02-02 02:24:46]

Description was changed from

==========
WebView shell: automatically escape URLs

BUG=685419
==========

to

==========
WebView shell: automatically escape URLs
    
Let WebView deal with weird URLs, rather than handling errors in the 
shell browser. WebView will automatically escape illegal characters
where appropriate, and display appropriate error pages for bad URLs.
                    
This was tested like so: 
                            
adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d URL 
                                    
where URL was:
                                            
https://example.com (should work normally)
example.com (should become http://example.com)
blah://example.com (should show WebView's ERR_UNKNOWN_URL_SCHEME page)
http://example.com/a^b (should become http://example.com/a%5Eb)
                                                                
BUG=685419
TEST=manual
==========

[paulmiller, 2017-02-02 02:27:04]

The CQ bit was checked by paulmiller@chromium.org

[paulmiller, 2017-02-02 02:27:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2668273003/#ps20001
(title: "more descriptive")

[commit-bot: I haz the power, 2017-02-02 02:27:24]

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

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486002424375530,
"parent_rev": "2cd39227cc6df6de783c7749a20d3cc5f52f382a", "commit_rev":
"8d24b2d5f9d6e67d53f2aa8e532430f2dd096d59"}

[commit-bot: I haz the power, 2017-02-02 05:05:57]

Description was changed from

==========
WebView shell: automatically escape URLs
    
Let WebView deal with weird URLs, rather than handling errors in the 
shell browser. WebView will automatically escape illegal characters
where appropriate, and display appropriate error pages for bad URLs.
                    
This was tested like so: 
                            
adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d URL 
                                    
where URL was:
                                            
https://example.com (should work normally)
example.com (should become http://example.com)
blah://example.com (should show WebView's ERR_UNKNOWN_URL_SCHEME page)
http://example.com/a^b (should become http://example.com/a%5Eb)
                                                                
BUG=685419
TEST=manual
==========

to

==========
WebView shell: automatically escape URLs
    
Let WebView deal with weird URLs, rather than handling errors in the 
shell browser. WebView will automatically escape illegal characters
where appropriate, and display appropriate error pages for bad URLs.
                    
This was tested like so: 
                            
adb shell am start -n org.chromium.webview_shell/.WebViewBrowserActivity -d URL 
                                    
where URL was:
                                            
https://example.com (should work normally)
example.com (should become http://example.com)
blah://example.com (should show WebView's ERR_UNKNOWN_URL_SCHEME page)
http://example.com/a^b (should become http://example.com/a%5Eb)
                                                                
BUG=685419
TEST=manual

Review-Url: https://codereview.chromium.org/2668273003
Cr-Commit-Position: refs/heads/master@{#447697}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d24b2d5f9d6e67d53f2aa8e5324...
==========

[commit-bot: I haz the power, 2017-02-02 05:05:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8d24b2d5f9d6e67d53f2aa8e5324...