560467 - Implement tab-browser.TabTracker

Status: RESOLVED FIXED

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

Description

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

Attachment: patch v1: adds whenTabClosed function

The selection module requires the tab-browser module to export a whenTabClosed function that consumers can use to get notified when a tab is closed, so the tab-browser should export such a function.

The patch I'm attaching is from Eric Jung (updated by me to apply to the trunk).  In bug 559641, comment 5, Eric says that the test in the patch doesn't work, however.

Atul: can you take a look and see if you can figure out what's going on?

Comment 1

[Atul Varma [:atul]]

Comment on attachment 440167 [details] [diff] [review]
patch v1: adds whenTabClosed function

>+exports.whenTabClosed = function whenTabClosed(callback) { 
>+  var cb = require("errors").catchAndLog(function eventHandler(event) {
>+    // Not sure all if all of the following checks are necessary
>+    if (event.target && event.target.linkedBrowser && event.target.linkedBrowser.contentWindow) {
>+      callback(event.target.linkedBrowser.contentWindow);
>+    }
>+  });

Could you please wrap the above lines to be less than 80 characters each? Thanks!

>+    onUntrack: function(tabBrowser) {
>+      tabBrowser.tabContainer.removeEventListener("TabClose", cb, false);
>+      /* Window is closing. Callback for each tab about to close */
>+      for (let i=0; i<tabBrowser.tabContainer.children.length; i++) {
>+        let tab = tabBrowser.tabContainer.children.item(i);
>+        callback(tab.linkedBrowser.contentWindow);
>+      }

Actually, the tab browser's window isn't necessarily closing here, it's just being untracked by the code--meaning that, e.g. the extension may be uninstalled but the window could still be open.  We might actually want to modify window-utils.WindowTracker and tab-browser.Tracker to pass along the reason for the untracking, if you need to distinguish between a browser window being closed vs. just being untracked.

An alternative is to add a 'close' event handler to every XUL window in your onTrack() or something, but it'd probably be more efficient (and more reusable) for us to just modify WindowTracker/tab-browser.Tracker.

>+exports.testWhenTabClosed = function(test) {
>+  if (!tabBrowser.isAppSupported())
>+    return;
>+
>+  let tracker = tabBrowser.whenContentLoaded(
>+    function(window) {
>+      browser.addTab("data:text/html," + html);
>+      browser.addTab("data:text/html," + html);
>+    });

Where's 'browser' defined? And 'html'?

>+  let count = 0;
>+  tabBrowser.whenTabClosed(
>+    function(window) {
>+      if (++count == 3) {
>+        browserWindow.close();
>+        timer.setTimeout(function() { tracker.unload();
>+                                      test.done();},
>+                         ARB_DELAY);
>+      }
>+    });

Note that you probably also want to get the return value from tabBrowser.whenTabClosed() and unload it before finishing up the test--otherwise it'll be hanging around for the rest of the test suite, and will still be called when tabs are closed.

Comment 2

[Atul Varma [:atul]]

Attachment: alternative fix that adds tab-browser.TabTracker() instead of whenTabClosed().

Comment 3

[Atul Varma [:atul]]

The patch I just added is an alternative solution to the same root problem. Eric's latest patch uses tab-browser.whenContentLoaded() to register selection listeners, but the problem with that is that it only gets called on DOMContentLoaded events--meaning that e.g. if the (restartless) extension is enabled while 10 tabs are opened, the selection of those tabs won't actually be accessible until the user manually reloads each of them.

TabTracker is intended to solve this problem the same way window-utils.WindowTracker and tab-browser.Tracker do:

  * Upon instantiation with a delegate object, it calls the delegate's onTrack() method for each existing tab in the application (spanning all browser windows).

  * It keeps calling the delegate's onTrack() as new tabs are opened, and onUntrack() as they're closed.

  * Finally, when the TabTracker's unload() method is called--usually when the containing extension is uninstalled or disabled--it calls the delegate's onUntrack() for each tab that is still open.

Both delegate methods are passed a single argument, which is the xul:tab element that is being tracked or untracked.

Comment 4

[Atul Varma [:atul]]

Eric asked me if I could take over this bug, so I am re-assigning it to myself.

Note that TabTracker will also be useful in the implementation of bug 549317.

Comment 5

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

Atul: is your patch ready for review?

Comment 6

[Atul Varma [:atul]]

Oh, sorry--I think I'd like feedback on it from one of you peer-folk, but it's not quite ready for full-blown review... Need to add documentation and more tests, at the very least!

Comment 7

[Atul Varma [:atul]]

Should I un-assign myself from this bug and let someone else either take over the patch or write a new one, as per the Jetpack meeting yesterday, or should I finish this and then let someone else build on it to finish bug 549317?

Comment 8

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

Since this is in flight, and selection needs it, it would be useful if you could finish it, at least to the extent needed to support selection.

Comment 9

[Atul Varma [:atul]]

Ok--just talked to Dietrich, who said he's able to take this off my plate to implement Tabs, so I'm unassigning myself from this. Let me know if you have any issues with this.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: wip1

- move tab-browser to tabs
- add validation to addTab
- add onLoad handler to addTab
- started conversion from using setTimeout in tests to deterministic testing via events

Comment 11

[Eric Jung [:ericjung]]

I'm trying to use this patch in bug 547092. It's not clear if I should write:

var tabTracker = require("tabs").TabTracker(SelectionListenerManager);
or
var tabTracker = require("tabs").Tracker(SelectionListenerManager);

SelectionListenerManager is:

var SelectionListenerManager = {

  // ... <snip/> ...

  onTrack: function(tab) {
    // ... <snip/> ...
  },

  onUntrack: function(tab) {
    // ... <snip/> ...
  }
};

With both usages (TabTracker and Tracker), I get a similar exception. The first usage yields:

<snip/>
  File "resource://jid0-rqmlwn85vsodjxxby1desrzlhiw-jetpack-core-lib/window-utils.js", line 81, in _regWindow
    this.delegate.onTrack(window);
  File "resource://jid0-rqmlwn85vsodjxxby1desrzlhiw-jetpack-core-lib/tabs.js", line 105, in onTrack
    this._delegate.onTrack(browser);
TypeError: this._delegate.onTrack is not a function

The second usage yields:

<snip/>
    this._windowTracker = new windowUtils.WindowTracker(this);
  File "resource://jid0-rqmlwn85vsodjxxby1desrzlhiw-jetpack-core-lib/window-utils.js", line 58, in WindowTracker
    this._regWindow(window);
  File "resource://jid0-rqmlwn85vsodjxxby1desrzlhiw-jetpack-core-lib/window-utils.js", line 81, in _regWindow
    this.delegate.onTrack(window);
TypeError: this.delegate.onTrack is not a function

What am I doing wrong?

Comment 12

[Dietrich Ayala (:dietrich)]

(In reply to comment #11)
> I'm trying to use this patch in bug 547092. It's not clear if I should write:
> 
> var tabTracker = require("tabs").TabTracker(SelectionListenerManager);
> or
> var tabTracker = require("tabs").Tracker(SelectionListenerManager);

The first is for tracking tabbrowsers, which I'm not sure needs to be publicly exposed.

The second is for tracking tabs, and is what I think you want.

> line 81, in _regWindow
>     this.delegate.onTrack(window);
> TypeError: this.delegate.onTrack is not a function
> 
> What am I doing wrong?

I'm not sure! I'm working on this code now, will try to reproduce. You're using 0.4 + the latest patch on this bug?

Comment 13

[Dietrich Ayala (:dietrich)]

Hm, the unit test uses:

var tabTracker = new tabs.TabTracker(delegate);

Maybe it doesn't properly support construction sans the new operator?

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> Maybe it doesn't properly support construction sans the new operator?

Confirmed. "new" works. no "new" gets Eric's error.

Convention for "high level" modules is to not use "new", so I'll fix this.

Comment 15

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

(In reply to comment #14)
> Convention for "high level" modules is to not use "new", so I'll fix this.

Right!  But note that it should still be possible to use "new" with high level modules.  It just shouldn't be necessary.

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: wip2

ok, thanks for clarifying Myk. this patch supports both for the TabTracker constructor.

Comment 17

[Dietrich Ayala (:dietrich)]

For all y'all: Is there a use-case for exposing the tabbrowser tracker now that TabTracker exists? It seems like the window tracker + tab tracker would be enough, and less confusing, for most cases I can think of.

Comment 18

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

(In reply to comment #17)
> For all y'all: Is there a use-case for exposing the tabbrowser tracker now that
> TabTracker exists? It seems like the window tracker + tab tracker would be
> enough, and less confusing, for most cases I can think of.

That makes sense to me, too.

Comment 19

[Atul Varma [:atul]]

The only use I had for the tabbrowser tracker was for implementing tabbrowser.whenContentLoaded(), which is already exposed, so I'm fine with un-exposing the tabbrowser tracker.

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #19)
> The only use I had for the tabbrowser tracker was for implementing
> tabbrowser.whenContentLoaded(), which is already exposed, so I'm fine with
> un-exposing the tabbrowser tracker.

Per the JEP, this will be converted to tabs.onDOMReady (which i think should be tabs.onReady actually). So even that is going to change, but will still be available.

Comment 21

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

(In reply to comment #20)
> (In reply to comment #19)
> > The only use I had for the tabbrowser tracker was for implementing
> > tabbrowser.whenContentLoaded(), which is already exposed, so I'm fine with
> > un-exposing the tabbrowser tracker.
> 
> Per the JEP, this will be converted to tabs.onDOMReady (which i think should be
> tabs.onReady actually).

Yes, onReady!

Comment 22

[Eric Jung [:ericjung]]

Sorry for being dense, but how do i get the window of the object passed to the onTrack() and onUntrack() callbacks?

Comment 23

[Eric Jung [:ericjung]]

(In reply to comment #22)
> Sorry for being dense, but how do i get the window of the object passed to the
> onTrack() and onUntrack() callbacks?

obj.ownerDocuement.defaultView

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: v1

ready for review.

- includes Atul's tabtracker changes
- made app detection and handling consistent w/ other modules
- updates tests to be event-driven instead of settimeouts
- added a few waits in the tests to allow window open/close breathing room, as well as in addTab when opening a new window (we need to figure this out globally)
- added input validation to addTab
- Tracker and TabTracker use the public ctor api

Comment 25

[Dietrich Ayala (:dietrich)]

- the patch also includes a new onLoad option to addTab, which is a callback function that's called when the tab loads. the XUL element for the tab is passed to the callback.

Comment 26

[Dietrich Ayala (:dietrich)]

Attachment: v1 + docs

Same patch, now with docs.

Comment 27

[Atul Varma [:atul]]

Comment on attachment 448121 [details] [diff] [review]
v1 + docs

>+<api name="Tracker">
>+@method
>+Register a delegate object to be notified when tabbrowsers are created
>+and destroyed.

I should have actually written these Tracker docs myself, and I guess I'll
just change it after your commit, but one of the confusing things
about Tracker (which in retrospect I'd probably change the design of)
is that it's really something for bookkeeping information about all
tabbrowsers in an app, rather than just ones that come into existence
or go out of existence while the Tracker object is alive.

I originally wrote it to relieve folks of having to remember to both
iterate through all already-existing tabs using one API call *and*
then create an observer to keep track of all new tabs that are made,
but it ended up just confusing people. This is kinda due to it being
possible for a tab to have existed before an addon was installed, and
to outlive an addon. Anyways, I will try to improve these docs to
warn developers of the complexities involved.

Regarding the API, I recall one of the concerns of Ubiquity developers
being that they wanted page-opening to take the user's preferences
into account, so that e.g. if they had configured firefox to
"open links in a new window" rather than a new tab, the api call
would do that. I'm not sure if the Tabs API accounts for this at
all but I figured I'd mention it.

The code itself, insofar as the Jetpack Platform is concerned, looks good--I'd request an additional review from Drew or Myk or someone who knows XUL better to double-check that work if you think it'd help, though.

Comment 28

[Eric Jung [:ericjung]]

(In reply to comment #27)
> I originally wrote it to relieve folks of having to remember to both
> iterate through all already-existing tabs using one API call *and*
> then create an observer to keep track of all new tabs that are made,

I think this is a noble idea and shouldn't be let go. The two cases you describe are required by any addon that needs to watch tabs. Naive tab watchers will surely forget to account for existing tabs, perhaps resulting in an anti-pattern (I admit I'm probably using that phrase inaccurately, but you get my point).

Comment 29

[Dietrich Ayala (:dietrich)]

> I should have actually written these Tracker docs myself, and I guess I'll
> just change it after your commit, but one of the confusing things
> about Tracker (which in retrospect I'd probably change the design of)
> is that it's really something for bookkeeping information about all
> tabbrowsers in an app, rather than just ones that come into existence
> or go out of existence while the Tracker object is alive.

I did note that behavior wrt to previously-existing items in both tabbrowser tracker and tab tracker docs, after the delegate apidocs.

And for the record, I'm fairly confident that you'd redesign every API anytime you look sideways at it, and find ways to improve it each time ;)

> Regarding the API, I recall one of the concerns of Ubiquity developers
> being that they wanted page-opening to take the user's preferences
> into account

Agreed, that's ideal but I wouldn't block on it at this point. I'll file a followup bug to ensure this happens before 1.0.

Comment 30

[Dietrich Ayala (:dietrich)]

(In reply to comment #29)
> Agreed, that's ideal but I wouldn't block on it at this point. I'll file a
> followup bug to ensure this happens before 1.0.

bug 569799

Comment 31

[Dietrich Ayala (:dietrich)]

I realized that my default browser had been switched back to 3.6 (thanks ubuntu :P), and when running these tests under trunk, a new browser window couldn't be opened properly. The problem affects the widget tests too, and maybe others. Addressing the issue bug 570240.

Comment 32

[Dietrich Ayala (:dietrich)]

Not going to block on that. Fixed locally in this patch for now and checked in. Will update to use the new window-utils api when that lands.

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

Comment 33

[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".