[webkit-reviews] review denied: [Bug 76816] Implement the URL decomposition IDL attributes : [Attachment 130641] Updated Patch

[bugzilla-daemon at webkit.org]

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