1363215 - Replace calls to __defineGetter__ and __defineSetter__ on top-level this with Object.defineProperty

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Andrew McCreight [:mccr8]]

__defineGetter__ and __defineSetter__ are deprecated, and their use on top level |this| objects in .jsms gets in the way of bug 1186409, because |this| ends up not being a top-level window, so it doesn't have that method on it. I also fixed a few other places that were in the same file as uses on top-level this to avoid introducing too much inconsistency. I think this is really only needed for the one place I changed this that is in a .jsm.

I set configurable and enumerable to true. This seems to be how the old ones are defined per the ECMAScript spec: https://tc39.github.io/ecma262/#sec-additional-ecmascript-features-for-web-browsers

Also, this is required to avoid some test failures of the form "test left unexpected property on window" because things like the getter for AddonManager in browser/base/content/browser.js delete the existing property and replace it with a normal one (which is enumerable), so you have to make sure the original getter is also enumerable.

Comment 1

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0174293131420623c1b34f1bc20ceba815d9e114

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1363215 - Replace calls to __define{Getter,Setter}__ on top-level this with Object.defineProperty.

__define{Getter,Setter}__ are deprecated, and are not defined on
NonSyntacticVariablesObjects, so these calls get in the way of sharing
globals between different .jsms. Probably only the DownloadUtils.jsm
change is really needed for that.

configurable and enumerable are both set to true to match the existing
behavior. If enumerable is set to false, then tests fail, because some
of the getters overwrite the getter with a regular property.

Review commit: https://reviewboard.mozilla.org/r/137532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137532/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8865948 [details]
Bug 1363215 - Replace calls to __define{Getter,Setter}__ on top-level this with Object.defineProperty.

https://reviewboard.mozilla.org/r/137532/#review140796

Comment 4

[:Gijs (he/him)]

Florian, do we have a bug on switching over to the new/standards syntax and making __define* an eslint error?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96dd2e2d8d16
Replace calls to __define{Getter,Setter}__ on top-level this with Object.defineProperty. r=Gijs

Comment 6

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #4)
> Florian, do we have a bug on switching over to the new/standards syntax and
> making __define* an eslint error?

Not that I'm aware of. That's likely scriptable, although the result will be unfortunate where the current version fits on a single line.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eab436c452e9
Replace calls to __define{Getter,Setter}__ on top-level this with Object.defineProperty. r=Gijs

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eab436c452e9