Bug 78990 - Add an empty skeleton of KURL for WTFURL
Summary: Add an empty skeleton of KURL for WTFURL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-18 22:20 PST by Benjamin Poulain
Modified: 2012-02-27 13:13 PST (History)
2 users (show)

See Also:


Attachments
Patch (31.53 KB, patch)
2012-02-18 22:32 PST, Benjamin Poulain
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-02-18 22:20:31 PST
Get something to start: an empty skeleton of KURL that we can fill with WTFURL.
Comment 1 Benjamin Poulain 2012-02-18 22:32:55 PST
Created attachment 127729 [details]
Patch
Comment 2 Adam Barth 2012-02-19 00:10:07 PST
Comment on attachment 127729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127729&action=review

This looks fine except for the RefPtr versus plain data member issue.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-22323
> -				BAB22AD414B7A02D00D8ABA6 /* FractionalLayoutUnit.h in Headers */,

Is this change spurious?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:24288
>  				144FCE5D14EC79E7000D17A3 /* FractionalLayoutSize.h in Headers */,
> +				26A5034E14F0983600AA730D /* KURLWTFURLImpl.h in Headers */,

Please run sort-xcodeproj before landing.

> Source/WebCore/platform/KURL.h:249
> +    RefPtr<KURLWTFURLImpl> m_urlImpl;

I'm not sure we want to use a RefPtr here.  That will be an extra allocation for each KURL, which seems like it would affect performance.  Notice that KURLGooglePrivate is just a plain data member.

> Source/WebCore/platform/KURLWTFURLImpl.h:37
> +class KURLWTFURLImpl : public RefCounted<KURLWTFURLImpl> {

KURLWTFURLImpl ...  Oh man.  What a name.
Comment 3 Benjamin Poulain 2012-02-19 00:29:57 PST
Thanks for the review.

> > Source/WebCore/platform/KURL.h:249
> > +    RefPtr<KURLWTFURLImpl> m_urlImpl;
> 
> I'm not sure we want to use a RefPtr here.  That will be an extra allocation for each KURL, which seems like it would affect performance.  Notice that KURLGooglePrivate is just a plain data member.

My plan was to have KURLWTFURLImpl implicitly shared.

I have a hunch we do few URL creation, and a lot more of passing KURL around. I can try to confirm this before going forward with the idea.

> > Source/WebCore/platform/KURLWTFURLImpl.h:37
> > +class KURLWTFURLImpl : public RefCounted<KURLWTFURLImpl> {
> 
> KURLWTFURLImpl ...  Oh man.  What a name.

Yep, I am not happy about any of the new filenames. :(
Hopefully we can just drop the WTFURL in the end.
Comment 4 Adam Barth 2012-02-20 19:35:45 PST
Ok.  We can do performance tests and change the design later if needed.
Comment 5 Benjamin Poulain 2012-02-25 14:31:25 PST
Committed r108907: <http://trac.webkit.org/changeset/108907>