Implement serialization/deserialization of geometry interfaces

Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

BUG=721331
Review-Url: https://codereview.chromium.org/2874203003
Cr-Commit-Position: refs/heads/master@{#475371}
Committed: https://chromium.googlesource.com/chromium/src/+/9e8060a43d58b5e94f8c88518283376242e286b5

[fserb, 2017-05-12 15:14:15]

Description was changed from

==========
Implement serialization/deserialization of geometry interfaces

BUG=721331
==========

to

==========
Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

I'll got through them once they are merged.

BUG=721331
==========

[fserb, 2017-05-12 15:14:20]

Description was changed from

==========
Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

I'll got through them once they are merged.

BUG=721331
==========

to

==========
Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

BUG=721331
==========

[fserb, 2017-05-12 15:17:58]

fserb@chromium.org changed reviewers:
+ haraken@chromium.org, junov@chromium.org

[haraken, 2017-05-12 15:31:45]

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

[haraken, 2017-05-12 15:31:45]

+jbroman for the serializer

[simonp, 2017-05-15 13:34:17]

simonp@opera.com changed reviewers:
+ simonp@opera.com

[simonp, 2017-05-15 13:34:18]

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:387:
case kDOMMatrix2DTag: {
This is not supported by the spec; per spec all elements will be copied, which
could preserve -0 for m13 with is2D being true.

However, that could well be argued to be a spec bug. I'd be happy to fix the
spec to match this, and test this case in WPT.

[simonp, 2017-05-15 13:45:09]

On 2017/05/15 13:34:18, simonp wrote:
> However, that could well be argued to be a spec bug. I'd be happy to fix the
> spec to match this, and test this case in WPT.

https://github.com/w3c/fxtf-drafts/issues/161

[jbroman, 2017-05-15 17:02:55]

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializationTag.h:62:
kDOMPointTag = 'Q',                 // x:Double, y:Double, z:Double, w:Double
At times like these I wish we had just used consecutive integers (but it's a
pain to come up with new ones).

Some of these do collide with the V8-side tags (e.g. U and I were integers), but
this prime real estate is now available due to V8 explicitly labelling Blink
objects.

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:349:
double x = 0, y = 0, z = 0, w = 1;
nit: this is a little repetitive to confirm that it's correct (though I believe
it is). Could also do something like:

DOMPointInit pointInits[4];
for (DOMPointInit& init : pointInits) {
  double x = 0, y = 0, z = 0, w = 0;
  if (!ReadDouble(&x) || !ReadDouble(&y) || !ReadDouble(&z) ||
      !ReadDouble(&w))
    return nullptr;
  init.setX(x);
  init.setY(y);
  init.setZ(z);
  init.setW(w);
}
DOMQuad* quad = DOMQuad::Create(pointInits[0], pointInits[1], pointInits[2],
pointInits[3]);

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:454:
DOMMatrix* matrix = DOMMatrix::fromMatrixForSerialization(init);
Easy to change later, but do you really want to make a dictionary for this, or
would it be more efficient (and less repetitive) to do something like this:

// M11, M12, ..., M44
double matrix[16];
for (double& d : matrix) {
  if (!ReadDouble(&d))
    return nullptr;
}
DOMMatrix* matrix = new DOMMatrix(matrix, WTF_ARRAY_LENGTH(matrix));

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:324:
WriteDouble(quad->p1()->x());
nit: or, this could be:

for (const DOMPoint* point : {quad->p1(), quad->p2(), quad->p3(), quad->p4()}) {
  WriteDouble(point->x());
  WriteDouble(point->y());
  WriteDouble(point->z());
  WriteDouble(point->w());
}

At the moment I have to scan this to make sure it's correct, which is a little
tedious. The compiler might well unroll it again anyhow.

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:251:
V8DOMPointReadOnly::toImpl(result.As<v8::Object>());
Suggest also checking that the result isn't a DOMPoint, given that it's a
subclass (but we presumably don't want a DOMPoint to emerge)?
EXPECT_FALSE(V8DOMPoint::hasInstance(...)) would suffices. Ditto for similar
below.

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:498:

Please also add decode tests to ensure that if we modify the wire format for
geometry types going forward, we can still deserialize ones that are written
before. (This is necessary because the data may be stored on disk, in
IndexedDB.)

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:820:
// TEST(V8ScriptValueSerializerTest, TransferDOMPoint) {
Delete this?

[jbroman, 2017-05-15 17:03:29]

(to be clear, my nits about loop unrolling are optional, but I personally find
them easier to verify without having to double-check for copy/paste errors)

[fserb, 2017-05-15 17:52:17]

thanks for the comments, the code is much better now. :)

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializationTag.h:62:
kDOMPointTag = 'Q',                 // x:Double, y:Double, z:Double, w:Double
On 2017/05/15 at 17:02:54, jbroman wrote:
> At times like these I wish we had just used consecutive integers (but it's a
pain to come up with new ones).
> 
> Some of these do collide with the V8-side tags (e.g. U and I were integers),
but this prime real estate is now available due to V8 explicitly labelling Blink
objects.

Do those need to be printable chars? Or could I create a CL that just make every
single one of those a number?

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:349:
double x = 0, y = 0, z = 0, w = 1;
On 2017/05/15 at 17:02:55, jbroman wrote:
> nit: this is a little repetitive to confirm that it's correct (though I
believe it is). Could also do something like:
> 
> DOMPointInit pointInits[4];
> for (DOMPointInit& init : pointInits) {
>   double x = 0, y = 0, z = 0, w = 0;
>   if (!ReadDouble(&x) || !ReadDouble(&y) || !ReadDouble(&z) ||
>       !ReadDouble(&w))
>     return nullptr;
>   init.setX(x);
>   init.setY(y);
>   init.setZ(z);
>   init.setW(w);
> }
> DOMQuad* quad = DOMQuad::Create(pointInits[0], pointInits[1], pointInits[2],
pointInits[3]);

much better. I don't know why I just repeteated it. It was late, I was tired. :)
thanks.

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:387:
case kDOMMatrix2DTag: {
On 2017/05/15 at 13:34:18, simonp wrote:
> This is not supported by the spec; per spec all elements will be copied, which
could preserve -0 for m13 with is2D being true.
> 
> However, that could well be argued to be a spec bug. I'd be happy to fix the
spec to match this, and test this case in WPT.

It was partially unintended. I did it like this because there's no
readBool/writeBool, so I just decided to use a different tag, etc. But yeah, I
think it makes sense. Thanks for pushing this to the spec.

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:454:
DOMMatrix* matrix = DOMMatrix::fromMatrixForSerialization(init);
On 2017/05/15 at 17:02:55, jbroman wrote:
> Easy to change later, but do you really want to make a dictionary for this, or
would it be more efficient (and less repetitive) to do something like this:
> 
> // M11, M12, ..., M44
> double matrix[16];
> for (double& d : matrix) {
>   if (!ReadDouble(&d))
>     return nullptr;
> }
> DOMMatrix* matrix = new DOMMatrix(matrix, WTF_ARRAY_LENGTH(matrix));

done. Also updated the interfaces.

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:324:
WriteDouble(quad->p1()->x());
On 2017/05/15 at 17:02:55, jbroman wrote:
> nit: or, this could be:
> 
> for (const DOMPoint* point : {quad->p1(), quad->p2(), quad->p3(), quad->p4()})
{
>   WriteDouble(point->x());
>   WriteDouble(point->y());
>   WriteDouble(point->z());
>   WriteDouble(point->w());
> }
> 
> At the moment I have to scan this to make sure it's correct, which is a little
tedious. The compiler might well unroll it again anyhow.

much better. ;)

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:251:
V8DOMPointReadOnly::toImpl(result.As<v8::Object>());
On 2017/05/15 at 17:02:55, jbroman wrote:
> Suggest also checking that the result isn't a DOMPoint, given that it's a
subclass (but we presumably don't want a DOMPoint to emerge)?
EXPECT_FALSE(V8DOMPoint::hasInstance(...)) would suffices. Ditto for similar
below.

good catch. :)

[jbroman, 2017-05-15 18:00:30]

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializationTag.h:62:
kDOMPointTag = 'Q',                 // x:Double, y:Double, z:Double, w:Double
On 2017/05/15 at 17:52:17, fserb wrote:
> On 2017/05/15 at 17:02:54, jbroman wrote:
> > At times like these I wish we had just used consecutive integers (but it's a
pain to come up with new ones).
> > 
> > Some of these do collide with the V8-side tags (e.g. U and I were integers),
but this prime real estate is now available due to V8 explicitly labelling Blink
objects.
> 
> Do those need to be printable chars? Or could I create a CL that just make
every single one of those a number?

They don't need to be printable, no. But the existing tags do need to be the
same as they were in the past, because this data is persisted to disk. So we
could change them all to decimal/hexadecimal constants and encourage people to
fill up the rest from the bottom up, but it's still left looking kinda strange.

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

https://codereview.chromium.org/2874203003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:387:
case kDOMMatrix2DTag: {
On 2017/05/15 at 17:52:17, fserb wrote:
> On 2017/05/15 at 13:34:18, simonp wrote:
> > This is not supported by the spec; per spec all elements will be copied,
which could preserve -0 for m13 with is2D being true.
> > 
> > However, that could well be argued to be a spec bug. I'd be happy to fix the
spec to match this, and test this case in WPT.
> 
> It was partially unintended. I did it like this because there's no
readBool/writeBool, so I just decided to use a different tag, etc. But yeah, I
think it makes sense. Thanks for pushing this to the spec.

FWIW, you could just write 0 or 1 as a 32-bit integer. It'll only take one byte
anyhow (due to variable-length encoding). If that makes your life easier and
makes things more clearly spec-aligned, feel free to do that.

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

https://codereview.chromium.org/2874203003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:314:
if (!point)
nit: These null checks don't really add much (except maybe to suggest that null
is possible?). "return point;" will already return null if point is null (and
there's no other logic being skipped here). Maybe just "return
DOMPoint::Create(...);"? (here and below)

[fserb, 2017-05-15 18:27:28]

I think the whole is2D thing is potentially weird to begin with.
Let's see how the conversation on the spec goes. If there's a good reason to
keep the full matrix, I'll come back to the code and do it the other way.

Done the other change.

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

https://codereview.chromium.org/2874203003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:314:
if (!point)
On 2017/05/15 at 18:00:30, jbroman wrote:
> nit: These null checks don't really add much (except maybe to suggest that
null is possible?). "return point;" will already return null if point is null
(and there's no other logic being skipped here). Maybe just "return
DOMPoint::Create(...);"? (here and below)

yep. done.

[simonp, 2017-05-18 07:13:42]

On 2017/05/15 18:27:28, fserb wrote:
> I think the whole is2D thing is potentially weird to begin with.
> Let's see how the conversation on the spec goes. If there's a good reason to
> keep the full matrix, I'll come back to the code and do it the other way.

https://github.com/w3c/fxtf-drafts/pull/162 is now merged; tests updated in
https://github.com/w3c/web-platform-tests/pull/5937

[fserb, 2017-05-18 17:16:39]

On 2017/05/18 at 07:13:42, simonp wrote:
> On 2017/05/15 18:27:28, fserb wrote:
> > I think the whole is2D thing is potentially weird to begin with.
> > Let's see how the conversation on the spec goes. If there's a good reason to
> > keep the full matrix, I'll come back to the code and do it the other way.
> 
> https://github.com/w3c/fxtf-drafts/pull/162 is now merged; tests updated in
https://github.com/w3c/web-platform-tests/pull/5937

Awesome so this is unblocked and ready.
I need the LGTM folks, make it rain. ;)

[jbroman, 2017-05-23 19:10:01]

https://codereview.chromium.org/2874203003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp
(right):

https://codereview.chromium.org/2874203003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:825:
// TEST(V8ScriptValueSerializerTest, TransferDOMPoint) {
Please still remove this commented code, and add decode tests.

[fserb, 2017-05-24 17:51:54]

donerino.

https://codereview.chromium.org/2874203003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp
(right):

https://codereview.chromium.org/2874203003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:825:
// TEST(V8ScriptValueSerializerTest, TransferDOMPoint) {
On 2017/05/23 at 19:10:01, jbroman wrote:
> Please still remove this commented code, and add decode tests.

I don't know what this was doing here. Sorry. Done.

[jbroman, 2017-05-24 17:53:34]

lgtm

[fserb, 2017-05-24 17:59:34]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2017-05-24 18:00:34]

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, 2017-05-24 19:44:41]

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

[commit-bot: I haz the power, 2017-05-24 19:44:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fserb, 2017-05-29 17:19:24]

The CQ bit was checked by fserb@chromium.org

[fserb, 2017-05-29 17:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2874203003/#ps120001
(title: "x")

[commit-bot: I haz the power, 2017-05-29 17:19:31]

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, 2017-05-29 19:25:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496078364237180,
"parent_rev": "168dc2f3cb966091dcca1abb798b5c4ebd7c5437", "commit_rev":
"9e8060a43d58b5e94f8c88518283376242e286b5"}

[commit-bot: I haz the power, 2017-05-29 19:25:44]

Description was changed from

==========
Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

BUG=721331
==========

to

==========
Implement serialization/deserialization of geometry interfaces

The WPT tests are in good shape but not submitted yet:
https://github.com/w3c/web-platform-tests/pull/5888

BUG=721331

Review-Url: https://codereview.chromium.org/2874203003
Cr-Commit-Position: refs/heads/master@{#475371}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e8060a43d58b5e94f8c88518283...
==========

[commit-bot: I haz the power, 2017-05-29 19:25:45]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9e8060a43d58b5e94f8c88518283...