Remove structured cloning restriction over accessor properties.

Remove structured cloning restriction over accessor properties.

The structured cloning algorithm,

 http://www.whatwg.org/specs/web-apps/current-work/#safe-passing-of-structured-data

treats accessor and non-accessor properties the same on an object
being cloned. For a short two month period, the spec'ed algorithm
imposed a stringent restriction on the cloning of accessor properties
-- no accessor property value could appear within/inside the value of
another accessor property. If any did, their values were mapped to
'null'; see

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

The structured cloning implementation provided this; a change that was
subsequently reverted and generalized to allow accessor properties to
be cloned without any side conditions on how they are nested. Follow
up on that spec change here and do the same. Other depth controls on
nested object structures are still effective.

R=mkwst
BUG=278883
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162969

[sof, 2013-12-01 19:44:11]

At your leisure, please have a look.

[Mike West, 2013-12-01 20:26:49]

LGTM. This seems like a pretty reasonable change to the spec, and appears to
match Firefox's behavior.

I'd suggest adding a bit more detail to the commit message, basically cutting
bits and pieces of the comment from the test JS file and pasting it into the
description box.

https://codereview.chromium.org/97883002/diff/1/LayoutTests/fast/dom/Window/s...
File LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js (right):

https://codereview.chromium.org/97883002/diff/1/LayoutTests/fast/dom/Window/s...
LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js:170: // accessor
properties overall.
I'd suggest just adding something like "Verify that accessors and non-accessors
are treated equivalently." as a comment in this file. The justification for that
change isn't really relevant to the test, and could be moved out to the commit
message.

[sof, 2013-12-01 21:35:11]

https://codereview.chromium.org/97883002/diff/1/LayoutTests/fast/dom/Window/s...
File LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js (right):

https://codereview.chromium.org/97883002/diff/1/LayoutTests/fast/dom/Window/s...
LayoutTests/fast/dom/Window/script-tests/postmessage-clone.js:170: // accessor
properties overall.
On 2013/12/01 20:26:49, Mike West wrote:
> I'd suggest just adding something like "Verify that accessors and
non-accessors
> are treated equivalently." as a comment in this file. The justification for
that
> change isn't really relevant to the test, and could be moved out to the commit
> message.

Done; indeed, information better kept with the commit message.

[commit-bot: I haz the power, 2013-12-01 21:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/97883002/20001

[Dmitry Lomov (no reviews), 2013-12-01 22:06:00]

On 2013/12/01 21:54:58, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/sigbjornf%40opera.com/97883002/20001

not lgtm (yet!)
needs test for cyclic getter:

var circular = {get self() {return circular}};

Thanks!

[Dmitry Lomov (no reviews), 2013-12-01 22:10:15]

On 2013/12/01 22:06:00, Dmitry Lomov (chromium) wrote:
> On 2013/12/01 21:54:58, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/sigbjornf%2540opera.com/97883002/20001
> 
> not lgtm (yet!)
> needs test for cyclic getter:
> 
> var circular = {get self() {return circular}};
> 
> Thanks!

(also consider deeper cyclic structure)

[sof, 2013-12-01 22:23:34]

On 2013/12/01 22:06:00, Dmitry Lomov (chromium) wrote:
> On 2013/12/01 21:54:58, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/sigbjornf%2540opera.com/97883002/20001
> 
> not lgtm (yet!)
> needs test for cyclic getter:
> 
> var circular = {get self() {return circular}};
> 
> Thanks!

Certainly, now done.

[ Sorry for springing this CL (and cq addition) on you both so late on a Sunday
eve. ]

[Dmitry Lomov (no reviews), 2013-12-01 22:27:50]

Could you hold off CQ until tomorrow? I like your change in general, and
Mike likes it too, but I just need to think about it more than I can this
Sunday night (and I have spent some time with this code before :)). Thanks
much!
On Dec 1, 2013 11:23 PM, <sigbjornf@opera.com> wrote:

> On 2013/12/01 22:06:00, Dmitry Lomov (chromium) wrote:
>
>> On 2013/12/01 21:54:58, I haz the power (commit-bot) wrote:
>> > CQ is trying da patch. Follow status at
>> >
>>
> https://chromium-status.appspot.com/cq/sigbjornf%
> 2540opera.com/97883002/20001
>
>  not lgtm (yet!)
>> needs test for cyclic getter:
>>
>
>  var circular = {get self() {return circular}};
>>
>
>  Thanks!
>>
>
> Certainly, now done.
>
> [ Sorry for springing this CL (and cq addition) on you both so late on a
> Sunday
> eve. ]
>
> https://codereview.chromium.org/97883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2013-12-01 22:33:56]

On 2013/12/01 22:27:50, Dmitry Lomov (chromium) wrote:
> Could you hold off CQ until tomorrow? I like your change in general, and
> Mike likes it too, but I just need to think about it more than I can this
> Sunday night (and I have spent some time with this code before :)). Thanks
> much!

of course; no particular rush - can definitely wait until you have time to think
through all aspects.

[Dmitry Lomov (no reviews), 2013-12-02 07:51:12]

lgtm
This is a great simplification - thanks a lot for holding off and letting my
tiny brain catch up :)

[commit-bot: I haz the power, 2013-12-02 07:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/97883002/40001

[sof, 2013-12-02 08:19:11]

On 2013/12/02 07:51:12, Dmitry Lomov (chromium) wrote:
> lgtm
> This is a great simplification - thanks a lot for holding off and letting my
> tiny brain catch up :)

Thanks :)

This temporary accessor restriction spec change passed me by at the time
(Opera's Carakan never implemented it.)

[commit-bot: I haz the power, 2013-12-02 08:45:13]

Change committed as 162969