Support URL fragment resolution againt 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::Content() convenience is added to retrieve the whole spec sans-scheme.

Note the KURL blink API will not have the corresponding API change as non-hierarchical schemes never had any components parsed (i.e. path was always empty, users always hand-picked the content out of the spec in those cases). BUT KURL *will* now support non-hierarchical #fragment relative resolution as a result of this patch in line with GURL.

BUG=291747

[joth, 2013-09-14 23:40:50]

Brett - PTAL if this is inline with the direction you were after.
I probably still have some more //chrome users of path() to hunt down and change
over, thus far I've found very few. But still would like to get feedback on the
core change.

Also: can I run this test against blink specific test trybots somehow?

Cheers

[brettw, 2013-09-16 16:44:03]

I didn't check the details yet, let me know when that's ready. I think this is
basically what I had in mind at a high level.

I was talking to abarth about this and he recommended coordinating with the
WebKit gardener when we land this since probably some layout tests will need to
be updated.

https://codereview.chromium.org/23526048/diff/43001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228
url/gurl.h:228: bool HasContent() const {
Is there any use for this function? I'm inclined to not do this since it's
actually nontrivial to compute. Probably most callers that are interested should
just get the content and compare to the empty string if they need to do some
error checking.

https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
File url/third_party/mozilla/url_parse.h (right):

https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
url/third_party/mozilla/url_parse.h:185: Component Content() const;
Can we call this "GetContents()" here and in GURL. Inline getters are named like
vars but out-of-line ones use "Get".

[joth, 2013-09-16 19:47:33]

AFAICT blink is completely unchanged by this patch: whenever it is
inpecting KURL for a non-hierarchical protocol it would avoid path() and
instead hand-write code to skip the first N chars from spec in order to
skip past the scheme.

Although there's likely an exception to this that I've not yet found... I'm
almost ready with this patch except for the
"FormStructureBrowserTest.DataDrivenHeuristics02" failures. These load a
large page like the one below and smashes it into a data: url to load into
WebContents. I can't workout why, but for some reason that causes blink to
see a part of the page twice. (perhaps, bit we parse between first ? and
following #, i.e. the 'query' segment of our parsed dataL URL)
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...


On 16 September 2013 09:44, <brettw@chromium.org> wrote:

> I didn't check the details yet, let me know when that's ready. I think
> this is
> basically what I had in mind at a high level.
>
> I was talking to abarth about this and he recommended coordinating with the
> WebKit gardener when we land this since probably some layout tests will
> need to
> be updated.
>
>
>
https://codereview.chromium.**org/23526048/diff/43001/url/**gurl.h<https://co...
> File url/gurl.h (right):
>
> https://codereview.chromium.**org/23526048/diff/43001/url/**
>
gurl.h#newcode228<https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228>
> url/gurl.h:228: bool HasContent() const {
> Is there any use for this function? I'm inclined to not do this since
> it's actually nontrivial to compute. Probably most callers that are
> interested should just get the content and compare to the empty string
> if they need to do some error checking.
>
> https://codereview.chromium.**org/23526048/diff/43001/url/**
>
third_party/mozilla/url_parse.**h<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h>
> File url/third_party/mozilla/url_**parse.h (right):
>
> https://codereview.chromium.**org/23526048/diff/43001/url/**
>
third_party/mozilla/url_parse.**h#newcode185<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h#newcode185>
> url/third_party/mozilla/url_**parse.h:185: Component Content() const;
> Can we call this "GetContents()" here and in GURL. Inline getters are
> named like vars but out-of-line ones use "Get".
>
>
https://codereview.chromium.**org/23526048/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-09-16 22:02:55]

OK I got a handle on the problem.

All the links and sub-resources in the html in
chrome/test/data/autofill/heuristics/input/  have been set to be "#" (see
https://chromiumcodereview.appspot.com/22040002)

Prior to my CL these would all fail to resolve relatively against the base
data:.... URL and so would default back to "about:blank" and so were
effectively no-ops. However with this change # can now "successfully"
resolve against the source data: URL. This is mostly harmless for the link
and img src tags, however for <iframe> tags this gives a curious effect of
causing the page to load itself recursively within the iframe, BUT
truncating on the first '#' character discovered within that data: URL (as
that's what the new resolve-relative rules say should happen when # is
resolved against "data:text/html,AB#CD").

Possible approaches to take here:-

"fix the test"
1/ modify the test to use about:blank as its base
2/ modify the html to use about:blank in those links instead of "#" (at
least, for iframes) (or even, delete iframs out of the tests as they're not
adding anything to the pages as they stand. @isherman?)

"gracefully deal with it"
3/ blacklist "data:" URLs for all relative resolve operations. (then, why
not blacklist javascript: too?)
4/ somehow address this case specifically at the iframe loading
relative-resolving code.


Other? Preference on which I should pursue first?
Cheers!







On 16 September 2013 12:47, Jonathan Dixon <joth@chromium.org> wrote:

> AFAICT blink is completely unchanged by this patch: whenever it is
> inpecting KURL for a non-hierarchical protocol it would avoid path() and
> instead hand-write code to skip the first N chars from spec in order to
> skip past the scheme.
>
> Although there's likely an exception to this that I've not yet found...
> I'm almost ready with this patch except for the
> "FormStructureBrowserTest.DataDrivenHeuristics02" failures. These load a
> large page like the one below and smashes it into a data: url to load into
> WebContents. I can't workout why, but for some reason that causes blink to
> see a part of the page twice. (perhaps, bit we parse between first ? and
> following #, i.e. the 'query' segment of our parsed dataL URL)
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...
>
>
> On 16 September 2013 09:44, <brettw@chromium.org> wrote:
>
>> I didn't check the details yet, let me know when that's ready. I think
>> this is
>> basically what I had in mind at a high level.
>>
>> I was talking to abarth about this and he recommended coordinating with
>> the
>> WebKit gardener when we land this since probably some layout tests will
>> need to
>> be updated.
>>
>>
>>
https://codereview.chromium.**org/23526048/diff/43001/url/**gurl.h<https://co...
>> File url/gurl.h (right):
>>
>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>
gurl.h#newcode228<https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228>
>> url/gurl.h:228: bool HasContent() const {
>> Is there any use for this function? I'm inclined to not do this since
>> it's actually nontrivial to compute. Probably most callers that are
>> interested should just get the content and compare to the empty string
>> if they need to do some error checking.
>>
>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>
third_party/mozilla/url_parse.**h<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h>
>> File url/third_party/mozilla/url_**parse.h (right):
>>
>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>
third_party/mozilla/url_parse.**h#newcode185<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h#newcode185>
>> url/third_party/mozilla/url_**parse.h:185: Component Content() const;
>> Can we call this "GetContents()" here and in GURL. Inline getters are
>> named like vars but out-of-line ones use "Get".
>>
>>
https://codereview.chromium.**org/23526048/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-09-16 22:08:43]

On 16 September 2013 15:02, Jonathan Dixon <joth@chromium.org> wrote:

> OK I got a handle on the problem.
>
> All the links and sub-resources in the html in
> chrome/test/data/autofill/heuristics/input/  have been set to be "#" (see
> https://chromiumcodereview.appspot.com/22040002)
>
> Wrong link :) I meant this one: http://codereview.chromium.org/7493036


> Prior to my CL these would all fail to resolve relatively against the base
> data:.... URL and so would default back to "about:blank" and so were
> effectively no-ops. However with this change # can now "successfully"
> resolve against the source data: URL. This is mostly harmless for the link
> and img src tags, however for <iframe> tags this gives a curious effect of
> causing the page to load itself recursively within the iframe, BUT
> truncating on the first '#' character discovered within that data: URL (as
> that's what the new resolve-relative rules say should happen when # is
> resolved against "data:text/html,AB#CD").
>
> Possible approaches to take here:-
>
> "fix the test"
> 1/ modify the test to use about:blank as its base
> 2/ modify the html to use about:blank in those links instead of "#" (at
> least, for iframes) (or even, delete iframs out of the tests as they're not
> adding anything to the pages as they stand. @isherman?)
>
> "gracefully deal with it"
> 3/ blacklist "data:" URLs for all relative resolve operations. (then, why
> not blacklist javascript: too?)
> 4/ somehow address this case specifically at the iframe loading
> relative-resolving code.
>
>
> Other? Preference on which I should pursue first?
> Cheers!
>
>
>
>
>
>
>
> On 16 September 2013 12:47, Jonathan Dixon <joth@chromium.org> wrote:
>
>> AFAICT blink is completely unchanged by this patch: whenever it is
>> inpecting KURL for a non-hierarchical protocol it would avoid path() and
>> instead hand-write code to skip the first N chars from spec in order to
>> skip past the scheme.
>>
>> Although there's likely an exception to this that I've not yet found...
>> I'm almost ready with this patch except for the
>> "FormStructureBrowserTest.DataDrivenHeuristics02" failures. These load a
>> large page like the one below and smashes it into a data: url to load into
>> WebContents. I can't workout why, but for some reason that causes blink to
>> see a part of the page twice. (perhaps, bit we parse between first ? and
>> following #, i.e. the 'query' segment of our parsed dataL URL)
>>
>>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...
>>
>>
>> On 16 September 2013 09:44, <brettw@chromium.org> wrote:
>>
>>> I didn't check the details yet, let me know when that's ready. I think
>>> this is
>>> basically what I had in mind at a high level.
>>>
>>> I was talking to abarth about this and he recommended coordinating with
>>> the
>>> WebKit gardener when we land this since probably some layout tests will
>>> need to
>>> be updated.
>>>
>>>
>>>
https://codereview.chromium.**org/23526048/diff/43001/url/**gurl.h<https://co...
>>> File url/gurl.h (right):
>>>
>>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>>
gurl.h#newcode228<https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228>
>>> url/gurl.h:228: bool HasContent() const {
>>> Is there any use for this function? I'm inclined to not do this since
>>> it's actually nontrivial to compute. Probably most callers that are
>>> interested should just get the content and compare to the empty string
>>> if they need to do some error checking.
>>>
>>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>>
third_party/mozilla/url_parse.**h<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h>
>>> File url/third_party/mozilla/url_**parse.h (right):
>>>
>>> https://codereview.chromium.**org/23526048/diff/43001/url/**
>>>
third_party/mozilla/url_parse.**h#newcode185<https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/url_parse.h#newcode185>
>>> url/third_party/mozilla/url_**parse.h:185: Component Content() const;
>>> Can we call this "GetContents()" here and in GURL. Inline getters are
>>> named like vars but out-of-line ones use "Get".
>>>
>>>
https://codereview.chromium.**org/23526048/<https://codereview.chromium.org/2...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2013-09-16 22:57:58]

I would have expected a lot of changes since suddenly data and about
URLs have reference fragments and stuff. At least in WebKit, there
were a bunch of tests that used this that broke under the Chromium
port. Maybe these were all deleted though.

Brett

On Mon, Sep 16, 2013 at 12:47 PM, Jonathan Dixon <joth@chromium.org> wrote:
> AFAICT blink is completely unchanged by this patch: whenever it is inpecting
> KURL for a non-hierarchical protocol it would avoid path() and instead
> hand-write code to skip the first N chars from spec in order to skip past
> the scheme.
>
> Although there's likely an exception to this that I've not yet found... I'm
> almost ready with this patch except for the
> "FormStructureBrowserTest.DataDrivenHeuristics02" failures. These load a
> large page like the one below and smashes it into a data: url to load into
> WebContents. I can't workout why, but for some reason that causes blink to
> see a part of the page twice. (perhaps, bit we parse between first ? and
> following #, i.e. the 'query' segment of our parsed dataL URL)
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...
>
>
> On 16 September 2013 09:44, <brettw@chromium.org> wrote:
>>
>> I didn't check the details yet, let me know when that's ready. I think
>> this is
>> basically what I had in mind at a high level.
>>
>> I was talking to abarth about this and he recommended coordinating with
>> the
>> WebKit gardener when we land this since probably some layout tests will
>> need to
>> be updated.
>>
>>
>> https://codereview.chromium.org/23526048/diff/43001/url/gurl.h
>> File url/gurl.h (right):
>>
>> https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228
>> url/gurl.h:228: bool HasContent() const {
>> Is there any use for this function? I'm inclined to not do this since
>> it's actually nontrivial to compute. Probably most callers that are
>> interested should just get the content and compare to the empty string
>> if they need to do some error checking.
>>
>>
>>
https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
>> File url/third_party/mozilla/url_parse.h (right):
>>
>>
>>
https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
>> url/third_party/mozilla/url_parse.h:185: Component Content() const;
>> Can we call this "GetContents()" here and in GURL. Inline getters are
>> named like vars but out-of-line ones use "Get".
>>
>> https://codereview.chromium.org/23526048/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-09-16 23:07:43]

ah I see! yes this sounds like something to coordinate with the gardener.

One thought: I can add the GURL::GetContent() API and update chromium code
to use that in a separate (preparatory) patch? That way the final patch to
change url_parse is more self contained. (Fewer OWNER tails to chase)



On 16 September 2013 15:57, Brett Wilson <brettw@chromium.org> wrote:

> I would have expected a lot of changes since suddenly data and about
> URLs have reference fragments and stuff. At least in WebKit, there
> were a bunch of tests that used this that broke under the Chromium
> port. Maybe these were all deleted though.
>
> Brett
>
> On Mon, Sep 16, 2013 at 12:47 PM, Jonathan Dixon <joth@chromium.org>
> wrote:
> > AFAICT blink is completely unchanged by this patch: whenever it is
> inpecting
> > KURL for a non-hierarchical protocol it would avoid path() and instead
> > hand-write code to skip the first N chars from spec in order to skip past
> > the scheme.
> >
> > Although there's likely an exception to this that I've not yet found...
> I'm
> > almost ready with this patch except for the
> > "FormStructureBrowserTest.DataDrivenHeuristics02" failures. These load a
> > large page like the one below and smashes it into a data: url to load
> into
> > WebContents. I can't workout why, but for some reason that causes blink
> to
> > see a part of the page twice. (perhaps, bit we parse between first ? and
> > following #, i.e. the 'query' segment of our parsed dataL URL)
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...
> >
> >
> > On 16 September 2013 09:44, <brettw@chromium.org> wrote:
> >>
> >> I didn't check the details yet, let me know when that's ready. I think
> >> this is
> >> basically what I had in mind at a high level.
> >>
> >> I was talking to abarth about this and he recommended coordinating with
> >> the
> >> WebKit gardener when we land this since probably some layout tests will
> >> need to
> >> be updated.
> >>
> >>
> >> https://codereview.chromium.org/23526048/diff/43001/url/gurl.h
> >> File url/gurl.h (right):
> >>
> >>
> https://codereview.chromium.org/23526048/diff/43001/url/gurl.h#newcode228
> >> url/gurl.h:228: bool HasContent() const {
> >> Is there any use for this function? I'm inclined to not do this since
> >> it's actually nontrivial to compute. Probably most callers that are
> >> interested should just get the content and compare to the empty string
> >> if they need to do some error checking.
> >>
> >>
> >>
>
https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
> >> File url/third_party/mozilla/url_parse.h (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/23526048/diff/43001/url/third_party/mozilla/u...
> >> url/third_party/mozilla/url_parse.h:185: Component Content() const;
> >> Can we call this "GetContents()" here and in GURL. Inline getters are
> >> named like vars but out-of-line ones use "Get".
> >>
> >> https://codereview.chromium.org/23526048/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2013-09-16 23:59:27]

On 2013/09/16 22:02:55, joth wrote:
> OK I got a handle on the problem.
> 
> All the links and sub-resources in the html in
> chrome/test/data/autofill/heuristics/input/  have been set to be "#" (see
> https://chromiumcodereview.appspot.com/22040002)
> 
> Prior to my CL these would all fail to resolve relatively against the base
> data:.... URL and so would default back to "about:blank" and so were
> effectively no-ops. However with this change # can now "successfully"
> resolve against the source data: URL. This is mostly harmless for the link
> and img src tags, however for <iframe> tags this gives a curious effect of
> causing the page to load itself recursively within the iframe, BUT
> truncating on the first '#' character discovered within that data: URL (as
> that's what the new resolve-relative rules say should happen when # is
> resolved against "data:text/html,AB#CD").
> 
> Possible approaches to take here:-
> 
> "fix the test"
> 1/ modify the test to use about:blank as its base
> 2/ modify the html to use about:blank in those links instead of "#" (at
> least, for iframes) (or even, delete iframs out of the tests as they're not
> adding anything to the pages as they stand. @isherman?)

If the test files are incorrect, I don't mind fixing them to be more reasonable.
 I believe that the existence of some of the <iframe>s in the DOM is relevant to
the tests but we can have them point to about:blank or some other URL that won't
cause recursive loading.

[brettw, 2013-09-17 16:47:32]

On Mon, Sep 16, 2013 at 4:07 PM, Jonathan Dixon <joth@chromium.org> wrote:
> ah I see! yes this sounds like something to coordinate with the gardener.
>
> One thought: I can add the GURL::GetContent() API and update chromium code
> to use that in a separate (preparatory) patch? That way the final patch to
> change url_parse is more self contained. (Fewer OWNER tails to chase)

This sounds like a good idea to me.

Brett

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-09-19 00:42:16]

+kristianm is going to help me out with some of the steps here.

[joth, 2013-09-20 23:00:41]

This combined patch is now obsolete as it's been split into parts:

https://codereview.chromium.org/23549039/
https://codereview.chromium.org/23835019/