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+

Jocelyn Turcotte
Reported 2013-02-11 11:53:40 PST
[Qt][WK2] Favor QUrl to WTF::String in the Qt API layer
Attachments
Patch (13.58 KB, patch)
2013-02-11 11:55 PST, Jocelyn Turcotte 		no flags
DetailsFormatted DiffDiff
Patch (9.76 KB, patch)
2013-02-14 06:30 PST, Jocelyn Turcotte 		allan.jensen: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jocelyn Turcotte
Comment 1 2013-02-11 11:55:56 PST
Created attachment 187631 [details] Patch
Kenneth Rohde Christiansen
Comment 2 2013-02-12 05:35:27 PST
Comment on attachment 187631 [details] Patch nice
Allan Sandfeld Jensen
Comment 3 2013-02-12 06:06:20 PST
Comment on attachment 187631 [details] Patch LGTM, but needs owner to approve.
Simon Hausmann
Comment 4 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?
Jocelyn Turcotte
Comment 5 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.
Jocelyn Turcotte
Comment 6 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.
Jocelyn Turcotte
Comment 7 2013-02-14 06:32:08 PST
WKURLCreateWithQString is going to be added in the patch for bug 109564.
Allan Sandfeld Jensen
Comment 8 2013-02-15 05:10:24 PST
Comment on attachment 188338 [details] Patch LGTM, but needs approval by owners.
Benjamin Poulain
Comment 9 2013-03-11 14:38:45 PDT
Comment on attachment 188338 [details] Patch Signed off by me.
Jocelyn Turcotte
Comment 10 2013-03-12 04:13:33 PDT
Committed r145512: <http://trac.webkit.org/changeset/145512>
Note You need to log in before you can comment on or make changes to this bug.