Optimize WebURL -> GURL conversions

Optimize WebURL -> GURL conversions

When rendering sina.com.cn, we spend 5% of our total time converting between
WebURL and GURL. Prior to this CL, the conversion involved a number of copies:

1) From the String to a temporary buffer in String::utf8.
2) From the temporary buffer to a CString.
3) From the CString to a std::string.

After this CL, we are able to copy directly from the String into the GURL.
Instead of WebURL storing the spec in a CString, we now store the spec as a
String, which lets WebURL simply ref the same buffer used by KURL. When
building the GURL, we copy directly out of the String (assume it's ASCII, which
is usually the case for canonicalized URLs) into a temporary std::string, which
we move all the way into the GURL.

R=jyasskin@chromium.org
BUG=261412
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155006

[Jeffrey Yasskin, 2013-07-24 21:23:51]

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h
File public/platform/WebURL.h (right):

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h#newc...
public/platform/WebURL.h:124: return isNull() ? GURL() :
GURL(m_string.toUTF8String(), m_parsed, m_isValid);
In order to avoid a copy here, you need to use a GURL constructor that takes its
argument by value and swaps it into the member.

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h#newc...
public/platform/WebURL.h:141: return !a.string().equals(b.string());
s/!//, right?

[abarth-chromium, 2013-07-24 21:57:34]

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h
File public/platform/WebURL.h (right):

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h#newc...
public/platform/WebURL.h:124: return isNull() ? GURL() :
GURL(m_string.toUTF8String(), m_parsed, m_isValid);
On 2013/07/24 21:23:51, Jeffrey Yasskin wrote:
> In order to avoid a copy here, you need to use a GURL constructor that takes
its
> argument by value and swaps it into the member.

Sorry for not cross-linking the CLs.  The CL that introduces that constructor is
in https://codereview.chromium.org/20127002/.

https://codereview.chromium.org/20135002/diff/1/public/platform/WebURL.h#newc...
public/platform/WebURL.h:141: return !a.string().equals(b.string());
On 2013/07/24 21:23:51, Jeffrey Yasskin wrote:
> s/!//, right?

Yes!  :)

[abarth-chromium, 2013-07-24 21:58:35]

PTAL

[abarth-chromium, 2013-07-24 21:59:10]

https://codereview.chromium.org/20135002/diff/6001/public/platform/WebString.h
File public/platform/WebString.h (right):

https://codereview.chromium.org/20135002/diff/6001/public/platform/WebString....
public/platform/WebString.h:94: BLINK_COMMON_EXPORT std::string toUTF8String()
const;
I'm not super happy with this name...  Open to suggestions.  ;)

[Jeffrey Yasskin, 2013-07-24 22:23:20]

lgtm

https://codereview.chromium.org/20135002/diff/6001/public/platform/WebString.h
File public/platform/WebString.h (right):

https://codereview.chromium.org/20135002/diff/6001/public/platform/WebString....
public/platform/WebString.h:94: BLINK_COMMON_EXPORT std::string toUTF8String()
const;
On 2013/07/24 21:59:10, abarth wrote:
> I'm not super happy with this name...  Open to suggestions.  ;)

It would be clearer if it mentioned "std", but it doesn't seem terrible to me as
it is.

[eseidel, 2013-07-24 22:56:16]

lgtm

looks reasonable.

https://codereview.chromium.org/20135002/diff/6001/Source/core/platform/chrom...
File Source/core/platform/chromium/support/WebString.cpp (right):

https://codereview.chromium.org/20135002/diff/6001/Source/core/platform/chrom...
Source/core/platform/chromium/support/WebString.cpp:111: return m_private.get()
< s.m_private.get();
This looks like a pointer compare...

[abarth-chromium, 2013-07-24 23:08:38]

On 2013/07/24 22:56:16, eseidel wrote:
> lgtm
> 
> looks reasonable.
> 
>
https://codereview.chromium.org/20135002/diff/6001/Source/core/platform/chrom...
> File Source/core/platform/chromium/support/WebString.cpp (right):
> 
>
https://codereview.chromium.org/20135002/diff/6001/Source/core/platform/chrom...
> Source/core/platform/chromium/support/WebString.cpp:111: return
m_private.get()
> < s.m_private.get();
> This looks like a pointer compare...

I've just removed the operator.  It's easier that way.

[abarth-chromium, 2013-07-24 23:10:04]

This CL now depends on https://codereview.chromium.org/20101007 as well.

[darin (slow to review), 2013-07-24 23:16:50]

https://codereview.chromium.org/20135002/diff/13001/public/platform/WebString.h
File public/platform/WebString.h (right):

https://codereview.chromium.org/20135002/diff/13001/public/platform/WebString...
public/platform/WebString.h:93: BLINK_COMMON_EXPORT std::string toUTF8String()
const;
This is sort of a new thing for us.  There's a lot of places in Chromium that
use the .utf8() accessor when they really want a std::string.  It might be nice
if the thing that everyone should be using instead had the shorter name.  Hmm...

[jamesr, 2013-07-24 23:18:32]

Now that WebString can efficiently handle 1 byte strings, is there any reason to
have WebCString?

[abarth-chromium, 2013-07-24 23:22:42]

https://codereview.chromium.org/20135002/diff/13001/public/platform/WebString.h
File public/platform/WebString.h (right):

https://codereview.chromium.org/20135002/diff/13001/public/platform/WebString...
public/platform/WebString.h:93: BLINK_COMMON_EXPORT std::string toUTF8String()
const;
On 2013/07/24 23:16:51, darin wrote:
> This is sort of a new thing for us.  There's a lot of places in Chromium that
> use the .utf8() accessor when they really want a std::string.  It might be
nice
> if the thing that everyone should be using instead had the shorter name. 
Hmm...

Yes.  I suspect most (all?) the callers of WebString::utf8 should be moved over
to this API...  We could name it toUTF8 (to mirror the fromUTF8 API) and then
delete utf8.  If we want to use the utf8 name, we'll need to move everyone to a
temporary name while we change all the callers.

[abarth-chromium, 2013-07-24 23:25:37]

In code search, I don't see any obvious reasons for WebCString to exist.  Maybe
we should remove it from the API and use std::string everywhere instead?

[darin (slow to review), 2013-07-24 23:26:39]

That sounds pretty good to me!


On Wed, Jul 24, 2013 at 4:25 PM, <abarth@chromium.org> wrote:

> In code search, I don't see any obvious reasons for WebCString to exist.
>  Maybe
> we should remove it from the API and use std::string everywhere instead?
>
>
https://codereview.chromium.**org/20135002/<https://codereview.chromium.org/2...
>

[abarth-chromium, 2013-07-24 23:36:57]

On 2013/07/24 23:26:39, darin wrote:
> That sounds pretty good to me!

At darin's suggestion, I tried just changing the return type of WebString::utf8
to std::string, and things appear to compile.  I'm going to break that off into
a separate CL.

[commit-bot: I haz the power, 2013-07-26 16:55:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/20135002/25001

[commit-bot: I haz the power, 2013-07-26 19:54:52]

Change committed as 155006