[webkit-reviews] review denied: [Bug 29972] Implement URL decomposition IDL attributes for HTMLAnchorElement : [Attachment 42233] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 29972: Implement URL decomposition IDL attributes for HTMLAnchorElement
https://bugs.webkit.org/show_bug.cgi?id=29972

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // Returns true if it is allowed to set the host and port for the URL
> +    // This is allowed if the URL is valid, is hierarchical and has a
> +    // server based naming authority.
> +    // IE8 allows setting the hsot for foo:// protocol, so does WebKit.
> +    bool canSetHostOrPort() const { return isHierarchical(); }

Typo: "host" is spelled "hsot" here. The comment says something about a server
based naming authority, but the function body doesn't have any code to do that.


I think a better comment would be:

    // Returns true if you can set the host and port for the URL.
    // Non-hierarchical URLs don't have a host and port.

Unless that comment is wrong in some way.

> +    // Returns true if the URL is hierarchical.
> +    bool canSetPathname() const { return isHierarchical(); }

This comment doesn't add anything. It just repeats whats in the function body.
Please leave it out or use a comment like the one I suggested above.

>      // For canonicalization, ensure we have a '/' for no path.
> -    // Only do this for http and https.
> -    if (m_protocolInHTTPFamily && pathEnd - pathStart == 0)
> +    // Do this only for hierarchical URL with protocol http or https.
> +    if (m_protocolInHTTPFamily && hierarchical && pathEnd == pathStart)
>	   *p++ = '/';

Does the bug fix above have any effect on existing code, outside the new
HTMLAnchorElement functions? I assume the answer is yes, and if so, we need a
test case to show the fix and to help us make sure the effect is a desirable
one.

Besides null it is probably worthwhile to test setting to undefined.

Generally enough here is good that I'd like to say review+, but I don't really
like those comments and I'd prefer not to land them that way, so I'll say
review- so you can fix them.