[Webkit-unassigned] [Bug 76816] Implement the URL decomposition IDL attributes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76816


Erik Arvidsson <arv at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #130641|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #34 from Erik Arvidsson <arv at chromium.org>  2012-03-07 10:39:10 PST ---
(From update of attachment 130641)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.