Make Window's WebIDL properties be own properties of window

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

(Depends on: 2 bugs, {addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla28
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 disabled, firefox29 disabled, firefox30 disabled)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 824031 [details] [diff] [review]
v1
Attachment #824031 - Flags: review?(bzbarsky)
Can we make nsINode::WrapObject final now?
(Assignee)

Comment 2

4 years ago
No (see for example Element::WrapObject).
https://github.com/w3c/web-platform-tests/pull/394
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.
Attachment #824031 - Flags: review?(bzbarsky) → review+
\o/
(Assignee)

Comment 6

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

Comment 7

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

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

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea34e69d171
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
(Assignee)

Comment 10

4 years ago
With extended timeout for the failing test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/834b35ebbce7
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.
Flags: needinfo?(mihai.sucan)
I think this regressed Bug 916851 rather badly. Verifying.
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
https://hg.mozilla.org/mozilla-central/rev/834b35ebbce7
https://hg.mozilla.org/mozilla-central/rev/c6ebee99e089
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
So yes, this did regress Bug 916851 about 50%.
Depends on: 916851

Updated

4 years ago
No longer depends on: 916851

Updated

4 years ago
Depends on: 935283
Duplicate of this bug: 781739
https://hg.mozilla.org/mozilla-central/rev/66fe02c5c163
Depends on: 935441
Depends on: 935442
(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.
Flags: needinfo?(mihai.sucan)
Depends on: 936129
Depends on: 943958
Keywords: addon-compat, dev-doc-needed

Comment 19

4 years ago
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? :-(
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

4 years ago
(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!
> 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.
Depends on: 943065
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.
> 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...
(Assignee)

Comment 25

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

Updated

4 years ago
Depends on: 952365

Updated

4 years ago
Keywords: site-compat
Added this to https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility

Please feel free to edit/clarify the description.
Keywords: dev-doc-needed → dev-doc-complete
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
Blocks: 975699
Depends on: 973702
Depends on: 976920
Created attachment 8381883 [details] [diff] [review]
Mostly back out  for now; only define the unforgeable properties on the window object itself.
Assignee: peterv → bzbarsky
Attachment #8381883 - Attachment is obsolete: true
Assignee: bzbarsky → peterv
Adding a disabled flag as per Bug 976920.
status-firefox28: --- → disabled

Updated

4 years ago
status-firefox29: --- → disabled
status-firefox30: --- → disabled
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
Depends on: 1048196

Updated

3 years ago
Depends on: 1066883
You need to log in before you can comment on or make changes to this bug.