Bug 109468 - [Qt][WK2] Favor QUrl and QString over WTF::String in the Qt API layer
Summary: [Qt][WK2] Favor QUrl and QString over WTF::String in the Qt API layer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks: 109564
  Show dependency tree / graph
 
Reported: 2013-02-11 11:53 PST by Jocelyn Turcotte
Modified: 2013-03-12 04:13 PDT (History)
7 users (show)

See Also:

Attachments
Patch (13.58 KB, patch)
2013-02-11 11:55 PST, Jocelyn Turcotte 		no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2013-02-14 06:30 PST, Jocelyn Turcotte 		allan.jensen: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>