[webkit-reviews] review granted: [Bug 28852] Rename KURL single argument constructor to avoid confusion : [Attachment 38830] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 28852: Rename KURL single argument constructor to avoid confusion
https://bugs.webkit.org/show_bug.cgi?id=28852

Attachment 38830: proposed patch
https://bugs.webkit.org/attachment.cgi?id=38830&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Maybe the name of this should tie in with the name of the function that lets
you get the string out of the KURL in the first place.

I wish there was a comment mentioning that it's usually a design mistake to be
re-creating the KURL from an already parsed string unless we want to save
storage by storing a string instead of a KURL. Is that true? If so, where
should we say it.

> -	   KURL completedURL = url.isEmpty() ? KURL("") : completeURL(exec,
url);
> +	   KURL completedURL = url.isEmpty() ? KURL(ParsedURLString, "") :
completeURL(exec, url);

I think a function named emptyURL() in KURL.h would be even better for cases
like this.

> -    KURL base(getAttribute(baseAttr));
> +    KURL base(ParsedURLString, getAttribute(baseAttr));

Can this possibly be right? Can't the attribute be set to anything? What
guarantees that it's a parsed URL string?

r=me on the mechanical change, clearly correct, but I think we still have a
ways to go to make this clear