Bug 62610 – WebFrame::url() should use the one true URL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

WebFrame::url() should use the one true URL

[Adam Barth]

Created attachment 97034 [details]
Patch

[Viet-Trung Luu]

Thanks. You should update the comment in WebFrame.h too....

[Adam Barth]

> You should update the comment in WebFrame.h too....

Silly comments.  This is WebKit!  :)

[Adam Barth]

Created attachment 97040 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 97040 [details]
Patch

please confirm that this does not regress chromium tests.

[Darin Fisher (:fishd, Google)]

i think it would help if WebFrame.h documented that this url may not be the same as the datasource url.  that is not obvious.  we need people to understand when to use which.

[Adam Barth]

(In reply to comment #6)
> i think it would help if WebFrame.h documented that this url may not be the same as the datasource url.  that is not obvious.  we need people to understand when to use which.

Why would anyone want to use the datasource URL?

[Adam Barth]

Comment on attachment 97040 [details]
Patch

I ran this patch through "gcl try" and got green boxes.  Let's try landing it for realz.

[WebKit Review Bot]

Comment on attachment 97040 [details]
Patch

Clearing flags on attachment: 97040

Committed r88812: <http://trac.webkit.org/changeset/88812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > i think it would help if WebFrame.h documented that this url may not be the same as the datasource url.  that is not obvious.  we need people to understand when to use which.
> 
> Why would anyone want to use the datasource URL?

Hmm, well... I'm not sure.  Maybe it is useful if you want to see what URL was used to generate the document initially?  Are you saying that we should remove it?  I think it will be hard to remove WebURLRequest::url(), so we are left with the possibility for confusion between WebFrame::url() and WebDataSource::request().url().

The previous API had them as equivalents.  Now, they are different, and yet the documentation leaves this unclear.  Have you checked the chromium codebase to see who is using WebDataSource::request().url() versus WebFrame::url()?  Maybe there are some subtle issues that will result from making them different that are not covered by tests?

[Darin Fisher (:fishd, Google)]

By the way, I would have preferred to see WebFrame::url() removed in favor of adding WebDocument::url().  That may help make the distinction more clear between these two URLs.

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> By the way, I would have preferred to see WebFrame::url() removed in favor of adding WebDocument::url().  That may help make the distinction more clear between these two URLs.

I used the past tense improperly.  "I now prefer" ... sorry for not thinking this through so well back when I first reviewed the change.

[Adam Barth]

> Hmm, well... I'm not sure.  Maybe it is useful if you want to see what URL was used to generate the document initially?  Are you saying that we should remove it?  I think it will be hard to remove WebURLRequest::url(), so we are left with the possibility for confusion between WebFrame::url() and WebDataSource::request().url().

I'm not sure there are any use cases for looking at that URL.  I suspect any code that does so is wrong ins some obscure cases.

> The previous API had them as equivalents.  Now, they are different, and yet the documentation leaves this unclear.

Which documentation?  I removed the comment in WebFrame.h.  There's no reason they're related at all.

> Have you checked the chromium codebase to see who is using WebDataSource::request().url() versus WebFrame::url()?

Nope, but I see lots of confused code in ContentSettings.  It's confused in this way and a bunch of other ways.

> Maybe there are some subtle issues that will result from making them different that are not covered by tests?

I'm not sure I understand where you're coming from.  The URL of the document is a function of the DOM.  Anything related to WebURLRequest is related to the network stack.  There's no necessary connection between the network stack and the DOM.  Any connection that might exist is an implementation detail of WebKit and not something the embedder should know about.

> By the way, I would have preferred to see WebFrame::url() removed in favor of adding WebDocument::url().  That may help make the distinction more clear between these two URLs.

Ok.  I'm happy to remove any APIs on WebFrame that relate to the document and force them all to go through WebDocument.  That's what we've been doing internally in WebCore for a while now.  In principle, WebFrame shouldn't know anything about the contained document except which document it contains.

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
> > Hmm, well... I'm not sure.  Maybe it is useful if you want to see what URL was used to generate the document initially?  Are you saying that we should remove it?  I think it will be hard to remove WebURLRequest::url(), so we are left with the possibility for confusion between WebFrame::url() and WebDataSource::request().url().
> 
> I'm not sure there are any use cases for looking at that URL.  I suspect any code that does so is wrong ins some obscure cases.

Probably so.


> > The previous API had them as equivalents.  Now, they are different, and yet the documentation leaves this unclear.
> 
> Which documentation?  I removed the comment in WebFrame.h.  There's no reason they're related at all.

I meant the _lack of_ documentation leaves this unclear.  One might guess that the two URLs are the same thing.

 
> > Have you checked the chromium codebase to see who is using WebDataSource::request().url() versus WebFrame::url()?
> 
> Nope, but I see lots of confused code in ContentSettings.  It's confused in this way and a bunch of other ways.
> 
> > Maybe there are some subtle issues that will result from making them different that are not covered by tests?
> 
> I'm not sure I understand where you're coming from.  The URL of the document is a function of the DOM.  Anything related to WebURLRequest is related to the network stack.  There's no necessary connection between the network stack and the DOM.  Any connection that might exist is an implementation detail of WebKit and not something the embedder should know about.

Maybe this change will only have the function of fixing bugs.  Or, maybe it will introduce some subtle regression?  I can't tell.


> > By the way, I would have preferred to see WebFrame::url() removed in favor of adding WebDocument::url().  That may help make the distinction more clear between these two URLs.
> 
> Ok.  I'm happy to remove any APIs on WebFrame that relate to the document and force them all to go through WebDocument.  That's what we've been doing internally in WebCore for a while now.  In principle, WebFrame shouldn't know anything about the contained document except which document it contains.

Yeah, I think moving it to WebDocument will be a win in many regards.  It makes it self-documenting that it is the URL of the document.  One can see the connection to the DOM spec if they are unsure.  It also makes the relationship to WebDataSource::request().url() more distant.  The WebDocument doesn't have a WebDataSource.

[Adam Barth]

OK.  Will do.  Thanks.