Make v8AtomicString take a StringView.

Make v8AtomicString take a StringView.

By accepting a StringView v8AtomicString, which is used for property
access all over the engine, can have the length of the input literal
strings be computed at compile time. For example
v8AtomicString(isolate, "foo") will now compile the length of 3 into the
binary directly.

This patch also makes v8AtomicString take 8/16 bit strings like all of
the other string handling functions and moves the utf8 logic into a new
function v8StringFromUtf8. This makes the code more clear since blink
rarely ever uses utf8 strings, and should also be faster since v8
strings are either latin1 or utf16 just like blink, so calling the utf8
methods for property access was going through a bunch of logic on the
string that wasn't needed.

The two callers that actually expect utf8 inputs have been changed, and
one caller in WindowProxy::setSecurityToken was switched to not using
utf8 since it wasn't actually needed as it was going from a blink String
to utf8 and then back into a v8::String which is the same representation
as the blink string.

BUG=615174
Committed: https://crrev.com/a348d19c81ac29626a38a47a683856f22102bb42
Cr-Commit-Position: refs/heads/master@{#405011}

[esprehn, 2016-07-13 00:45:58]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[esprehn, 2016-07-13 00:48:53]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org

[esprehn, 2016-07-13 00:48:54]

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (left):

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:428: if
(LIKELY(v8::String::NewFromUtf8(isolate, str, v8::NewStringType::kInternalized,
length).ToLocal(&value)))
v8CallOrCrash is a call to FromJust() which contains a V8_LIKELY macro.

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (left):

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:469: CString
utf8Token = token.utf8();
Doing this meant we were allocating an extra temporary string plus we were doing
latin1 -> utf8 -> latin1 here for no reason.

[commit-bot: I haz the power, 2016-07-13 00:48:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-07-13 01:12:50]

LGTM with a comment.

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp:195:
v8::Local<v8::String> jsonString = v8StringFromUtf8(scriptState->isolate(),
utf8Data, length);

Can we use v8AtomicString(..., StringView(utf8Data))?

I'm not really happy about introducing v8StringFromUtf8 since it shouldn't be
encouraged.

[esprehn, 2016-07-13 01:50:30]

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp:195:
v8::Local<v8::String> jsonString = v8StringFromUtf8(scriptState->isolate(),
utf8Data, length);
On 2016/07/13 at 01:12:49, haraken wrote:
> Can we use v8AtomicString(..., StringView(utf8Data))?

That would mangle the utf8 data which needs to be inflated to utf16 if it
contains anything outside latin1. StringView can also only be latin1 or utf16
just like StringImpl, there's no way to put utf8 data into it without doing a
conversion. We could do that conversion but it's kind of wasteful since v8 can
do it for us internally without the extra copy.

> 
> I'm not really happy about introducing v8StringFromUtf8 since it shouldn't be
encouraged.

Just like people don't use String::fromUTF8 very much I'm hopeful they won't use
this either.

[esprehn, 2016-07-13 01:53:11]

Description was changed from

==========
Make v8AtomicString take a StringView.

By accepting a StringView v8AtomicString, which is used for property
access all over the engine, can have the length of the input literal
strings be computed at compile time. For example
v8AtomicString(isolate, "foo") will now compile the length of 3 into the
binary directly.

This patch also makes v8AtomicString take 8/16 bit strings like all of
the other string handling functions and moves the utf8 logic into a new
function v8StringFromUtf8. This makes the code more clear since blink
rarely ever uses utf8 strings, and should also be faster since v8
strings are either latin1 or utf16 just like blink, so calling the utf8
methods for property access was going through a bunch of logic on the
string that wasn't needed

The two callers that actually expect utf8 inputs have been changed, and
one caller in WindowProxy::setSecurityToken was switched to not using
utf8 since it wasn't actually needed as it was going from a blink String
to utf8 and then back into a v8::String which is the same representation
as the blink string.

BUG=615174
==========

to

==========
Make v8AtomicString take a StringView.

By accepting a StringView v8AtomicString, which is used for property
access all over the engine, can have the length of the input literal
strings be computed at compile time. For example
v8AtomicString(isolate, "foo") will now compile the length of 3 into the
binary directly.

This patch also makes v8AtomicString take 8/16 bit strings like all of
the other string handling functions and moves the utf8 logic into a new
function v8StringFromUtf8. This makes the code more clear since blink
rarely ever uses utf8 strings, and should also be faster since v8
strings are either latin1 or utf16 just like blink, so calling the utf8
methods for property access was going through a bunch of logic on the
string that wasn't needed.

The two callers that actually expect utf8 inputs have been changed, and
one caller in WindowProxy::setSecurityToken was switched to not using
utf8 since it wasn't actually needed as it was going from a blink String
to utf8 and then back into a v8::String which is the same representation
as the blink string.

BUG=615174
==========

[haraken, 2016-07-13 01:56:50]

On 2016/07/13 01:50:30, esprehn wrote:
>
https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp (right):
> 
>
https://codereview.chromium.org/2142373002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp:195:
> v8::Local<v8::String> jsonString = v8StringFromUtf8(scriptState->isolate(),
> utf8Data, length);
> On 2016/07/13 at 01:12:49, haraken wrote:
> > Can we use v8AtomicString(..., StringView(utf8Data))?
> 
> That would mangle the utf8 data which needs to be inflated to utf16 if it
> contains anything outside latin1. StringView can also only be latin1 or utf16
> just like StringImpl, there's no way to put utf8 data into it without doing a
> conversion. We could do that conversion but it's kind of wasteful since v8 can
> do it for us internally without the extra copy.
> 
> > 
> > I'm not really happy about introducing v8StringFromUtf8 since it shouldn't
be
> encouraged.
> 
> Just like people don't use String::fromUTF8 very much I'm hopeful they won't
use
> this either.

ok :)

[commit-bot: I haz the power, 2016-07-13 02:06:36]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 02:06:37]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[esprehn, 2016-07-13 02:21:57]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2016-07-13 02:21:58]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2142373002/#ps20001
(title: "Woops, pass the length.")

[commit-bot: I haz the power, 2016-07-13 02:24:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 04:19:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-13 04:19:04]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 04:21:05]

Description was changed from

==========
Make v8AtomicString take a StringView.

By accepting a StringView v8AtomicString, which is used for property
access all over the engine, can have the length of the input literal
strings be computed at compile time. For example
v8AtomicString(isolate, "foo") will now compile the length of 3 into the
binary directly.

This patch also makes v8AtomicString take 8/16 bit strings like all of
the other string handling functions and moves the utf8 logic into a new
function v8StringFromUtf8. This makes the code more clear since blink
rarely ever uses utf8 strings, and should also be faster since v8
strings are either latin1 or utf16 just like blink, so calling the utf8
methods for property access was going through a bunch of logic on the
string that wasn't needed.

The two callers that actually expect utf8 inputs have been changed, and
one caller in WindowProxy::setSecurityToken was switched to not using
utf8 since it wasn't actually needed as it was going from a blink String
to utf8 and then back into a v8::String which is the same representation
as the blink string.

BUG=615174
==========

to

==========
Make v8AtomicString take a StringView.

By accepting a StringView v8AtomicString, which is used for property
access all over the engine, can have the length of the input literal
strings be computed at compile time. For example
v8AtomicString(isolate, "foo") will now compile the length of 3 into the
binary directly.

This patch also makes v8AtomicString take 8/16 bit strings like all of
the other string handling functions and moves the utf8 logic into a new
function v8StringFromUtf8. This makes the code more clear since blink
rarely ever uses utf8 strings, and should also be faster since v8
strings are either latin1 or utf16 just like blink, so calling the utf8
methods for property access was going through a bunch of logic on the
string that wasn't needed.

The two callers that actually expect utf8 inputs have been changed, and
one caller in WindowProxy::setSecurityToken was switched to not using
utf8 since it wasn't actually needed as it was going from a blink String
to utf8 and then back into a v8::String which is the same representation
as the blink string.

BUG=615174

Committed: https://crrev.com/a348d19c81ac29626a38a47a683856f22102bb42
Cr-Commit-Position: refs/heads/master@{#405011}
==========

[commit-bot: I haz the power, 2016-07-13 04:21:06]

Patchset 2 (id:??) landed as
https://crrev.com/a348d19c81ac29626a38a47a683856f22102bb42
Cr-Commit-Position: refs/heads/master@{#405011}