380168 - FUEL 0.2

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[John Resig]

This is a ticket to track the progress of FUEL 0.2. More information about this release can be found here:
http://wiki.mozilla.org/FUEL/0.2

Currently, the following features are guaranteed to be in 0.2:
- Browser/BrowserTab (by John Resig)
- Bookmarks (by Mark Finkle)

The following are "if we have time before 3.0a5":
- File I/O (by Neal Deakin)
- Database (light SQLite wrapper)

Comment 1

[alexander :surkov (:asurkov)]

It looks fuel library is designed for firefox extension development only and it looks the part of the library would be helpful for mozilla based application development. Should fuel be a part of toolkit rather than firefox?

Comment 2

[John Resig]

(In reply to comment #1)
> It looks fuel library is designed for firefox extension development only and it
> looks the part of the library would be helpful for mozilla based application
> development. Should fuel be a part of toolkit rather than firefox?

Currently, our primary objective is to improve Firefox extension development. Incidentally, a number of the things that we're improving our universally useful (preferences, file i/o). One goal for this quarter is to have some part of Firefox using Fuel; I suspect that at about that time we'll have support for our features within the full toolkit.

At least for this 0.2 release, we're focusing on Firefox Extension development, so the current categorization should stand.

Comment 3

[Ben Basson]

(In reply to comment #1)
> It looks fuel library is designed for firefox extension development only and it
> looks the part of the library would be helpful for mozilla based application
> development. Should fuel be a part of toolkit rather than firefox?

It depends on what FUEL is trying to be. Personally, I see one API and two logical sets of code that are being mixed together.

FUEL 0.1 seems wholly generic and could easily slot into the toolkit. Personally, that's where I'd prefer to see those particular APIs, because things like application properties, extensions and preferences are generic and can be applied to any toolkit application. The only real exception from FUEL 0.1 in my mind is session storage, which should probably remain in browser.

FUEL 0.2 on the other hand is mostly browser-centric, providing APIs for toolbars, bookmarks and tabbed browser. The odd ones out in FUEL 0.2 are Database and DatabaseQuery, which are generic and should probably be in toolkit. The same goes for file I/O.

I think the way forward is to make FUEL whatever the developers want it to be in terms of a consistent and uniform API on which to build Firefox code and extension code, but have the generic functionality moved to the toolkit and called by a set of simple FUEL wrappers. 

That way, the FUEL APIs are preserved for Firefox and presented within the immediate browser source code, but the useful functionality is not lost to other potential callers since the generic processing would be toolkit based.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #3)
> I think the way forward is to make FUEL whatever the developers want it to be
> in terms of a consistent and uniform API on which to build Firefox code and
> extension code, but have the generic functionality moved to the toolkit and
> called by a set of simple FUEL wrappers. 

Are you offering to do that factoring and movement?  So far, no advocates for other applications or toolkits have been willing to undertake that, presumably because it's not as important to them as are other things, and we already have a lot of work to do with FUEL as it is.  Authors can always package their own copy of FUEL in their extension or XULRunner application if they want it to work with other application environments; I've done it, it's a matter of dropping in 2 files.

(We originally looked at putting it in toolkit, but the impurity of having some parts of the FUEL API just not work when not in a Firefox context was deemed unacceptable by the toolkit owners, which is certainly their right.)

If someone wants it moved to toolkit and refactored, they should file another bug, at enhancement severity, and provide a patch within it.  It's not something that will be done by John or Mark in the FUEL 0.2 timeframe, so it's out of scope for this bug.  Thanks!

Comment 5

[Ben Basson]

The timeframe for that request is pretty unimportant to me, just airing my views in reply to comment 1. I'll file a follow-up bug next time I start work on a XULRunner app that could benefit from FUEL. As you say, it's simple to duplicate the files for now.

Comment 6

[/\/\arcio Galli]

Folks I wonder If FUEL .2 could serve as a means to facilitate communication ( and/or management ) between extensions. I am working in a First Run function Extension ( for Mozilla / Doug Turner too ) and part of my goals is to find how FirstRun could fit, use, or help FUEL project.

Comment 7

[Neil Deakin]

Filed bug 380813 on the scriptable IO part.

Comment 8

[Nickolay_Ponomarev]

One comment on the overall design:

Many FUEL objects have cousins in existing object model - for example fuelIPreferenceBranch is essentially the same thing as nsIPrefBranch. FUEL 0.2 adds more such objects - Browser is essentially the same thing as the <tabbrowser> element, Tab is similar to a <browser>/<tab> pair in a <tabbrowser>, etc.

I can see how creating these separate interfaces leads to a simple and easy-to-use API for the common actions, but I think creating a separate object hierarchy is wrong in the long run. The reason is that FUEL can't abstract away all the Firefox's object model, so extension authors (and certainly Firefox code using FUEL) will have to work with the 'old' objects too.

For example, when handling a DOM click event, the event's target can be a <tab> element, which is different from the FUEL 'Tab' object. Or you might need to get the <tab> object given the FUEL Tab object to do something advanced to it (like update the .className). The API page doesn't list the ways to go from the Tab object to the <tab>/<browser> element (and vice versa), but I've seen jresig mention there will be a way to get a DOM element from a FUEL object.

So an extension author needs to remember that both the FUEL objects and their 'older' counterparts exist, and he also needs to remember which methods are available on what object (to convert between the objects appropriately). [BTW, similar properties are named differently on the FUEL-flavored and the old-style objects (e.g. Browser.activeTab vs. <tabbrowser>.selectedTab, Tab.document vs. <browser>.contentDocument) - that makes work harder when using both, I can tell you from my experience with XPCOM strings :) ]

I think that we should instead expose and improve existing objects whenever possible. Start with listing the common actions, check where our object model is too hard to use (due to having complex API or being undocumented), fix those. It would have larger impact, since it affects the new and experienced developers (including Firefox devs) alike.

For example (from http://developer.mozilla.org/en/docs/Code_snippets:Tabbed_browser#Notification_when_a_tab_is_added_or_removed_.28.3C.3D_Firefox_1.5.29)
* Common problem: get notified when tabs are added or removed
* API issue: no API, you have to dig inside the anonymous content and use mutation listeners to get the appropriate notifications.
* Solution: introduce and document dedicated events for these notifications.
While the documentation could be improved further, I think it's a good example of incremental API improvements, that benefits everyone.

Another benefit of this approach is that you don't have to write wrappers for APIs that are already good (e.g. <tabbrowser>.addTab is no harder to use than Browser.open).

Have you considered this and, if so, what is the reason you decided to write the wrappers from scratch? Is the goal to make the most common actions very easy for newcomers (and how much is helping advanced developers valued?). Are there maybe other ways to help the new developers (a simplified reference, with examples like on the MDC Code snippets maybe)?

Comment 9

[Nickolay_Ponomarev]

(comment 8 contd.: AFAIK the places API was designed from scratch. FUEL 0.2 has wrappers for that API. I admit I'm not familiar with the places API, but could you explain why you needed to create another API?)

Some minor points:
1. I think an "Array<Window> windows" and maybe "mainWindows" (for browser.xul windows) would be a useful attribute on Application. Each object in the array is a simple DOM window. Then the Firefox-specific 'browsers' attribute could be avoided (in favor of app.mainWindows[i].gBrowser).
2. >> |Browser.events| is an |Events| <<
See my comment about EventTarget in bug 380363. I haven't received a clear explanation why it's worth separating the 'events' object from the object whose events are being handled.
3. How are things like 'Array<Browser>' exposed via the IDL? (Speaking of which, are FUEL APIs exposed through IDL because you want them to (e.g. to make them usable from Python) or because it's not possible to expose a pure JS object to all chrome scripts without hacking DOM/XPConnect code?).
4. There are multiple subtle differences between the old object model and the proposed FUEL API, which I'm not listing for now (hoping that you reconsider the decision to have the wrappers completely isolated from the old OM).

Comment 10

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

Attachment: FUEL 0.2 - Bookmarks Only

This patch adds a places/bookmarks wrapper API to FUEL. See http://wiki.mozilla.org/FUEL/0.2/API

Comment 11

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

Attachment: FUEL 0.2 - Bookmarks Only and leak fixes

Same as previous patch, but with some leak cleanup

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(Not blocking as it's a P2 on the PRD, but if there are parts of this that are required to fix problems with the already-landed FUEL 0.1, please nominate those as seperate individual blocking bugs)

Comment 13

[Brett Zamir]

Hello, two small things:
1) Could this bug be opened up to voting? I for one would like to show my strong support for it.
2) As far as Nickolay Ponomarev's questions about why the team was not writing the wrappers from scratch, while I am not involved in this at all, I'd like to say that I think while no doubt improvements should be made to existing objects where possible (and where people have the skills to do this), I for one feel that being confined to ANY XPCOM calls is both ugly and cumbersome. While being open to new developers is of huge--and in my opinion often underestimated--importance, I think this can have benefit to all developers.

I also hope that these wrappers can end up on the code snippets and integrated with other pages, so that FUEL doesn't content itself with being some obscure add-on that only knowledgeable developers will know how to add themselves.

thanks,
Brett

Comment 14

[Nickolay_Ponomarev]

> I for one feel that being confined to ANY XPCOM calls is 
> both ugly and cumbersome.
I'm not sure I understand your point. You know that any call to FUEL _is_ an XPCOM call, right?

My point is that the core APIs don't always have to be that hard to use, and we should be trying to improve those where possible. (And create the wrappers or core API additions to make the really common stuff trivial).

The plan for FUEL is to ship with Firefox, as I understand it, so don't worry about it being yet another jslib.

Comment 15

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

Attachment: FUEL 0.2 - Bookmarks, Browser and Chrome Tests

This patch contains the latest code for FUEL 0.2

There are 3 "sections" in the patch:
1. code for Browser and BrowserTab related wrappers & chrome tests
2. code for Bookmarks, BookmarkFolder and Annotation wrappers & chrome tests
3. code for the old unit tests that were converted to chrome (XUL)

Comment 16

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

Comment on attachment 268131 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests

gavin - could you review the Browser, BrowserTab and related chrome tests?

Comment 17

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

Comment on attachment 268131 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests

dietrich - could you review the Bookmark, BookmarkFolder, Annotation and related chrome tests?

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 268131 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests


> /**
>+ * Interface representing a collection of annotations associated
>+ * with a bookmark or bookmark folder.
>+ */
>+[scriptable, uuid(335c9292-91a1-4ca0-ad0b-07d5f63ed6cd)]
>+interface fuelIAnnotations : nsISupports
>+{
>+  /**
>+   * Determines if an annotation exists with the given name.
>+   * @param   aName
>+   *          The name of the annotation
>+   * @returns true if an annotation exists with the given name,
>+   *          false otherwise.
>+   */
>+  boolean has(in AString aName);
>+
>+  /**
>+   * Gets the value of an annotation with the given name.
>+   * @param   aName
>+   *          The name of the annotation
>+   * @returns A variant containing the value of the annotation. Supports
>+   *          string, boolean and number.
>+   */
>+  nsIVariant get(in AString aName);
>+
>+  /**
>+   * Sets the value of an annotation with the given name.
>+   * @param   aName
>+   *          The name of the annotation
>+   * @param   aValue
>+   *          The new value of the annotation. Supports string, boolean
>+   *          and number
>+   * @param   aExpiration
>+   *          The expiration policy for the annotation.
>+   *          See nsIAnnotationService.
>+   */
>+  void set(in AString aName, in nsIVariant aValue, in PRInt32 aExpiration);
>+
>+  /**
>+   * Removes the named annotation from the owner item. Can also remove all
>+   * annotations.
>+   * @param   aName
>+   *          The name of annotation. If null, all annotations are removed.
>+   */
>+  void remove(in AString aName);
>+};

it may be handy to have |length| property as well.

>+
>+
>+/**
>+ * Interface representing a bookmark item.
>+ */
>+[scriptable, uuid(808585b6-7568-4b26-8c62-545221bf2b8c)]
>+interface fuelIBookmark : nsISupports
>+{
>+  /**
>+   * The title of the bookmark.
>+   */
>+  attribute AString title;
>+
>+  /**
>+   * The url of the bookmark.
>+   */
>+  attribute AString uri;

why string instead of nsIURI? IMHO we should encourage extension developers to use nsIURI, even at the expense of absolute ease.

>+
>+  /**
>+   * The description of the bookmark.
>+   */
>+  attribute AString description;
>+
>+  /**
>+   * The keywords associated with the bookmark.
>+   */
>+  attribute AString keyword;

please fix the comment to match the singular attribute name.

>+
>+  /**
>+   * The type of the bookmark.
>+   * values: "bookmark", "separator"
>+   */
>+  readonly attribute AString type;
>+
>+  /**
>+   * The parent folder of the bookmark.
>+   */
>+  readonly attribute fuelIBookmarkFolder parent;

is there a reason why this is read-only?

http://lxr.mozilla.org/mozilla/source/toolkit/components/places/public/nsINavBookmarksService.idl#297

>+
>+  /**
>+   * The annotations object for the bookmark.
>+   */
>+  readonly attribute fuelIAnnotations annotations;
>+
>+  /**
>+   * The events object for the bookmark.
>+   * supports: "remove", "change", "visit", "move"
>+   */
>+  readonly attribute fuelIEvents events;
>+
>+  /**
>+   * Removes the bookmark from the parent folder.
>+   */
>+  void remove();

the comment is ambiguous wrt to whether the item is actually deleted. if this actually removes the item permanently, please update the comment to say something to that effect.

>+}; 
>+
>+
>+/**
>+ * Interface representing a bookmark folder or container. Folders
>+ * can hold bookmarks, separators and other folders.
>+ */

by "container", do you mean that this interface supports something other than bookmark folders?

>+[scriptable, uuid(9f42fe20-52de-4a55-8632-a459c7716aa0)]
>+interface fuelIBookmarkFolder : nsISupports
>+{
>+
>+  /**
>+   * The title of the folder.
>+   */
>+  attribute AString title;
>+
>+  /**
>+   * The description of the folder.
>+   */
>+  attribute AString description;
>+
>+  /**
>+   * The type of the folder.
>+   * values: "folder"
>+   */
>+  readonly attribute AString type;
>+
>+  /**
>+   * The parent folder of the folder.
>+   */
>+  readonly attribute fuelIBookmarkFolder parent;
>+
>+  /**
>+   * The annotations object for the folder.
>+   */
>+  readonly attribute fuelIAnnotations annotations;
>+
>+  /**
>+   * The events object for the folder.
>+   * supports: "add", "addchild", "remove", "removechild", "change", "move"
>+   */
>+  readonly attribute fuelIEvents events;
>+
>+  /**
>+   * Array of all bookmarks, spearators and folders contained
>+   * in this folder.
>+   */
>+  readonly attribute nsIVariant all;
>+
>+  fuelIBookmark addBookmark(in AString aId, in AString aURI);
>+  fuelIBookmarkFolder addFolder(in AString aId);
>+
>+  void remove();
>+};
>+

> 
>+//=================================================
>+// Singleton that holds serivces and utilities

nit: spelling


>+//=================================================
>+// Annotations implementation
>+function Annotations(aId) {
>+  this._id = aId;
>+}

hm. the annotations service now has separate apis for annotating uris and bookmark items. are you planning on supporting uri annotations? regardless, it'd be good to have this named such that it's clear that this only works for bookmark items.

>+
>+Annotations.prototype = {
>+  has : function(aName) {
>+    return Utilities.annotations.itemHasAnnotation(this._id, aName);
>+  },
>+  
>+  get : function(aName) {
>+    return Utilities.annotations.getItemAnnotationString(this._id, aName);
>+  },

please use getItemAnnotationType to figure out the proper type for the annotation, and use the type-specific accessor.

>+  
>+  set : function(aName, aValue, aExpiration) {
>+    var type = aValue != null ? aValue.constructor.name : "";
>+    
>+    switch (type) {
>+      case "String":
>+        Utilities.annotations.setItemAnnotationString(this._id, aName, aValue, 0, aExpiration);
>+        break;
>+      case "Boolean":
>+        Utilities.annotations.setItemAnnotationInt32(this._id, aName, aValue, 0, aExpiration);
>+        break;
>+      case "Number":
>+        Utilities.annotations.setItemAnnotationInt32(this._id, aName, aValue, 0, aExpiration);
>+        break;
>+      default:
>+        throw("Unknown annotation value specified.");
>+    }
>+  },
>+    
>+  remove : function(aName) {
>+    if (aName == null)
>+      Utilities.annotations.removeItemAnnotations(this._id);
>+    else
>+      Utilities.annotations.removeItemAnnotation(this._id, aName);
>+  }
>+};
>+
>+
>+//=================================================
>+// Bookmark implementation
>+function Bookmark(aId, aParent, aType) {
>+  this._id = aId;
>+  this._parent = aParent;
>+  this._type = aType || "unknown";

i think it's reasonable to set bookmark as the default here.

>+  this._annotations = new Annotations(this._id);
>+  this._events = new Events();
>+
>+  Utilities.bookmarks.addObserver(this, false);  
>+                                 
>+  var self = this;
>+  gShutdown.push(function() { self._shutdown(); });
>+}
>+
>+Bookmark.prototype = {
>+  _shutdown : function() {
>+    this._annotations = null;
>+    this._events = null;
>+    
>+    Utilities.bookmarks.removeObserver(this);  
>+  },
>+  
>+  get title() {
>+    return Utilities.bookmarks.getItemTitle(this._id);
>+  },
>+
>+  set title(aTitle) {
>+    Utilities.bookmarks.setItemTitle(this._id, aTitle);
>+  },
>+
>+  get uri() {
>+    return Utilities.bookmarks.getBookmarkURI(this._id).spec;
>+  },
>+
>+  set uri(aSpec) {
>+    return Utilities.bookmarks.changeBookmarkURI(this._id, Utilities.specToURI(aSpec));
>+  },
>+
>+  get description() {
>+    return this._annotations.get("bookmarkProperties/description");
>+  },
>+
>+  set description(aDesc) {
>+    this._annotations.set("bookmarkProperties/description", aDesc, Ci.nsIAnnotationService.EXPIRE_NEVER);
>+  },

hm, so this is likely a design issue with having "official" annotations, but i worry that consumers may remove all annotations using the FUEL annotations.remove(null) api, not knowing that they've also just removed the bookmark description, as well as any microsummary annotations, etc.

i think that the remove(null) functionality for the annotations implementation should go bye-bye. deleting data is one area that doesn't necessarily benefit from ease-of-access.

>+
>+  get keyword() {
>+    return Utilities.bookmarks.getKeywordForBookmark(this._id);
>+  },
>+
>+  set keyword(aKeyword) {
>+    Utilities.bookmarks.setKeywordForBookmark(this._id, aKeyword);
>+  },
>+
>+  get type() {
>+    return this._type;
>+  },
>+
>+  get parent() {
>+    return this._parent;
>+  },
>+  
>+  get annotations() {
>+    return this._annotations;
>+  },
>+  
>+  get events() {
>+    return this._events;
>+  },
>+  
>+  remove : function() {
>+    Utilities.bookmarks.removeItem(this._id);
>+  },
>+  
>+  // observer
>+  onBeginUpdateBatch : function() {
>+  },
>+
>+  onEndUpdateBatch : function() {
>+  },
>+
>+  onItemAdded : function(id, folder, index) {
>+    // bookmark object doesn't exist at this point
>+  },
>+
>+  onItemRemoved : function(id, folder, index) {
>+    if (this._id == id)
>+      this._events.dispatch("remove", "");
>+  },
>+
>+  onItemChanged : function(id, property, isAnnotationProperty, value) {
>+    if (this._id == id)
>+      this._events.dispatch("change", property);
>+  },
>+
>+  onItemVisited: function(id, visitID, time) {
>+  },
>+
>+  onItemMoved: function(id, oldParent, oldIndex, newParent, newIndex) {
>+    if (this._id == id)
>+      this._events.dispatch("move", newParent); // xxx - get new parent
>+  },
>+
>+  QueryInterface: function(iid) {
>+    if (iid.equals(Ci.fuelIBookmark) ||
>+        iid.equals(Ci.nsINavBookmarkObserver) ||
>+        iid.equals(Ci.nsISupports)) {
>+      return this;
>+    }
>+    throw Component.result.NS_ERROR_NO_INTERFACE;
>+  }
>+}; 
>+
>+
>+//=================================================
>+// BookmarkFolder implementation
>+function BookmarkFolder(aId, aParent) {
>+  this._id = aId;
>+  if (this._id == null)
>+    this._id = Utilities.bookmarks.bookmarksRoot;
>+  
>+  this._parent = aParent;
>+                                 
>+  this._annotations = new Annotations(this._id);
>+  this._events = new Events();
>+
>+  Utilities.bookmarks.addObserver(this, false);  
>+
>+  var self = this;
>+  gShutdown.push(function() { self._shutdown(); });
>+}
>+
>+BookmarkFolder.prototype = {
>+  _shutdown : function() {
>+    this._annotations = null;
>+    this._events = null;
>+    
>+    Utilities.bookmarks.removeObserver(this);  
>+  },
>+  
>+  get title() {
>+    return Utilities.bookmarks.getItemTitle(this._id);
>+  },
>+
>+  set title(aTitle) {
>+    Utilities.bookmarks.setItemTitle(this._id, aTitle);
>+  },
>+
>+  get description() {
>+    return this._annotations.get("bookmarkProperties/description");
>+  },
>+
>+  set description(aDesc) {
>+    this._annotations.set("bookmarkProperties/description", aDesc, Ci.nsIAnnotationService.EXPIRE_NEVER);
>+  },
>+
>+  get type() {
>+    return "folder";
>+  },
>+
>+  get parent() {
>+    return this._parent;
>+  },
>+
>+  get annotations() {
>+    return this._annotations;
>+  },
>+  
>+  get events() {
>+    return this._events;
>+  },
>+  
>+  get all() {

"all" seems an ambiguous name for this. maybe contents, or children? you use "child" for the event names below, so maybe children would be the consistent choice.

>+    var items = [];
>+    
>+    var options = Utilities.history.getNewQueryOptions();
>+    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);

nit: setting the grouping is not necessary. it's implied if you use setFolders.

>+    var query = Utilities.history.getNewQuery();
>+    query.setFolders([this._id], 1);
>+    var result = Utilities.history.executeQuery(query, options);
>+    var rootNode = result.root;
>+    rootNode.containerOpen = true;
>+    var cc = rootNode.childCount;
>+    for (var i=0; i<cc; ++i) {

style nit: spaces before/after operators

r- until we figure out the annotations issues.

finally, nickolay brought up some good questions in comment #9, which don't seem to have been addressed (or at least not in subsequent bug comments anyway).

Comment 19

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

Comment on attachment 268131 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests

There are a bunch of tabs scattered throughout this patch, please remove them before landing :)

>Index: public/fuelIApplication.idl

>+/**
>+ * Interface representing a browser window.
>+ */
>+[scriptable, uuid(207edb28-eb5e-424e-a862-b0e97C8de866)]
>+interface fuelIBrowser : nsISupports

Hmm, fuelIBrowserWindow, perhaps? "fuelIBrowser" looks like it could be a sibling or subclass of "fuelIApplication".

>+/**
>+ * Interface representing a browser tab.
>+ */

>+interface fuelIBrowserTab : nsISupports
>+{
>+  /**
>+   * The current url of this tab.
>+   */
>+  attribute AString url;

I wonder if exposing this as a string is a good idea... How likely is it that users of this attribute will end up needing an nsIURI for what they want to do? I'm afraid that this will encourage extension authors to rely on manual URI parsing/string munging.

>+  /**
>+   * The currently-active tab within the browser window.
>+   */
>+  readonly attribute nsIDOMHTMLDocument document;

Wrong comment here...

>+  /**
>+   * Close the browser tab.
>+   */
>+  void close();

Hmm, I wonder if it's worth noting that this might not actually result in the tab closing, should the site have an onbeforeunload handler that prevents the closure (e.g. Zimbra).

>+  /**
>+   * An array of browser windows within the application.
>+   */
>+  readonly attribute nsIVariant browsers;

This is a bit confusing, because we're using "browsers" to refer to an array of <tabbrowser> elements, that themselves contain <browser>s. I'm not sure if we really want to expose that detail to users of this API, but saying that this returns "an array of windows" when it's really an array of <tabbrowsers> is wrong, I think.

>+  /**
>+   * The currently active browser window.
>+   */
>+  readonly attribute fuelIBrowser activeBrowser;

Again, this doesn't return a window, in the commonly-used sense of the word. The underlying object model is something like window->tabbrowser->tab->browser, and we seem to be ignoring the window and tab parts of that chain. Maybe that's OK, because they're not something that users commonly need to access? Application->Browsers->Tabs maps loosely to Application->Tabbrowsers->Browsers, I guess? I'm a bit worried that this could be confusing to authors who are used to working with the current ad-hoc tabbrowser API (activeBrowser sounds similar to currentBrowser, which is a property of a tabbrowser, etc).

I guess this ties in to what Nickolay mentioned in comment 9 - is it worth essentially duplicating the tabbrowser API, rather than just providing easy access to the <tabbrowser> element and improving it's API directly?

>Index: src/fuelApplication.js

>+const Ci = Components.interfaces;
>+const Cc = Components.classes;
>+
>+const nsISupports = Ci.nsISupports;
>+const nsIClassInfo = Ci.nsIClassInfo;
>+const nsIObserver = Ci.nsIObserver;
>+const fuelIApplication = Ci.fuelIApplication;

> // Preferences constants
>+const nsIPrefService = Ci.nsIPrefService;
>+const nsIPrefBranch = Ci.nsIPrefBranch;
>+const nsIPrefBranch2 = Ci.nsIPrefBranch2;
>+const nsISupportsString = Ci.nsISupportsString;

Is it still worth having these constants, now that they're only 3 characters shorter?

>@@ -508,34 +511,574 @@ Extensions.prototype = {

>+// Singleton that holds serivces and utilities
>+var Utilities = {

>+  _window : null,
>+  get window() {

Just "window" is a bit confusing, I think, perhaps "windowmediator", or "windowm"?

>+  specToURI : function(aSpec) {

>+  },

This function is traditionally called "makeURI()" in other browser/toolkit code, perhaps it'd be best to stay consistent?

>+  free : function() {
>+    this._bookmarks = null;
>+    this._livemarks = null;
>+    this._annotations = null;
>+    this._history = null;
>+  }

Is there a reason you omit _window here?

>+Browser.prototype = {

>+  insertBefore : function(aInsert, aBefore) {
>+  	this._browser.mTabContainer.insertBefore(aInsert, aBefore);
>+  },

>+  append : function(aInsert) {
>+  	this._browser.mTabContainer.appendChild(aInsert);
>+  },

These two methods take fuelIBrowserTabs, I doubt the DOM methods they map to will deal with those very well :)

>+// BrowserTab implementation
>+function BrowserTab(aBrowser, aBrowserTab) {

These argument names are confusing... aTabBrowser and aBrowser are better fits, I guess.

>+BrowserTab.prototype = {

>+  set url(aSpec) {
>+    return this._browsertab.currentURI.spec = aSpec;
>+  },

This isn't tested, and I doubt it will work, setting the spec of the currentURI won't trigger a new load (and is wrong in general, you need to call nsIIOService::newURI to make sure you get the right protocol handler, or let loadURI do it).

>+  get index() {
>+  	var tabs = this._browser.mTabContainer.childNodes;

this._browser.mTabs

>+  get document() {
>+  	return this._browsertab.content.document;
>+  },

This looks wrong, <browser>s don't have a content property as far as I know. I think you want this._browsertab.contentDocument.

>+  _event : function(aEvent) {
>+  	if (aEvent.type == "load" && (!aEvent.originalTarget instanceof HTMLDocument ||
>+  		aEvent.originalTarget.defaultView.frameElement))
>+  		return;

Add a comment that explains what this code is for?

>+  _getTab : function() {
>+  	var tabs = this._browser.mTabContainer.childNodes;

this._browser.mTabs;

>Index: test/test_Browser.xul

Looks like you're missing tests for Tab.[close,focus,document,browser,events] and Browser.[insertBefore,append,events] (some of which would have caught some bugs mentioned above), I guess you planned on adding those later?

Comment 20

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

(In reply to comment #9)

In general, FUEL is designed for beginner/intermediate extension developers. Yes, it can be used by more experienced developers (or by Firefox developers) too. We are not trying to wrap the entire Mozilla API surface area. Besides creating a simple to use API, we are trying to create a firewall between some of the more "fluid" parts of Firefox/Mozilla code and extensions.

That's the main reason for not making the FUEL API mirror the underying APIs. Another issue is Mozilla 2. We don't know how the API's will change for Firefox 4.

I do think we should continue to make improvements to the underlying APIs as you point out. However, FUEL's goal of hiding XPCOM can not be met with API improvements alone.

> (comment 8 contd.: AFAIK the places API was designed from scratch. FUEL 0.2 has
> wrappers for that API. I admit I'm not familiar with the places API, but could
> you explain why you needed to create another API?)

Places is a flat service API that uses id's (URL or long) to identify resources. It is nice and simple. FUEL wrapper provides a more hierarchal model. It also tries to remove the whole "TYPE_*" system from annotations.

> 
> Some minor points:
> 1. I think an "Array<Window> windows" and maybe "mainWindows" (for browser.xul
> windows) would be a useful attribute on Application. Each object in the array
> is a simple DOM window. Then the Firefox-specific 'browsers' attribute could be
> avoided (in favor of app.mainWindows[i].gBrowser).

we changed to "windows" and "activeWindow", but still do not use "gBrowser" as that is too much of an implementation detail.

> 2. >> |Browser.events| is an |Events| <<
> See my comment about EventTarget in bug 380363. I haven't received a clear
> explanation why it's worth separating the 'events' object from the object whose
> events are being handled.

Maybe it isn't worth it, but the API is consistent with "prefs" and "storage". and there is barely any extra typing (obj.events.addListener vs obj.addEventListener)

> 3. How are things like 'Array<Browser>' exposed via the IDL? (Speaking of
> which, are FUEL APIs exposed through IDL because you want them to (e.g. to make
> them usable from Python) or because it's not possible to expose a pure JS
> object to all chrome scripts without hacking DOM/XPConnect code?).

As nsiVariant.
Because it's not possible to expose a pure JS object to all chrome scripts without hacking DOM/XPConnect code.

> 4. There are multiple subtle differences between the old object model and the
> proposed FUEL API, which I'm not listing for now (hoping that you reconsider
> the decision to have the wrappers completely isolated from the old OM).
> 

See above.

These answers are mine and may not represent everyone's view. We are certainly open to feedback, patches and criticism.

Comment 21

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

Attachment: FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

Comment 22

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

Comment on attachment 269252 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

last minute change coming

Comment 23

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

Attachment: FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

Comment 24

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

(In reply to comment #18)

> it may be handy to have |length| property as well.

added .names array instead

> >+  attribute AString uri;
> 
> why string instead of nsIURI? IMHO we should encourage extension developers to
> use nsIURI, even at the expense of absolute ease.

ok, switched to nsIURI. we'll see if any other feedback comes in. Enn's IO patch has a simple | IO.newURI("url"); | method that can be used as a short cut

> please fix the comment to match the singular attribute name.

done.

> >+  readonly attribute fuelIBookmarkFolder parent;
> 
> is there a reason why this is read-only?

changed to read/write. uses .moveItem

> >+/**
> >+ * Interface representing a bookmark folder or container. Folders
> >+ * can hold bookmarks, separators and other folders.
> >+ */
> 
> by "container", do you mean that this interface supports something other than
> bookmark folders?

removed the word "container" to avoid confusion

> >+//=================================================
> >+// Singleton that holds serivces and utilities
> 
> nit: spelling

fixed

> >+//=================================================
> >+// Annotations implementation
> >+function Annotations(aId) {
> >+  this._id = aId;
> >+}
> 
> hm. the annotations service now has separate apis for annotating uris and
> bookmark items. are you planning on supporting uri annotations? regardless,
> it'd be good to have this named such that it's clear that this only works for
> bookmark items.

leaving as is for now. We may add support for URI annotations later, but the interface should not change, only the implementation.

> >+  get : function(aName) {
> >+    return Utilities.annotations.getItemAnnotationString(this._id, aName);
> >+  },
> 
> please use getItemAnnotationType to figure out the proper type for the
> annotation, and use the type-specific accessor.

done

> >+function Bookmark(aId, aParent, aType) {
> >+  this._id = aId;
> >+  this._parent = aParent;
> >+  this._type = aType || "unknown";
> 
> i think it's reasonable to set bookmark as the default here.
> 

done

> >+  set description(aDesc) {
> >+    this._annotations.set("bookmarkProperties/description", aDesc, Ci.nsIAnnotationService.EXPIRE_NEVER);
> >+  },
> 
> hm, so this is likely a design issue with having "official" annotations, but i
> worry that consumers may remove all annotations using the FUEL
> annotations.remove(null) api, not knowing that they've also just removed the
> bookmark description, as well as any microsummary annotations, etc.
> 
> i think that the remove(null) functionality for the annotations implementation
> should go bye-bye. deleting data is one area that doesn't necessarily benefit
> from ease-of-access.
> 

no long support | remove(null); |

> >+BookmarkFolder.prototype = {
...
> >+  get all() {
> 
> "all" seems an ambiguous name for this. maybe contents, or children? you use
> "child" for the event names below, so maybe children would be the consistent
> choice.

changed to | children |

> 
> >+    var items = [];
> >+    
> >+    var options = Utilities.history.getNewQueryOptions();
> >+    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
> 
> nit: setting the grouping is not necessary. it's implied if you use setFolders.

removed grouping

> 
> finally, nickolay brought up some good questions in comment #9, which don't
> seem to have been addressed (or at least not in subsequent bug comments
> anyway).
> 

responded to nickolay's c#9

Comment 25

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

(In reply to comment #19)

> >+/**
> >+ * Interface representing a browser window.
> >+ */
> >+[scriptable, uuid(207edb28-eb5e-424e-a862-b0e97C8de866)]
> >+interface fuelIBrowser : nsISupports
> 
> Hmm, fuelIBrowserWindow, perhaps? "fuelIBrowser" looks like it could be a
> sibling or subclass of "fuelIApplication".

fuelIWindow

> >+interface fuelIBrowserTab : nsISupports
> >+{
> >+  /**
> >+   * The current url of this tab.
> >+   */
> >+  attribute AString url;
> 
> I wonder if exposing this as a string is a good idea... How likely is it that
> users of this attribute will end up needing an nsIURI for what they want to do?
> I'm afraid that this will encourage extension authors to rely on manual URI
> parsing/string munging.

now uses nsIURI.

> 
> >+  /**
> >+   * The currently-active tab within the browser window.
> >+   */
> >+  readonly attribute nsIDOMHTMLDocument document;
> 
> Wrong comment here...

fixed

> 
> >+  /**
> >+   * Close the browser tab.
> >+   */
> >+  void close();
> 
> Hmm, I wonder if it's worth noting that this might not actually result in the
> tab closing, should the site have an onbeforeunload handler that prevents the
> closure (e.g. Zimbra).

added note in comment

> 
> >+  /**
> >+   * An array of browser windows within the application.
> >+   */
> >+  readonly attribute nsIVariant browsers;
> 
> This is a bit confusing, because we're using "browsers" to refer to an array of
> <tabbrowser> elements, that themselves contain <browser>s. I'm not sure if we
> really want to expose that detail to users of this API, but saying that this
> returns "an array of windows" when it's really an array of <tabbrowsers> is
> wrong, I think.

now called | windows |
> 
> >+  /**
> >+   * The currently active browser window.
> >+   */
> >+  readonly attribute fuelIBrowser activeBrowser;
> 
> Again, this doesn't return a window, in the commonly-used sense of the word.
> The underlying object model is something like window->tabbrowser->tab->browser,
> and we seem to be ignoring the window and tab parts of that chain. Maybe that's
> OK, because they're not something that users commonly need to access?
> Application->Browsers->Tabs maps loosely to Application->Tabbrowsers->Browsers,
> I guess? I'm a bit worried that this could be confusing to authors who are used
> to working with the current ad-hoc tabbrowser API (activeBrowser sounds similar
> to currentBrowser, which is a property of a tabbrowser, etc).
> 
> I guess this ties in to what Nickolay mentioned in comment 9 - is it worth
> essentially duplicating the tabbrowser API, rather than just providing easy
> access to the <tabbrowser> element and improving it's API directly?

now called | activeWindow |

> 
> >Index: src/fuelApplication.js
> 
> >+const Ci = Components.interfaces;
> >+const Cc = Components.classes;
> >+
> >+const nsISupports = Ci.nsISupports;
> >+const nsIClassInfo = Ci.nsIClassInfo;
> >+const nsIObserver = Ci.nsIObserver;
> >+const fuelIApplication = Ci.fuelIApplication;
> 
> > // Preferences constants
> >+const nsIPrefService = Ci.nsIPrefService;
> >+const nsIPrefBranch = Ci.nsIPrefBranch;
> >+const nsIPrefBranch2 = Ci.nsIPrefBranch2;
> >+const nsISupportsString = Ci.nsISupportsString;
> 
> Is it still worth having these constants, now that they're only 3 characters
> shorter?

removed

> 
> >@@ -508,34 +511,574 @@ Extensions.prototype = {
> 
> >+// Singleton that holds serivces and utilities
> >+var Utilities = {
> 
> >+  _window : null,
> >+  get window() {
> 
> Just "window" is a bit confusing, I think, perhaps "windowmediator", or
> "windowm"?

now called | windowMediator |

> 
> >+  specToURI : function(aSpec) {
> 
> >+  },
> 
> This function is traditionally called "makeURI()" in other browser/toolkit
> code, perhaps it'd be best to stay consistent?

now called | makeURI |

> 
> >+  free : function() {
> >+    this._bookmarks = null;
> >+    this._livemarks = null;
> >+    this._annotations = null;
> >+    this._history = null;
> >+  }
> 
> Is there a reason you omit _window here?

added | this._window = null; |

> 
> >+Browser.prototype = {
> 
> >+  insertBefore : function(aInsert, aBefore) {
> >+  	this._browser.mTabContainer.insertBefore(aInsert, aBefore);
> >+  },
> 
> >+  append : function(aInsert) {
> >+  	this._browser.mTabContainer.appendChild(aInsert);
> >+  },
> 
> These two methods take fuelIBrowserTabs, I doubt the DOM methods they map to
> will deal with those very well :)

fixed. but we also changed to use | this._browser.moveTabTo | which also takes <tab> not <browser>

> 
> >+// BrowserTab implementation
> >+function BrowserTab(aBrowser, aBrowserTab) {
> 
> These argument names are confusing... aTabBrowser and aBrowser are better fits,
> I guess.

now uses | BrowserTab(aWindow, aBrowserTab) |

> 
> >+BrowserTab.prototype = {
> 
> >+  set url(aSpec) {
> >+    return this._browsertab.currentURI.spec = aSpec;
> >+  },
> 
> This isn't tested, and I doubt it will work, setting the spec of the currentURI
> won't trigger a new load (and is wrong in general, you need to call
> nsIIOService::newURI to make sure you get the right protocol handler, or let
> loadURI do it).

switched to | loadURI |

> 
> >+  get index() {
> >+  	var tabs = this._browser.mTabContainer.childNodes;
> 
> this._browser.mTabs

switched

> 
> >+  get document() {
> >+  	return this._browsertab.content.document;
> >+  },
> 
> This looks wrong, <browser>s don't have a content property as far as I know. I
> think you want this._browsertab.contentDocument.

switched to | contentDocument |

> 
> >+  _event : function(aEvent) {
> >+  	if (aEvent.type == "load" && (!aEvent.originalTarget instanceof HTMLDocument ||
> >+  		aEvent.originalTarget.defaultView.frameElement))
> >+  		return;
> 
> Add a comment that explains what this code is for?

done

> 
> >+  _getTab : function() {
> >+  	var tabs = this._browser.mTabContainer.childNodes;
> 
> this._browser.mTabs;

switched

> 
> >Index: test/test_Browser.xul
> 
> Looks like you're missing tests for Tab.[close,focus,document,browser,events]
> and Browser.[insertBefore,append,events] (some of which would have caught some
> bugs mentioned above), I guess you planned on adding those later?
> 

to give better coverage, added more tests for close, focus, document, names, events, moveToEnd

Comment 26

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

Comment on attachment 269259 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

ready for review of bookmark related code

Comment 27

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

Comment on attachment 269259 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

ready for re-review of browser related stuff and tests

Comment 28

[Nickolay_Ponomarev]

(In reply to comment #20)
> In general, FUEL is designed for beginner/intermediate extension developers.

OK, thanks for clarifying. I don't think keeping the wrapper in total isolation from the real platform is the right thing to do, but I've already said that.

> Yes, it can be used by more experienced developers (or by Firefox developers)
> too.

That's hard - for the reasons I listed in my previous posts (subtle differences between "the fuel way" and "the 'xpcom' way" and the lack of two way links). I'm not sure I like the idea of using fuel in Firefox itself anymore.

> [...] FUEL's goal of hiding XPCOM can not be met with API improvements alone

I didn't understand what you meant by "hiding XPCOM".

> we changed to "windows" and "activeWindow", but still do not use "gBrowser" as
> that is too much of an implementation detail.
> 
Thanks.. That's "browserWindows", not "windows". Same for "activeWindow".

> > 2. >> |Browser.events| is an |Events| <<
> > See my comment about EventTarget in bug 380363. I haven't received a clear
> > explanation why it's worth separating the 'events' object from the object
> > whose events are being handled.
> 
> Maybe it isn't worth it, but the API is consistent with "prefs" and "storage".
> and there is barely any extra typing (obj.events.addListener vs
> obj.addEventListener)
> 
So why not change it for prefs and storage too?

> > 4. There are multiple subtle differences between the old object model and the
> > proposed FUEL API, which I'm not listing for now (hoping that you reconsider
> > the decision to have the wrappers completely isolated from the old OM).
> > 
> 
> See above.
> 
You didn't answer this (eg. see the activeTab vs selectedTab example in my previous post). Is this by design?

Thanks for your answer. Much appreciated.

Comment 29

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

Comment on attachment 269259 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)

>Index: public/fuelIApplication.idl

>@@ -335,16 +340,281 @@ interface fuelIExtensions : nsISupports

>+  /**
>+   * Adds a new child separtor to this folder.
>+   */
>+  fuelIBookmark addSepartor();

Typo? :)

>+interface fuelIWindow : nsISupports

>+  /**
>+   * Open a new browser tab, pointing to the specified URL.
>+   * @param   aURL
>+   *          The url to open the browser tab to
>+   */
>+  fuelIBrowserTab open(in nsIURI aURL);

nit: aURI

>+interface fuelIBrowserTab : nsISupports
>+{
>+  /**
>+   * The current url of this tab.
>+   */
>+  attribute nsIURI url;

nit: uri

>+  /**
>+   * Close the browser tab. This may not actually close the window
>+   * as script may abort the close operation.
>+   */
>+  void close();

nit: "close the window" -> "close the tab"

>Index: src/fuelApplication.js

>+Window.prototype = {

>+function BrowserTab(aWindow, aBrowserTab) {
>+  this._window = aWindow;
>+  this._browser = aWindow.getBrowser();
>+  this._browsertab = aBrowserTab;

I still think it'd be clearer if the implementation used the internal names, so something like:
  this._window = aWindow;
  this._tabbrowser = aWindow.getBrowser();
  this._browser = aBrowser;

>+  this._events = new Events();
>+  this._cleanup = {};
>+  
>+  this._watch("load");
>+                                 
>+  var self = this;
>+  gShutdown.push(function() { self._shutdown(); });
>+}

>+BrowserTab.prototype = {
>+  get url() {
>+    return this._browsertab.currentURI;
>+  },
>+  
>+  set url(aURI) {
>+    return this._browsertab.loadURI(aURI, null, null);
>+  },

loadURI doesn't return a URI, so returning it from the setter here doesn't really make sense. Not to mention that it could throw, and it's effects are asynchronous (i.e. getting uri right after setting it might not return the expected result). I think it's best to make uri readonly, and have a separate loadURI (or load, or whatever) method.

>+  get index() {
>+    var tabs = this._browser.mTabs;
>+    for (var i=0; i<tabs.length; i++) {
>+      if (tabs[i].linkedBrowser == this._browsertab)
>+        return i;
>+    }
>+    return -1;
>+  },

>+  /*
>+   * Helper used to determine the index offset of the browsertab
>+   */
>+  _getTab : function() {
>+    var tabs = this._browser.mTabs;
>+    for (var i=0; i<tabs.length; i++) {
>+      if (tabs[i].linkedBrowser == this._browsertab)
>+        return tabs[i];
>+    }
>+    return null;
>+  },

return tabs[this.index] || null;, instead of duplicating the loop from the index getter?

>@@ -652,65 +1241,85 @@ Application.prototype = {
>+  get bookmarks() {
>+    if (this._bookmarks == null)
>+        this._bookmarks = new BookmarkFolder(null, null);

>+  get windows() {

>+  while (enum.hasMoreElements())
>+    win.push(new Window(enum.getNext()));
>+  
>+  return win;
>+  },

nit: indentation wrong in a couple places this section, please fix before landing.

>Index: test/test_Browser.xul

>+function test_Browser() {

>+    // check event
>+    todo_is(gTabOpenCount, 2, "Checking event handler for tab open");

File a bug on this test, if it's really not working?

Looks like you're still missing tests for fuelIBrowserTab.moveBefore, fuelIBrowserTab.events.

r=me with those addressed.

Comment 30

[Sergey Sokoloff]

The bug should probably be retargeted from Firefox 3 alpha5 to something like Firefox 3 alpha7, because alpha5 is already out, and alpha6 is freezing tomorrow night (June 27th) at 11:59 PM PDT, see http://developer.mozilla.org/devnews/index.php/2007/06/26/alpha-6-freeze-june-27th-1159-pm-pdt/ for details.

Retargeting would probably benefit some people who search Bugzilla by Target Milestone field. However, I do not insist.

Comment 31

[Sergey Sokoloff]

Oops. Sorry, people, I forgot there's no alpha7. I've just re-read http://wiki.mozilla.org/Firefox3/Schedule and found that alpha6 is the last of Gran Paradiso alphas. The first beta, in late July, is their successor.

P. S.
Thanks to Shawn Wilsher for changing the target milestone.

Comment 32

[Dietrich Ayala (:dietrich)]

Comment on attachment 269259 [details] [diff] [review]
FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 2)


nit: still some tabs in test_ApplicationPrefs.xul, test_Extensions.xul

>+  /**
>+   * Array of all bookmarks, spearators and folders contained

nit: spelling

>+
>+  /**
>+   * Adds a new child separtor to this folder.
>+   */
>+  fuelIBookmark addSepartor();

spelling x2. also, please add this to the test suite. a test for this would've failed and uncovered the problem. hm, would be good know what else is not under test.

>+
>+  /**
>+   * Adds a new child folder to this folder.
>+   * @param   aTitle
>+   *          The title of folder.
>+   */
>+  fuelIBookmarkFolder addFolder(in AString aTitle);
>+
>+  void remove();

nit: please add jsdoc to remove()

>+
>+Annotations.prototype = {
>+  get names() {
>+    var namesForItem = Utilities.annotations.getItemAnnotationNames(this._id, {});
>+    return namesForItem;

why the instance variable instead of just returning?

>+  
>+  set parent(aBookmarkFolder) {
>+    Utilities.bookmarks.moveItem(this._id, aFolder._id, Utilities.bookmarks.DEFAULT_INDEX);
>+    this._parent = new BookmarkFolder(this._parent._id, this._parent.parent);
>+  },

s/aFolder/aBookmarkFolder/

hm, is the 2nd line necessary? looks like onItemMoved already does that part.

again, if this was under test, it would've been caught. (can you tell i'm chugging the unit-testing kool-aid?)

>+  
>+  remove : function() {
>+    Utilities.bookmarks.removeItem(this._id);
>+  },

hm, will this object continue to receive notifications? should we unhook the observer here, call shutdown()? this goes for all these objects that support remove(). this should be filed as a followup bug if this is a problem and the change is too big to make A6.

>+  set parent(aFolder) {
>+    Utilities.bookmarks.moveItem(this._id, aFolder._id, Utilities.bookmarks.DEFAULT_INDEX);
>+    this._parent = new BookmarkFolder(this._parent._id, this._parent.parent);
>+  },

same comment from previously about the second line.

r=me with these comments fixed, and under test where appropriate.

Comment 33

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

Attachment: FUEL 0.2 - Bookmarks, Browser and Chrome Tests (take 3)

This is the patch that is landing

Comment 34

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

(In reply to comment #29)

> >+  /**
> >+   * Adds a new child separtor to this folder.
> >+   */
> >+  fuelIBookmark addSepartor();
> 
> Typo? :)

fixed

> 
> >+interface fuelIWindow : nsISupports
> 
> >+  /**
> >+   * Open a new browser tab, pointing to the specified URL.
> >+   * @param   aURL
> >+   *          The url to open the browser tab to
> >+   */
> >+  fuelIBrowserTab open(in nsIURI aURL);
> 
> nit: aURI
done

> 
> >+interface fuelIBrowserTab : nsISupports
> >+{
> >+  /**
> >+   * The current url of this tab.
> >+   */
> >+  attribute nsIURI url;
> 
> nit: uri

done

> 
> >+  /**
> >+   * Close the browser tab. This may not actually close the window
> >+   * as script may abort the close operation.
> >+   */
> >+  void close();
> 
> nit: "close the window" -> "close the tab"

done

> 
> >Index: src/fuelApplication.js
> 
> >+Window.prototype = {
> 
> >+function BrowserTab(aWindow, aBrowserTab) {
> >+  this._window = aWindow;
> >+  this._browser = aWindow.getBrowser();
> >+  this._browsertab = aBrowserTab;
> 
> I still think it'd be clearer if the implementation used the internal names, so
> something like:
>   this._window = aWindow;
>   this._tabbrowser = aWindow.getBrowser();
>   this._browser = aBrowser;

done

> >+  
> >+  set url(aURI) {
> >+    return this._browsertab.loadURI(aURI, null, null);
> >+  },
> 
> loadURI doesn't return a URI, so returning it from the setter here doesn't
> really make sense. Not to mention that it could throw, and it's effects are
> asynchronous (i.e. getting uri right after setting it might not return the
> expected result). I think it's best to make uri readonly, and have a separate
> loadURI (or load, or whatever) method.

added "load" and made "uri" readonly

> 
> >+  get index() {
> >+    var tabs = this._browser.mTabs;
> >+    for (var i=0; i<tabs.length; i++) {
> >+      if (tabs[i].linkedBrowser == this._browsertab)
> >+        return i;
> >+    }
> >+    return -1;
> >+  },
> 
> >+  /*
> >+   * Helper used to determine the index offset of the browsertab
> >+   */
> >+  _getTab : function() {
> >+    var tabs = this._browser.mTabs;
> >+    for (var i=0; i<tabs.length; i++) {
> >+      if (tabs[i].linkedBrowser == this._browsertab)
> >+        return tabs[i];
> >+    }
> >+    return null;
> >+  },
> 
> return tabs[this.index] || null;, instead of duplicating the loop from the
> index getter?

changed

> 
> >@@ -652,65 +1241,85 @@ Application.prototype = {
> >+  get bookmarks() {
> >+    if (this._bookmarks == null)
> >+        this._bookmarks = new BookmarkFolder(null, null);
> 
> >+  get windows() {
> 
> >+  while (enum.hasMoreElements())
> >+    win.push(new Window(enum.getNext()));
> >+  
> >+  return win;
> >+  },
> 
> nit: indentation wrong in a couple places this section, please fix before
> landing.

fixed

> 
> >Index: test/test_Browser.xul
> 
> >+function test_Browser() {
> 
> >+    // check event
> >+    todo_is(gTabOpenCount, 2, "Checking event handler for tab open");
> 
> File a bug on this test, if it's really not working?
> 
> Looks like you're still missing tests for fuelIBrowserTab.moveBefore,
> fuelIBrowserTab.events.

added tests for fuelIBrowserTab.events. will add more tests later.

Comment 35

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

(In reply to comment #32)

> (From update of attachment 269259 [details] [diff] [review])
> 
> nit: still some tabs in test_ApplicationPrefs.xul, test_Extensions.xul

fixed

> 
> >+  /**
> >+   * Array of all bookmarks, spearators and folders contained
> 
> nit: spelling

fixed

> 
> >+
> >+  /**
> >+   * Adds a new child separtor to this folder.
> >+   */
> >+  fuelIBookmark addSepartor();
> 
> spelling x2. also, please add this to the test suite. a test for this would've
> failed and uncovered the problem. hm, would be good know what else is not under
> test.

fixed and added test

> 
> >+
> >+  /**
> >+   * Adds a new child folder to this folder.
> >+   * @param   aTitle
> >+   *          The title of folder.
> >+   */
> >+  fuelIBookmarkFolder addFolder(in AString aTitle);
> >+
> >+  void remove();
> 
> nit: please add jsdoc to remove()

done

> 
> >+
> >+Annotations.prototype = {
> >+  get names() {
> >+    var namesForItem = Utilities.annotations.getItemAnnotationNames(this._id, {});
> >+    return namesForItem;
> 
> why the instance variable instead of just returning?

changed to just return

> 
> >+  
> >+  set parent(aBookmarkFolder) {
> >+    Utilities.bookmarks.moveItem(this._id, aFolder._id, Utilities.bookmarks.DEFAULT_INDEX);
> >+    this._parent = new BookmarkFolder(this._parent._id, this._parent.parent);
> >+  },
> 
> s/aFolder/aBookmarkFolder/

fixed

> 
> hm, is the 2nd line necessary? looks like onItemMoved already does that part.
> 
> again, if this was under test, it would've been caught. (can you tell i'm
> chugging the unit-testing kool-aid?)

and added test

> 
> >+  
> >+  remove : function() {
> >+    Utilities.bookmarks.removeItem(this._id);
> >+  },
> 
> hm, will this object continue to receive notifications? should we unhook the
> observer here, call shutdown()? this goes for all these objects that support
> remove(). this should be filed as a followup bug if this is a problem and the
> change is too big to make A6.

will make a followup bug

> 
> >+  set parent(aFolder) {
> >+    Utilities.bookmarks.moveItem(this._id, aFolder._id, Utilities.bookmarks.DEFAULT_INDEX);
> >+    this._parent = new BookmarkFolder(this._parent._id, this._parent.parent);
> >+  },
> 
> same comment from previously about the second line.

fixed

Comment 36

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

The unit testing tinderboxes are orange because of this checkin.

Comment 37

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

*** 16 INFO Running chrome://mochikit/content/chrome/browser/fuel/test/test_Application.xul...
*** 17 INFO PASS | Check global access to Application
*** 18 INFO PASS | Check to see if an ID exists for the Application
*** 19 INFO PASS | Check to see if a name exists for the Application
*** 20 INFO PASS | Check to see if a version exists for the Application
*** 22 ERROR FAIL | Error thrown during test: elem has no properties | got 0, expected 1
*** 23 ERROR FAIL | Test timed out. | 
kill TERM 9883
Process killed. Took 2 seconds to die.

Comment 38

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

backed out.

Since chrome-based tests are failing, might try to re-land with HTML based tests

Comment 39

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

Attachment: FUEL 0.2 - Bookmarks, Browser and HTML Tests (take 4)

Same as previous patch but with chrome tests converted to HTML tests.  We will get the chrome tests working later

Comment 40

[Asaf Romano (gone)]

The annotations API has changed much under bug 331654. In FUEL you should probably just mirror the new nsIVariant-based API (get/set-Page/Item-Annotation methods). These don't work for binary annotations, I don't there's much value in exposing binary annotations from FUEL though.

Comment 41

[Asaf Romano (gone)]

Attachment: fix fuel bustage

checked in, will get post-facto review.

Checking in src/fuelApplication.js;
/cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v  <--  fuelApplication.js
new revision: 1.14; previous revision: 1.13
done