[webkit-reviews] review denied: [Bug 76816] Implement the URL decomposition IDL attributes : [Attachment 130641] Updated Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 7 10:39:10 PST 2012
Erik Arvidsson <arv at chromium.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 76816: Implement the URL decomposition IDL attributes
https://bugs.webkit.org/show_bug.cgi?id=76816
Attachment 130641: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=130641&action=review
------- Additional Comments from Erik Arvidsson <arv at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130641&action=review
Thanks for expanding the tests. It is very helpful.
We need to test non ascii values in all of theses tests too.
I found some incorrect results. Please fix implementation.
Did we resolve how to reduce the code duplication?
> Source/WebCore/html/DOMURL.cpp:119
> + m_href = KURL(KURL(KURL(), baseURL), url);
This cannot be right? Do we really need to call KURL three times here?
> Source/WebCore/html/DOMURL.h:79
> + void setHref(const String& value) { m_href = KURL(KURL(), value); }
Same here. Can't this be done with a single call to KURL?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:23
> +
Do we want some mailto: tests? How about other common things like file system
urls?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:29
> +shouldBe("domurl.href", "'https://www.otherdomain.com:0/path/'");
As 0 a valid port?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:23
> +
Please include mailto: too
> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:30
> +shouldBe("domurl.href", "'https://www.otherdomain.com:8080/path/'");
You can use shouldBeEqualToString to simplify these
> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:43
> +shouldBe("domurl.href", "'https://:8080/path/'");
Is this really what we want?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-pathname.html:22
> +debug("pathname to null");
> +var domurl = new webkitURL("https://www.mydomain.com/?key=value");
> +shouldBeEqualToString("domurl.pathname", "/");
Try this one too
"https://www.mydomain.com?key=value"
> LayoutTests/fast/dom/DOMURL/domurl-attribute-pathname.html:23
> +
Do we want to try some ".." paths too? I'm not sure it matters
> LayoutTests/fast/dom/DOMURL/domurl-attribute-port.html:42
> +shouldBe("domurl.href", "'http://webkit.org:4/%00'");
This looks wrong
> LayoutTests/fast/dom/DOMURL/domurl-attribute-protocol.html:28
> +shouldBe("domurl.href", "'http://www.mydomain.com/path/'");
Is this the right behavior?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-protocol.html:32
> +shouldBe("domurl.href", "'http://www.mydomain.com/path/'");
Same here?
> LayoutTests/fast/dom/DOMURL/domurl-attribute-search.html:19
> +
Please test these too:
"https://www.mydomain.com/path/?a?b?c"
"https://www.mydomain.com/path/#?key=value"
"https://www.mydomain.com/path/?"
> LayoutTests/fast/dom/DOMURL/domurl-attribute-search.html:51
> +shouldBe("domurl.href", "'https://www.mydomain.com/path/?key=value'");
This is wrong. It needs to clear the search
More information about the webkit-reviews
mailing list