62610 2011-06-13 16:55:14 -0700 WebFrame::url() should use the one true URL 2011-06-14 13:16:00 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 abarth abarth fishd viettrungluu webkit.review.bot oldest_to_newest 420054 0 abarth 2011-06-13 16:55:14 -0700 WebFrame::url() should use the one true URL 420056 1 97034 abarth 2011-06-13 16:56:15 -0700 Created attachment 97034 Patch 420073 2 viettrungluu 2011-06-13 17:08:53 -0700 Thanks. You should update the comment in WebFrame.h too.... 420081 3 abarth 2011-06-13 17:17:39 -0700 > You should update the comment in WebFrame.h too.... Silly comments. This is WebKit! :) 420082 4 97040 abarth 2011-06-13 17:19:13 -0700 Created attachment 97040 Patch 420164 5 97040 fishd 2011-06-13 19:40:15 -0700 Comment on attachment 97040 Patch please confirm that this does not regress chromium tests. 420167 6 fishd 2011-06-13 19:43:27 -0700 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. 420221 7 abarth 2011-06-13 22:40:45 -0700 (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? 420494 8 97040 abarth 2011-06-14 09:35:17 -0700 Comment on attachment 97040 Patch I ran this patch through "gcl try" and got green boxes. Let's try landing it for realz. 420502 9 97040 webkit.review.bot 2011-06-14 09:47:02 -0700 Comment on attachment 97040 Patch Clearing flags on attachment: 97040 Committed r88812: <http://trac.webkit.org/changeset/88812> 420503 10 webkit.review.bot 2011-06-14 09:47:06 -0700 All reviewed patches have been landed. Closing bug. 420552 11 fishd 2011-06-14 10:41:42 -0700 (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? 420553 12 fishd 2011-06-14 10:42:58 -0700 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. 420556 13 fishd 2011-06-14 10:43:40 -0700 (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. 420576 14 abarth 2011-06-14 11:01:30 -0700 > 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. 420664 15 fishd 2011-06-14 12:48:43 -0700 (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. 420684 16 abarth 2011-06-14 13:16:00 -0700 OK. Will do. Thanks. 97034 2011-06-13 16:56:15 -0700 2011-06-13 17:19:10 -0700 Patch bug-62610-20110613165614.patch text/plain 1270 abarth SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA4ODczMykKKysrIFNvdXJj ZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDYtMTMgIEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZWJGcmFtZTo6dXJsKCkg c2hvdWxkIHVzZSB0aGUgb25lIHRydWUgVVJMCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD02MjYxMAorCisgICAgICAgIFRoZSBEb2N1bWVudCdzIFVSTCBp cyB0aGUgb25lIHRydWUgVVJMLiAgVGhvdSBzaGFsdCBoYXZlIG5vIFVSTHMgYmVmb3JlCisgICAg ICAgIERvY3VtZW50Ojp1cmwoKS4KKworICAgICAgICAqIHNyYy9XZWJGcmFtZUltcGwuY3BwOgor ICAgICAgICAoV2ViS2l0OjpXZWJGcmFtZUltcGw6OnVybCk6CisKIDIwMTEtMDYtMTMgIERtaXRy eSBMb21vdiAgPGRzbG9tb3ZAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBBZGFt IEJhcnRoLgpJbmRleDogU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUltcGwu Y3BwCShyZXZpc2lvbiA4ODczMSkKKysrIFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYkZy YW1lSW1wbC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUxOSwxMCArNTE5LDcgQEAgbG9uZyBsb25n IFdlYkZyYW1lSW1wbDo6aWRlbnRpZmllcigpIGNvbgogCiBXZWJVUkwgV2ViRnJhbWVJbXBsOjp1 cmwoKSBjb25zdAogewotICAgIGNvbnN0IFdlYkRhdGFTb3VyY2UqIGRzID0gZGF0YVNvdXJjZSgp OwotICAgIGlmICghZHMpCi0gICAgICAgIHJldHVybiBXZWJVUkwoKTsKLSAgICByZXR1cm4gZHMt PnJlcXVlc3QoKS51cmwoKTsKKyAgICByZXR1cm4gbV9mcmFtZS0+ZG9jdW1lbnQoKS0+dXJsKCk7 CiB9CiAKIFdlYlZlY3RvcjxXZWJJY29uVVJMPiBXZWJGcmFtZUltcGw6Omljb25VUkxzKGludCBp Y29uVHlwZXMpIGNvbnN0Cg== 97040 2011-06-13 17:19:13 -0700 2011-06-14 09:47:02 -0700 Patch bug-62610-20110613171912.patch text/plain 1928 abarth SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA4ODczMykKKysrIFNvdXJj ZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDYtMTMgIEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZWJGcmFtZTo6dXJsKCkg c2hvdWxkIHVzZSB0aGUgb25lIHRydWUgVVJMCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD02MjYxMAorCisgICAgICAgIFRoZSBEb2N1bWVudCdzIFVSTCBp cyB0aGUgb25lIHRydWUgVVJMLiAgVGhvdSBzaGFsdCBoYXZlIG5vIFVSTHMgYmVmb3JlCisgICAg ICAgIERvY3VtZW50Ojp1cmwoKS4KKworICAgICAgICAqIHNyYy9XZWJGcmFtZUltcGwuY3BwOgor ICAgICAgICAoV2ViS2l0OjpXZWJGcmFtZUltcGw6OnVybCk6CisKIDIwMTEtMDYtMTMgIERtaXRy eSBMb21vdiAgPGRzbG9tb3ZAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBBZGFt IEJhcnRoLgpJbmRleDogU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJsaWMvV2ViRnJhbWUuaAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJGcmFtZS5oCShy ZXZpc2lvbiA4ODczMSkKKysrIFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYkZyYW1l LmgJKHdvcmtpbmcgY29weSkKQEAgLTEyOCw4ICsxMjgsNyBAQCBwdWJsaWM6CiAgICAgLy8gQSBn bG9iYWxseSB1bmlxdWUgaWRlbnRpZmllciBmb3IgdGhpcyBmcmFtZS4KICAgICB2aXJ0dWFsIGxv bmcgbG9uZyBpZGVudGlmaWVyKCkgY29uc3QgPSAwOwogCi0gICAgLy8gVGhlIHVybCBvZiB0aGUg ZG9jdW1lbnQgbG9hZGVkIGluIHRoaXMgZnJhbWUuICBUaGlzIGlzIGVxdWl2YWxlbnQgdG8KLSAg ICAvLyBkYXRhU291cmNlKCktPnJlcXVlc3QoKS51cmwoKS4KKyAgICAvLyBUaGUgdXJsIG9mIHRo ZSBkb2N1bWVudCBsb2FkZWQgaW4gdGhpcyBmcmFtZS4KICAgICB2aXJ0dWFsIFdlYlVSTCB1cmwo KSBjb25zdCA9IDA7CiAKICAgICAvLyBUaGUgdXJscyBvZiB0aGUgZ2l2ZW4gY29tYmluYXRpb24g dHlwZXMgb2YgZmF2aWNvbiAoaWYgYW55KSBzcGVjaWZpZWQgYnkKSW5kZXg6IFNvdXJjZS9XZWJL aXQvY2hyb21pdW0vc3JjL1dlYkZyYW1lSW1wbC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcAkocmV2aXNpb24gODg3MzEpCisrKyBT b3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCSh3b3JraW5nIGNvcHkp CkBAIC01MTksMTAgKzUxOSw3IEBAIGxvbmcgbG9uZyBXZWJGcmFtZUltcGw6OmlkZW50aWZpZXIo KSBjb24KIAogV2ViVVJMIFdlYkZyYW1lSW1wbDo6dXJsKCkgY29uc3QKIHsKLSAgICBjb25zdCBX ZWJEYXRhU291cmNlKiBkcyA9IGRhdGFTb3VyY2UoKTsKLSAgICBpZiAoIWRzKQotICAgICAgICBy ZXR1cm4gV2ViVVJMKCk7Ci0gICAgcmV0dXJuIGRzLT5yZXF1ZXN0KCkudXJsKCk7CisgICAgcmV0 dXJuIG1fZnJhbWUtPmRvY3VtZW50KCktPnVybCgpOwogfQogCiBXZWJWZWN0b3I8V2ViSWNvblVS TD4gV2ViRnJhbWVJbXBsOjppY29uVVJMcyhpbnQgaWNvblR5cGVzKSBjb25zdAo=