Support structured clone atop v8::ValueSerializer behind a RuntimeEnabledFeature.

Support structured clone atop v8::ValueSerializer behind a RuntimeEnabledFeature.

There is further refactoring to be done to clean this up, but this is sufficient
to make things mostly work. Notably this still does a copy into a 16-bit WTF::String
because at present there is a significant amount of other code that assumes this.

BUG=148757
Committed: https://crrev.com/e28e1e567c840430f2262b0df778cc07285a8c64
Cr-Commit-Position: refs/heads/master@{#415195}

[jbroman, 2016-08-25 19:47:27]

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

[commit-bot: I haz the power, 2016-08-25 19:48:35]

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

[jbroman, 2016-08-25 19:50:27]

Description was changed from

==========
Support structured clone atop v8::ValueSerializer behind a
RuntimeEnabledFeature.

BUG=148757
==========

to

==========
Support structured clone atop v8::ValueSerializer behind a
RuntimeEnabledFeature.

There is further refactoring to be done to clean this up, but this is sufficient
to make things mostly work. Notably this still does a copy into a 16-bit
WTF::String
because at present there is a significant amount of other code that assumes
this.

BUG=148757
==========

[commit-bot: I haz the power, 2016-08-25 20:04:46]

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

[commit-bot: I haz the power, 2016-08-25 20:04:47]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jbroman, 2016-08-27 23:22:54]

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

[commit-bot: I haz the power, 2016-08-27 23:22:57]

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

[jbroman, 2016-08-27 23:31:12]

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

[commit-bot: I haz the power, 2016-08-27 23:31:24]

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-08-28 01:12:27]

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

[commit-bot: I haz the power, 2016-08-28 01:12:28]

Dry run: 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-08-28 01:25:01]

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

[commit-bot: I haz the power, 2016-08-28 01:25:10]

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-08-28 03:00:39]

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

[commit-bot: I haz the power, 2016-08-28 03:00:40]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-08-28 18:29:43]

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

[jbroman, 2016-08-28 18:29:44]

Further refactoring likely to follow, but this gets something working end to
end.

[haraken, 2016-08-29 14:12:14]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-08-29 14:12:14]

LGTM on my side.

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

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:742: if
(!serializer.WriteValue(context(), value).FromMaybe(false)) {

Use To instead.

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:745: } else
{

This shouldn't happen. To returns false if and only if there is an exception.

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:2155: if
(!deserializer.ReadHeader().FromMaybe(false))

Use To instead.

[jbroman, 2016-08-29 14:29:37]

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

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:742: if
(!serializer.WriteValue(context(), value).FromMaybe(false)) {
On 2016/08/29 at 14:12:14, haraken wrote:
> Use To instead.

Can do, though doing so is more verbose in this case:

bool success;
if (!serializer.WriteValue(context(), value).To(&success) && success) {
}

Although Just(false) isn't used; like other V8 APIs only Just(true) and
Nothing<bool>() can actually be returned.

May I ask why To is preferred for this case?

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:745: } else
{
On 2016/08/29 at 14:12:14, haraken wrote:
> This shouldn't happen. To returns false if and only if there is an exception.

Technically, Maybe::To returns false if and only if the value was constructed
with Nothing<T>.

At the moment, this does not necessarily indicate a pending exception, because
there is not yet agreement about how to handle DataCloneError in this interface.
Options include:
- not throwing an exception, and expecting the client to throw one in that event
(there are other V8 APIs that return a failure without a thrown exception, like
v8::Object::GetRealNamedPropertyInPrototypeChain).
- throwing a specially marked exception, which Blink catches and rethrows as a
DataCloneError DOMException
- having V8 call into Blink to throw the kind of exception it expects

[haraken, 2016-08-29 16:25:01]

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

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:742: if
(!serializer.WriteValue(context(), value).FromMaybe(false)) {
On 2016/08/29 14:29:37, jbroman wrote:
> On 2016/08/29 at 14:12:14, haraken wrote:
> > Use To instead.
> 
> Can do, though doing so is more verbose in this case:
> 
> bool success;
> if (!serializer.WriteValue(context(), value).To(&success) && success) {
> }
> 
> Although Just(false) isn't used; like other V8 APIs only Just(true) and
> Nothing<bool>() can actually be returned.
> 
> May I ask why To is preferred for this case?

V8 APIs around Maybe/MaybeLocal are really confusing, and we've decided to unify
them into To/ToLocal.

https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/vG...

[jbroman, 2016-08-29 17:03:42]

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

[jbroman, 2016-08-29 17:07:41]

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

https://codereview.chromium.org/2277403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:742: if
(!serializer.WriteValue(context(), value).FromMaybe(false)) {
On 2016/08/29 at 16:25:01, haraken wrote:
> On 2016/08/29 14:29:37, jbroman wrote:
> > On 2016/08/29 at 14:12:14, haraken wrote:
> > > Use To instead.
> > 
> > Can do, though doing so is more verbose in this case:
> > 
> > bool success;
> > if (!serializer.WriteValue(context(), value).To(&success) && success) {
> > }
> > 
> > Although Just(false) isn't used; like other V8 APIs only Just(true) and
> > Nothing<bool>() can actually be returned.
> > 
> > May I ask why To is preferred for this case?
> 
> V8 APIs around Maybe/MaybeLocal are really confusing, and we've decided to
unify them into To/ToLocal.
> 
>
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/vG...

Done.

[haraken, 2016-08-29 17:09:06]

LGTM

https://codereview.chromium.org/2277403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2277403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:744: if
(m_tryCatch.HasCaught()) {

Add a TODO about how to handle the exception.

[jbroman, 2016-08-29 17:15:30]

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

[commit-bot: I haz the power, 2016-08-29 17:15:57]

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

[jbroman, 2016-08-29 17:16:15]

https://codereview.chromium.org/2277403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2277403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:744: if
(m_tryCatch.HasCaught()) {
On 2016/08/29 at 17:09:05, haraken wrote:
> Add a TODO about how to handle the exception.

Done.

[jbroman, 2016-08-29 17:35:32]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-08-29 17:35:33]

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/2277403002/#ps60001
(title: "leave TODO")

[commit-bot: I haz the power, 2016-08-29 17:50:32]

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

[jbroman, 2016-08-29 19:47:40]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-08-29 19:47:49]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-08-29 20:15:39]

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-08-30 06:03:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 06:06:08]

Description was changed from

==========
Support structured clone atop v8::ValueSerializer behind a
RuntimeEnabledFeature.

There is further refactoring to be done to clean this up, but this is sufficient
to make things mostly work. Notably this still does a copy into a 16-bit
WTF::String
because at present there is a significant amount of other code that assumes
this.

BUG=148757
==========

to

==========
Support structured clone atop v8::ValueSerializer behind a
RuntimeEnabledFeature.

There is further refactoring to be done to clean this up, but this is sufficient
to make things mostly work. Notably this still does a copy into a 16-bit
WTF::String
because at present there is a significant amount of other code that assumes
this.

BUG=148757

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

[commit-bot: I haz the power, 2016-08-30 06:06:09]

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