XPCOMUtils should provide a convenient way to define lazy getters

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

8 years ago
It's a very common pattern to define lazy getters for services in component constructors like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesAutoComplete.js#247

sayrer suggested that we make this a method on XPCOMUtils so we share more code.

I'm thinking adding two methods looking something like this:
defineLazyGetter: function(aObject, aName, aLambda)
{
  aObject.__defineGetter__(aName, function() {
    delete aObject[aName];
    return aObject[aName] = aLambda();
  });
},
defineLazyServiceGetter: function(aObject, aName, aContract, aCID)
{
  this.defineLazyGetter(aObject, aName, function() {
    return Components.classes[aContract].getService(aCID);
  });
}

patch coming in a bit

Comment 1

8 years ago
sounds good to me
(Assignee)

Comment 2

8 years ago
Created attachment 393034 [details] [diff] [review]
v1.0
Attachment #393034 - Flags: superreview?(benjamin)
Attachment #393034 - Flags: review?(sayrer)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review sayrer][needs sr bsmedberg]
(Assignee)

Updated

8 years ago
Blocks: 508902

Updated

8 years ago
Attachment #393034 - Flags: review?(sayrer) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [needs review sayrer][needs sr bsmedberg] → [needs sr bsmedberg]

Updated

8 years ago
Attachment #393034 - Flags: superreview?(benjamin) → superreview+

Comment 3

8 years ago
Comment on attachment 393034 [details] [diff] [review]
v1.0

>diff --git a/js/src/xpconnect/loader/XPCOMUtils.jsm b/js/src/xpconnect/loader/XPCOMUtils.jsm

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

pretty sure tab-width should be 8 or some equally huge number so that we notice bad tabs in the file... the coding style says so at least!


> var XPCOMUtils = {
>   /**
>    * Generate a QueryInterface implementation. The returned function must be
>    * assigned to the 'QueryInterface' property of a JS object. When invoked on
>    * that object, it checks if the given iid is listed in the |interfaces|
>@@ -223,16 +225,57 @@ var XPCOMUtils = {
> 
>       canUnload: function(compMgr) {
>         return true;
>       }
>     };
>   },
> 
>   /**
>+   * Defines a getter on a specified object that will be created upon first use.
>+   *
>+   * @param aObject
>+   *        The object to define the lazy getter on.
>+   * @param aName
>+   *        The name of the getter to define on aObject.
>+   * @param aLambda
>+   *        A function that returns what the getter should return.  This will
>+   *        only ever be called once.
>+   */
>+  defineLazyGetter: function XPCU_defineLazyGetter(aObject, aName, aLambda)
>+  {
>+    aObject.__defineGetter__(aName, function() {
>+      delete aObject[aName];
>+      return aObject[aName] = aLambda();
>+    });
>+  },
>+
>+  /**
>+   * Defines a getter on a specified object for a service.  The service will not
>+   * be obtained until first use.
>+   *
>+   * @param aObject
>+   *        The object to define the lazy getter on.
>+   * @param aName
>+   *        The name of the getter to define on aObject for the service.
>+   * @param aContract
>+   *        The contract used to obtain the service.
>+   * @param aInterfaceName
>+   *        The name of the interface to query the service to.
>+   */
>+  defineLazyServiceGetter: function XPCU_defineLazyServiceGetter(aObject, aName,
>+                                                                 aContract,
>+                                                                 aInterfaceName)
>+  {
>+    this.defineLazyGetter(aObject, aName, function XPCU_serviceLambda() {
>+      return Cc[aContract].getService(Ci[aInterfaceName]);
>+    });
>+  },
>+
>+  /**
>    * Convenience access to category manager
>    */
>   get categoryManager() {
>     return Components.classes["@mozilla.org/categorymanager;1"]
>            .getService(Ci.nsICategoryManager);
>   },
> 
>   /**
>diff --git a/js/src/xpconnect/tests/unit/test_xpcomutils.js b/js/src/xpconnect/tests/unit/test_xpcomutils.js
>--- a/js/src/xpconnect/tests/unit/test_xpcomutils.js
>+++ b/js/src/xpconnect/tests/unit/test_xpcomutils.js
>@@ -1,23 +1,131 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * vim: sw=4 ts=4 sts=4 et
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Necko Test Code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Boris Zbarsky <bzbarsky@mit.edu> (Original Author)
>+ *   Shawn Wilsher <me@shawnwilsher.com>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+/**
>+ * This file tests the methods on XPCOMUtils.jsm.
>+ */
>+
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> 
>-var x = {
>-    QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIClassInfo,
>-					   "nsIDOMNode"])
>-};
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
> 
>-function run_test() {
>+////////////////////////////////////////////////////////////////////////////////
>+//// Tests
>+
>+function test_generateQI_string_names()
>+{
>+    var x = {
>+        QueryInterface: XPCOMUtils.generateQI([
>+            Components.interfaces.nsIClassInfo,
>+            "nsIDOMNode"
>+        ])
>+    };
>+
>     try {
>-	x.QueryInterface(Components.interfaces.nsIClassInfo);
>+        x.QueryInterface(Components.interfaces.nsIClassInfo);
>     } catch(e) {
>-	do_throw("Should QI to nsIClassInfo");
>+        do_throw("Should QI to nsIClassInfo");
>     }
>     try {
>-	x.QueryInterface(Components.interfaces.nsIDOMNode);
>+        x.QueryInterface(Components.interfaces.nsIDOMNode);
>     } catch(e) {
>-	do_throw("Should QI to nsIDOMNode");
>+        do_throw("Should QI to nsIDOMNode");
>     }
>     try {
>-	x.QueryInterface(Components.interfaces.nsIDOMDocument);
>-	do_throw("QI should not have succeeded!");
>+        x.QueryInterface(Components.interfaces.nsIDOMDocument);
>+        do_throw("QI should not have succeeded!");
>     } catch(e) {}
> }
>+
>+function test_defineLazyGetter()
>+{
>+    let accessCount = 0;
>+    let obj = { };
>+    const TEST_VALUE = "test value";
>+    XPCOMUtils.defineLazyGetter(obj, "foo", function() {
>+        accessCount++;
>+        return TEST_VALUE;
>+    });
>+    do_check_eq(accessCount, 0);
>+
>+    // Get the property, making sure the access count has increased.
>+    do_check_eq(obj.foo, TEST_VALUE);
>+    do_check_eq(accessCount, 1);
>+
>+    // Get the property once more, making sure the access count has not
>+    // increased.
>+    do_check_eq(obj.foo, TEST_VALUE);
>+    do_check_eq(accessCount, 1);
>+}
>+
>+function test_defineLazyServiceGetter()
>+{
>+    let obj = { };
>+    XPCOMUtils.defineLazyServiceGetter(obj, "service",
>+                                       "@mozilla.org/consoleservice;1",
>+                                       "nsIConsoleService");
>+    let service = Cc["@mozilla.org/consoleservice;1"].
>+                  getService(Ci.nsIConsoleService);
>+
>+    // Check that the lazy service getter and the actual service have the same
>+    // properties.
>+    for (let prop in obj.service)
>+        do_check_true(prop in service);
>+    for (let prop in service)
>+        do_check_true(prop in obj.service);
>+}
>+
>+////////////////////////////////////////////////////////////////////////////////
>+//// Test Runner
>+
>+let tests = [
>+    test_generateQI_string_names,
>+    test_defineLazyGetter,
>+    test_defineLazyServiceGetter,
>+];
>+
>+function run_test()
>+{
>+    tests.forEach(function(test) {
>+        print("Running next test: " + test.name);
>+        test();
>+    });
>+}
(Assignee)

Updated

8 years ago
Whiteboard: [needs sr bsmedberg]
(Assignee)

Comment 4

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f2ebd467b1cd
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

8 years ago
Blocks: 513710

Comment 5

8 years ago
Comment on attachment 393034 [details] [diff] [review]
v1.0

>+      return aObject[aName] = aLambda();
So, what scope does aLambda get called with?
Would aLambda.call(aObject) be more useful?
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> Would aLambda.call(aObject) be more useful?
I actually changed it to aLambda.apply in bug 513710 because of the scope issue.
(Assignee)

Updated

8 years ago
Keywords: dev-doc-needed
Comment on attachment 393034 [details] [diff] [review]
v1.0

since more code is starting using these, i'd like to see them on 1.9.2, otherwise any patch using this should be unbitrotted.
Attachment #393034 - Flags: approval1.9.2?
(Assignee)

Comment 8

8 years ago
Comment on attachment 393034 [details] [diff] [review]
v1.0

If you want this in 1.9.2 Marco, you need to take the change I did in bug 513710 for this to be useful.  This would need a new patch up on this bug (and probably a test for that behavior since it looks like I neglected to add one).
Attachment #393034 - Flags: approval1.9.2?
There's now documentation for this here; I've also documented the rest of XPCOMUtils.jsm. I'd appreciate it if someone could give it a once-over:

https://developer.mozilla.org/en/JavaScript_code_modules/XPCOMUtils.jsm

(Just a note, I'm still doing some style cleanup work on this at this time, but the content itself is as done as it's going to be for now).

Can someone point me to an example of XPCOMUtils.jsm actually in use that I can look at?
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> Can someone point me to an example of XPCOMUtils.jsm actually in use that I can
> look at?
What bits are you actually interested in seeing?  nsPlacesDBFlush.js is a good example of everything else in this file before this bug landed.  If you want examples with the new code, I can provide that as well.
(Assignee)

Comment 11

8 years ago
I modified the documentation slightly to indicate how |this| is handled when calling aLambda.
I'd like an example with the new stuff too, yes.
(Assignee)

Comment 13

8 years ago
Actually, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesDBFlush.js has everything.
Created attachment 404343 [details] [diff] [review]
scope test v1.0

adds a test for scope change, if r+ i'll attach a rollup patch for 1.9.2 including all needed changes.
Attachment #404343 - Flags: review?(sdwilsh)
(Assignee)

Comment 15

8 years ago
Comment on attachment 404343 [details] [diff] [review]
scope test v1.0

r=sdwilsh

Thanks!
Attachment #404343 - Flags: review?(sdwilsh) → review+
Created attachment 404345 [details] [diff] [review]
1.9.2 roll-up patch

includes all needed changes for 1.9.2
Attachment #404345 - Flags: approval1.9.2?
I can see where XPCOMUtils.defineLazyServiceGetter is coming from, but what's the deal with XPCOMUtils.defineLazyGetter? It doesn't seem to have anything to do with XPCOM.
scope test on trunk
http://hg.mozilla.org/mozilla-central/rev/1cd24ecc343d
Blocks: 474056

Updated

8 years ago
Attachment #404345 - Flags: approval1.9.2? → approval1.9.2+
Pushed to mozilla-1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/50f79db3c211

The documentation should be changed to reflect the API's availability on 1.9.2.
status1.9.2: --- → beta1-fixed
Keywords: dev-doc-complete → dev-doc-needed
Doc updated to reflect this being added to 1.9.2.
Keywords: dev-doc-needed → dev-doc-complete

Updated

7 years ago
Blocks: 581307
You need to log in before you can comment on or make changes to this bug.