932322 - Make Window's WebIDL properties be own properties of window

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can we make nsINode::WrapObject final now?

Comment 2

[Peter Van der Beken [:peterv]]

No (see for example Element::WrapObject).

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://github.com/w3c/web-platform-tests/pull/394

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 824031 [details] [diff] [review]
v1

>+nsWindowSH::PostCreate(nsIXPConnectWrappedNative *wrapper,
>+  NS_ASSERTION(!sgo || sgo->GetGlobalJSObject() == nullptr,

How could sgo be null here?  We should assert it's not, I would think.

>-  static inline bool IsReadonlyReplaceable(jsid id)

Now that IsReadonlyReplaceable is gone, can you get rid of some of those jsids it was using?

>+++ b/dom/imptests/failures/html/html/browsers/the-window-object/test_window-properties.html.json

Followup to get this test fixed?  Or will that happen when we flip Window.prototype to WebIDL bindings?

>+++ b/dom/tests/mochitest/general/test_interfaces.html
>+var prefixedProperties =

Could you please document that this is needed because runTest() thinks strings starting with "moz" and a capital letter are interface names?

We still need codegen changes, in a followup, to not break this when we switch Window to actual WebIDL bindings, right?

r=me with the above nits.  This is awesome.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

\o/

Comment 6

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e697e7dcc321

Comment 7

[Ed Morley [:emorley]]

Backed out because this depended on bug 932309, which needed to be backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f97de1c0727

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aea34e69d171

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for making Linux32 opt mochitest-bc perma-orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d4847dfd5c

https://tbpl.mozilla.org/php/getParsedLog.php?id=30087228&tree=Mozilla-Inbound

Comment 10

[Peter Van der Beken [:peterv]]

With extended timeout for the failing test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/834b35ebbce7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Apparently a timeout of 4 still isn't enough. Upped it to 6.
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fe02c5c163

Am I understanding correctly that this test will get perpetually slower as we continue to add more properties? That smacks of very poor test design. I think the next time I see this test fail, I'm just going to disable it. This is already becoming a joke.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I think this regressed Bug 916851 rather badly. Verifying.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Regression: Mozilla-Inbound-Non-PGO - Dromaeo (CSS) - Ubuntu HW 12.04 - 2.7% decrease
-------------------------------------------------------------------------------------
    Previous: avg 4953.760 stddev 40.139 of 12 runs up to revision ba666d2dbb97
    New     : avg 4819.818 stddev 43.211 of 12 runs since revision 834b35ebbce7
    Change  : -133.942 (2.7% / z=3.337)
    Graph   : http://mzl.la/1a5OLL4

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ba666d2dbb97&tochange=834b35ebbce7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/834b35ebbce7
https://hg.mozilla.org/mozilla-central/rev/c6ebee99e089

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

So yes, this did regress Bug 916851 about 50%.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66fe02c5c163

Comment 18

[Mihai Sucan [:msucan]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #11)
> Apparently a timeout of 4 still isn't enough. Upped it to 6.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/66fe02c5c163
> 
> Am I understanding correctly that this test will get perpetually slower as
> we continue to add more properties? That smacks of very poor test design. I
> think the next time I see this test fail, I'm just going to disable it. This
> is already becoming a joke.

Unfortunately, this was not expected. Populating the variables view shouldn't take so much with all of those window properties. Afaik, we had some work done for improving the performance of vview.

If the test fails again, please feel free to disable the test and file a bug - please CC me and vporof.

Comment 19

[David Bruant]

Is there more to document than the trap described in bug 943958 ?

If this breaks an add-on, doesn't it have high chance to also break the web? :-(

Comment 20

[Andrew McCreight [:mccr8]]

Well, any sites that rely on this may already be broken in Chrome.  I made a similar change before, and the only evidence of breakage I saw was a single site for a local club, and the page was broken in Chrome, which already had our behavior for that particular property.

Comment 21

[David Bruant]

(In reply to Andrew McCreight [:mccr8] from comment #20)
> Well, any sites that rely on this may already be broken in Chrome.  I made a
> similar change before, and the only evidence of breakage I saw was a single
> site for a local club, and the page was broken in Chrome, which already had
> our behavior for that particular property.
Oh yeah I had forgotten that it's already Chrome's behavior, my bad. Thanks!

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Is there more to document than the trap described in bug 943958 ?

No.

> doesn't it have high chance to also break the web?

Maybe.

> any sites that rely on this may already be broken in Chrome.

Sort of.  Chrome puts idl attributes on the window but methods on the prototype.  We now put both on the window.

The good news is that methods are just value props, so doing:

  var setTimeout = 5;

works fine and sets it to 5.  It's probably not optimized as well as a var with some other name, though.  So you have to go out of your way to be broken by methods moving off the proto.  For example, you have to be setting Window.prototype.setTimeout to to a function of your own or some such.  See also bug 943065.

Comment 23

[Masatoshi Kimura [:emk]]

Chrome (or WebKit/Blink) just keeps pre-ES5 (or old IE) behavior (methods are on prototype, attributes are defined as own properties). It is not specific to the window object.
I don't understand why the spec try to continue inventing new behaviors, breaking the compatibility, and diverging implementations rather than converging them.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I don't understand why the spec try to continue inventing new behaviors

In this case, see bug 943065 comment 6 for an example that necessitated the new behavior...

Comment 25

[Peter Van der Beken [:peterv]]

I'll also note that the spec explicitly documents why [Global] is specified the way it is: http://heycam.github.io/webidl/#Global

Comment 26

[Kohei Yoshino [:kohei]]

Added this to https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility

Please feel free to edit/clarify the description.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Note that this broke IBM Rational Change (which was apparently also broken in Chrome/Safari).  See http://forums.mozillazine.org/viewtopic.php?f=23&t=2802773

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Mostly back out for now; only define the unforgeable properties on the window object itself.

Comment 29

[Kohei Yoshino [:kohei]]

Adding a disabled flag as per Bug 976920.

Comment 30

[Kohei Yoshino [:kohei]]

Added back to the 31 site compat doc:

https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility#DOM
https://twitter.com/FxSiteCompat/status/477510724810334208