Teach WebString about 8 bit strings

Teach WebString about 8 bit strings

Previously we always upconverted strings backing WebString to have both a 16
bit and an 8 bit buffer whenever we converted a WebString to a string16.
There's no reason to store a shadow 16 bit buffer in the String for this use
case. It's just as fast to copy out an 8 bit buffer into a string16 as it is to
copy out a 16 bit buffer.

This CL removes the vast majority of the string up converstions for Mobile
Gmail. In a future CL, we should remove WebString::data() to prevent Chromium
code from accidentially upconverting the underlying String.

R=brettw@chromium.org, eseidel@chromium.org, jamesr@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151535

[abarth-chromium, 2013-05-24 01:54:41]

I need to benchmark it, but I suspect this saves around 50 kB of memory for
Mobile Gmail.

[eseidel, 2013-05-24 06:06:09]

lgtm, but I don't think I haz teh power.

[jamesr, 2013-05-24 06:11:32]

lgtm

You probably want a V8 person to check over that isolate change, I don't
understand it.

[darin (slow to review), 2013-05-24 06:12:52]

https://codereview.chromium.org/15866003/diff/3001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp (right):

https://codereview.chromium.org/15866003/diff/3001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:63:
newInfo.platformPath.assign(String(info.platformPath));
nit: there's an implicit casting operator to WTF::String from WebString.  maybe
that makes the explicit String() construct unnecessary here?

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

https://codereview.chromium.org/15866003/diff/3001/public/platform/WebString....
public/platform/WebString.h:125: size_t len = length();
nit: this seems like a lot of code to inline into all of the callsites where
casting to string16 is required.  perhaps a higher level non-inline method would
be helpful.  hmm, but you are calling two different string16 constructors below.
 hmm...

https://codereview.chromium.org/15866003/diff/3001/public/platform/WebString....
public/platform/WebString.h:157: return NullableString16(operator string16(),
false);
nit: You could reduce this code down to just:

  return NullableString16(operator string16(), isNull());

[abarth-chromium, 2013-05-24 06:17:44]

https://codereview.chromium.org/15866003/diff/3001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp (right):

https://codereview.chromium.org/15866003/diff/3001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:63:
newInfo.platformPath.assign(String(info.platformPath));
On 2013/05/24 06:12:53, darin wrote:
> nit: there's an implicit casting operator to WTF::String from WebString. 
maybe
> that makes the explicit String() construct unnecessary here?

I'll try it without the explicit String.  It's an assignment from one WebString
to another.

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

https://codereview.chromium.org/15866003/diff/3001/public/platform/WebString....
public/platform/WebString.h:157: return NullableString16(operator string16(),
false);
On 2013/05/24 06:12:53, darin wrote:
> nit: You could reduce this code down to just:
> 
>   return NullableString16(operator string16(), isNull());

Good idea.

[abarth-chromium, 2013-05-24 06:18:26]

On 2013/05/24 06:11:32, jamesr wrote:
> You probably want a V8 person to check over that isolate change, I don't
> understand it.

That's a normal pattern that we're using all over the place these days.  This
code was just old and doing things manually.

[abarth-chromium, 2013-05-24 07:21:46]

Darin was worried about binary size.  The old approach added 42 kB to
libcontent.so.  This new approach shrinks libcontent.so by 8 kB.

The new CL relies on a function that doesn't yet exist in base.  I'll upload the
CL that adds it separately.

[abarth-chromium, 2013-05-24 07:29:14]

The base side of this CL is in https://codereview.chromium.org/15720004

[abarth-chromium, 2013-05-24 07:34:35]

+brettw: Darin thought we might benefit from your taking a look at this CL (and
another one I'm about to CC you on).

[brettw, 2013-05-24 14:38:37]

Do we intend to use the new base function elsewhere? It seems kind of specific
for this case. It would help to see the base function definition.

[abarth-chromium, 2013-05-24 16:23:34]

On 2013/05/24 14:38:37, brettw wrote:
> Do we intend to use the new base function elsewhere?

Nope.

> It seems kind of specific for this case.

Yes.  It's very specific for this use case.  We don't want to inline the
function into the header because it bloats the binary by 42 kB.  We can't put
the definition inside libwebkit.so because the function needs to see string16.h.
 That means we need to put it out-of-line in Chromium somewhere.  Putting it in
base lets us avoid having a circular dependency between, e.g., webkit/glue and
Blink.

> It would help to see the base function definition.

The definition is in https://codereview.chromium.org/15720004

[brettw, 2013-05-28 18:19:02]

LGTM for this once we resolve the other one.

[abarth-chromium, 2013-05-29 01:02:37]

Committed patchset #4 manually as r151339 (presubmit successful).

[abarth-chromium, 2013-05-29 01:21:58]

This got rolled out.  Not everyone who depends on WebKit links in webkit_glue. 
In particular, CC uses WebString but doesn't depend on webkit_glue.  IMHO, the
best solution here is to put this function in base.  It's a bit quirky, but it's
concerned with concepts that base understands (strings and charset conversions).

[brettw, 2013-05-29 03:11:59]

On 2013/05/29 01:21:58, abarth wrote:
> This got rolled out.  Not everyone who depends on WebKit links in webkit_glue.

> In particular, CC uses WebString but doesn't depend on webkit_glue.  IMHO, the
> best solution here is to put this function in base.  It's a bit quirky, but
it's
> concerned with concepts that base understands (strings and charset
conversions).

OK

[abarth-chromium, 2013-05-30 00:14:09]

Committed patchset #5 manually as r151425 (presubmit successful).

[commit-bot: I haz the power, 2013-05-31 08:32:38]

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

[commit-bot: I haz the power, 2013-05-31 08:32:59]

Change committed as 151535