[webkit-reviews] review granted: [Bug 98184] HTMLBaseElement href attribute binding returns wrong URL : [Attachment 166903] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 98184: HTMLBaseElement href attribute binding returns wrong URL
https://bugs.webkit.org/show_bug.cgi?id=98184

Attachment 166903: Patch
https://bugs.webkit.org/attachment.cgi?id=166903&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(href
Attr)), 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:1
1
> +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?