RESOLVED FIXED 109468
[Qt][WK2] Favor QUrl and QString over WTF::String in the Qt API layer 
https://bugs.webkit.org/show_bug.cgi?id=109468
Summary [Qt][WK2] Favor QUrl and QString over WTF::String in the Qt API layer
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.