549317 - Implement Tabs interface

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Daniel Buchner [:dbuc]]

Aza is running point, see JEP for further details: https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/110

Comment 1

[Atul Varma [:atul]]

Unassigning myself from this as per yesterday's Jetpack meeting.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: wip1

some infrastructure pieces for tabs singleton, tab objects, activeTab, open(), and events.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: wip2

* more open() tests
* open() in specific window
* tab properties
* tab methods
* onOpen, onClose and their tests

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: wip3

* more tab properties + test
* open() passes tab obj to onLoad listener
* onLoad
* onActivate
* onDeactivate

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: wip4

* tab.activate
* tab.load
* tab.move
* more tests

Comment 6

[Dietrich Ayala (:dietrich)]

not implemented yet:

* tab.thumbnail
* moving tabs to different windows
* mozafterpaint event
* tabs.tabs iteration

Comment 7

[Dietrich Ayala (:dietrich)]

note: there are a couple of tests that fail... sometimes.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 450385 [details] [diff] [review]
wip4

>+let tabConstructor = apiUtils.publicConstructor(function(element) {
>+  Components.utils.import("resource://gre/modules/NetUtil.jsm");
>+  let win = element.ownerDocument.defaultView;
>+  if (!win)
>+    throw new Error("element has no window.");
>+  let browser = win.gBrowser.getBrowserForTab(element);
>+  let contentWin = browser.contentWindow;
>+  let contentDoc = browser.contentDocument;
>+
>+  this.__defineGetter__("title", function() contentDoc.title);
>+  this.__defineGetter__("location", function() contentDoc.location);
>+  this.__defineGetter__("contentWindow", function() contentWin);
>+  this.__defineGetter__("contentDocument", function() contentDoc);
>+  this.__defineGetter__("favicon", function() {
>+    let pageURI = NetUtil.newURI(contentDoc.location);
>+    return Cc["@mozilla.org/browser/favicon-service;1"].
>+           getService(Ci.nsIFaviconService).
>+           getFaviconImageForPage(pageURI).spec;
>+  });
>+  this.__defineGetter__("style", function() null); // TODO
>+  this.__defineGetter__("index", function() win.gBrowser.getBrowserIndexForDocument(contentDoc));
>+  this.__defineGetter__("window", function() win);

We don't have a good story right now for exposing chrome windows to high-level clients.  So for the time being, I think it would be better to leave this as a TODO, along with all other window-related parts of this API.

Also, which ones of all these properties can be set and which only get should be noted in the end-user docs.

>+  this.load = function(url) contentDoc.location = url;

I know the JEP specifies this, but there's a location getter, so why not a setter, instead of load()?

>+  this.close = function() win.gBrowser.removeTab(element);

removeTab() is a no-op if the tab is the only one, so maybe it would be useful to check if there are no tabs and if so throw an error?  Hmm, maybe not.  At least it should be documented.

>+  this.move = function(index, window) {

I'd leave this as a TODO along with the window getter.

>+/**
>+ * Tabs singleton exposed as require("tabs").tabs.
>+ */
>+let tabs = {};
>+exports.tabs = tabs;

The JEP specifies this, but there's no reason to attach everything to another singleton since the module itself is a singleton.  (I think we've made that Official Policy now...)

>+/**
>+ * tabs.activeTab
>+ */
>+tabs.__defineGetter__("activeTab", function() {
>+  const wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+  let mainWindow = wm.getMostRecentWindow("navigator:browser");
>+  return tabConstructor(mainWindow.gBrowser.selectedTab);
>+});

Not saying that needs to be done right now, but we should be trying to push all XPCOM use out of the implementations of high-level modules into lower-level modules so we can flip a security switch at some point.  In this case, tab-browser seems like the right lower-level counterpart.

>+tabs.__defineSetter__("activeTab", function(val) {
>+  if (!val || !val.tagName == "tab")
>+    throw new Error("activeTab requires a valid tab");
>+  const wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+  let mainWindow = wm.getMostRecentWindow("navigator:browser");
>+  mainWindow.gBrowser.selectedTab = val;
>+});

The JEP doesn't say, but if we have an activeTab setter, it should take a Tab, not a XUL element.  I'm not against this setter, but it duplicates the functionality of Tab.activate(), and I'd prefer if there were only one way to activate a tab.

>+/**
>+ * tabs.open - open a URL in a new tab
>+ */
>+function open(url, options) {

Hmm, in the other APIs we always have either a single options param, or other params but no options.  I like splitting out url though, since it's by far the common option.  But the case of an undefined url and defined options looks a little weird:

  open(undefined, { ... });

It would be nice to sniff the first arg, and if it's a string assume it's a URL, and if it's an object assume it's an options object.  I'm not sure if that's in keeping with what Myk wants, though.

>+  options = apiUtils.validateOptions(options, {
>+    // TODO: take URL object instead of string (bug 564524)
>+    url: {
>+      is: ["string"],
>+      ok: function (v) !!v,
>+      msg: "The url parameter must have be a non-empty string."
>+    },
>+    window: {
>+      is: ["undefined", "null", "object"],
>+    },

I'd leave this as a TODO, chrome window story, yada yada.

>+    inNewWindow: {
>+      is: ["undefined", "null", "boolean"]
>+    },
>+    openInBackground: {
>+      is: ["undefined", "null", "boolean"]
>+    },

inBackground seems like a better name, to match inNewWindow.

>+    onLoad: {
>+      is: ["undefined", "null", "function"]
>+    }

Also, is there any reason "null" is an allowed type for all of these?  undefined and boolean are enough I think.

>+  if (!win || options.inNewWindow) {
>+    let urlStr = Cc["@mozilla.org/supports-string;1"]
>+                 .createInstance(Ci.nsISupportsString);
>+    urlStr.data = options.url;
>+    const ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>+             .getService(Ci.nsIWindowWatcher);
>+    win = ww.openWindow(null, "chrome://browser/content/browser.xul",
>+                        null, "chrome", urlStr);
>+    win.addEventListener("load", function(e) { 
>+      if (e.target && e.target.defaultView == win) {
>+        win.removeEventListener("load", arguments.callee, false);
>+        let tabEl = win.gBrowser.tabContainer.childNodes[0];
>+        let tabBrowser = win.gBrowser.getBrowserForTab(tabEl);
>+        tabBrowser.addEventListener("load", function(e) {
>+          tabBrowser.removeEventListener("load", arguments.callee, true);
>+          let tab = tabConstructor(tabEl);
>+          require("timer").setTimeout(function () {
>+            require("errors").catchAndLog(function(e) options.onLoad(e))(e);
>+          }, 10);
>+        }, true);
>+      }
>+    }, false);
>+  } else {
>+    win.focus();
>+    let tabEl = win.gBrowser.addTab(options.url);
>+    if (!options.openInBackground)
>+      win.gBrowser.selectedTab = tabEl;
>+    if (options.onLoad) {
>+      let tabBrowser = win.gBrowser.getBrowserForTab(tabEl);
>+      tabBrowser.addEventListener("load", function(e) {
>+        // remove event handler from addTab - don't want to be notified
>+        // for subsequent loads in same tab.
>+        tabBrowser.removeEventListener("load", arguments.callee, true);
>+        let tab = tabConstructor(tabEl);
>+        require("timer").setTimeout(function() {
>+          require("errors").catchAndLog(function(tab) options.onLoad(tab))(tab);
>+        }, 10);
>+      }, true);
>+    }
>+  }

It'd be great to split these two cases into their own private helper functions.  Oh actually, can't you use tab-browser here, for all of this?

>+// Tracker for all tabs across all windows
>+// This is tab-browser.TabTracker, but with
>+// support for additional events added.
>+function TabTracker(delegate) {

Is there any reason not to add all this functionality to tab-browser.TabTracker?

>+// Set up the event handlers for tabs singleton.
>+let eventCollections = {};
>+events.forEach(function(eventHandler) {
>+  // create a collection for each event
>+  collection.addCollectionProperty(eventCollections, eventHandler);
>+  // make tabs getter for each event, for adding via collection
>+  tabs.__defineGetter__(eventHandler, function() eventCollections[eventHandler]);
>+  // make tabs setter for each event, for adding via property assignment
>+  tabs.__defineSetter__(eventHandler, function(val) eventCollections[eventHandler].add(val));
>+});

Why don't you just do:

  collection.addCollectionProperty(tabs, eventHandler);

That sets everything up for you.

BTW, I wonder if we should update context-menu to expose Tab objects rather than content windows and content documents.

Comment 9

[Drew Willcoxon :adw]

(In reply to comment #8)
> >+  this.move = function(index, window) {
> 
> I'd leave this as a TODO along with the window getter.

Er, the window param I mean.

Comment 10

[Dietrich Ayala (:dietrich)]

> We don't have a good story right now for exposing chrome windows to high-level
> clients.  So for the time being, I think it would be better to leave this as a
> TODO, along with all other window-related parts of this API.

ok, makes sense. i keep hitting my head against the lack of a decent window api, so i'll file a bug for that for 0.6. commented out all of those features for now.

> Also, which ones of all these properties can be set and which only get should
> be noted in the end-user docs.

will do.

> I know the JEP specifies this, but there's a location getter, so why not a
> setter, instead of load()?

yeah seems redundant, fixed.

> removeTab() is a no-op if the tab is the only one, so maybe it would be useful
> to check if there are no tabs and if so throw an error?  Hmm, maybe not.  At
> least it should be documented.

doesn't seem like an error state. i'll doc it.

> The JEP specifies this, but there's no reason to attach everything to another
> singleton since the module itself is a singleton.  (I think we've made that
> Official Policy now...)

done. it did seem awkward.

> Not saying that needs to be done right now, but we should be trying to push all
> XPCOM use out of the implementations of high-level modules into lower-level
> modules so we can flip a security switch at some point.  In this case,
> tab-browser seems like the right lower-level counterpart.

fixed. added the api to tab-browser.

> The JEP doesn't say, but if we have an activeTab setter, it should take a Tab,
> not a XUL element.  I'm not against this setter, but it duplicates the
> functionality of Tab.activate(), and I'd prefer if there were only one way to
> activate a tab.

agreed, no need to duplicate. and to use it you'd need a tab object anyway.

> It would be nice to sniff the first arg, and if it's a string assume it's a
> URL, and if it's an object assume it's an options object.  I'm not sure if
> that's in keeping with what Myk wants, though.

i like that flexibility, so fixed. will get myk's thoughts in the next round.

> inBackground seems like a better name, to match inNewWindow.

agreed, fixed

> Also, is there any reason "null" is an allowed type for all of these? 
> undefined and boolean are enough I think.

fixed

> It'd be great to split these two cases into their own private helper functions.
>  Oh actually, can't you use tab-browser here, for all of this?

fixed and fixed

> Is there any reason not to add all this functionality to
> tab-browser.TabTracker?

i didn't want to add a bunch of event churn to every TabTracker instance. this listens to a lot more events, some of which are much higher volume.

> Why don't you just do:
> 
>   collection.addCollectionProperty(tabs, eventHandler);
> 
> That sets everything up for you.

hm, i thought i couldn't do that *and* define a setter, but it appears to work a-ok, so fixed.

> BTW, I wonder if we should update context-menu to expose Tab objects rather
> than content windows and content documents.

if it's high-level, then probably so.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: wip5

Comment 12

[Dietrich Ayala (:dietrich)]

filed bug 571449 for a high-level window api.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: wip6

* fixes for drew's comments
* add tabs iterator
* add paint event

i removed all the chrome window code. we can add back in later when needed.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: wip6.1

removed some accidentally included changes.

Comment 15

[Dietrich Ayala (:dietrich)]

Notes about the event handlers:

1. I've left out the 'event' parameter specified in the JEP, since listeners can access chrome objects through it. If there are significant use-cases that require access to more than is available through the 'tab' object, then we can explore protected 'event' objects at that point.

2. I didn't add the event handlers to the 'tab' objects yet. The next rev will have that and docs.

Comment 16

[Dietrich Ayala (:dietrich)]

Once I add events on to the tab objects, maybe we don't need the onLoad callback for tabs.open anymore. Or, that API could accept all the applicable events in it's options parameter?

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: wip7

* docs

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 450857 [details] [diff] [review]
wip7

This looks pretty good!  I noticed a couple issues, but both the code and the API seem pretty close.  Tab activation and the "activeTab" getter is the major trouble spot, see below...

(In reply to comment #8)

> It would be nice to sniff the first arg, and if it's a string assume it's a
> URL, and if it's an object assume it's an options object.  I'm not sure if
> that's in keeping with what Myk wants, though.

It's polymorphic, I love it!

However, the current implementation offers three possibilities:

  open(url)
  open(url, options)
  open(options)

For the sake of simplicity, I think there should be only two:

  open(url) <-- the simplest possible API for the common case
  open(options) <-- the simplest possible API for other cases


> BTW, I wonder if we should update context-menu to expose Tab objects rather
> than content windows and content documents.

Yes, that's a great idea!


That reminds me to note that both the Context Menu and the Tabs APIs will have to change to accommodate E10S.  We planned to do all the E10S stuff for 0.5, but that doesn't seem likely to happen at this point (and it's fine to land the Tabs API in the meantime).


>diff --git a/packages/jetpack-core/docs/tab-browser.md b/packages/jetpack-core/docs/tab-browser.md

>+<api name="activeTab">
>+@property {object}
>+
>+The currently focused tab.

I want to avoid the use of the word "focus" to describe active tabs, since "focus" has a specific, different meaning in HTML interfaces (and, indeed, in XUL ones).  I recommend we describe this instead as "the foreground tab".

However, read below for more on issues with this property.


>+    let tabs = require("tabs");
>+    for each (tab in tabs.tabs) {
>+      console.log(tab.title);
>+    }

For simplicity, the iterator should be the exported singleton itself (as in the latest iteration of the Selection API), making this:

    let tabs = require("tabs");
    for each (tab in tabs)
      console.log(tab.title);

See the end of this review for the code that'll do this.


>+@param url {string}
>+String URL to be opened in the new tab.
>+This is an optional parameter. Callers can choose to put the URL in the options
>+object instead.

Hmm, I wonder if this should be "content" instead of "url" and accept a string of HTML in addition to a URL, for consistency with other APIs that open frames and load content into them.


>+@prop [inNewWindow] {boolean}
>+If present and true, a new browser window will be opened and the URL will be
>+opened in the first tab in that window. This is an optional property.
>+
>+@prop [inBackground] {boolean}
>+If present and true, the new tab will be opened to the right of the active tab
>+and will not be focused. This is an optional property.

It's unclear whether and how these take into account user and default preferences, which can be complicated and tricky.  That's ok, it just suggests to me that we might find interesting edge cases that require us to refine their behavior in the future.


>+@prop [onLoad] {function}
>+A callback function that is called when the tab has loaded. This does not mean
>+that the URL content has loaded, only that the actual browser tab is fully visible to
>+the user. This is an optional property.

If this signifies the completion of the tabs.open request, then I suggest calling it onOpen rather than onLoad, since that's symmetric with the method name and more clearly identifies the event about which the consumer is being notified.  It's not clear to me what the use case is for this, though.  What can a consumer do when the tab has been opened that it can't do right after opening the tab (or after the content has been loaded)?


>+<api name="onClose">
>+@property {object}
>+Firedo when a tab is closed.

Nit: Firedo -> Fired


>+<api name="title">
>+@property {string}
>+The title of the page currently loaded in the tab.
>+This property is read only.

Nit: here and elsewhere, read only -> read-only 


>+<api name="activate">
>+@method 
>+Make the tab the active tab in it's containing window.
>+</api>

Nit: it's -> its

I agree with Drew that we should prefer having only one way to do this.  Unfortunately, figuring out what that way should be is tricky.

My first instinct is that we should allow setting activeTab, as that means there's one less property for developers to learn, and it's also consistent with the use of such getters/setters elsewhere in our API set (although at the moment I can't remember which ones follow this pattern).

However, the more I think about it, the more I think we're conflating two possible meanings of "active":

* the foreground tab in each individual window (this is the conventional
  meaning in APIs that only provide window-centric access to tabs);
* the foreground tab in the frontmost (top, most recent) browser window
  (this is a reasonable [mis]interpretation in APIs like ours that pool tabs
  across windows).

If we're willing to make "active" have the latter meaning (foreground tab in frontmost window), then it makes sense to have a tabs.activeTab setter that puts a tab into the same state represented by the tabs.activeTab getter (i.e. that makes the tab be the foreground tab in the frontmost window by raising both the tab and the window to the tops of their respective stacks).

But we're also planning to have a Windows API, and I think it would make more sense to distinguish between window and tab state, with the latter not implicitly including the former, so "active" for a tab always means only "foreground tab within its containing window" and not also "and in the frontmost window".

And in that case, tabs.activeTab doesn't work, neither as a setter nor as a getter, because there can be multiple "active" tabs, one per window.

So this is what I recommend:

* there is no tabs.activeTab;
* Tab has an isActive property whose boolean value specifies whether or not
  a tab is active in its containing window;
* Tab has an activate() method that activates the tab in its containing window;
* tabs.open opens tabs in the frontmost window.

Then, when we implement the Windows API, we figure out the right API for addons to raise a window, open a tab in a non-frontmost window, etc.

This does mean that developers won't be able to activate a tab and raise its window at the same time until we implement the Windows API.  I'm ok with that, especially as the common case is for users to have a single browser window, as long as we do implement a Windows API within a reasonable timeframe.

The other danger with this approach is the possibility that when we implement the Windows API we'll realize that isActive and activate() duplicate functionality better provided in the Window API.  I've done a bit of thinking about what a Windows API might look like, and I don't think that's the case, but in any case I don't think we can plan for such unknowns.  We need to bite the bullet and deal with such problems as they arise.


>+<api name="move">
>+@method 
>+Move the tab to the specified index it's containing window.

Nit: it's -> its


>+// move the active tab one position to the right
>+tabs.activeTab.move(tabs.activeTabe.index + 1);

Nit: activeTabe -> activeTab


>diff --git a/packages/jetpack-core/lib/tabs.js b/packages/jetpack-core/lib/tabs.js

>+  _safeDOMContentLoaded: function safeDOMContentLoaded(event) {
...
>+    let index = tabBrowser.getBrowserIndexForDocument(event.target /* content doc */);
...
>+    let index = this._tabs.indexOf(tab);

"index" is being redeclared here.


>+// Iterator for all tabs
>+function tabsIterator() {
>+  for (let i = 0; i < eventsTabTracker._tabs.length; i++)
>+    yield tabConstructor(eventsTabTracker._tabs[i]);
>+}
>+exports.tabs = tabsIterator();

Do this instead, so the iterator is on the singleton rather than a property of it:

exports.__iterator__ = function tabsIterator() {
  for (let i = 0; i < eventsTabTracker._tabs.length; i++)
    yield tabConstructor(eventsTabTracker._tabs[i]);
}

Comment 19

[Dietrich Ayala (:dietrich)]

(In reply to comment #18)
> That reminds me to note that both the Context Menu and the Tabs APIs will have
> to change to accommodate E10S.  We planned to do all the E10S stuff for 0.5,
> but that doesn't seem likely to happen at this point (and it's fine to land the
> Tabs API in the meantime).

Yeah I've been assuming e10s migration is moved to 0.6 as that platform code is not yet landed. I should've have brought that assumption up at last week's meeting to see if anyone shared it with me :(

> >+@param url {string}

> Hmm, I wonder if this should be "content" instead of "url" and accept a string
> of HTML in addition to a URL, for consistency with other APIs that open frames
> and load content into them.

Hm, we should probably start to move away from the "magic string", towards our eventual system of using content-frames for content and URL objects for all URLs.

How about I change it to a URL object, and we punt on accepting content here until content-frames are ready to go?

> It's unclear whether and how these take into account user and default
> preferences, which can be complicated and tricky.  That's ok, it just suggests
> to me that we might find interesting edge cases that require us to refine their
> behavior in the future.

Not going to try to tackle it this release. Filed bug 569799 a while back to ensure it doesn't get lost though.

> If this signifies the completion of the tabs.open request, then I suggest
> calling it onOpen rather than onLoad, since that's symmetric with the method
> name and more clearly identifies the event about which the consumer is being
> notified.  It's not clear to me what the use case is for this, though.  What
> can a consumer do when the tab has been opened that it can't do right after
> opening the tab (or after the content has been loaded)?

s/onLoad/onOpen/ is absolutely right, fixed.

A common use-case might be to open a tab, get access to the tab object via the onOpen listener, and then attach onReady listeners to do stuff when content is ready. This is far more clean IMO than the non-tab-specific approach which is to set up an onReady listener for all tabs, and then open a tab with a given URL and filter on all the onReady events for all tabs for your URL.

> So this is what I recommend:
> 
> * there is no tabs.activeTab;
> * Tab has an isActive property whose boolean value specifies whether or not
>   a tab is active in its containing window;
> * Tab has an activate() method that activates the tab in its containing window;
> * tabs.open opens tabs in the frontmost window.
> 
> Then, when we implement the Windows API, we figure out the right API for addons
> to raise a window, open a tab in a non-frontmost window, etc.
> 
> This does mean that developers won't be able to activate a tab and raise its
> window at the same time until we implement the Windows API.  I'm ok with that,
> especially as the common case is for users to have a single browser window, as
> long as we do implement a Windows API within a reasonable timeframe.

Hm, I disagree. I think that it's very important that we keep the tabs.activeTab getter. It's a very common case in extension development, and I don't want to reduce the ease with which the active tab and it's data can be accessed. I also think that tab.activate() (or tabs.activeTab setter) should both activate the tab and focus that tab's window. I posit that the use-case for bring a tab to the foreground in a background window is nearly non-existent.

Your proposal means that to find the active tab, authors have to iterate through all open tabs, collect all with isActive=true and then find some way to see which window is focused (which we don't offer yet). Hm, actually there's also no way to determine if a tab is in the focused window yet, and we removed tab.window, so afaict there's no way to make a background tab the foreground tab and be sure that it's in the focused window. This means there's basically no way to truly set or get the active tab with certainty that it's actually active.

As you said, multi-window scenarios are the less common case. I think that the common case shouldn't suffer for it. The tabs api should be tabs-oriented as a first priority, instead of making concessions to the uncommon case.

WRT to making a tab active via tabs.activeTab setter or tab.activate(), I think I favor an activeTab setter, due to your "one less property" comment. Reduction ftw.

Most everything  else is fixed in the latest patch. I haven't implemented an activeTab setter, since that's still up in the air. And per-tab event listening is close, should get it done today.

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: wip8

* fix myk's comments
* per-tab events

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: wip9

* per-tab events working, tested, documented

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: wip10

* add tab thumbnail support

Comment 23

[Dietrich Ayala (:dietrich)]

Attachment: wip11

* the activation changes are complete (i'd forgotten to remove tab.activate)
* the tests are more reliable

only thing left to do is tracking down a leak.

i'd like to automate testing of the contents of the tab thumbnails, but am not sure how to do that, so not blocking on it. manual tests looked fine.

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: wip12

* leaking mostly fixed. now only bringing the total tracked objects at the end of the test run up by 2, instead of hundreds. however, i noticed that my unload()s are being called after the memory tracking message is dumped, so i'm not sure it's actually leaking. i'll follow up with atul when he's online again.

* some code simplifications made while leak fixing.

Comment 25

[Dietrich Ayala (:dietrich)]

Attachment: wip13

* includes improvements to the widgets test

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 451489 [details] [diff] [review]
wip13

Overall, this looks great.  I'm seeing one strange test failure that only manifests when I run both tab-browser and tabs tests on Linux via |-F tab|, but I can't reproduce on Mac or when I run module-specific tests (and perhaps even when I run all tests, although the output of the test harness is unclear there).

So r=myk, I don't think what I'm seeing should block checkin, although we certainly do want to look into it further.

My only other comment is that we should audit the use of catchAndLog and figure out what the |this| object should be for the various callbacks this code calls, changing it if necessary.  But I'm ok with doing this in followup work.

Comment 27

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/labs/jetpack-sdk/rev/a3b6decaa648

Comment 28

[Dietrich Ayala (:dietrich)]

follow-ups:

bug 572458 - tab iterator test failure myk saw
bug 572456 - audit 'this' in event handlers

Comment 29

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".