372069 - Add FUEL 0.1 code to trunk

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: FUEL 0.1 IDL, Code and tests

FUEL 0.1 is developed in the FUEL_DEVEL_BRANCH. It is ready for review and inclusion into the trunk.

The API reference is located at the given URL and the code comes with a collection of mochitest unit tests.

Comment 1

[Dietrich Ayala (:dietrich)]

Comment on attachment 256751 [details] [diff] [review]
FUEL 0.1 IDL, Code and tests

Please replace tabs with spaces in code. JS style guidelines for /browser are available at http://developer.mozilla.org/en/docs/Javascript_Style_Guide.

>Index: browser/fuel/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/browser/fuel/Makefile.in,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 Makefile.in
>--- browser/fuel/Makefile.in 30 Jan 2007 19:50:54 -0000  1.1
>+++ browser/fuel/Makefile.in 28 Feb 2007 04:16:02 -0000
>@@ -32,16 +32,20 @@
> # 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 *****
> 
> DEPTH   = ../..
> topsrcdir = @top_srcdir@
>-srcdir  = @srcdir@
>+srcdir    = @srcdir@
> VPATH   = @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> DIRS = public src
> 
>+ifdef MOZ_MOCHITEST
>+DIRS += test
>+endif
>+
> include $(topsrcdir)/config/rules.mk
>\ No newline at end of file

Add a newline. There's a few more of these in the patch.

>Index: browser/fuel/public/nsIApplication.idl
>===================================================================
>RCS file: browser/fuel/public/nsIApplication.idl
>diff -N browser/fuel/public/nsIApplication.idl
>--- /dev/null  1 Jan 1970 00:00:00 -0000
>+++ browser/fuel/public/nsIApplication.idl 28 Feb 2007 04:16:02 -0000
>@@ -0,0 +1,378 @@
>+#include "nsISupports.idl"
>+#include "nsIVariant.idl"
>+
>+interface nsIPreference;
>+
>+/**
>+ * Interface that gives simplified access to the console
>+ */
>+[scriptable, uuid(ae8482e0-aa5a-11db-abbd-0800200c9a66)]
>+interface nsIConsole : nsISupports
>+{
>+  /**
>+   * Sends a given string to the console.
>+   * @param   msg
>+   *          The text to send to the console
>+   */
>+  void log(in string msg);
>+
>+  /**
>+   * Opens the error console window.
>+   */
>+  void open();
>+};

Use AString for string input parameters, and please prefix input parameters with "a", eg: aMsg. Please apply this where appropriate elsewhere in this patch.

More about what string types to use where is available in the XPCOM string guide:

http://developer.mozilla.org/en/docs/XPCOM:Strings

>+
>+/**
>+ * Interface hold information about an event.
>+ */

s/hold/to hold/

>+[scriptable, function, uuid(05281820-ab62-11db-abbd-0800200c9a66)]
>+interface nsIEventItem : nsISupports
>+{
>+  /**
>+   * The type of the event
>+   */
>+  readonly attribute string type;

What are the possible values for this? Can you elaborate in the docs?

>+
>+  /**
>+   * Can hold extra details and data associated with the event. This
>+   * is optional and event specific.
>+   */
>+  readonly attribute string data;
>+
>+  /**
>+   * 
>+   */
>+  void preventDefault();
>+}; 
>+

preventDefault needs docs.

>+
>+/**
>+ * Interface used as a callback for listening to events.
>+ */
>+[scriptable, function, uuid(2dfe3a50-ab2f-11db-abbd-0800200c9a66)]
>+interface nsIEventListener : nsISupports
>+{
>+  /**
>+   * This method is called whenever an event occurs of the type for which 
>+   * the EventListener interface was registered.
>+   *
>+   * @param   event
>+   *            The EventItem contains contextual information about the 
>+   *            event. It also contains the preventDefault method which
>+   *            is used to cancel default action.
>+   */
>+  void handleEvent(in nsIEventItem event);
>+}; 

nit: s/cancel default/cancel the default/

>+
>+
>+/**
>+ * Interface for supporting custom events.
>+ */
>+[scriptable, uuid(3a8ec9d0-ab19-11db-abbd-0800200c9a66)]
>+interface nsIEvents : nsISupports
>+{
>+  /**
>+   * Adds an event listener from the list. Calling remove
>+   * with arguments which do not identify any currently registered
>+   * EventListener has no effect.
>+   * @param   event
>+   *          The name of an event
>+   * @param   listener
>+   *          The reference to a listener
>+   */
>+  void addListener(in string event, in nsIEventListener listener);

In the first sentence: s/from/to/ 

In the second sentent: s/remove/add/

>+
>+  /**
>+   * Removes an event listener from the list. Calling remove
>+   * with arguments which do not identify any currently registered
>+   * EventListener has no effect.
>+   * @param   event
>+   *          The name of an event
>+   * @param   listener
>+   *          The reference to a listener
>+   */
>+  void removeListener(in string event, in nsIEventListener listener);
>+}; 
>+
>+
>+/**
>+ * Interface for simplified access to preferences. This interface is
>+ * designed to be used in a predefined context. As such, the branch should
>+ * be preset to a value appropriate to the context.
>+ */

There's something Rumsfeldian about this description. Given that the aim of FUEL
is to *simplify* things for developers, please rephrase this, explaining what you
mean by |context|, and perhaps give an example.

>+[scriptable, uuid(ce697d40-aa5a-11db-abbd-0800200c9a66)]
>+interface nsIPreferenceBranch : nsISupports
>+{
>+  /**
>+   * The name of the branch root.
>+   */
>+  readonly attribute string root;
>+  
>+  /**
>+   * All extensions in this preference area.
>+   */
>+  readonly attribute nsIVariant all;
>+  

If this variant is going to be of a consistent type, please document
what the expected type is.

>+  /**
>+   * The events object for the preferences
>+   */
>+  readonly attribute nsIEvents events;
>+  
>+  /**
>+   * Check to see if a preference exists.
>+   * @param   name
>+   *          The name of preference
>+   * @returns true if the preference exists, false if not
>+   */
>+  nsIVariant has(in string name);

Is there a reason you're not returning a boolean here?

>+  
>+  /**
>+   * Gets an object representing a preference
>+   * @param   name
>+   *          The name of preference
>+   * @returns a preference object, or null if the preference does not exist
>+   */
>+  nsIPreference get(in string name);
>+  
>+  /**
>+   * Gets the value of a preference. Returns a default value if
>+   * the preference does not exist.
>+   * @param   name
>+   *          The name of preference
>+   * @param   defaultValue
>+   *          The value to return if preference does not exist
>+   * @returns value of the preference or the given default value if preference
>+   *          does not exists.
>+   */
>+  nsIVariant getValue(in string name, in nsIVariant defaultValue);
>+
>+  /**
>+   * Sets the value of a storage item with the given name.
>+   * @param   name
>+   *          The name of an item
>+   * @param   value
>+   *          The value to assign to the item
>+   */
>+  nsIVariant setValue(in string name, in nsIVariant value);

What is the variant returned from this?

>+
>+  /**
>+   * Resets all preferences in a branch back to their default values.
>+   */
>+  void reset();
>+};
>+
>+/**
>+ * Interface for accessing a single preference.
>+ */

It would be worth mentioning whether this is live or can get stale.

That's as far as I can get tonight. I'll try to get to the rest tomorrow :)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: FUEL 0.1 IDL, Code and tests - updated

(In reply to comment #1)
> (From update of attachment 256751 [details] [diff] [review])
> Please replace tabs with spaces in code. JS style guidelines for /browser are
> available at http://developer.mozilla.org/en/docs/Javascript_Style_Guide.
> 

replaced tabs with spaces in code

> > include $(topsrcdir)/config/rules.mk
> >\ No newline at end of file
> 
> Add a newline. There's a few more of these in the patch.

added newlines to end of makefiles and html tests

> >+  /**
> >+   * Sends a given string to the console.
> >+   * @param   msg
> >+   *          The text to send to the console
> >+   */
> >+  void log(in string msg);
> >+
> 
> Use AString for string input parameters, and please prefix input parameters
> with "a", eg: aMsg. Please apply this where appropriate elsewhere in this
> patch.
> 

Switched to AString for string input parameters
added the "a" prefix to parameters

> 
> >+
> >+/**
> >+ * Interface hold information about an event.
> >+ */
> 
> s/hold/to hold/
> 

done

> >+[scriptable, function, uuid(05281820-ab62-11db-abbd-0800200c9a66)]
> >+interface nsIEventItem : nsISupports
> >+{
> >+  /**
> >+   * The type of the event
> >+   */
> >+  readonly attribute string type;
> 
> What are the possible values for this? Can you elaborate in the docs?
> 

It's not really an enumerable type - it is the name of the event. Modeled on the DOM event. Changed comment to specify that it is the name of the event 

> >+
> >+  /**
> >+   * 
> >+   */
> >+  void preventDefault();
> >+}; 
> >+
> 
> preventDefault needs docs.
> 

done

> >+  /**
> >+   * This method is called whenever an event occurs of the type for which 
> >+   * the EventListener interface was registered.
> >+   *
> >+   * @param   event
> >+   *            The EventItem contains contextual information about the 
> >+   *            event. It also contains the preventDefault method which
> >+   *            is used to cancel default action.
> >+   */
> >+  void handleEvent(in nsIEventItem event);
> >+}; 
> 
> nit: s/cancel default/cancel the default/
> 

done

> >+  /**
> >+   * Adds an event listener from the list. Calling remove
> >+   * with arguments which do not identify any currently registered
> >+   * EventListener has no effect.
> >+   * @param   event
> >+   *          The name of an event
> >+   * @param   listener
> >+   *          The reference to a listener
> >+   */
> >+  void addListener(in string event, in nsIEventListener listener);
> 
> In the first sentence: s/from/to/ 
> In the second sentent: s/remove/add/

done

> >+/**
> >+ * Interface for simplified access to preferences. This interface is
> >+ * designed to be used in a predefined context. As such, the branch should
> >+ * be preset to a value appropriate to the context.
> >+ */
> 
> There's something Rumsfeldian about this description. Given that the aim of
> FUEL
> is to *simplify* things for developers, please rephrase this, explaining what
> you
> mean by |context|, and perhaps give an example.

rewrote the comment to make it clear that the root of the branch can be auto-set by the object that owns this PreferenceBranch.

> 
> >+[scriptable, uuid(ce697d40-aa5a-11db-abbd-0800200c9a66)]
> >+interface nsIPreferenceBranch : nsISupports
> >+{
> >+  /**
> >+   * The name of the branch root.
> >+   */
> >+  readonly attribute string root;
> >+  
> >+  /**
> >+   * All extensions in this preference area.
> >+   */
> >+  readonly attribute nsIVariant all;
> >+  
> 
> If this variant is going to be of a consistent type, please document
> what the expected type is.

documented the type to be an array of nsIPreference

> >+  /**
> >+   * Check to see if a preference exists.
> >+   * @param   name
> >+   *          The name of preference
> >+   * @returns true if the preference exists, false if not
> >+   */
> >+  nsIVariant has(in string name);
> 
> Is there a reason you're not returning a boolean here?

nope, should be boolean

> >+  /**
> >+   * Sets the value of a storage item with the given name.
> >+   * @param   name
> >+   *          The name of an item
> >+   * @param   value
> >+   *          The value to assign to the item
> >+   */
> >+  nsIVariant setValue(in string name, in nsIVariant value);
> 
> What is the variant returned from this?

nothing, should be void

> >+
> >+/**
> >+ * Interface for accessing a single preference.
> >+ */
> 
> It would be worth mentioning whether this is live or can get stale.

done

----

this patch also addresses:
* an observer leak (nsIPreferenceBranch and nsIExtension)
* adds names to anonymous functions
* fixes some bugs found while reviewing

Comment 3

[Nickolay_Ponomarev]

(In reply to comment #2)
> * an observer leak (nsIPreferenceBranch and nsIExtension)

Do you realize you didn't fix the leak for any practical purpose? Checking at shutdown is just one of the ways we measure leaks, this doesn't mean you can keep unneeded stuff until shutdown and release it only then. Worse, you now cache all items ever created, even if they don't use observers.

And before you switch to using weak references, see bug 356599.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> (In reply to comment #2)
> > * an observer leak (nsIPreferenceBranch and nsIExtension)
> 
> Do you realize you didn't fix the leak for any practical purpose? Checking at
> shutdown is just one of the ways we measure leaks, this doesn't mean you can
> keep unneeded stuff until shutdown and release it only then. Worse, you now
> cache all items ever created, even if they don't use observers.

Yes, we know it. The goal would be to | addObservers | when the first listener is added and | removeObserver | after the last listener is removed for any given object. But that was causing Firefox to crash. This is a temporary fix.

Part of the goal of 0.1 is to get it in the trunk so people can work with it and provide feedback. If the current state is not good enough for trunk, we'll wait.

Comment 5

[Shawn Wilsher :sdwilsh]

As per conversations in #extdev, what about using the prefix 'fuel' instead of 'ns' for the FUEL interfaces?  There is no technical reason to use 'ns', just inertia, but it'd make a bit more sense to differentiate these interfaces with a different prefix.

+  // XXX: Disabled until we can figure out the wrapped object issues
is this a note for yourselves, or a known bug?  If it's the latter, it'd be best to add a bug number.

+  var os = Components.classes["@mozilla.org/observer-service;1"]
In some places you use '@mozilla.org/...' and in others you use "@mozilla.org/...".  I'm wondering why it isn't consistent throughout the file.

+  QueryInterface: function app_qi(aIID) {
+    // add any other interfaces you support here
+    if (!aIID.equals(nsIApplication) &&
+        !aIID.equals(nsIObserver) &&
+        !aIID.equals(nsIClassInfo) &&
+        !aIID.equals(nsISupports))
+    {
+        throw Components.results.NS_ERROR_NO_INTERFACE;
+    }
+    return this;
+  },
Ok, so this is probably personal preference, but this seems a bit confusing with all the not's and and's.  What about:

  QueryInterface: function app_qi(aIID) {
    // add any other interfaces you support here
    if (aIID.equals(nsIApplication) ||
        aIID.equals(nsIObserver) ||
        aIID.equals(nsIClassInfo) ||
        aIID.equals(nsISupports)) {
      return this;
    }
    throw Components.results.NS_ERROR_NO_INTERFACE;
  },
Also, a style nit there - looks like everywhere else in your code you have the opening brace after the closing paren of the if statement.

I know I'm being picky - sorry!

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 257043 [details] [diff] [review]
FUEL 0.1 IDL, Code and tests - updated

I don't have time for this in the immediate future, so clearing review request for now. Recommended mfinkle get review from a browser peer, since that'll be required for landing.

Comment 7

[Nickolay_Ponomarev]

Why is it in mozilla/browser anyway? Seems like toolkit-ish material (i.e. not specific to browser).

The FUEL homepage at http://wiki.mozilla.org/FUEL only talks about ext developers (and its design is affected by the fact it tries to be familiar to webdev types). On the other hand I heard FUEL will be used in Firefox to simplify common tasks. What is the truth?

As I mentioned on IRC, you don't really need the getters, since xpconnect ensures the properties are read-only if they're marked as such. Lots of getters make it much harder to navigate through the code, and the printout is a dozen of pages, largely due to the getters. Save the trees!

Also repeating an IRC comment, my firm belief is that our current preferences API does not support auto-detection of pref types, because it can't auto-detect if a persisted property is a string or a complex pref (say, file). Not sure what the solution to this problem will be, but you can't use a pref wrapper that ignores the existence of nsIFile values.

Another concern I have is that the library does a lot of things even if it's not actually used, but I suppose it can be optimized later (e.g. creating the new objects on the first call into a getter).

As for
> The goal would be to | addObservers | when the first listener
> is added and | removeObserver | after the last listener is removed for any
> given object.
So you make the call to removeListener required? That would be quite a gotcha for web developers, esp. when the API strives to look like the DOM api.

I had some nits about the actual code (ignoring the API issues), but those got lost somewhere.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 257043 [details] [diff] [review]
FUEL 0.1 IDL, Code and tests - updated

Apologies for the delay, subsequent patches should be quicker to review now that I'm more familiar with the API.

Some general comments:
I'm a little bit worried about having interfaces with names so similar to existing related interfaces (e.g. nsIPrefBranch & nsIPreferenceBranch, nsIConsole & nsIConsoleService, etc). Seems like it might lead to confusion. What do you think about giving all of the FUEL interfaces a common prefix (fuelI instead of nsI? nsIFUEL?), or some other identifier that groups and differentiates them?

I think it would also be a good idea to refer to documentation for other interfaces when all the new interface does is duplicate functionality from the other (often using the same implementation, even). The documentation for nsIConsole::log might mention nsIConsoleService::logStringMessage, for example, and the documentation for the nsIApplication methods that are just copied from nsIXULAppInfo should definitely have an @see pointing to the relevant nsIXULAppInfo methods. Some of the event interfaces could point to the DOM Event interfaces, though I suppose the two models differ enough that that could just be confusing.

The code is a bit inconsistent about the bracing/parenthesis style. The generally agreed-upon convention for new code in /browser is to omit brackets for single line clauses, and to only put spaces after the comma in argument lists.

And now for some more specific comments:

>Index: browser/fuel/public/nsIApplication.idl

>+interface nsIConsole : nsISupports

>+  /**
>+   * Opens the error console window.
>+   */
>+  void open();

Should probably specify what happens when the console is already open (it gets focused).

>+interface nsIEventItem : nsISupports

>+   * Can hold extra details and data associated with the event. This
>+   * is optional and event specific.

What does "is optional" mean? Does it throw if not set, or just return an empty string? Looks like it currently will return null, but that should probably be specified.

>+interface nsIEventListener : nsISupports

>+   * @param   aEvent
>+   *          The EventItem contains contextual information about the 
>+   *          event. It also contains the preventDefault method which
>+   *          is used to cancel the default action.

I think it suffices to say that aEvent is the EventItem for the associated event (the rest is already defined by the EventItem interface).

>+interface nsIEvents : nsISupports

>+   * Adds an event listener to the list. Attempts to add duplicate
>+   * instances are ignored.

"duplicate instances" isn't very clear, it could be interpreted as "can't add two different listeners for a given event". The comment at http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/public/idl/events/nsIDOMEventTarget.idl&rev=1.9&mark=61-65#55 seems to specify this pretty clearly, perhaps you should copy it or add a reference.

>+interface nsIPreferenceBranch : nsISupports

Might be worth documenting that the various "name" parameters on this interface are all relative to the root of the branch.

>+   * Array of nsIPreference listing all preferences in this branch.
>+   */
>+  readonly attribute nsIVariant all;

Wow, I didn't know it was possible to use nsIVariant to have an IDL attribute that returns an actual JS array.

>+interface nsIPreference : nsISupports

>+  /**
>+   * The preferences object for the extension. Defaults to the
>+   * "extensions.@id-of-extension." branch.
>+   */

Is that "@" a typo? Are there plans to allow extensions to specify their own branch instead of the default? If not, this should just say "maps to" or something like that instead of "Defaults to".

>+interface nsIApplication : nsISupports

>+  /**
>+   * The events object for the application.
>+   */
>+  readonly attribute nsIEvents events;

It would be a good idea to document what "events" will be dispatched for the Application object. This also applies to the other objects with an "events" attribute. 

>Index: browser/fuel/src/nsApplication.js

>+ * The Initial Developer of the Original Code is Mozilla.
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.
 
I think the original developer should be "Mozilla Corporation" and not just "Mozilla". This applies to some of the other license headers too.

>+PreferenceBranch.prototype = {

>+  has : function prefs_has(aName) {
>+    return !!this._prefs.getPrefType( aName );
>+  },

This should perhaps be compared to Components.interfaces.nsIPrefBranch.PREF_INVALID instead of relying on the fact that it's 0?

>+  get : function prefs_get(aName) {
>+    return this.has( aName ) ? new Preference( aName, this ) : null;
>+  },

The rest of this file uses |has() && new Foo() || null;|, it wouldn't hurt to be consistent. I prefer this style because it avoids the need to remember that "&&" binds tighter than "||", but it doesn't matter that much.

>+// Extension constructor
>+function Extension(aItem) {

>+  var installPref = "install-event-fired";
>+  if ( !this._prefs.has(installPref) ) {
>+    this._prefs.setValue(installPref, true);
>+    this._firstRun = true;
>+  }

Since you assume that a non-existent pref means "first run", you're relying on the fact that this constructor is called for all extensions at every startup, right? Is that garanteed to be true? It doesn't seem to be - if no one calls Extensions.all() or Extensions.get() during an extension's actual first run, this constructor won't be called, and the "firstRun" pref won't be set. Or am I missing something here?

>+Extensions.prototype = {

>+  find : function exts_find(aOptions) {
>+    var retVal = [],
>+      items = this._extmgr.getItemList( 2, {} );

s/2/Components.interfaces.nsIUpdateItem.TYPE_EXTENSION/ ? This initialization style is a bit confusing, at first glance it looks like you forgot a "var".

>+  has : function exts_has(aId) {
>+    return !!(this._extmgr && this._extmgr.getItemForID(aId).type);

Getting .type will throw if getItemForID() returns null. Shouldn't you remove the .type and just check for a non-null return from getItemForID()? I'm also not sure that it makes sense to return false in the case of a null _extmgr. That's really an exceptional case, and should probably throw an exception accordingly.

>+  get : function exts_get(aId) {
>+    return this.has(aId) && new Extension(this._extmgr.getItemForID(aId)) || null;

Calling has() means an extra unnecessary call to getItemForID. Since has()'s null check isn't needed if you make it throw per the prevous comment, I think it would be more straightforward to just |return new Extension(this._extmgr.getItemForID(aID));|.

>+Application.prototype = {

>+  // for nsIObserver
>+  observe: function app_observe(aSubject, aTopic, aData) {
>+    if (aTopic == "app-startup") {

>+    else if (aTopic == "xpcom-shutdown") {

Need to be careful here, it's possible for xpcom-shutdown to fire without app-startup being called (and timeless will likely file a bug on you if it causes problems, e.g. bug 375805). This shouldn't be a problem at the moment because the only things you rely on if aTopic == "xpcom-shutdown" are initialized in the constructor (the observers and _events) or in the global scope (gShutdown), but observers of the your Application "unload" event could potentially be confused. Very much an edge case, though, so probably not worth worrying about.

I think this is otherwise fine, and the tests are great. I'd be fine with you landing this as-is, and addressing my comments (as well as Nickolay's and Shawn's) as followup patches, if you think that's easier, so I'll grant r+, but I'd like to take a look at the revised and/or followup patches too.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: FUEL 0.1 IDL, Code and tests - update 2

patch addresses comment #5 and comment #8.  Details to follow.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> Some general comments:
> I'm a little bit worried about having interfaces with names so similar to
> existing related interfaces (e.g. nsIPrefBranch & nsIPreferenceBranch,
> nsIConsole & nsIConsoleService, etc). Seems like it might lead to confusion.
> What do you think about giving all of the FUEL interfaces a common prefix
> (fuelI instead of nsI? nsIFUEL?), or some other identifier that groups and
> differentiates them?

changed prefix to "fuelI" for interfaces and IDL file. Using "fuel" for source file prefix.
> 
> 
> The code is a bit inconsistent about the bracing/parenthesis style. The
> generally agreed-upon convention for new code in /browser is to omit brackets
> for single line clauses, and to only put spaces after the comma in argument
> lists.

Addressed the bracing and spacing issues

> 
> >+interface nsIConsole : nsISupports
> 
> >+  /**
> >+   * Opens the error console window.
> >+   */
> >+  void open();
> 
> Should probably specify what happens when the console is already open (it gets
> focused).

done

> 
> >+interface nsIEventItem : nsISupports
> 
> >+   * Can hold extra details and data associated with the event. This
> >+   * is optional and event specific.
> 
> What does "is optional" mean? Does it throw if not set, or just return an empty
> string? Looks like it currently will return null, but that should probably be
> specified.
> 

done. added additional comment.

> >+interface nsIEventListener : nsISupports
> 
> >+   * @param   aEvent
> >+   *          The EventItem contains contextual information about the 
> >+   *          event. It also contains the preventDefault method which
> >+   *          is used to cancel the default action.
> 
> I think it suffices to say that aEvent is the EventItem for the associated
> event (the rest is already defined by the EventItem interface).
> 

done

> >+interface nsIEvents : nsISupports
> 
> >+   * Adds an event listener to the list. Attempts to add duplicate
> >+   * instances are ignored.
> 
> "duplicate instances" isn't very clear, it could be interpreted as "can't add
> two different listeners for a given event". The comment at
> 

added additional comments to clarify

> 
> >+interface nsIPreferenceBranch : nsISupports
> 
> Might be worth documenting that the various "name" parameters on this interface
> are all relative to the root of the branch.

done in additional comment

> 
> >+interface nsIPreference : nsISupports
> 
> >+  /**
> >+   * The preferences object for the extension. Defaults to the
> >+   * "extensions.@id-of-extension." branch.
> >+   */
> 
> Is that "@" a typo? Are there plans to allow extensions to specify their own
> branch instead of the default? If not, this should just say "maps to" or
> something like that instead of "Defaults to".

switched to "extensions.<extensionid>." as we did in another place.

> 
> >+interface nsIApplication : nsISupports
> 
> >+  /**
> >+   * The events object for the application.
> >+   */
> >+  readonly attribute nsIEvents events;
> 
> It would be a good idea to document what "events" will be dispatched for the
> Application object. This also applies to the other objects with an "events"
> attribute. 

added a "supports:" section to each events attribute

> 
> >+ * The Initial Developer of the Original Code is Mozilla.
> >+ * Portions created by the Initial Developer are Copyright (C) 2006
> >+ * the Initial Developer. All Rights Reserved.
> 
> I think the original developer should be "Mozilla Corporation" and not just
> "Mozilla". This applies to some of the other license headers too.

changed

> 
> >+PreferenceBranch.prototype = {
> 
> >+  has : function prefs_has(aName) {
> >+    return !!this._prefs.getPrefType( aName );
> >+  },
> 
> This should perhaps be compared to
> Components.interfaces.nsIPrefBranch.PREF_INVALID instead of relying on the fact
> that it's 0?

changed

> 
> >+  get : function prefs_get(aName) {
> >+    return this.has( aName ) ? new Preference( aName, this ) : null;
> >+  },
> 
> The rest of this file uses |has() && new Foo() || null;|, it wouldn't hurt to
> be consistent. I prefer this style because it avoids the need to remember that
> "&&" binds tighter than "||", but it doesn't matter that much.

changed other places to look like this style, since its easier to comprehend

> 
> >+// Extension constructor
> >+function Extension(aItem) {
> 
> >+  var installPref = "install-event-fired";
> >+  if ( !this._prefs.has(installPref) ) {
> >+    this._prefs.setValue(installPref, true);
> >+    this._firstRun = true;
> >+  }
> 
> Since you assume that a non-existent pref means "first run", you're relying on
> the fact that this constructor is called for all extensions at every startup,
> right? Is that garanteed to be true? It doesn't seem to be - if no one calls
> Extensions.all() or Extensions.get() during an extension's actual first run,
> this constructor won't be called, and the "firstRun" pref won't be set. Or am I
> missing something here?

You are correct. We need to add some mechanism to make this work better. Holding off until the next patch.

> 
> >+Extensions.prototype = {
> 
> >+  find : function exts_find(aOptions) {
> >+    var retVal = [],
> >+      items = this._extmgr.getItemList( 2, {} );
> 
> s/2/Components.interfaces.nsIUpdateItem.TYPE_EXTENSION/ ? This initialization
> style is a bit confusing, at first glance it looks like you forgot a "var".

changed to nsIUpdateItem.TYPE_EXTENSION and changed to separate "var" statements

> 
> >+  has : function exts_has(aId) {
> >+    return !!(this._extmgr && this._extmgr.getItemForID(aId).type);
> 
> Getting .type will throw if getItemForID() returns null. Shouldn't you remove
> the .type and just check for a non-null return from getItemForID()? I'm also
> not sure that it makes sense to return false in the case of a null _extmgr.
> That's really an exceptional case, and should probably throw an exception
> accordingly.

getItemForID does not seem to return null. That's why we check for an invlaid type instead.

> 
> >+  get : function exts_get(aId) {
> >+    return this.has(aId) && new Extension(this._extmgr.getItemForID(aId)) || null;
> 
> Calling has() means an extra unnecessary call to getItemForID. Since has()'s
> null check isn't needed if you make it throw per the prevous comment, I think
> it would be more straightforward to just |return new
> Extension(this._extmgr.getItemForID(aID));|.

I left the has() check in for now since some developers may not call has() before get() and I don't want it to return an invalid Extension

> 
> >+Application.prototype = {
> 
> >+  // for nsIObserver
> >+  observe: function app_observe(aSubject, aTopic, aData) {
> >+    if (aTopic == "app-startup") {
> 
> >+    else if (aTopic == "xpcom-shutdown") {
> 
> Need to be careful here, it's possible for xpcom-shutdown to fire without
> app-startup being called (and timeless will likely file a bug on you if it
> causes problems, e.g. bug 375805). This shouldn't be a problem at the moment
> because the only things you rely on if aTopic == "xpcom-shutdown" are
> initialized in the constructor (the observers and _events) or in the global
> scope (gShutdown), but observers of the your Application "unload" event could
> potentially be confused. Very much an edge case, though, so probably not worth
> worrying about.
> 

noted, but I'll hold any changes for now.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> getItemForID does not seem to return null. That's why we check for an invlaid
> type instead.

Hmm, that's strange. Looking at the implementation, it looks like it should, for an invalid ID...

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> (In reply to comment #10)
> > getItemForID does not seem to return null. That's why we check for an invlaid
> > type instead.
> 
> Hmm, that's strange. Looking at the implementation, it looks like it should,
> for an invalid ID...
> 

Exactly. However, during the mochitest creation, we found that something was returned and the tests proceeded to fail. I tried changing the code for the last patch, but again the tests started failing.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The checkin for this bug increased the leak statistics on tinderbox:
http://tinderbox.mozilla.org/Firefox/

Comment 14

[pd]

Congratulations on getting the ball rolling Mark and reviewers.

Comment 15

[Nils Maier [:nmaier]]

Minor bug I noticed:
fuelIPreference::modified should be readonly.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> fuelIPreference::modified should be readonly.

I've filed bug 379141 for that, and a few other issues with the patch (listed in the "depends on" field. This bug is FIXED.

Comment 17

[John Resig]

You can now track the development of FUEL 0.2 in bug 380168. It will be focusing on BrowserTabs, Bookmarks, File I/O, and SQLite.

Comment 18

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

when I run mochitest, test_Extensions fails in two cases
not ok - Check a find for all extensions got 2, expected 1
not ok - Check an extension preference root got "extensions.{cf2812dc-6a7c-4402-b639-4d277dac4c36}.", expected "[e-mail]."

Bug in the tests?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> "extensions.{cf2812dc-6a7c-4402-b639-4d277dac4c36}.", expected
> "[e-mail]."

That's the Xforms GUID. The test doesn't account for multiple installed extensions, since there is only one by default. 

Comment 20

[:Gavin Sharp [email: [e-mail]]]

You should file a bug, I guess. I don't really see a solution to that problem, other than removing the test's dependence on DOMi being the first (and only) extension.

Comment 21

[John Resig]

(In reply to comment #18)
> when I run mochitest, test_Extensions fails in two cases
> not ok - Check a find for all extensions got 2, expected 1
> not ok - Check an extension preference root got
> "extensions.{cf2812dc-6a7c-4402-b639-4d277dac4c36}.", expected
> "[e-mail]."

The test suite is, currently, only designed to run with a default build, with one extension (namely DOM Inspector) installed. That way we can completely know what extensions are expected and test for them. I don't really see any other way around it, considering that in order to successfully test it, you must know the expected outcome. I guess the only other solution would be to compile a list via traditional XPCOM and compare against that, but it's not clear if that's worth the trouble.

Comment 22

[Eric Shepherd [:sheppy]]

We already have some documentation on this; is it adequate?  Mark, will there be more examples posted on MDC?

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #22)
> We already have some documentation on this; is it adequate?  Mark, will there
> be more examples posted on MDC?
> 

Sheppy - I will add some examples to the interface docs

Comment 24

[Eric Shepherd [:sheppy]]

This documentation still needs some work and examples.  Just a reminder. :)