[Webkit-unassigned] [Bug 29972] Implement URL decomposition IDL attributes for HTMLAnchorElement

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42233|review?                     |review-
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2009-10-30 15:30:48 PDT ---
(From update of attachment 42233)
> +    // 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.

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