Escape url to be used in Java layer

Escape url to be used in Java layer

GURL can contain unescaped characters. So it should be escaped when it goes
to Java layer.

BUG=419257

[Jaekyun Seok (inactive), 2014-10-02 13:36:51]

jaekyun@chromium.org changed reviewers:
+ jochen@chromium.org, mkosiba@chromium.org

[Jaekyun Seok (inactive), 2014-10-02 13:36:52]

Please review this change.

[Jaekyun Seok (inactive), 2014-10-02 15:31:19]

https://codereview.chromium.org/615853006/diff/1/components/navigation_interc...
File components/navigation_interception/navigation_params_android.cc (right):

https://codereview.chromium.org/615853006/diff/1/components/navigation_interc...
components/navigation_interception/navigation_params_android.cc:19: env,
net::EscapeExternalHandlerValue(params.url().spec()));
This doesn't seem a proper method for web url escaping. For example,
"https://www.google.com/#q=s" is escaped to "https://www.google.com/%23q=s".

I'm digging more.

[Jaekyun Seok (inactive), 2014-10-02 16:30:18]

https://codereview.chromium.org/615853006/diff/1/components/navigation_interc...
File components/navigation_interception/navigation_params_android.cc (right):

https://codereview.chromium.org/615853006/diff/1/components/navigation_interc...
components/navigation_interception/navigation_params_android.cc:19: env,
net::EscapeExternalHandlerValue(params.url().spec()));
I added my own logic to escape only queries.

[mkosiba (inactive), 2014-10-02 17:54:47]

I suspect this will break WebView apps many of which expect specific strings to
appear in this callback (they shouldn't, but they do).

[Jaekyun Seok (inactive), 2014-10-02 22:28:47]

On 2014/10/02 17:54:47, mkosiba wrote:
> I suspect this will break WebView apps many of which expect specific strings
to
> appear in this callback (they shouldn't, but they do).

I don't think that escaping queries will be a problem because escaped characters
in a url query is generally expected. And this will not change any actual
contents.
For example, both "https://www.google.co.kr/webhp?q=a|b|c" and
"https://www.google.co.kr/webhp?q=a%7Cb%7Cc" will be converted to
"https://www.google.co.kr/webhp?q=a%7Cb%7Cc".

[Jaekyun Seok (inactive), 2014-10-02 23:48:22]

jaekyun@chromium.org changed reviewers:
+ erikwright@chromium.org

[Jaekyun Seok (inactive), 2014-10-02 23:48:22]

I added a function EscapeQueryParameters to escape url queries before sending it
to Java layer.

[Jaekyun Seok (inactive), 2014-10-03 00:07:18]

Could you please speed up reviewing this change a little more because the
original issue is P0?

[mkosiba (inactive), 2014-10-03 10:04:35]

On 2014/10/03 00:07:18, Jaekyun Seok wrote:
> Could you please speed up reviewing this change a little more because the
> original issue is P0?

sorry for being in the wrong timezone.

[mkosiba (inactive), 2014-10-03 10:35:33]

+cc brettw,

Brett, do you know why GURL doesn't escape the '|' (pipe) character in the query
params? Is that a bug or is it intentional?

Jaekyun,
  Like I said, WebView apps use string operations to deal with query parameters
and I'm sure many of them will _expect_ the URL string to be not escaped only
because "that's the way it used to work". 
  Luckily there is an easy way out of this problem. The navigation_interception
component can be trivially extended in the chrome/ layer. I even prepared a
patch for you: https://chrome-internal-review.googlesource.com/178308 That way
you can do whatever escaping you need in chrome without affecting the WebView.
  Finally, I see that you didn't write any tests - from my experience this kind
stuff is fiddly to get right so I suggest you add some tests. You might want to
look at url_canon_unittest for some inspiration.

[Jaekyun Seok (inactive), 2014-10-03 12:30:39]

Martin, really thank you for your review and the patch.

I will add some tests to the patch.

And I will close this issue after getting Brett's answer.

Thanks,

[brettw, 2014-10-03 16:43:43]

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

[brettw, 2014-10-03 16:43:43]

Generally it doesn't escape stuff unless it needs to. In particular for the
query section, IE is very permissive and we generally match that. | wouldn't be
allowed in some other sections.

[Jaekyun Seok (inactive), 2014-10-06 00:02:00]

Thanks for your answer. According to that, it seems reasonable to escape query
section to convert a url to a valid uri string.

[mkosiba (inactive), 2014-10-06 09:08:15]

On 2014/10/03 16:43:43, brettw wrote:
> Generally it doesn't escape stuff unless it needs to. In particular for the
> query section, IE is very permissive and we generally match that. | wouldn't
be
> allowed in some other sections.

Thanks Brett!

Given that not escaping '|' is WAI and we'd like to keep that behavior for the
WebView.

Jaekyun, looks like we can close this CL.

[Jaekyun Seok (inactive), 2014-10-06 13:15:20]

I closed this CL.