Closed
Bug 584532
Opened 14 years ago
Closed 14 years ago
Land Tabs module to get Tab* events from any window
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 4.0b4
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
Attachments
(1 file, 2 obsolete files)
5.79 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Reviewing/landing bug 580952 in mozilla-central separate from the rest of tabcandy.
Assignee | ||
Comment 1•14 years ago
|
||
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..
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?
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.
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?
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.
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.
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();?
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.
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.
Comment 11•14 years ago
|
||
You could always use Tabs rather than self. Also /generally/ using "T" instead of "this" seemed nasty to me.
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?
Assignee | ||
Updated•14 years ago
|
Attachment #462965 -
Attachment is obsolete: true
Attachment #462965 -
Flags: review?(gavin.sharp)
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).
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #463682 -
Flags: review?(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.
Attachment #463682 -
Flags: review?(dao) → review-
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.
Attachment #463682 -
Attachment is obsolete: true
Attachment #464237 -
Flags: review?(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.
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
Attachment #464237 -
Flags: review?(dao) → review+
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
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.
Comment 21•14 years ago
|
||
(In reply to comment #19) > > function observer(subject, topic, data) { > Hey, that's neat! Thank bug 538920! :)
You need to log in
before you can comment on or make changes to this bug.
Description
•