Bug 28852 – Rename KURL single argument constructor to avoid confusion

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Alexey Proskuryakov]

KURL(urlString) feels like something one should use by default, but it really isn't. This form can only be used with strings that are normalized in KURL sense - most notably, with strings that are returned by KURL::string() and stored as String objects to reduce memory usage as compared to KURL objects.

[Alexey Proskuryakov]

Created attachment 38830 [details]
proposed patch

[Darin Adler]

Comment on attachment 38830 [details]
proposed patch

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

[Alexey Proskuryakov]

> 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.

I think I said something similar in ChangeLog, but having a comment in code seems fine. I'm going to add the following before the constructor declaration: "It is usually best to avoid repeatedly parsing a string, unless memory saving outweigh the possible slow-downs."

> > -    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?

Yes, this is wrong. Looks like document base should serve as base for this one. I'll file a bug.

This only affects xml:base.

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

I think that the main shortcoming here is that this is not enforced by the compiler in any way.

[Alexey Proskuryakov]

Filed bug 28858 for xml:base and baseURI.

[Alexey Proskuryakov]

Committed revision 47907.