Bug 109468

Summary: [Qt][WK2] Favor QUrl and QString over WTF::String in the Qt API layer
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, cmarcelo, hausmann, jturcotte, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109564    
Attachments:
Description Flags
Patch
none
Patch allan.jensen: review+

Description Jocelyn Turcotte 2013-02-11 11:53:40 PST
[Qt][WK2] Favor QUrl to WTF::String in the Qt API layer
Comment 1 Jocelyn Turcotte 2013-02-11 11:55:56 PST
Created attachment 187631 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-12 05:35:27 PST
Comment on attachment 187631 [details]
Patch

nice
Comment 3 Allan Sandfeld Jensen 2013-02-12 06:06:20 PST
Comment on attachment 187631 [details]
Patch

LGTM, but needs owner to approve.
Comment 4 Simon Hausmann 2013-02-13 01:56:40 PST
Comment on attachment 187631 [details]
Patch

Are you sure that this is the way to go? I recall Tor Arne deliberately converting from QUrl to WTFString for m_currentURL, etc. after a fair amount of thought.

WKURLRef is backed by a WebURL, which is a WTFString. It also has a KURL member that is parsed on-demand. Creating a WTFString from a QString is virtually free (we adopt the QStringData). Later when converting such a WTFString back to a QString, it is also free (if it was previously adopted).

QUrl on the other hand does actual url parsing as the first thing in its constructor. I feel that we only need to do this when returning a URL through our API layer, but for everything below I'm not sure QUrl's parsing gives us anything.

Perhaps we need a WKURLCreateWithQString?
Comment 5 Jocelyn Turcotte 2013-02-13 02:14:19 PST
(In reply to comment #4)
> (From update of attachment 187631 [details])
> Are you sure that this is the way to go? I recall Tor Arne deliberately converting from QUrl to WTFString for m_currentURL, etc. after a fair amount of thought.

No I'm not sure actually, see my comment on bug #109564.

> WKURLRef is backed by a WebURL, which is a WTFString. It also has a KURL member that is parsed on-demand. Creating a WTFString from a QString is virtually free (we adopt the QStringData). Later when converting such a WTFString back to a QString, it is also free (if it was previously adopted).
> 
> QUrl on the other hand does actual url parsing as the first thing in its constructor. I feel that we only need to do this when returning a URL through our API layer, but for everything below I'm not sure QUrl's parsing gives us anything.
> 
> Perhaps we need a WKURLCreateWithQString?

This sounds like a better idea, let me investigate this.
Comment 6 Jocelyn Turcotte 2013-02-14 06:30:52 PST
Created attachment 188338 [details]
Patch

Now using QUrl only for the icon URL, QString for the page URL.
Comment 7 Jocelyn Turcotte 2013-02-14 06:32:08 PST
WKURLCreateWithQString is going to be added in the patch for bug 109564.
Comment 8 Allan Sandfeld Jensen 2013-02-15 05:10:24 PST
Comment on attachment 188338 [details]
Patch

LGTM, but needs approval by owners.
Comment 9 Benjamin Poulain 2013-03-11 14:38:45 PDT
Comment on attachment 188338 [details]
Patch

Signed off by me.
Comment 10 Jocelyn Turcotte 2013-03-12 04:13:33 PDT
Committed r145512: <http://trac.webkit.org/changeset/145512>