Support URL fragment resolution againt non-hierarchical schemes

Support URL fragment resolution against non-hierarchical schemes

Support URL fragment resolution against non-hierarchical schemes
As a result, data: about: etc now have 'query' and 'ref' components
parsed; as a result a new GURL::GetContent() convenience is added to
retrieve the spec with the scheme stripped off.

A complication in supporting this is that we now need to allow whitespace
to trailing whitespace to be preserved when transferring url_parse::Parsed
structs between KURL and GURL. Without this, the URL prior to the
#fragment can change (i.e. whitespace stripped) when following an anchor
link which breaks the page (causes reload from source). See
http://crbug.com/291747 for more details on this.

R=brettw@chromium.org
TBR=cbentzel@chromium.org
BUG=291747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236917

[joth, 2013-09-20 22:59:39]

Brett - this is now ready for full review.

Notes:
- depends on https://codereview.chromium.org/23549039/ for the new API to be
added and callers updated.
- I've taken the easy way out in the GURL url_parse::Parsed import validation,
and skipping it for non-hierarchical schemes (but added a test to explicitly
check whitespace is handled correctly). I think this approach keeps this patch
much more focus, while keeping that debug validation intact for the most
important 99% use-case of http: family urls.


Thanks!

[joth, 2013-10-20 04:31:25]

+abarth,brettw - PTAL

as per our separate email conversation, here's the updated version that allows
trailing whitespace to b retained on path URLs.
This still depends on https://codereview.chromium.org/23549039/ but that should
be landing pretty soon :)

Cheers

[joth, 2013-11-01 06:32:54]

https://codereview.chromium.org/23835019/diff/324001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/23835019/diff/324001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:714:
embedder.test.assertEq(dataUrl, webview.getAttribute('src'));
this is the sort of statement that fails without my test code edit: |dataUrl| is
a plain (unencoded) string, whereas getAttribute() is returning a GURL
canonicalized url, so now has strings as spaces.

I think my fix to always pass in encoded data: urls is correct. But oh the
irony: day after launching android webview here I am breaking the <webview> API
to fix a backward compat break we just made in the WebView API.

[brettw, 2013-11-20 00:03:51]

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc#newcode20
url/gurl.cc:20: // TODO(joth): Move to appropriate place in file, justing
putting this here
Thanks, I think you can move this now.

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc#newcode28
url/gurl.cc:28: bool trim_path_end) {
One line.

https://codereview.chromium.org/23835019/diff/694001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl.h#newcode367
url/gurl.h:367: void InitCanonical(const STR& input_spec,
One line since it fits.

https://codereview.chromium.org/23835019/diff/694001/url/gurl_unittest.cc
File url/gurl_unittest.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl_unittest.cc#new...
url/gurl_unittest.cc:374: 
Nit: extra blank

https://codereview.chromium.org/23835019/diff/694001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:487: begin = parsed->scheme.end() + 1;
I'd rather introduce a new after_scheme variable here than changing what "begin"
means midfunction.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc
File url/url_canon_pathurl.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:16: template<typename CHAR, typename UCHAR>
This should probably have a comment that separator can be 0 to skip adding.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:21: url_parse::Component* new_parsed) {
Can this be called "new_component" instead?

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:61: success &= DoCanonicalizePathComponent<CHAR,
UCHAR>(
Can you add a comment here that for path URLs we let them have these components,
but canonicalize them using the weaker path URL rules.

It should work, but I want to be sure we get a test case for ReplaceComponents
where we change the scheme from a path to a regular one and the canonicalization
of these elements are updated to the more strict rules, and we lose any trailing
whitespace.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_relative.cc
File url/url_canon_relative.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_relative.c...
url/url_canon_relative.cc:104: url_parse::ParsePathURL(url, url_len, true,
&url_parsed);
This codepath is called a lot and ParsePathURL and CountCharactersBefore aren't
super fast. Is there a way we can get what you need with less work?

https://codereview.chromium.org/23835019/diff/694001/url/url_parse_internal.h
File url/url_parse_internal.h (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_parse_internal.h...
url/url_parse_internal.h:37: if (!trim_path_end)
This would read a little easier for me with e condition reversed (white loop
inside if block).

[joth, 2013-11-21 00:08:44]

Thanks for the review Brett! Think I got everything.

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc#newcode20
url/gurl.cc:20: // TODO(joth): Move to appropriate place in file, justing
putting this here
On 2013/11/20 00:03:51, brettw wrote:
> Thanks, I think you can move this now.

Done.

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc#newcode28
url/gurl.cc:28: bool trim_path_end) {
On 2013/11/20 00:03:51, brettw wrote:
> One line.

Done.

https://codereview.chromium.org/23835019/diff/694001/url/gurl.cc#newcode148
url/gurl.cc:148: GURL test_url(spec_, RETAIN_TRAILING_PATH_WHITEPACE);
I added an extra comment here too:

     // We need to retain trailing whitespace on path URLs, as the |parsed_|
     // spec we originally received may legitimately contain trailing white-
     // space on the path or  components e.g. if the #ref has been
     // removed from a "foo:hello #ref" URL (see http://crbug.com/291747).

https://codereview.chromium.org/23835019/diff/694001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl.h#newcode367
url/gurl.h:367: void InitCanonical(const STR& input_spec,
On 2013/11/20 00:03:51, brettw wrote:
> One line since it fits.

Done.

https://codereview.chromium.org/23835019/diff/694001/url/gurl_unittest.cc
File url/gurl_unittest.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/gurl_unittest.cc#new...
url/gurl_unittest.cc:374: 
On 2013/11/20 00:03:51, brettw wrote:
> Nit: extra blank

Done.

https://codereview.chromium.org/23835019/diff/694001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:487: begin = parsed->scheme.end() + 1;
On 2013/11/20 00:03:51, brettw wrote:
> I'd rather introduce a new after_scheme variable here than changing what
"begin"
> means midfunction.

Done.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc
File url/url_canon_pathurl.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:16: template<typename CHAR, typename UCHAR>
On 2013/11/20 00:03:51, brettw wrote:
> This should probably have a comment that separator can be 0 to skip adding.

Done.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:21: url_parse::Component* new_parsed) {
On 2013/11/20 00:03:51, brettw wrote:
> Can this be called "new_component" instead?

Done.

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_pathurl.cc...
url/url_canon_pathurl.cc:61: success &= DoCanonicalizePathComponent<CHAR,
UCHAR>(
On 2013/11/20 00:03:51, brettw wrote:
> Can you add a comment here that for path URLs we let them have these
components,
> but canonicalize them using the weaker path URL rules.
> 

Done.

> It should work, but I want to be sure we get a test case for ReplaceComponents
> where we change the scheme from a path to a regular one and the
canonicalization
> of these elements are updated to the more strict rules, and we lose any
trailing
> whitespace.

Added a case to
URLUtilTest, ReplaceScheme

"myscheme:example.com/ hello # world " =>
"http://example.com/%20hello%20# world"

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_relative.cc
File url/url_canon_relative.cc (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_canon_relative.c...
url/url_canon_relative.cc:104: url_parse::ParsePathURL(url, url_len, true,
&url_parsed);
On 2013/11/20 00:03:51, brettw wrote:
> This codepath is called a lot and ParsePathURL and CountCharactersBefore
aren't
> super fast. Is there a way we can get what you need with less work?

Done.

Reverted to the scheme lookup to as it was before (url_parse::ExtractScheme) and
looking for bare fragment by inspecting the first char of url for '#'

https://codereview.chromium.org/23835019/diff/694001/url/url_parse_internal.h
File url/url_parse_internal.h (right):

https://codereview.chromium.org/23835019/diff/694001/url/url_parse_internal.h...
url/url_parse_internal.h:37: if (!trim_path_end)
On 2013/11/20 00:03:51, brettw wrote:
> This would read a little easier for me with e condition reversed (white loop
> inside if block).

Done.

[brettw, 2013-11-21 23:26:07]

lgtm

https://codereview.chromium.org/23835019/diff/824001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/23835019/diff/824001/url/url_util_unittest.cc...
url/url_util_unittest.cc:277: {"about:blank", "#id42", true, "about:blank#id42"
},
Can you also add a test for resolving " #id42" (with space at the beginning? I
think this should be skipped.

[joth, 2013-11-22 06:00:44]

Thanks for you time on this Brett! I'll send a PSA to chromium-dev when it lands
I think, just in case this is visible to any other users.

https://codereview.chromium.org/23835019/diff/824001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/23835019/diff/824001/url/url_util_unittest.cc...
url/url_util_unittest.cc:277: {"about:blank", "#id42", true, "about:blank#id42"
},
On 2013/11/21 23:26:07, brettw wrote:
> Can you also add a test for resolving " #id42" (with space at the beginning? I
> think this should be skipped.

Done. (Your assumption is right).
This test also covers a small bug I had noticed & fixed in
https://codereview.chromium.org/23835019/diff2/824001:954001/url/url_canon_re...
-- in my previous edit I was using *url to check the first char when I should
have used url[begin]

[commit-bot: I haz the power, 2013-11-22 06:03:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/23835019/954001

[commit-bot: I haz the power, 2013-11-22 06:18:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[joth, 2013-11-22 19:02:22]

+cbentzel@chromium.org for net/base/*

Thanks!

[commit-bot: I haz the power, 2013-11-23 00:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/23835019/954001

[commit-bot: I haz the power, 2013-11-23 00:50:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[joth, 2013-11-23 01:54:02]

Committed patchset #11 manually as r236917 (presubmit successful).