Ed Lee :Mardak Assignee
	Description • 14 years ago

Reviewing/landing bug 580952 in mozilla-central separate from the rest of tabcandy.

	Ed Lee :Mardak Assignee
	Comment 2 • 14 years ago

I was thinking about changing the callback names to just match the original events, so no aliasing of TabAttrModified to onChange.

Not sure if it should be Tabs.onSelect vs Tabs.Select vs Tabs.TabSelect though..

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 14 years ago

We shouldn't use "browsers" to refer to browser DOM windows. "BrowserWindows", perhaps. Maybe it would be a good idea to pass the window to the callbacks as well? I guess you can get it otherwise using ownerDocument.defaultView... Otherwise this looks good to me, though I haven't looked into users of it (tabcandy) very much. Are there other potential users of it in existing frontend code?

	Ed Lee :Mardak Assignee
	Comment 4 • 14 years ago

(In reply to comment #3)
> Maybe it would be a good idea to pass the window to the callbacks as well?
The current tabcandy codes just that to filter out non-current-window events:

Tabs.onSelect(function() {
  if (this.ownerDocument.defaultView != gWindow)
    return;
...
});

Regarding API, not sure if it should be Tabs.onSelect(callback, thisObj) and call the callback with thisObj as this and the tab as the first argument. This would make the unbind code a bit more complicated though.

Also, any preference on the public api naming? Especially for TabAttrModified?

(In reply to comment #3)
> Are there other potential users of it in existing frontend code?
This api is for code that doesn't live/need to live in each browser window instance. I believe this is mostly for other code that lives in js modules, which exist outside of any browser instance.

	Dão Gottwald [:dao]
	Comment 5 • 14 years ago

(In reply to comment #4)
> This api is for code that doesn't live/need to live in each browser window
> instance. I believe this is mostly for other code that lives in js modules,
> which exist outside of any browser instance.

If so, why the (this.ownerDocument.defaultView != gWindow) check from above?

	Ed Lee :Mardak Assignee
	Comment 6 • 14 years ago

It's in preparation for bug 578512 where part of the fix to the bug would be to split the code into a "model" service that lives independent of a browser window.

	Ed Lee :Mardak Assignee
	Comment 7 • 14 years ago

Er. The tab events from all browsers is in preparation for bug 578512. The reason why we currently have the != gWindow check is to avoid breaking things right now such as trying to do gBrowser.removeTab(tab) where the tab doesn't exist in this current gBrowser.

	Dão Gottwald [:dao]
	Comment 8 • 14 years ago

Well then landing a module to track this across windows seems premature at least.

Also the let Tabs = let (T = {...}) T.init(); pattern looks pretty ugly. Is it somehow better than let Tabs = {...}; Tabs.init();?

	Dão Gottwald [:dao]
	Comment 9 • 14 years ago

Comment on attachment 462965 [details] [diff] [review]
v1

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsISupports
>+  //////////////////////////////////////////////////////////////////////////////
>+
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),

There's no need for this.

	Ed Lee :Mardak Assignee
	Comment 10 • 14 years ago

(In reply to comment #8)
> Well then landing a module to track this across windows seems premature at
> least.
It was also to replace the existing tab wrapping code that was inherited from the old jetpack code. But yeah, it was somewhat premature as we're not entirely leveraging the benefit of the new module.

> Also the let Tabs = let (T = {...}) T.init(); pattern looks pretty ugly. Is it
> somehow better than let Tabs = {...}; Tabs.init();?
T acts as "this" so that there's no need to do stuff like "let self = this;" all over the place when writing callback code.

	Dão Gottwald [:dao]
	Comment 11 • 14 years ago

You could always use Tabs rather than self.
Also /generally/ using "T" instead of "this" seemed nasty to me.

	(no longer active)
	Comment 12 • 14 years ago

Gavin will be away for a few days, so maybe you'd want to try to ask dolske for review?

	Ed Lee :Mardak Assignee
	Comment 13 • 14 years ago

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/0e2c2d0a0721
Prepare for bug 584532 by renaming Tabs and T -> AllTabs, allTabs -> tabs, browsers -> browserWindows, and removing QI(nsIObserver).

	Dão Gottwald [:dao]
	Comment 15 • 14 years ago

Comment on attachment 463682 [details] [diff] [review]
v2

>+    return Array.concat.apply({}, browserWindows.map(function(browserWindow) {
>+      return Array.slice(browserWindow.gBrowser.tabs);
>+    }));

s/{}/null/

>+   * @usage AllTabs.onSelect(handleSelect);
>+   *        AllTabs.onSelect.unbind(handleSelect);

This API looks pretty strange, the second line in particular. There's going to be a native Function.prototype.bind, which your unbind apparently has nothing to do with.

How about AllTabs.(un)register("select", handleSelect);?

>+   *        Callback that gets called with the tab being closed as "this" and
>+   *        the event as the first argument.

The first part seems unnecessary. What happens if the caller passes this.handleClose.bind(this)?

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// Private
>+  //////////////////////////////////////////////////////////////////////////////

Don't export this stuff, just make it functions and variables in the global scope.

I think we should actually land this in tabview/modules/. We can always move it later when the module is used, has proven useful and the API had a chance to mature.

	Ed Lee :Mardak Assignee
	Comment 16 • 14 years ago

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/ce0c948f1554
Move private variables/methods into local declarations not exported by AllTabs and change the interface to .register("change") instead of .onChange() for bug 584532.

	Dão Gottwald [:dao]
	Comment 17 • 14 years ago

(In reply to comment #16)
> http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/ce0c948f1554
> Move private variables/methods into local declarations not exported by AllTabs
> and change the interface to .register("change") instead of .onChange() for bug
> 584532.

--- a/browser/base/content/tabview/ui.js
+++ b/browser/base/content/tabview/ui.js

-            if (this && this.tabItem)
-              this.tabItem.setZoomPrep(false);
+            if (tab && tab.tabItem)
+              tab.tabItem.setZoomPrep(false);

Not really related to this bug, but it looks like tabItem should have a clearer name (tabviewItem?) and start with an underscore, since this is non-tabbrowser.xml code setting a custom property on a tabbrowser node.

	Dão Gottwald [:dao]
	Comment 18 • 14 years ago

Comment on attachment 464237 [details] [diff] [review]
v3

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+const Cr = Components.results;

Remove all but Cu.

>+  register: function register(eventName, callback) {
>+    // Either create the first entry or add additional callbacks
>+    let listeners = eventListeners[eventName];
>+    if (listeners == null)

>+  unregister: function unregister(eventName, callback) {
>+    // Nothing to remove for this event
>+    let listeners = eventListeners[eventName];
>+    if (listeners == null)

>+      // Make sure we've gotten listeners before trying to call
>+      let listeners = eventListeners[eventName];
>+      if (listeners == null)

if (!listeners)

>+      let tab = event.originalTarget;

event.target

>+      // Make a copy of the listeners, so it can't change as we call back
>+      listeners.slice().forEach(function(callback) {
>+        try {
>+          callback(tab, event);
>+        }
>+        // Ignore failures from the callback
>+        catch(ex) {}

catch (ex) {
  Cu.reportError(ex);
}

>+let observer = {
>+  observe: function observe(subject, topic, data) {
>+    switch (topic) {
>+      case "domwindowopened":
>+        subject.addEventListener("load", function() {
>+          subject.removeEventListener("load", arguments.callee, false);
>+
>+          // Now that the window has loaded, only register on browser windows
>+          let doc = subject.document.documentElement;
>+          if (doc.getAttribute("windowtype") == "navigator:browser")
>+            registerBrowserWindow(subject);
>+        }, false);
>+        break;
>+    }
>+  }
>+};

flatten this, i.e.:

function observer(subject, topic, data) {
  ...
}

instead of:

let observer = {
  observe: function observe(subject, topic, data) {
    ...
  }
};

r=me with these changes

	Ed Lee :Mardak Assignee
	Comment 19 • 14 years ago

(In reply to comment #18)
> function observer(subject, topic, data) {
> instead of:
> let observer = {
>   observe: function observe(subject, topic, data) {
Hey, that's neat!

http://hg.mozilla.org/mozilla-central/rev/b369f6e53db9

Landed with revision history:
http://hg.mozilla.org/mozilla-central/log/b369f6e53db9/browser/base/content/tabview/modules/AllTabs.jsm

	Ed Lee :Mardak Assignee
	Comment 20 • 14 years ago

(In reply to comment #18)
> Comment on attachment 464237 [details] [diff] [review]
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/d06bed205b76
Address review comments from bug 584532 comment 18: !listeners, event.target, observers function.

	Justin Dolske [:Dolske]
	Comment 21 • 14 years ago

(In reply to comment #19)
> > function observer(subject, topic, data) {
> Hey, that's neat!

Thank bug 538920! :)