We currently use [Reflect, URL] which resolves the reflected attribute relative to the base element of the document. In the case of the base element we need to resolve the attribute relative to the URL of the document and not the base element itself.
Created attachment 166727 [details] Patch
Comment on attachment 166727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166727&action=review > Source/WebCore/html/HTMLBaseElement.cpp:81 > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(getAttribute(hrefAttr)), document()->url()); fastGetAttribute Does this correctly if the attribute is missing?
(In reply to comment #2) > (From update of attachment 166727 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166727&action=review > > > Source/WebCore/html/HTMLBaseElement.cpp:81 > > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(getAttribute(hrefAttr)), document()->url()); > > fastGetAttribute fastGetAttribute should work but [Reflect, URL] uses getAttribute so I decided to use that... I'll change it. > Does this correctly if the attribute is missing? I tried this manually because I wasn't sure the existing tests would cover this.
Created attachment 166734 [details] Change to use fastGetAttribute
> fastGetAttribute should work but [Reflect, URL] uses getAttribute so I decided to use that... I'll change it. That's because Reflect isn't smart enough to know whether this is one of the slow attributes. :)
Is there a spec we can reference here? Have you tested the behavior in other browsers?
Does this fix a particular web site? I don't doubt that we should make this change, I'd just like to know if we're improving compat and/or interop.
http://code.google.com/p/chromium/issues/detail?id=140200
With this patch we match Firefox.
Look like a duplicate of bug 19998, and possibly bug 47348.
Comment on attachment 166734 [details] Change to use fastGetAttribute View in context: https://bugs.webkit.org/attachment.cgi?id=166734&action=review > Source/WebCore/html/HTMLBaseElement.cpp:82 > +String HTMLBaseElement::href() const > +{ > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url()); > +} How does this compile? completeURL returns a KURL but this function returns a String. This needs a comment explaining how it differs from the getURLAttribute function. It’s not enough to have it in the change log. > Source/WebCore/html/HTMLBaseElement.cpp:84 > +void HTMLBaseElement::setHref(const String& v) Please use words rather than single letters for argument names. > Source/WebCore/html/HTMLBaseElement.h:35 > + void setHref(const String&); Type here should be const AtomicString& rather than const String&, because the former is the type that setAttribute takes. The code generated by [Reflect] correctly uses AtomicString.
(In reply to comment #10) > Look like a duplicate of bug 19998, and possibly bug 47348. I did look at 19998 before filing this bug. It is not a duplicate bug. It does look like a dupe of 47348, even though the comments in there have contradictions.
*** Bug 47348 has been marked as a duplicate of this bug. ***
Created attachment 166903 [details] Patch
Comment on attachment 166903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review > Source/WebCore/ChangeLog:17 > + (WebCore): Please leave meaningless lines like this out of the change log even though the buggy script puts them in. > Source/WebCore/ChangeLog:18 > + (WebCore::HTMLBaseElement::setHref): Worth commenting on this function here. > Source/WebCore/ChangeLog:20 > + (HTMLBaseElement): Please leave meaningless lines like this out of the change log even though the buggy script puts them in. > Source/WebCore/html/HTMLBaseElement.cpp:83 > + // This does not use getURLAttribute because we need to resolve this relative to the document > + // and not relative to the base element itself. > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url()); I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element. You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut: // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL; // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s // URL and ignore the base URL. After writing that comment, I realize that this code may be incorrect. Passing the document’s URL for baseURLOverride prevents the Document::completeURL function from considering the parent document’s base URL. Is that OK? In what cases do the parent document base URLs come into play? The test case won’t cover this, because it’s only a single frame, so there’s a chance we have broken something here that is not covered by any test case. > LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase-expected.txt:11 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +Base case, rebase URL without attribute having been set > +PASS a.href is "http://old_base/foo?query" > +PASS a.href is "http://new_base/foo?query" This looks silly. You should look at other tests that run at load event time, to see if there’s a technique that can prevent writing out the results in this backwards order. > LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:19 > + base.href = 'http://old_base/'; > + a.href = 'foo?query'; > + shouldBeEqualToString('a.href', 'http://old_base/foo?query'); > + base.href = 'http://new_base/'; > + shouldBeEqualToString('a.href', 'http://new_base/foo?query'); Test cases like this can be written in a way that makes the test output much clearer. shouldBeEqualToString('base.href = "http://old_base/"; a.href = "foo?query"; a.href', 'http://old_base/foo?query'); If the test is written in this fashion then the test output makes more clear what’s being tested. > LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23 > +<script src="../../js/resources/js-test-post.js"></script> I don’t think this is needed. > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding-expected.txt:1 > +PASS It’s a little weak to have a test write out only PASS rather than indicating what it’s testing. I know it’s hard to write out URLs without being dependent on file system location, so that’s an excuse, but it’s even better if you can show what’s being tested and what the result is, somehow. > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:5 > +if (self.testRunner) It’s curious to use self.testRunner here when all other tests idiomatically use window.testRunner. Any reason to be non-traditional here?
Created attachment 166939 [details] Patch
Comment on attachment 166903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review >> Source/WebCore/html/HTMLBaseElement.cpp:83 >> + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url()); > > I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element. > > You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut: > > // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL; > // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s > // URL and ignore the base URL. > > After writing that comment, I realize that this code may be incorrect. > > Passing the document’s URL for baseURLOverride prevents the Document::completeURL function from considering the parent document’s base URL. Is that OK? In what cases do the parent document base URLs come into play? The test case won’t cover this, because it’s only a single frame, so there’s a chance we have broken something here that is not covered by any test case. Yeah, this definitely does not do what I expected. >> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23 >> +<script src="../../js/resources/js-test-post.js"></script> > > I don’t think this is needed. I cleaned up the tests a bit.
(In reply to comment #17) > (From update of attachment 166903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review > > >> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23 > >> +<script src="../../js/resources/js-test-post.js"></script> > > > > I don’t think this is needed. It's the bit that checks that the test was successfully parsed and then prints out TEST COMPLETED. We never got around to moving this into js-test-pre.js and Alexey had an objection that doing so made js-test-pre.js too complicated. I still think we should do it since people are just leaving it out now and we get worse tests.
Darin, do you want to take a second look?
Comment on attachment 166939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166939&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You need to add a description here. > Source/WebCore/html/HTMLBaseElement.cpp:82 > + // This does not use the getURLAttribute function because that will resolve relative to the documentâs base URL; Nit: documentâs > Source/WebCore/html/HTMLBaseElement.cpp:85 > + // return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url()); Why do we need to include this commented out code? > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:16 > +shouldBeTrue("endsWith(base.href, 'foo/bar/')"); > +shouldBeFalse("endsWith(base.href, 'foo/bar/foo/bar/')"); I don't understand why this test is called href-attribute-binding since we didn't make any changes to the binding code. How about something like href-attribute-resolves-with-respect-to-document?
(In reply to comment #17) > (From update of attachment 166903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review > > >> Source/WebCore/html/HTMLBaseElement.cpp:83 > >> + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url()); > > > > I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element. > > > > You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut: > > > > // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL; > > // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s > > // URL and ignore the base URL. > > > > After writing that comment, I realize that this code may be incorrect. Can we add a test to ensure we don't make the same mistake in the future?
Comment on attachment 166939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166939&action=review >> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:16 >> +shouldBeFalse("endsWith(base.href, 'foo/bar/foo/bar/')"); > > I don't understand why this test is called href-attribute-binding since we didn't make any changes to the binding code. > How about something like href-attribute-resolves-with-respect-to-document? The buggy behavior was due to us using the [Reflect, URL] in the bindings when using the default behavior does not work for this. Anyway, I'm not attached to the name. I'll rename the file.
Created attachment 168429 [details] Patch
Comment on attachment 168429 [details] Patch Attachment 168429 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14265358 New failing tests: fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html fast/dom/element-attribute-js-null.html
Created attachment 168443 [details] Patch
Comment on attachment 168443 [details] Patch Attachment 168443 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14300001 New failing tests: fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html fast/js/random-array-gc-stress.html
(In reply to comment #26) > New failing tests: > fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html I'm looking into this now. It turns out that the Apple port of DRT crashes with testRunner.setCanOpenWindows(). I filed https://bugs.webkit.org/show_bug.cgi?id=99465 Also, KURL and GURL behaves different. No surprises there :'( I'll have to handle this slightly differently
Created attachment 168956 [details] Patch
PTAL I removed the window.open test (it was not really needed to cover the issue in the first patch). I had to manually handle invalid and about:blank URL since KURL and GURL resolves URLs differently in those cases.
Created attachment 169183 [details] Patch
Comment on attachment 169183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169183&action=review > LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:16 > +base.href = 'http://old_base/'; > +a.href = 'foo?query'; > +shouldBeEqualToString('a.href', 'http://old_base/foo?query'); > +base.href = 'http://new_base/'; > +shouldBeEqualToString('a.href', 'http://new_base/foo?query'); Did you not want to do Darin's comment about making this test output cleaner? > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:8 > + // HACK: setCanOpenWindows needs the test to be async or the Apple port crashes. s/HACK/FIXME > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:49 > +// Make sure that we don't use the creator document as the base. > +debug('createHTMLDocument'); > +var doc = document.implementation.createHTMLDocument(''); > +base = doc.head.appendChild(doc.createElement('base')); > +base.setAttribute('href', 'foo/bar/'); > +shouldBeEqualToString('base.href', ''); > + > +// Make sure that we use the parent document as the base. > +debug('iframe'); > +var iframe = document.body.appendChild(document.createElement('iframe')); > +base = iframe.contentDocument.head.appendChild(doc.createElement('base')); > +base.setAttribute('href', 'foo/bar/'); > +shouldBeEqualToString('base.href', ''); > + > +// Make sure that we don't use the opener document as the base. > +debug('window.open'); > +var win = window.open('about:blank'); > +base = win.document.head.appendChild(win.document.createElement('base')); > +base.setAttribute('href', 'foo/bar/'); > +shouldBeEqualToString('base.href', ''); Sorry, I don't understand these test cases. Also, the comments don't seem to match the test cases. Maybe I don't just don't understand the patch? > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:50 > +win.close(); Is this part of the async hack?
Comment on attachment 169183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169183&action=review I'll upload a new patch >> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:16 >> +shouldBeEqualToString('a.href', 'http://new_base/foo?query'); > > Did you not want to do Darin's comment about making this test output cleaner? I'm not sure I think document.querySelector('a').href is cleaner than a.href But I'll make the change anyway. >> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:49 >> +shouldBeEqualToString('base.href', ''); > > Sorry, I don't understand these test cases. Also, the comments don't seem to match the test cases. Maybe I don't just don't understand the patch? The first patch was using the wrong method to resolve the URL. Ryosuke wanted me to test the cases that would be broken if the wrong method was used. That is why iframe, createHTMLDocument and window.open are tested. When the document does not have a URL (because it does not have a creation context) the base URL cannot be resolved and we should use "" as the value of the href. This is no change in behavior, just more tests to ensure this did not break it (like it did in the first patch). >> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:50 >> +win.close(); > > Is this part of the async hack? No. Just closing it because it is annoying to keep opening new windows when testing in other browsers.
Created attachment 169416 [details] Patch
Comment on attachment 169416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169416&action=review > LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:29 > +// Make sure that we don't use the creator document as the base. Can you add a comment that these three cases should not resolved because nested document lack a url?
Comment on attachment 169416 [details] Patch This does not do the right thing when base.href is set to a URL that has a scheme.
Created attachment 169456 [details] Patch
Comment on attachment 169456 [details] Patch Clearing flags on attachment: 169456 Committed r132071: <http://trac.webkit.org/changeset/132071>
All reviewed patches have been landed. Closing bug.
> It does look like a dupe of 47348 Are you sure this is a dupe? Bug 47348 is about changing the behavior of HTMLAnchorElement when the base URI is updated. This bug seems to be about the behavior of HTMLBaseElement.