Open Bug 811857 Opened 12 years ago Updated 2 years ago

eliminate all uses of __[define,lookup][GS]etter__

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

People

(Reporter: dherman, Unassigned)

Details

Attachments

(1 file, 8 obsolete files)

Part 1: browser/ directory cleanup
20.80 KB, patch
mak
: feedback-
Details | Diff | Splinter Review 
All four of

    __defineGetter__
    __defineSetter__
    __lookupGetter__
    __lookupSetter__

are implementable using cleaner ES5 standard API's (Object.defineProperty and Object.getOwnPropertyDescriptor). This bug is for eliminating all uses of these API's in chrome code.

Dave
Assignee: nobody → anton
(Correct patch this time)

Try: https://tbpl.mozilla.org/?tree=Try&rev=745b8113eb60

Who can review this?
Attachment #687518 - Attachment is obsolete: true
Comment on attachment 687519 [details] [diff] [review]
Part 1: browser/ directory cleanup

Gavin, please redirect as needed?
Attachment #687519 - Flags: review?(gavin.sharp)
Comment on attachment 687519 [details] [diff] [review]
Part 1: browser/ directory cleanup

>@@ -4059,17 +4080,18 @@
>         // Remove all the notifications, except for those which want to
>         // persist across the first location change.
>         let nBox = gBrowser.getNotificationBox(selectedBrowser);
>         nBox.removeTransientNotifications();
> 
>         // Only need to call locationChange if the PopupNotifications object
>         // for this window has already been initialized (i.e. its getter no
>         // longer exists)
>-        if (!__lookupGetter__("PopupNotifications"))
>+        let desc = Object.getOwnPropertyDescriptor(this, "PopupNotifications");
>+        if (!desc || !desc.get)
>           PopupNotifications.locationChange();
>       }
>     }

'this' needs to be 'window' instead
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 687519 [details] [diff] [review]
Part 1: browser/ directory cleanup

So verbose! :/ I'm sure this was probably discussed at length somewhere, but I wonder why defineProperty's configurable/enumerable don't default to true...

This patch highlights a lot of existing places where we could be doing things more simply. If this bug is blocking other work and you don't want to get into replacing them all, that's fine - we can land a tweaked version of this patch. But I wanted to write down all the things that need changing so that we can at least file followups and get it done eventually.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+Object.defineProperty(this, "AddonManager", {
>+Object.defineProperty(this, "PluralForm", {
>+Object.defineProperty(window, name, {

It would be really nice to have these three use a common helper:

function defineOverrideableLazyProp(obj, name, getter) {
  Object.defineProperty(obj, name, {
    enumerable: true,
    configurable: true,
    get: getter,
    set: function (val) {
      delete obj[name];
      return obj[name] = val;
    }
  }
}

>-    if (!__lookupGetter__("PopupNotifications"))
>+    let desc = Object.getOwnPropertyDescriptor(this, "PopupNotifications");
>+    if (!desc || !desc.get)
>       PopupNotifications.iconBox = document.getElementById("notification-popup-box");

This doesn't look quite right... If !desc, won't referencing "PopupNotifications" throw? That's never really going to happen in practice, but could still avoid the confusion by using "if (desc && !desc.get)", I think (same applies to other such references to "PopupNotifications").

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js

>+Object.defineProperty(Sanitizer, "prefs", {

This should really just import XPCOMUtils/Services.jsm and use:
XPCOMUtils.defineLazyGetter(Sanitizer, "prefs", function () {
  return Services.prefs.getBranch(Sanitizer.prefDomain);
});

(and then we should file a followup to make use of Services.jsm across sanitize.js. And then maybe another one on moving it to toolkit so that Fennec and Firefox don't have forked copies of it :()

>diff --git a/browser/base/content/sanitizeDialog.js b/browser/base/content/sanitizeDialog.js

>+    Object.defineProperty(view, "rowCount",

>-        return this._rowCount + 1;
>+        return this._rowCount += 1;

Copy/pasto? Getter that has side effects doesn't look right!

(What is this code even trying to do? Need to look at this closer.)

>diff --git a/browser/base/content/test/browser_bug521216.js b/browser/base/content/test/browser_bug521216.js

>+Object.defineProperty(this, "tab", {

This getter seems to make the test more confusing than it needs to be. I'd just remove it and replace uses of it with "gBrowser.tabs[tabIndex]".

>diff --git a/browser/components/distribution.js b/browser/components/distribution.js

>   get _ini() {

Just replace this with:

get _ini() {
  delete this._ini;
  return this._ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].getService(Ci.nsIINIParserFactory).createINIParser(this._iniFile);
}

Same idea for all of the getters in this file (and we need a followup to make this file use Services.jsm too).

>diff --git a/browser/components/places/tests/browser/head.js b/browser/components/places/tests/browser/head.js

>+let (getter =

I think this would be clearer as:
let propDesc = Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId");
if (propDesc && propDesc.get)
  cachedLeftPaneFolderIdGetter = propDesc.get;

> // ...And restore it when test ends.

And this as:

let propDesc = Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId");
if (propDesc && !propDesc.get) {
  Object.defineProperty(PlacesUIUtils, "leftPaneFolderId", {
    enumerable: true,
    configurable: true,
    get: cachedLeftPaneFolderIdGetter
  });
}

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>   get _prefs() {

This should just be:

get _prefs() {
  delete this._prefs;
  return this._prefs = Services.prefs;
}

(this module already imports Services.jsm, we should get a bug on file to further its use throughout the file)

>diff --git a/browser/components/test/browser_bug538331.js b/browser/components/test/browser_bug538331.js

>+Object.defineProperty(this, "gBG", {

This can use XPCOMUtils.defineLazyGetter.

>diff --git a/browser/fuel/src/fuelApplication.js b/browser/fuel/src/fuelApplication.js

>   get bookmarks() {

All of the getters in this file should use the:

get foo() {
  delete this.foo;
  return this.foo = bar;
}
pattern.
Attachment #687519 - Flags: review?(gavin.sharp) → review-
Updated the patch based on reviewers' comments.

Try: https://tbpl.mozilla.org/?tree=Try&rev=52d343e55577

Gavin: just so I don't file unnecessary bugs, could you confirm the follow-ups below?

1. Use Services.jsm across sanitize.js
2. Use Services.jsm across distribution.js
3. Use Services.jsm across nsPrivateBrowsingService.js
Attachment #687519 - Attachment is obsolete: true
Attachment #697778 - Flags: review?(gavin.sharp)
Note that successive Object.defineProperty(obj, ...) calls can be combined into a single Object.defineProperties call, which may be clearer, depending -- not sure if you have any past the one that got quoted earlier (if that was even a direct quote).
Comment on attachment 697778 [details] [diff] [review]
Part 1: browser/ directory cleanup

>+function defineOverrideableLazyProp(obj, name, getter) {

We don't necessarily need that stuff to be overrideable. Extensions trying to set it would just cause a non-disruptive warning. The only thing we need to be able to set is gURLBar and we can explicitly 'delete' the property before setting it there.
Unfortunately deletability is not separated from other configurabilities.
Or are you complaining about the function name?
Comment on attachment 697778 [details] [diff] [review]
Part 1: browser/ directory cleanup

>     // Hacky: update the PopupNotifications' object's reference to the iconBox,
>     // if it already exists, since it may have changed if the URL bar was
>     // added/removed.
>-    if (!__lookupGetter__("PopupNotifications"))
>+    let desc = Object.getOwnPropertyDescriptor(window, "PopupNotifications");
>+    if (desc && !desc.get)

this can just be:

    if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get)

>   get bookmarks() {
>     let bookmarks = new BookmarkRoots();
>-    this.__defineGetter__("bookmarks", function() bookmarks);
>+
>+    Object.defineProperty(this, "bookmarks", {
>+      enumerable: true,
>+      configurable: true,
>+      get: function () bookmarks
>+    });
>+
>     return this.bookmarks;
>   },

how is this different from:

   get bookmarks() {
     delete this.bookmarks;
     return this.bookmarks = new BookmarkRoots();
   },
(In reply to Masatoshi Kimura [:emk] from comment #9)
> Unfortunately deletability is not separated from other configurabilities.
> Or are you complaining about the function name?

The whole function is unnecessary. We can just use the XPCOMUtils stuff.
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Masatoshi Kimura [:emk] from comment #9)
> > Unfortunately deletability is not separated from other configurabilities.
> > Or are you complaining about the function name?
> 
> The whole function is unnecessary. We can just use the XPCOMUtils stuff.

Does the error in bug 394759 comment 109 no longer occur? If so, do you know what changed that behavior?
It's a warning these days, as mentioned earlier ("Warning: TypeError: setting a property that has only a getter"). I don't know what changed the behavior.
There was a typo in my previous patch. Here's an updated version (no other changes yet).

Try: https://tbpl.mozilla.org/?tree=Try&rev=eb7fcdbe903d
Attachment #697778 - Attachment is obsolete: true
Attachment #697778 - Flags: review?(gavin.sharp)
Attachment #698186 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Another typo fix. Try: https://tbpl.mozilla.org/?tree=Try&rev=86a0b9ab4710
Attachment #698186 - Attachment is obsolete: true
Attachment #698186 - Flags: review?(gavin.sharp)
Attachment #699463 - Flags: review?(gavin.sharp)
Last build is clean. Gavin, are there any other comments you'd like me to address? I'm working on Part 2 now (all other directories except for js).
Anton, you haven't addressed any of my comments.
(In reply to Dão Gottwald [:dao] from comment #17)
> Anton, you haven't addressed any of my comments.

My bad. I thought I addressed them together with Gavin's in my previous patches. I'll check again and fix. Sorry about that.
(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 697778 [details] [diff] [review]
> Part 1: browser/ directory cleanup
> 
> >+function defineOverrideableLazyProp(obj, name, getter) {
> 
> We don't necessarily need that stuff to be overrideable. Extensions trying
> to set it would just cause a non-disruptive warning. The only thing we need
> to be able to set is gURLBar and we can explicitly 'delete' the property
> before setting it there.

Should this be part of this patch or a follow-up bug? Since this bug is simply about replacing deprecated constructs with their ES5 equivalents I'd rather file all other changes as follow ups.

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 697778 [details] [diff] [review]
> Part 1: browser/ directory cleanup
> 
> >     // Hacky: update the PopupNotifications' object's reference to the iconBox,
> >     // if it already exists, since it may have changed if the URL bar was
> >     // added/removed.
> >-    if (!__lookupGetter__("PopupNotifications"))
> >+    let desc = Object.getOwnPropertyDescriptor(window, "PopupNotifications");
> >+    if (desc && !desc.get)
> 
> this can just be:
> 
>     if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get)
>

Won't this throw if window object doesn't have a PopupNotifications property? Or does window object always come with this property? 

I'll upload a new patch with other comments address shortly.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a402e5ec6c1e
Attachment #699463 - Attachment is obsolete: true
Attachment #699463 - Flags: review?(gavin.sharp)
Attachment #702009 - Flags: review?(gavin.sharp)
Attachment #702009 - Flags: review?(dao)
(In reply to Anton Kovalyov (:anton) from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Comment on attachment 697778 [details] [diff] [review]
> > Part 1: browser/ directory cleanup
> > 
> > >+function defineOverrideableLazyProp(obj, name, getter) {
> > 
> > We don't necessarily need that stuff to be overrideable. Extensions trying
> > to set it would just cause a non-disruptive warning. The only thing we need
> > to be able to set is gURLBar and we can explicitly 'delete' the property
> > before setting it there.
> 
> Should this be part of this patch or a follow-up bug? Since this bug is
> simply about replacing deprecated constructs with their ES5 equivalents I'd
> rather file all other changes as follow ups.

It should be part of this patch, since defineOverrideableLazyProp would be a new pseudo API that extensions might start depending on, making it harder to remove it again.

> Won't this throw if window object doesn't have a PopupNotifications
> property? Or does window object always come with this property?

It always has this property (it's provided in the same file even).
Replaced defineOverrideableLazyProp with XPCOMUtils.defineLazyGetter and XPCOMUtils.defineLazyModuleGetter.

Try: https://tbpl.mozilla.org/?tree=Try&rev=334fc1ebd96e
Attachment #702009 - Attachment is obsolete: true
Attachment #702009 - Flags: review?(gavin.sharp)
Attachment #702009 - Flags: review?(dao)
Attachment #703028 - Flags: review?(gavin.sharp)
Attachment #703028 - Flags: review?(dao)
Comment on attachment 703028 [details] [diff] [review]
Part 1: browser/ directory cleanup

>-XPCOMUtils.defineLazyGetter(this, "gPrefService", function() {
>+XPCOMUtils.defineLazyGetter(window, "gPrefService", function() {
>   return Services.prefs;
> });

Avoid this change. For consistency with most other lazy getters being set in browser.js, I'd also prefer if you used 'this' rather than 'window' in the getters you're adding.

>-        if (!__lookupGetter__("PopupNotifications"))
>+        let desc = Object.getOwnPropertyDescriptor(window, "PopupNotifications");
>+        if (desc && !desc.get)
>           PopupNotifications.locationChange();

Write 'if (!Object.getOwnPropertyDescriptor(window, "PopupNotifications").get)' here too.

>--- a/browser/components/distribution.js
>+++ b/browser/components/distribution.js

>   get _ini() {
>-    let ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].
>-              getService(Ci.nsIINIParserFactory).
>-              createINIParser(this._iniFile);
>-    this.__defineGetter__("_ini", function() ini);
>-    return this._ini;
>+    delete this._ini;
>+    return this._ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].
>+      getService(Ci.nsIINIParserFactory).
>+      createINIParser(this._iniFile);
>   },

Format like this:

    return this._ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"]
                         .getService(Ci.nsIINIParserFactory)
                         .createINIParser(this._iniFile);
 
>--- a/browser/components/places/tests/browser/head.js
>+++ b/browser/components/places/tests/browser/head.js
>@@ -5,16 +5,21 @@
> 
> // We need to cache this before test runs...
> let cachedLeftPaneFolderIdGetter;
>-let (getter = PlacesUIUtils.__lookupGetter__("leftPaneFolderId")) {
>-  if (!cachedLeftPaneFolderIdGetter && typeof(getter) == "function")
>-    cachedLeftPaneFolderIdGetter = getter;
>+let propDesc = Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId");
>+
>+if (propDesc && propDesc.get) {
>+  cachedLeftPaneFolderIdGetter = propDesc.get;
> }

As previously with 'getter', declare 'propDesc' in a private scope:

let (propDesc = Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId")) {
  if (propDesc && propDesc.get)
    cachedLeftPaneFolderIdGetter = propDesc.get;
}

>--- a/browser/components/test/browser_bug538331.js
>+++ b/browser/components/test/browser_bug538331.js
>@@ -4,6 +4,8 @@
>  * http://creativecommons.org/publicdomain/zero/1.0/
>  */
> 
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+

Get rid of this import, as XPCOMUtils is already available in this file.

>--- a/browser/fuel/src/fuelApplication.js
>+++ b/browser/fuel/src/fuelApplication.js
>@@ -14,45 +14,39 @@
> // Singleton that holds services and utilities
> var Utilities = {
>   get bookmarks() {
>-    let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>-                    getService(Ci.nsINavBookmarksService);
>-    this.__defineGetter__("bookmarks", function() bookmarks);
>-    return this.bookmarks;
>+    delete this.bookmarks;
>+    return this.bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+      getService(Ci.nsINavBookmarksService);
>   },

Format like this:

    return this.bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]
                              .getService(Ci.nsINavBookmarksService);

>   get livemarks() {
>-    let livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
>-                    getService[Ci.mozIAsyncLivemarks].
>-                    QueryInterface(Ci.nsILivemarkService);
>-    this.__defineGetter__("livemarks", function() livemarks);
>-    return this.livemarks;
>+    delete this.livemarks;
>+    return this.livemarks = Cc["@mozilla.org/browser/livemark-service;2"].
>+      getService[Ci.mozIAsyncLivemarks].
>+      QueryInterface(Ci.nsILivemarkService);
>   },

ditto

>   get annotations() {
>-    let annotations = Cc["@mozilla.org/browser/annotation-service;1"].
>-                      getService(Ci.nsIAnnotationService);
>-    this.__defineGetter__("annotations", function() annotations);
>-    return this.annotations;
>+    delete this.annotations;
>+    return this.annotations = Cc["@mozilla.org/browser/annotation-service;1"].
>+      getService(Ci.nsIAnnotationService);
>   },

ditto

>   get history() {
>-    let history = Cc["@mozilla.org/browser/nav-history-service;1"].
>-                  getService(Ci.nsINavHistoryService);
>-    this.__defineGetter__("history", function() history);
>-    return this.history;
>+    delete this.history;
>+    return this.history = Cc["@mozilla.org/browser/nav-history-service;1"].
>+      getService(Ci.nsINavHistoryService);
>   },

ditto

>   get windowMediator() {
>-    let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
>-                         getService(Ci.nsIWindowMediator);
>-    this.__defineGetter__("windowMediator", function() windowMediator);
>-    return this.windowMediator;
>+    delete this.windowMediator;
>+    return this.windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
>+      getService(Ci.nsIWindowMediator);
>   },

ditto

r=me with these changes
Attachment #703028 - Flags: review?(dao) → review+
Last set of changes (thanks Dão!) before landing part 1. Carrying over r+.

I will open a separate bug for directories other than browser so that I could land and close this one. Reasoning: with the JS profiler and other projects I won't have much time in the following couple of weeks to work on part 2. But I also don't want already r+'ed part 1 to get outdated.

Very limited try push (just a smoke test, really): https://tbpl.mozilla.org/?tree=Try&rev=0b8c33c9a34d
Attachment #703028 - Attachment is obsolete: true
Attachment #703028 - Flags: review?(gavin.sharp)
Attachment #705605 - Flags: review+
Was re-reading the patch and decided to change the remaining lazy getters to use `this` instead of `window` for consistency throughout the whole browser.js.
Attachment #705605 - Attachment is obsolete: true
Attachment #705608 - Flags: review+
Comment on attachment 705608 [details] [diff] [review]
Part 1: browser/ directory cleanup

Review of attachment 705608 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
@@ +46,1 @@
>    },

afaik, we should not do lazy getters like this in prototypes. Try this in Scratchpad:

"use strict"
function test() {
}
test.prototype = {
  get p() {
    delete this.p;
    return this.p = 1;
  }
}
let t = new test();
alert(t.p);

you will get "Exception: setting a property that has only a getter"
the original code was complicated for a reason, delete removes the instance property, but then it can't set a new property cause the prototype getter is still there.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=385809#c5

This is repeated many times in the patch, we need a better solution.
Attachment #705608 - Flags: feedback-
I wonder why apparently no test complained about this.

How about using XPCOMUtils.defineLazyGetter in the constructor?
I think everything works as expected if you're not in strict mode. Indeed my test case in scratchpad works fine if you remove strict mode.
Regardless, even if we don't require strict mode, it's still wrong, sadly.

So yes, either we keep using the analogue of __defineGetter__, or we use
if (!this.__something) {
  ...
}
return this.__something;

or we use defineLazyGetter in the constructor, as you suggested.
>    return this.p = 1;

This will throw in strict mode and silently do no assignment in non-strict mode.  Try this:

function test() {
}
function x() {
  alert("called");
  return 1;
}
test.prototype = {
  get p() {
    delete this.p;
    return this.p = x();
  }
}
let t = new test();
t.p;
t.p;

What you want instead, I believe, is:

  Object.defineProperty(this, "p", { value: 1 });
(In reply to Boris Zbarsky (:bz) from comment #29)
> >    return this.p = 1;
> 
> This will throw in strict mode and silently do no assignment in non-strict
> mode. 

yes that's what my comment was pointing out :)
we could indeed do

  get p() {
    Object.defineProperty(this, "p", { value: somevalue,
                                       enumerable: true,
                                       configurable: true,
                                       writable: true });
    return this.p;
  }

but it's still too verbose compared to a simple defineLazyGetter in the constructor
> yes that's what my comment was pointing out :)

My point was that it doesn't work as expected in non-strict mode.  ;)
Comment on attachment 705608 [details] [diff] [review]
Part 1: browser/ directory cleanup

Removing r+.
Attachment #705608 - Flags: review+
Assignee: anton → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
No assignee, updating the status.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: