Move V8-based serialization into dedicated classes and add support for ArrayBuffer transfer.

Move V8-based serialization into dedicated classes and add support for ArrayBuffer transfer.

Eventually these classes are intended to replace ScriptValueSerializer and
ScriptValueDeserializer as they exist today.

BUG=chromium:148757
Committed: https://crrev.com/8687a21473775357a6a3dd63ab4cd21f4d47f1af
Cr-Commit-Position: refs/heads/master@{#416445}

[jbroman, 2016-09-01 17:36:39]

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

[commit-bot: I haz the power, 2016-09-01 17:37:11]

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

[jbroman, 2016-09-01 18:16:24]

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

[commit-bot: I haz the power, 2016-09-01 18:16:38]

Dry run: 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-09-01 19:36:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 19:36:27]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-09-01 19:41:10]

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

[jbroman, 2016-09-01 19:41:11]

Moving the calls to the V8 serialization code to a more separate home (so it has
room to grow).

[esprehn, 2016-09-01 21:41:21]

lgtm

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h:24:
// deserialize method will be invoked exactly once.
yay comments!

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:44:
std::vector<uint8_t> buffer = m_serializer.ReleaseBuffer();
we don't really allow std::vector inside blink, ideally we'd have avoided this
dep, though I'm not sure how. Using push_back and the unchecked [0] below is
doesn't match Vector's semantics either

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:65:
for (uint32_t i = 0; i < transferables->arrayBuffers.size(); i++) {
size_t ?

[haraken, 2016-09-02 00:19:52]

LGTM

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:60:
if (!contents.isShared()) {

if (contents.isShared()) {

} else {

}

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h:24:
// deserialize method will be invoked exactly once.

Can we add a DCHECK about this fact?

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:35:
if (!m_serializer.WriteValue(m_scriptState->context(), value).FromMaybe(false))
{

Can we use To instead of FromMaybe?

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:39:
exceptionState.throwDOMException(DataCloneError, "An object could not be
cloned.");

I asked this in the previous CL but we want to guarantee that To()/ToLocal()
returns false if and only if V8 throws an exception.

To make that invariant clearer, it's better to write:

bool result;
if (!m_serializer.WriteValue().To(&result)) {
  rethrowV8Exception();
  return nullptr;
}
if (!result) {
  exceptionState.throwDOMException();
  return nullptr;
}

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:49:
String(reinterpret_cast<const UChar*>(&buffer[0]), buffer.size() / 2));

Worth adding a comment that the binary is of 2 byte.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:27:
// method will be invoked exactly once.

Ditto. Add a DCHECK about this?

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:41:
Transferables* m_transferables;

Do we need this member?

[jbroman, 2016-09-02 15:54:25]

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

[commit-bot: I haz the power, 2016-09-02 15:54:41]

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

[jbroman, 2016-09-02 17:22:48]

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:60:
if (!contents.isShared()) {
On 2016/09/02 at 00:19:51, haraken wrote:
> if (contents.isShared()) {
> 
> } else {
> 
> }

I kind of liked having it in the same order it usually appears (ArrayBuffer,
then SharedArrayBuffer), but it's not a big deal, so done.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.h:24:
// deserialize method will be invoked exactly once.
On 2016/09/02 at 00:19:51, haraken wrote:
> Can we add a DCHECK about this fact?

OK, done.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:35:
if (!m_serializer.WriteValue(m_scriptState->context(), value).FromMaybe(false))
{
On 2016/09/02 at 00:19:51, haraken wrote:
> Can we use To instead of FromMaybe?

Whoops, I didn't merge that across to this branch. Done.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:39:
exceptionState.throwDOMException(DataCloneError, "An object could not be
cloned.");
On 2016/09/02 at 00:19:51, haraken wrote:
> I asked this in the previous CL but we want to guarantee that To()/ToLocal()
returns false if and only if V8 throws an exception.
> 
> To make that invariant clearer, it's better to write:
> 
> bool result;
> if (!m_serializer.WriteValue().To(&result)) {
>   rethrowV8Exception();
>   return nullptr;
> }
> if (!result) {
>   exceptionState.throwDOMException();
>   return nullptr;
> }

This isn't true until V8 rolls for
https://chromium.googlesource.com/v8/v8/+/f72351f5400151d9a4b63da2be4049f4c81...,
but I'll do it now.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:44:
std::vector<uint8_t> buffer = m_serializer.ReleaseBuffer();
On 2016/09/01 at 21:41:21, esprehn wrote:
> we don't really allow std::vector inside blink, ideally we'd have avoided this
dep, though I'm not sure how. Using push_back and the unchecked [0] below is
doesn't match Vector's semantics either

V8 can't use WTF (at least not without a complicated indirection), and there's
no way of "releasing" an std::vector into a WTF::Vector. I'd actually like to
push this a little further, so that this copy to/from a WTF::String isn't
required. I don't think this is a large risk of "infecting" the rest of Blink
with STL. To me it feels like how the rest of bindings uses v8's preferred types
for a number of things (like v8::Local<v8::Value>).

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:49:
String(reinterpret_cast<const UChar*>(&buffer[0]), buffer.size() / 2));
On 2016/09/02 at 00:19:51, haraken wrote:
> Worth adding a comment that the binary is of 2 byte.

Done, though as the TODO notes I hope to eventually kill this weird requirement.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:65:
for (uint32_t i = 0; i < transferables->arrayBuffers.size(); i++) {
On 2016/09/01 at 21:41:21, esprehn wrote:
> size_t ?

Could if you'd prefer, though JS array indexes must fit within uint32_t, so
that's what V8 takes here.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h
(right):

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:27:
// method will be invoked exactly once.
On 2016/09/02 at 00:19:52, haraken wrote:
> Ditto. Add a DCHECK about this?

The DCHECK(m_serializedScriptValue) is likely to catch the majority of these. In
the interest of being explicit, though, done.

https://codereview.chromium.org/2305703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:41:
Transferables* m_transferables;
On 2016/09/02 at 00:19:51, haraken wrote:
> Do we need this member?

Not yet. Removed; I'll add it back if I do.

[jbroman, 2016-09-02 17:23:00]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-09-02 17:23:01]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-09-02 17:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2305703002/#ps40001
(title: "reviewer comments")

[commit-bot: I haz the power, 2016-09-02 17:23:12]

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-09-02 19:05:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 19:05:52]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2016-09-02 19:09:10]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-09-02 19:09:52]

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-09-02 21:13:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 21:13:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2016-09-03 03:34:21]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-09-03 03:34:32]

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-09-03 05:10:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-03 05:12:30]

Description was changed from

==========
Move V8-based serialization into dedicated classes and add support for
ArrayBuffer transfer.

Eventually these classes are intended to replace ScriptValueSerializer and
ScriptValueDeserializer as they exist today.

BUG=chromium:148757
==========

to

==========
Move V8-based serialization into dedicated classes and add support for
ArrayBuffer transfer.

Eventually these classes are intended to replace ScriptValueSerializer and
ScriptValueDeserializer as they exist today.

BUG=chromium:148757

Committed: https://crrev.com/8687a21473775357a6a3dd63ab4cd21f4d47f1af
Cr-Commit-Position: refs/heads/master@{#416445}
==========

[commit-bot: I haz the power, 2016-09-03 05:12:31]

Patchset 3 (id:??) landed as
https://crrev.com/8687a21473775357a6a3dd63ab4cd21f4d47f1af
Cr-Commit-Position: refs/heads/master@{#416445}