| Summary: | WebFrame::url() should use the one true URL | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | fishd, viettrungluu, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Adam Barth
2011-06-13 16:55:14 PDT
Created attachment 97034 [details]
Patch
Thanks. You should update the comment in WebFrame.h too.... > You should update the comment in WebFrame.h too....
Silly comments. This is WebKit! :)
Created attachment 97040 [details]
Patch
Comment on attachment 97040 [details]
Patch
please confirm that this does not regress chromium tests.
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. (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? Comment on attachment 97040 [details]
Patch
I ran this patch through "gcl try" and got green boxes. Let's try landing it for realz.
Comment on attachment 97040 [details] Patch Clearing flags on attachment: 97040 Committed r88812: <http://trac.webkit.org/changeset/88812> All reviewed patches have been landed. Closing bug. (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? 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. (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. > 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. (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. OK. Will do. Thanks. |