Support structured cloning of ES'15 Map and Set

Support structured cloning of ES'15 Map and Set

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

[adamk, 2015-06-24 19:10:48]

adamk@chromium.org changed reviewers:
+ jsbell@chromium.org

[adamk, 2015-06-24 19:10:49]

This isn't quite ready to land yet (there's still at least one FIXME, and I need
to add proper comments to SerializationTag.h), but I'd like to get some eyeballs
on my ScriptValueSerializer code to make sure I'm doing it right (I at least
pass the tests I added).

[jsbell, 2015-06-24 20:30:43]

Initial feedback - all on the tests. You'll also want to bump the version in
SerializedScriptValue.h and add cases to
fast/storage/serialized-script-value.html

Code changes look okay, but apparently I haven't looked at our object/array
serialization in detail before (and/or recently).

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:134: assert_equals(clone.set.size,
orig.set.size, 'Clone should be the same size');
Should probably have asserts about emptySet/emptyMap sizes too.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:139: }, 'Verify: "Maps and Sets are
cloned..."');
The 'Verify: "xxxx..."' phrasing in tests above is quoting from the HTML spec
itself. I don't think this is a quote?

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:150: assert_true(clone.set ===
Array.from(clone.set)[0], 'Recursive sets should maintain identity');
Use assert_equals() for these (it does 'same value' equivalent to Object.is)

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:154: }, 'Verify: "Maps and Sets can
contain themselves..."');
Ditto.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:155: 
Can you add a test where a member of the map/set has a getter that mutates the
map/set itself?

[adamk, 2015-06-24 21:13:32]

I've bumped the version.

What tests would you like to see added to
fast/storage/serialized-script-value.html?

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:134: assert_equals(clone.set.size,
orig.set.size, 'Clone should be the same size');
On 2015/06/24 20:30:43, jsbell wrote:
> Should probably have asserts about emptySet/emptyMap sizes too.

Done.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:139: }, 'Verify: "Maps and Sets are
cloned..."');
On 2015/06/24 20:30:42, jsbell wrote:
> The 'Verify: "xxxx..."' phrasing in tests above is quoting from the HTML spec
> itself. I don't think this is a quote?

I've removed the quotes and the ellipses. Should I actually move these tests to
a different file?

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:150: assert_true(clone.set ===
Array.from(clone.set)[0], 'Recursive sets should maintain identity');
On 2015/06/24 20:30:43, jsbell wrote:
> Use assert_equals() for these (it does 'same value' equivalent to Object.is)

Done.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:154: }, 'Verify: "Maps and Sets can
contain themselves..."');
On 2015/06/24 20:30:43, jsbell wrote:
> Ditto.

Done.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:155: 
On 2015/06/24 20:30:42, jsbell wrote:
> Can you add a test where a member of the map/set has a getter that mutates the
> map/set itself?

Done. Should we file a bug for this?

[jsbell, 2015-06-24 21:37:11]

On 2015/06/24 21:13:32, adamk wrote:
> What tests would you like to see added to
> fast/storage/serialized-script-value.html?

I think just a simple Map([[1,2],[3,4]]) and Set([1,2]) example that records the
serialization format for posterity. (That's our best chance of catching
accidental changes to the format, which would pass all of our other
serialize/deserialize tests but cause older data on disk to fail to parse.)

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:139: }, 'Verify: "Maps and Sets are
cloned..."');
On 2015/06/24 21:13:31, adamk wrote:
> On 2015/06/24 20:30:42, jsbell wrote:
> > The 'Verify: "xxxx..."' phrasing in tests above is quoting from the HTML
spec
> > itself. I don't think this is a quote?
> 
> I've removed the quotes and the ellipses. Should I actually move these tests
to
> a different file?

I think this file is fine.

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:155: 
On 2015/06/24 21:13:32, adamk wrote:
> On 2015/06/24 20:30:42, jsbell wrote:
> > Can you add a test where a member of the map/set has a getter that mutates
the
> > map/set itself?
> 
> Done. Should we file a bug for this?

Against HTML? Probably.

One more edge case we should test: Map/Set containing a key and/or value that is
not cloneable (e.g. an Error)

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValueSerializer.cpp:1955: v8::Local<v8::Map> map;
Can this be merged with line 1959?

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValueSerializer.cpp:1978: v8::Local<v8::Set> set;
Merge w/ line 1982?

[adamk, 2015-06-24 22:50:31]

Handled last round of comments, modulo fast/storage/serialized-script-value.html
(which will take me a bit as I try to grok the current test structure).

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:155: 
On 2015/06/24 21:37:10, jsbell wrote:
> On 2015/06/24 21:13:32, adamk wrote:
> > On 2015/06/24 20:30:42, jsbell wrote:
> > > Can you add a test where a member of the map/set has a getter that mutates
> the
> > > map/set itself?
> > 
> > Done. Should we file a bug for this?
> 
> Against HTML? Probably.
> 
> One more edge case we should test: Map/Set containing a key and/or value that
is
> not cloneable (e.g. an Error)

Non-cloneability tests added.

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValueSerializer.cpp:1955: v8::Local<v8::Map> map;
On 2015/06/24 21:37:11, jsbell wrote:
> Can this be merged with line 1959?

Done.

https://codereview.chromium.org/1205973002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValueSerializer.cpp:1978: v8::Local<v8::Set> set;
On 2015/06/24 21:37:11, jsbell wrote:
> Merge w/ line 1982?

Done.

[adamk, 2015-06-24 23:19:56]

Okay, added a separate test for deserialization (you're so right, testharness is
awesome).

[adamk, 2015-06-24 23:27:19]

adamk@chromium.org changed reviewers:
+ chrishtr@chromium.org

[adamk, 2015-06-24 23:27:20]

+chrishtr for public/web/

[chrishtr, 2015-06-24 23:31:42]

lgtm

[jsbell, 2015-06-24 23:40:19]

lgtm

[adamk, 2015-06-24 23:48:53]

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1205973002/diff/20001/LayoutTests/fast/js/str...
LayoutTests/fast/js/structured-clone.html:155: 
On 2015/06/24 21:37:10, jsbell wrote:
> On 2015/06/24 21:13:32, adamk wrote:
> > On 2015/06/24 20:30:42, jsbell wrote:
> > > Can you add a test where a member of the map/set has a getter that mutates
> the
> > > map/set itself?
> > 
> > Done. Should we file a bug for this?
> 
> Against HTML? Probably.

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=28849

[adamk, 2015-06-24 23:53:54]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-06-24 23:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205973002/100001

[commit-bot: I haz the power, 2015-06-25 01:42:06]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197791

[Michael Achenbach, 2015-06-29 13:28:08]

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

[Michael Achenbach, 2015-06-29 13:28:08]

V8 API stability nag: According to the stability checker, this CL here might
have violated the V8 API stability window:
http://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabilit...

Contains:
https://chromium.googlesource.com/chromium/src/+log/5d56905d44c0a30cf0ced68f2...

Nothing bad happened this time, but please keep a large enough window between
v8-side api changes and blink-side consumers of these changes. Minimum is the
last used v8 canary version (that's what the bot checks against), better a week.
V8 always needs to be revertable cleanly to the last canary in case a canary is
broken.

If you think this is all nonsense, please ping me - the bot is new and not well
tested yet.