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

[bugzilla-daemon at webkit.org]

Erik Arvidsson <arv at chromium.org> has denied 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 169183: Patch
https://bugs.webkit.org/attachment.cgi?id=169183&action=review

------- Additional Comments from Erik Arvidsson <arv at chromium.org>
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-do
cument.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-do
cument.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.