Bug 28852 - Rename KURL single argument constructor to avoid confusion
: Rename KURL single argument constructor to avoid confusion
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-31 13:04 PST by
Modified: 2009-08-31 15:07 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-31 13:04:51 PST
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 From 2009-08-31 13:25:42 PST -------
Created an attachment (id=38830) [details]
proposed patch
------- Comment #2 From 2009-08-31 13:49:13 PST -------
(From update of attachment 38830 [details])
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 From 2009-08-31 14:46:57 PST -------
> 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 From 2009-08-31 14:51:42 PST -------
Filed bug 28858 for xml:base and baseURI.
------- Comment #5 From 2009-08-31 15:07:53 PST -------
Committed revision 47907.