Global constructors should be configurable and not enumerable

Global constructors should be configurable and not enumerable

Update bindings generator so that global constructors now have the following
attributes:
{ [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }
instead of previously:
{ [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: false }

The new behavior is according to the Web IDL specification (section 4.4):
http://dev.w3.org/cvsweb/~checkout~/2006/webapi/WebIDL/Overview.html?rev=1.617;content-type=text%2Fhtml#es-interfaces

This also matches the behavior of WebKit and Firefox.

BUG=235020
NOTRY=true

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

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

[do-not-use, 2013-04-24 15:21:44]

WebKit recently adopted this behavior in:
https://bugs.webkit.org/show_bug.cgi?id=110573

[haraken, 2013-04-24 15:23:53]

LGTM. This change looks safe, given the spec and the behavior of WebKit and
Firefox.

The diff of tests is hard to read, but you didn't remove any existing test,
right?

[do-not-use, 2013-04-24 15:34:00]

On 2013/04/24 15:23:53, haraken wrote:
> LGTM. This change looks safe, given the spec and the behavior of WebKit and
> Firefox.
> 
> The diff of tests is hard to read, but you didn't remove any existing test,
> right?

Well, I did remove LayoutTests/fast/js/global-constructors.html test case. It
was relying on the global constructors to be enumerable which they no longer
are. This test's purpose was to document the exposed global constructors but
this is no longer possible.

[arv (Not doing code reviews), 2013-04-24 15:34:08]

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/dom/DeviceOr...
File LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js
(right):

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/dom/DeviceOr...
LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js:15:
shouldBeFalse("hasDeviceOrientationEventProperty()");
The test on this line is a bit misleading now. Maybe remove it or change it to
test window.propertyIsEnumerable('DeviceOrientationEvent')?

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/dom/script-t...
File LayoutTests/fast/dom/script-tests/constructed-objects-prototypes.js (left):

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/dom/script-t...
LayoutTests/fast/dom/script-tests/constructed-objects-prototypes.js:24: for (var
property in globalObject) {
Can we change the loop to use Object.getOwnPropertyNames instead?

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/js/script-te...
File LayoutTests/fast/js/script-tests/global-constructors-attributes.js (right):

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/js/script-te...
LayoutTests/fast/js/script-tests/global-constructors-attributes.js:4: var test =
'Object.getOwnPropertyDescriptor('+object+", "+property+')';
whitespace around binops

https://codereview.chromium.org/14447006/diff/1/LayoutTests/fast/js/script-te...
LayoutTests/fast/js/script-tests/global-constructors-attributes.js:4: var test =
'Object.getOwnPropertyDescriptor('+object+", "+property+')';
Stick to either " or '

https://codereview.chromium.org/14447006/diff/1/LayoutTests/platform/chromium...
File
LayoutTests/platform/chromium-win/fast/dom/DeviceOrientation/window-property-expected.txt
(right):

https://codereview.chromium.org/14447006/diff/1/LayoutTests/platform/chromium...
LayoutTests/platform/chromium-win/fast/dom/DeviceOrientation/window-property-expected.txt:8:
PASS hasDeviceOrientationEventProperty() is false
Yeah, this reads wrong.

[arv (Not doing code reviews), 2013-04-24 15:35:24]

On 2013/04/24 15:34:00, cdumez wrote:
> On 2013/04/24 15:23:53, haraken wrote:
> > LGTM. This change looks safe, given the spec and the behavior of WebKit and
> > Firefox.
> > 
> > The diff of tests is hard to read, but you didn't remove any existing test,
> > right?
> 
> Well, I did remove LayoutTests/fast/js/global-constructors.html test case. It
> was relying on the global constructors to be enumerable which they no longer
> are. This test's purpose was to document the exposed global constructors but
> this is no longer possible.

Object.getOwnPropertyNames(window) gives you this list again.

[do-not-use, 2013-04-24 15:42:05]

On 2013/04/24 15:35:24, arv wrote:
> On 2013/04/24 15:34:00, cdumez wrote:
> > On 2013/04/24 15:23:53, haraken wrote:
> > > LGTM. This change looks safe, given the spec and the behavior of WebKit
and
> > > Firefox.
> > > 
> > > The diff of tests is hard to read, but you didn't remove any existing
test,
> > > right?
> > 
> > Well, I did remove LayoutTests/fast/js/global-constructors.html test case.
It
> > was relying on the global constructors to be enumerable which they no longer
> > are. This test's purpose was to document the exposed global constructors but
> > this is no longer possible.
> 
> Object.getOwnPropertyNames(window) gives you this list again.

Right. Much better indeed. I'll upload a new iteration of the CL.

[do-not-use, 2013-04-24 16:47:47]

On 2013/04/24 15:42:05, cdumez wrote:
> On 2013/04/24 15:35:24, arv wrote:
> > On 2013/04/24 15:34:00, cdumez wrote:
> > > On 2013/04/24 15:23:53, haraken wrote:
> > > > LGTM. This change looks safe, given the spec and the behavior of WebKit
> and
> > > > Firefox.
> > > > 
> > > > The diff of tests is hard to read, but you didn't remove any existing
> test,
> > > > right?
> > > 
> > > Well, I did remove LayoutTests/fast/js/global-constructors.html test case.
> It
> > > was relying on the global constructors to be enumerable which they no
longer
> > > are. This test's purpose was to document the exposed global constructors
but
> > > this is no longer possible.
> > 
> > Object.getOwnPropertyNames(window) gives you this list again.
> 
> Right. Much better indeed. I'll upload a new iteration of the CL.

I took Arv's feedback into consideration.

[arv (Not doing code reviews), 2013-04-24 17:07:07]

LGTM

Thanks, this makes the diff of the tests much smaller.

[do-not-use, 2013-04-25 06:06:38]

Could someone please cq+ ?

[commit-bot: I haz the power, 2013-04-25 06:16:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14447006/2002

[commit-bot: I haz the power, 2013-04-25 06:22:04]

Presubmit check for 14447006-2002 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 10 file(s).

[commit-bot: I haz the power, 2013-04-25 06:24:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14447006/2002

[commit-bot: I haz the power, 2013-04-25 06:30:54]

Presubmit check for 14447006-2002 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 10 file(s).

[eseidel, 2013-04-25 06:42:55]

Very interesting.  lgtm2.

[do-not-use, 2013-04-25 08:11:29]

Not sure why the CQ fails so I tried rebasing.

[commit-bot: I haz the power, 2013-04-25 08:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14447006/7011

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

Presubmit check for 14447006-7011 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 10 file(s).

[commit-bot: I haz the power, 2013-04-25 08:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14447006/7011

[commit-bot: I haz the power, 2013-04-25 08:27:46]

Presubmit check for 14447006-7011 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 10 file(s).

[do-not-use, 2013-04-25 13:33:53]

On 2013/04/25 08:27:46, I haz the power (commit-bot) wrote:
> Presubmit check for 14447006-7011 failed and returned exit status -2001.
> The presubmit check was hung. It took 360.0 seconds to execute and the time
> limit is 360.0 seconds.
> 
> INFO:root:Found 10 file(s).

Seems like presubmit check gets hung with this patch :/
Please tell me if I need to do something special.

[haraken, 2013-04-25 13:41:35]

I'll manually land it for you.

[haraken, 2013-04-25 13:45:21]

Committed patchset #5 manually as r149104 (presubmit successful).

[do-not-use, 2013-04-25 13:52:17]

On 2013/04/25 13:45:21, haraken wrote:
> Committed patchset #5 manually as r149104 (presubmit successful).

Thanks Kentaro!

[dglazkov, 2013-04-25 17:13:12]

The concat-while-having-a-bad-time.html test has been failing ever since this
patch landed.

[do-not-use, 2013-04-25 17:22:51]

On 2013/04/25 17:13:12, Dimitri Glazkov wrote:
> The concat-while-having-a-bad-time.html test has been failing ever since this
> patch landed.

Something went wrong with the commit. This test case was wrongly edited. I
uploaded a CL a fix the test case at:
https://codereview.chromium.org/14190020/

Thanks for notifying me.

[do-not-use, 2013-04-25 17:26:27]

haraken: I don't know what happened but it appears my new test case was not
committed either. The following files are missing although present in my CL:
 LayoutTests/fast/js/global-constructors-attributes.html 
 LayoutTests/fast/js/global-constructors-attributes-expected.txt
 LayoutTests/fast/js/script-tests/global-constructors-attributes.js

Should I upload a new CL to add them?

[haraken, 2013-04-25 17:28:22]

> Should I upload a new CL to add them?

I don't know why, but would you upload them? I'll land them manually.

[do-not-use, 2013-04-25 17:39:40]

On 2013/04/25 17:28:22, haraken wrote:
> > Should I upload a new CL to add them?
> 
> I don't know why, but would you upload them? I'll land them manually.

Ok, did so at https://codereview.chromium.org/14211015/

[haraken, 2013-04-25 19:13:06]

I'm sorry for the confusions... The patch was reverted in r149118.

Would you rebaseline the tests and reupload the entire patch?

[do-not-use, 2013-04-25 19:41:36]

On 2013/04/25 19:13:06, haraken wrote:
> I'm sorry for the confusions... The patch was reverted in r149118.
> 
> Would you rebaseline the tests and reupload the entire patch?

Sure, I rebaselined the fast/dom/DeviceOrientation/window-property.html test and
uploaded with --no-find-copies (to avoid issue with
global-constructors-attributes.html test this time).

[do-not-use, 2013-04-25 19:43:29]

On 2013/04/25 19:41:36, cdumez wrote:
> On 2013/04/25 19:13:06, haraken wrote:
> > I'm sorry for the confusions... The patch was reverted in r149118.
> > 
> > Would you rebaseline the tests and reupload the entire patch?
> 
> Sure, I rebaselined the fast/dom/DeviceOrientation/window-property.html test
and
> uploaded with --no-find-copies (to avoid issue with
> global-constructors-attributes.html test this time).

LayoutTests/fast/js/script-tests/global-constructors.js is marked as removed in
the CL even though it is not locally. I'm trying to figure out why :/

[do-not-use, 2013-04-25 20:08:55]

On 2013/04/25 19:43:29, cdumez wrote:
> On 2013/04/25 19:41:36, cdumez wrote:
> > On 2013/04/25 19:13:06, haraken wrote:
> > > I'm sorry for the confusions... The patch was reverted in r149118.
> > > 
> > > Would you rebaseline the tests and reupload the entire patch?
> > 
> > Sure, I rebaselined the fast/dom/DeviceOrientation/window-property.html test
> and
> > uploaded with --no-find-copies (to avoid issue with
> > global-constructors-attributes.html test this time).
> 
> LayoutTests/fast/js/script-tests/global-constructors.js is marked as removed
in
> the CL even though it is not locally. I'm trying to figure out why :/

Visibly just a display bug. Even though there is a 'D' next to
LayoutTests/fast/js/script-tests/global-constructors.js, the diff for this file
is correct.

[eseidel, 2013-04-25 20:21:21]

Filed https://code.google.com/p/chromium/issues/detail?id=235564 about the
rietveld display issue.

[haraken, 2013-04-26 06:33:14]

LGTM. Re-landing.

[commit-bot: I haz the power, 2013-04-26 06:33:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14447006/26012

[commit-bot: I haz the power, 2013-04-26 06:34:11]

Change committed as 149170