Bug 28852 - Rename KURL single argument constructor to avoid confusion
: Rename KURL single argument constructor to avoid confusion
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Alexey Proskuryakov
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-31 13:04 PDT by Alexey Proskuryakov
Modified: 2009-08-31 15:07 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (43.96 KB, patch)
2009-08-31 13:25 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2009-08-31 13:04:51 PDT
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.
Comment 1 Alexey Proskuryakov 2009-08-31 13:25:42 PDT
Created attachment 38830 [details]
proposed patch
Comment 2 Darin Adler 2009-08-31 13:49:13 PDT
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
Comment 3 Alexey Proskuryakov 2009-08-31 14:46:57 PDT
> 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.
Comment 4 Alexey Proskuryakov 2009-08-31 14:51:42 PDT
Filed bug 28858 for xml:base and baseURI.
Comment 5 Alexey Proskuryakov 2009-08-31 15:07:53 PDT
Committed revision 47907.