574217 - Land TabCandy on trunk

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Comment 1

[Ian Gilman [:iangilman]]

Looks like the thing to do is to make our own branch of mozilla-central, either replacing the current tabcandy branch, or as a new branch.

If we stick it in the tabcandy branch, can we make mozilla-central a subdirectory, so we can continue to store things like our documentation outside of the mozilla tree?

Comment 2

[Ian Gilman [:iangilman]]

If making it a sub directory in the current tabcandy repo would work (i.e. wouldn't cause merge headaches later when going to trunk), I say we do that. Otherwise I think we should start a fresh new branch, so we can keep those other items in the tabcandy repo. 

I'm afraid I just don't know enough about mercurial branching to know what's possible, however. Anyone here know? If not, who should we ask?

Once we've figured this out, I figure we can get started immediately on the integration process.

Comment 3

[Ed Lee :Mardak]

I'll put up a user repository where we can hack on integrating tabcandy with mozilla-central. It'll clone mozilla-central initially and once we have the filemap, I'll convert the tabcandy repo into it.

Comment 4

[Ian Gilman [:iangilman]]

Sounds awesome, Edward! Let me know when you've got it in place.

Comment 5

[imradyurrad]

What's the link to get the addon?

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to comment #5)
> What's the link to get the addon?
You can get the alpha version from http://bit.ly/aPAO1V

Comment 7

[Chris Barry [ChrisW_B]]

May I ask why this is only for Mac OS X? Is there another bug (that I can't find) for Windows?

Comment 8

[Aza Raskin [:aza]]

(In reply to comment #7)
> May I ask why this is only for Mac OS X? Is there another bug (that I can't
> find) for Windows?

This is for all platforms, the bug metadata has been fixed. Thanks for pointing that out!

Comment 9

[Ed Lee :Mardak]

Attachment: super.diff -U16

Dao, here's the diff of tabcandy-central against mozilla-central (ab337d46a681).

The TabCandy team would like to know what needs to be fixed before it can land on mozilla-central and what needs to be fixed before Firefox 4 (and perhaps before Beta 2).

We already have a number of bugs guessing at what you might comment on such as coding style bug 577968 or perhaps even switching over to js modules bug 577322. So a definitive list of what needs to be be fixed now before it can land on mozilla-central would be much more useful than guessing.

Comment 10

[Ed Lee :Mardak]

Attachment: super.diff -U16

Mitcho cleaned up a bunch of the code removing iQ.each uses and more!

Comment 11

[Ed Lee :Mardak]

Attachment: no-img.diff

Pretty much all of the tabcandy logic lives under browser/base/content/tabcandy in either "app" or "core". "app" mostly handles what get shown and interacted with while "core" generally provides various helpers/utils.

app: drag groups infoitems items storage tabitems trench ui
core: iq mirror stacktrace tabs utils

The code is already broken up into various logical groups with those files, so you can look at each individually with the main exception of "ui", which puts all the pieces together.

Comment 12

[Ed Lee :Mardak]

Attachment: no-new.diff

This patch just has the changes to existing files and none of the new files added by tabcandy. You can view them in the tabcandy-central repo:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/file/d632c190b125/browser/base/content/tabcandy

Other than the files under browser/base/content/tabcandy, there's a few other files:

browser/base/content/browser-tabcandy.js
browser/themes/gnomestripe/browser/tabcandy/platform.css
browser/themes/pinstripe/browser/tabcandy/platform.css
browser/themes/winstripe/browser/tabcandy/platform.css

Comment 13

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

Starting off with the very substantial comments... :) "TabCandy" as an internal name seems potentially confusing. I think something more generic like "TabView" would be better.

I think a "visibleTabs" getter on tabbrowser that just returns TabCandy.getVisibleTabs() would make things a bit cleaner (fewer touch points between tabcandy<->tabbrowser).

I'm kind of surprised that more places don't want to only deal with visible tabs - did you do an audit of other tabbrowser.tabs users?

I haven't reviewed any of the TabCandy code itself yet, I plan on doing that soon.

Comment 14

[Aza Raskin [:aza]]

For people reviewing, we've started a "Reviewers Guide" which will hopefully be helpful.

http://etherpad.mozilla.org:9000/tabcandy-reviewers-guide

Comment 15

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

My initial impression from quickly looking through the tabcandy-specific files is that there is a lot of "utility code" that just re-wraps existing Firefox elements/APIs into a different model. E.g.:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/file/d632c190b125/browser/base/content/tabcandy/app/ui.js#l49
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/file/d632c190b125/browser/base/content/tabcandy/core/utils.js#l452

The different API may be simpler to use, but I don't think we want to be introducing it piecemeal - "tabBar", "navBar", "activeTab" and "getCurrentWindow" are elements that Firefox code already has properties for (depending on the context), so we should just use those directly.

Comment 16

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

There seems to be a lot of pollution of the global scope (assigning properties to "window"). We need to avoid that for the chrome window scope, but I guess most (all?) of this code runs in the context of the tabcandy iframe rather than in the chrome window?

Comment 17

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

I think we should get separate bugs filed for landing/reviewing iq.js (and perhaps other easily-isolated utility modules), to have this bug focus on the tabcandy-specific code in particular.

Comment 18

[Ian Gilman [:iangilman]]

(In reply to comment #15)
> My initial impression from quickly looking through the tabcandy-specific files
> is that there is a lot of "utility code" that just re-wraps existing Firefox
> elements/APIs into a different model. 

Good point. We're now addressing that with bug 580878.

Comment 19

[Ian Gilman [:iangilman]]

(In reply to comment #16)
> There seems to be a lot of pollution of the global scope (assigning properties
> to "window"). We need to avoid that for the chrome window scope, but I guess
> most (all?) of this code runs in the context of the tabcandy iframe rather 
> than in the chrome window?

Yes, all that code is inside the TabCandy iFrame. Looks like we will be adding some JS modules (such as in bug 580952) that'll live outside of the iFrame, so we should make sure those don't add any unnecessary pollution. 

(In reply to comment #17)
> I think we should get separate bugs filed for landing/reviewing iq.js (and
> perhaps other easily-isolated utility modules), to have this bug focus on the
> tabcandy-specific code in particular.

iq.js will only be used inside the TabCandy iFrame, for whatever that's worth. At any rate, sounds fair. We've got a bit of clean-up we'd like to do, but then we'll submit iq.js (and possibly utils.js).

Comment 20

[Tan Wei Lin Jason [:Ryuji]]

Tab Candy have made the Title bar and the tab bar black in colour. Tab Candy also removed the Minimize, Restore Down/Maximize and Close Buttons just like what a persona did. Should I report this issue as a separate bug(s) or has this bug(s) been filed?

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #20)
> Tab Candy have made the Title bar and the tab bar black in colour. Tab Candy
> also removed the Minimize, Restore Down/Maximize and Close Buttons just like
> what a persona did. Should I report this issue as a separate bug(s) or has this
> bug(s) been filed?

Yes, please file a separate bug. Thanks.

Comment 22

[Glen A.]

(In reply to comment #13)
> Starting off with the very substantial comments... :) "TabCandy" as an internal
> name seems potentially confusing. I think something more generic like "TabView"
> would be better.
I don't like it as an "external" name either ... is this really what it's going to be called? Seems very "informal".

What about Tab Groups / Sets / Workspaces?

Poll? :-)

Comment 23

[dark_skeleton]

Personally I like the "TabCandy" name very much :)

Comment 24

[d]

I think having an "informal" name is a great way of showing that we are closer to the user than other larger companies in the browser market. Choosing a boring name would be a mistake, in my opinion.

Comment 25

[Pierre]

TabCandy is the perfect name.
It manages tabs, it is eye-candy, it is TabCandy !

Comment 26

[Tan Wei Lin Jason [:Ryuji]]

Personally, I think TabCandy sound nice. Candy is always link with sweet stuff, and what more this new feature is supposed to be a excellent and sweet feature to the users giving it a name associated to candy certainly helps.

Comment 27

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

Please, let's not let this bug devolve into a thread about naming.

Comment 28

[Aza Raskin [:aza]]

Json, Pierre, Magne, Dark Skeleton, and Glenn. I love the enthusiasm! However, as Gavin points out this isn't the place to have the discussion. Let's have the discussion instead here: http://feedback.mozillalabs.com/forums/56804-tabcandy

Comment 29

[Pierre]

(In reply to comment #28)
> Json, Pierre, Magne, Dark Skeleton, and Glenn. I love the enthusiasm! However,
> as Gavin points out this isn't the place to have the discussion. Let's have the
> discussion instead here: http://feedback.mozillalabs.com/forums/56804-tabcandy

Gavin, Aza, thanks for pointing out the inappropriate place for this discussion about naming. I would love being able to delete my comment (#25) to remove unnecessary "noise" in the current thread but it seems I can't. Let the admins feel free to delete it if they can. 

As suggested by Aza, I've just opened a thread here : http://feedback.mozillalabs.com/forums/56804-tabcandy/suggestions/938969-keep-the-name-tabcandy-?ref=title

Comment 30

[Ed Lee :Mardak]

Attachment: super.diff

Doesn't include iq/utils/tabs js files nor images nor the tabbrowser changes relating to showing/hiding tabs. Also profile.js is removed as that will be gone.

Comment 31

[Dietrich Ayala (:dietrich)]

This is about making TabCandy a Firefox feature, so after consulting #tabcandy, switching to Firefox for better tracking.

Comment 32

[Tan Wei Lin Jason [:Ryuji]]

Which beta will tab candy land?

Comment 33

[Michael Yoshitaka Erlewine [:mitcho]]

A new bug has been created for "The TabCandy 1.0 Experience" (bug 585689). All bugs which are part of the full Tab Candy spec, but do not block landing, have been moved there. New Tab Candy blockers should be added to that bug.

Comment 34

[Ian Gilman [:iangilman]]

(In reply to comment #32)
> Which beta will tab candy land?

We're shooting for beta 4...

Comment 35

[Ed Lee :Mardak]

Attachment: super diff 383716aa20b6

Comment 36

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

OK, don't freak out :) I am marking this blocking- for now, because at the time of the nomination, I cannot say for sure that I would hold the release for this feature.

Once we know that we can contain it safely, I may change the blocking notice. For now it requires explicit approval to get into the tree.

Comment 37

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

Comment on attachment 464501 [details] [diff] [review]
super diff 383716aa20b6

Perhaps even in advance of a full review, some high level feedback might be helpful?

Also: Ed, can you maybe (since this is a large patch) help orient Dolske to the various bits to get him up to speed quicker?

Comment 38

[Ed Lee :Mardak]

Attachment: super diff 09bc63c0e629

Comment 39

[Aza Raskin [:aza]]

(In reply to comment #37)
> Comment on attachment 464501 [details] [diff] [review]
> super diff 383716aa20b6
> 
> Perhaps even in advance of a full review, some high level feedback might be
> helpful?
> 
> Also: Ed, can you maybe (since this is a large patch) help orient Dolske to the
> various bits to get him up to speed quicker?

While not as good as an Ed intro, we do have http://etherpad.mozilla.org:9000/tabcandy-reviewers-guide which is a quick introduction and high-level overview.

Comment 40

[Ed Lee :Mardak]

Attachment: super diff -U8 04c3ca395c10

Was some m-c merge failure fixed now.

Comment 41

[Ed Lee :Mardak]

Attachment: super diff -U8 04c3ca395c10

Comment 42

[Justin Dolske [:Dolske]]

Comment on attachment 464646 [details] [diff] [review]
super diff 09bc63c0e629

Review Comments, Batch 1.
(Skipped browser-tabview.js, everything after tabbrowser.xml)


>+++ b/browser/base/Makefile.in
...
>+
>+libs::
>+	$(PYTHON) $(topsrcdir)/config/nsinstall.py $(srcdir)/content/tabview/modules/* $(FINAL_TARGET)/modules/tabview

Pretty sure $(NSINSTALL) is all you need here (it'll point to nsinstall.py).


>+++ b/browser/base/content/browser-menubar.inc
...
>+                <menuitem id="menu_tabview"
>+                          label="&showTabView.label;"
>+                          command="Browser:ToggleTabView"/>

Need an accesskey set here.


>+++ b/browser/base/content/browser.xul

>+      <menu id="context_tabViewMenu" class="menu-iconic" label="&moveTabTo.label;..." 
>+            accesskey="&moveTabTo.accesskey;">

The "..." in the label needs to be in the DTD string. Also make sure you use an actual ellipsis, not 3 periods. Here's one for free: …

>+        <menupopup id="context_tabViewMenuPopup" 
>+                   onpopupshowing="if (event.target == this) TabView.updateContextMenu(TabContextMenu.contextTab, this);">

Hmm, I don't think you need the event.target check here. I think this is only present on some of the other menupopups here because they, in turn, have submenu popups, so those submenu events bubble up through the DOM.

>+</vbox>
>+</deck>

Maybe add a comment here noting that the <iframe id="tab-view"> is dynamically appended as the 2nd child?


>+++ b/browser/base/content/tabbrowser.xml
...
>       <method name="updateTitlebar">
>         <body>
>           <![CDATA[
>-            this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
>+            if (TabView.isVisible()) {
>+              // ToDo: this will be removed when we gain ability to draw o the menu bar. 

Make sure a bug is on file and reference it here?

 
>+            var uniqueId = "panel" + Date.now() + position;

Err, |uniqueID| is already computed a few lines up from here (too much cut'n'paste).

(Filing a bug about this existing code, seems like it could fail if you open multiple tabs very quickly).

>+            this.mPanelContainer.lastChild.id = uniqueId;
>+            t.linkedPanel = uniqueId;
>+            t.linkedBrowser = b;
>+            t._tPos = position;
>+            if (t.previousSibling && t.previousSibling.selected)
>+              t.setAttribute("afterselected", true);

Actually, is any of this code supposed to be here? Almost all those lines already exist. Setting .lastChild.id is new, but seems very fragile. Better to pass, say, pass the ID to the thing appending the child (so it can set it), or look for a specific child to set the ID on?

Comment 43

[Justin Dolske [:Dolske]]

Comment on attachment 464646 [details] [diff] [review]
super diff 09bc63c0e629

(oops, clobbered the obsolete flag because Bugzilla sucks)

Comment 44

[Ed Lee :Mardak]

(In reply to comment #42)
> >+	$(PYTHON) $(topsrcdir)/config/nsinstall.py $(srcdir)/content/tabview/modules/* $(FINAL_TARGET)/modules/tabview
> Pretty sure $(NSINSTALL) is all you need here (it'll point to nsinstall.py).
nsinstall.py is only used on windows (?), but the explicit call to nsinstall.py is for the recursive behavior. It might not be as necessary for tabcandy as I think we'll have a pretty flat directory.

> >+            var uniqueId = "panel" + Date.now() + position;
Sorry, I did a bad m-c conflict resolution, but I've fixed it in the latest version.

Comment 45

[Justin Dolske [:Dolske]]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

>+++ b/browser/themes/winstripe/browser/tabview/platform.css

Nits: this file (just winstripe) has some awkwardly formatted CSS...

>+  position:absolute;

Space between "property:" and "value".

>+.newTabButtonAlt{

Space before {

>+.newTabButtonAlt>span{

Space between selectors (".newTabButtonAlt > span {")

>+  background-color: #D9E7FC;

Also, where are the random hex colors coming from? From a visual design tweaked in Photoshop, or are these intended to pick up system colors?

If the latter, they should be using the various system color names we have (and tested to make sure things look plausible on oddball system themes)... This could be done in a followup bug, I suppose.

Comment 46

[Aza Raskin [:aza]]

(In reply to comment #45)

> Also, where are the random hex colors coming from? From a visual design tweaked
> in Photoshop, or are these intended to pick up system colors?

A lot of these are coming from visual designs tweaked in Photoshop. For example the blue that we use on Windows machines comes from a value that Faaborg and I arrived at by looking through the system for colors that would work across Vista and 7 (until we have the ability to do glass in the background). For the Mac, that gradient comes from a mockup by Shorlander.

In the future, I know that the Firefox UX team is designing a look-and-feel for what the "backbone" of the browser should look like (i.e., for doing things like Sync sign-up). There isn't a patch for that yet, but when it does we can probably reuse their css values for a number of things.

Comment 47

[Michael Yoshitaka Erlewine [:mitcho]]

Just committed a number of fixes suggested by Dolske:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/2d9247f7bd95

(In reply to comment #42)
> Review Comments, Batch 1.
> (Skipped browser-tabview.js, everything after tabbrowser.xml)
> >+++ b/browser/base/content/browser-menubar.inc
> ...
> >+                <menuitem id="menu_tabview"
> >+                          label="&showTabView.label;"
> >+                          command="Browser:ToggleTabView"/>
> 
> Need an accesskey set here.

Done. viewToolbarsMenu.accesskey = V

> >+++ b/browser/base/content/browser.xul
> 
> >+      <menu id="context_tabViewMenu" class="menu-iconic" label="&moveTabTo.label;..." 
> >+            accesskey="&moveTabTo.accesskey;">
> 
> The "..." in the label needs to be in the DTD string. Also make sure you use an
> actual ellipsis, not 3 periods. Here's one for free: …

Thanks. They're expensive here in Boston.

> >+        <menupopup id="context_tabViewMenuPopup" 
> >+                   onpopupshowing="if (event.target == this) TabView.updateContextMenu(TabContextMenu.contextTab, this);">
> 
> Hmm, I don't think you need the event.target check here. I think this is only
> present on some of the other menupopups here because they, in turn, have
> submenu popups, so those submenu events bubble up through the DOM.

This actually turns out to be necessary, as other items get appended onto this popup via TabView.updateContextMenu. Removing it results in it not updating the submenu properly.

> >+</vbox>
> >+</deck>
> 
> Maybe add a comment here noting that the <iframe id="tab-view"> is dynamically
> appended as the 2nd child?

Done.

> >+++ b/browser/base/content/tabbrowser.xml
> ...
> >       <method name="updateTitlebar">
> >         <body>
> >           <![CDATA[
> >-            this.ownerDocument.title = this.getWindowTitleForBrowser(this.mCurrentBrowser);
> >+            if (TabView.isVisible()) {
> >+              // ToDo: this will be removed when we gain ability to draw o the menu bar. 
> 
> Make sure a bug is on file and reference it here?

Bug 586175

(In reply to comment #45)
> >+++ b/browser/themes/winstripe/browser/tabview/platform.css
> 
> Nits: this file (just winstripe) has some awkwardly formatted CSS...

Fixed.

I believe this (together with the comments from Ed + Aza above) cover all your questions and recommendations thus far. :)

Comment 48

[Justin Dolske [:Dolske]]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

>+++ b/browser/base/content/browser-tabview.js
...
>+    // ___ visibility
>+    this._sessionstore =
>+      Components.classes["@mozilla.org/browser/sessionstore;1"]
>+        .getService(Components.interfaces.nsISessionStore);

Nit: 

  this._sessionstore = Cc["@mozilla.org/browser/sessionstore;1"].
                       getService(Ci.nsISessionStore);

This is #included from browser.js, so Cc/Ci should already be available. Notice the "." placement, that's another /browser style quirk if you haven't seen it before. :)

>+    let data = this._sessionstore.getWindowValue(window, this._visibilityID);
>+    if (data) {
>+      data = JSON.parse(data);
>+      if (data && data.visible)
>+        this.show();
>+    }

Hmm. Seems like this could end up making session store generate a large blob of JSON (eg,when users have a gazillion tabs), just to decode it here for a single property?

Should look into having session store call TabView.init(), have TabView observe sessionstore-windows-restored, or something like that... This is in the window startup path, so ensuring we don't add overhead would be really good.

>+      var iframe = document.createElement("iframe");

Everyone knows that let is the new var! 90% of the file is already let's, just change these vars too?

>+  getWindowTitle: function() {
>+    let brandBundle = document.getElementById("bundle_brand");
>+    let brandShortName = brandBundle.getString("brandShortName");
>+    return gNavigatorBundle.getFormattedString("tabView.title", [brandShortName]);
>+  },

Should just make this into a memoizing getter, so we don't compute the string over and over.

>+  // Overrides the browser's keys for navigating between tab (outside of the
>+  // TabView UI) so they do the right thing in respect to groupItems.

Hmm. this isn't actually overriding anything, is it? They're effectively new keypress handlers for previously-unhandled key combos?

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

General comments:

1) In the MPL license header for new files, the "Initial Developer of the Original Code" should generally be "the Mozilla Foundation" for work done by folks on Mozilla's dime. I believe that covers everyone listed in the /browser/base/content/tabview/* files being added, so those should be bulk changed to list MoFo. [I believe typical style is to then list yourself in the "Contributors" section as "Rick Astley <[e-mail]> (original author)".]

2) Most of the contents of browser/base/content/tabview/tabview.css should move into browser/themes/*/browser/tabview/tabview.css, so that appearance is controllable by 3rd party themes. The rough guideline, AIUI, is to have structural/functional rules in /content  (eg various position: absolute, z-index, 100% width/height), and appearance rules in /themes (colors, images, shadows, borders). EG, compare browser/base/content/browser.css with browser/themes/winstripe/browser/browser.css. I don't think this needs to block the initial landing, but it should probably be sorted out before the beta (so themers don't scream at us too loudly). File a followup?


>+++ b/browser/themes/winstripe/browser/jar.mn
...
>+        skin/classic/aero/browser/tabview/tabview.png                (tabview/tabview.png)
>+	    skin/classic/aero/browser/tabview/tabview-16.png             (tabview/tabview-16.png)

Nit: you've got a tab on the tabview-16.png line.

>+++ b/browser/base/content/tabview/tabview.html
>+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>+                      "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

Snark: All the cool kids do HTML5 these days. :)

>+<body style="background-color: transparent !important;-moz-appearance: none !important;" transparent="true">

Move the style= into a |body| rule in browser/base/content/tabview/tabview.css

Nitpick: I don't think the !important is actually needed (what other rules are you trying to be more specific than?), and -moz-appearance shouldn't be needed either (there's no native platform/widget style for <body>).


>+++ b/browser/base/content/tabview/storage.js

>+Storage = {
>+  GROUP_DATA_IDENTIFIER:  "tabview-group",
>+  GROUPS_DATA_IDENTIFIER: "tabview-groups",
>+  TAB_DATA_IDENTIFIER:    "tabview-tab",
>+  UI_DATA_IDENTIFIER:    "tabview-ui",
>+  VISIBILITY_DATA_IDENTIFIER:    "tabview-visibility",

Trying hard not to be OCD about the almost-but-not-quite indenting alignment. :)

>+  init: function() {
>+    this._sessionStore =
>+      Components.classes["@mozilla.org/browser/sessionstore;1"]
>+        .getService(Components.interfaces.nsISessionStore);
>+  },

See earlier comment in this bug about preferred style for Cc[].getService(). Make a grep pass through other files here to catch any other instances?

>+/*         Utils.log("readTabData: " + this._sessionStore.getTabValue(tab, this.TAB_DATA_IDENTIFIER)); */

Remove commented-out code.

>+/*     Utils.log('tab', existingData); */

Ditto, and a few other places.

Comment 50

[Justin Dolske [:Dolske]]

Tomorrow (aka "later today", sigh) I'll finish up the review of the new browser/base/content/tabview/ pieces, but I've skimmed through all of them and don't see anything too alarming. In fact my general impression is that it looks like solid, well-written code.

I'll first focus on looking for any showstopper / major issues, but my initial impression is that it's more likely than not that review cycles won't stop those parts from landing... If needed I'll probably be ok with rubber-stamping them and doing a postfacto review, should it come to that.

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>               let ds = browser.docShell;
>               if (ds && ds.contentViewer && !ds.contentViewer.permitUnload())
>                 return false;
>             }
> 
>             var closeWindow = false;
>             var newTab = false;
>             if (this.tabs.length - this._removingTabs.length == 1) {
>-              closeWindow = aCloseWindowWithLastTab != null ? aCloseWindowWithLastTab :
>-                            !window.toolbar.visible ||
>-                              this.tabContainer._closeWindowWithLastTab;
>-
>-              // Closing the tab and replacing it with a blank one is notably slower
>-              // than closing the window right away. If the caller opts in, take
>-              // the fast path.
>-              if (closeWindow &&
>-                  aCloseWindowFastpath &&
>-                  this._removingTabs.length == 0 &&
>-                  (this._windowIsClosing = window.closeWindow(true)))
>-                return null;
>-
>-              newTab = true;
>-            }
>+              if (!TabView.isVisible()) {
>+                closeWindow = aCloseWindowWithLastTab != null ? aCloseWindowWithLastTab :
>+                              !window.toolbar.visible ||
>+                                this.tabContainer._closeWindowWithLastTab;
>+
>+                // Closing the tab and replacing it with a blank one is notably slower
>+                // than closing the window right away. If the caller opts in, take
>+                // the fast path.
>+                if (closeWindow &&
>+                   aCloseWindowFastpath &&
>+                   this._removingTabs.length == 0 &&
>+                   (this._windowIsClosing = window.closeWindow(true)))
>+                  return null;
>+
>+                newTab = true;
>+              }
>+           }

You're slightly messing up the indentation. Instead of if (...) { if (!TabView.isVisible()) {...} }, add !TabView.isVisible() to the first condition. (Not sure what this change is about in the first place, though.)

>--- /dev/null
>+++ b/browser/base/content/tabview/tabview.css

Remove non-behavioral CSS from here, put it in browser/themes/.

>+.guideTrench, .visibleTrench, .activeVisibleTrench {
>+  position: absolute;
>+}

general rule for CSS: \n after ,

>--- /dev/null
>+++ b/browser/themes/gnomestripe/browser/tabview/platform.css

Rename this file to tabview.css.

Comment 52

[Dão Gottwald [:dao]]

Given the code churn in tabbrowser.xml, please don't push the intermediate steps on top of each other, just the final patch.

Comment 53

[Ed Lee :Mardak]

mozilla-central landing rules say that sr is needed for new/modified apis, and bug 582023 landed without sr. Technically the landed files aren't available as APIs yet as they aren't packaged, but we'll need to make sure we get sr for this landing, which builds/packages the modules.

Comment 54

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

(In reply to comment #53)
> APIs yet as they aren't packaged, but we'll need to make sure we get sr for
> this landing, which builds/packages the modules.

Perhaps Vlad?

Comment 55

[Ed Lee :Mardak]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

The landing of tabcandy (as tabview in source) comes with a few new js modules, some tabbrowser/browser api changes, and tabcandy-related code that lives in its own iframe.

AllTabs.jsm: provides a way to register TabOpen, Tab*.. callbacks from any tab in any browser window
utils.jsm: adds some shape objects, parent "Subscribable" class, and utils for assert, log, typecheck, helpers

browser.xul adds a xul:deck with the main browser now as its child and the other deck entry is the tab-view iframe

tabbrowser.xml api changes mostly involve adding a showOnlyTheseTabs call that will set the hidden attribute on various tabs and a visibleTabs property

The rest of the added code gets loaded and lives in the tab-view iframe, so they aren't really exposed apis, but I suppose somebody could come along and view the iframe and its top level objects. Those objects include ui pieces like tab items, group items, trenches (snapping), dragging logic, persistent storage, dom helpers (jQuery-like), and ui glue.

Comment 56

[Ian Gilman [:iangilman]]

(In reply to comment #48)
> Comment on attachment 464655 [details] [diff] [review]

I've now addressed all of these: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/d4e77af949a1

... except the sessionstore suggestion:

> Hmm. Seems like this could end up making session store generate a large blob of
> JSON (eg,when users have a gazillion tabs), just to decode it here for a single
> property?
> 
> Should look into having session store call TabView.init(), have TabView observe
> sessionstore-windows-restored, or something like that... This is in the window
> startup path, so ensuring we don't add overhead would be really good.

As per discussion in #tabcandy with zpao and ehsan, sessionstore will load just the data we need, so this is not a concern. I have, however, removed the JSON, so we're storing just the single, straight, string:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/1eac1d8406e1

Comment 57

[Ian Gilman [:iangilman]]

(In reply to comment #49)
> Comment on attachment 464655 [details] [diff] [review]

All of the items from this comment have been addressed: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/ac7c82f52d1f

... except the body style item: 

> >+<body style="background-color: transparent !important;-moz-appearance: none !important;" transparent="true">
> 
> Move the style= into a |body| rule in browser/base/content/tabview/tabview.css
> 
> Nitpick: I don't think the !important is actually needed (what other rules are
> you trying to be more specific than?), and -moz-appearance shouldn't be needed
> either (there's no native platform/widget style for <body>).

... which is pending investigation.

Comment 58

[Ian Gilman [:iangilman]]

I've now dealt with dao's comments above, as well as dolske's body styling comment. 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/04f695c1e7db

... plus fixing an oversight of mine: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/e85eb7328de4

I believe all comments on this thread have now been attended to.

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/04f695c1e7db

Indentation in tabbrowser.xml is still off, as the superdiff will reveal.

What's the plan for content tabview.css vs. themes tabview.css?

Comment 60

[Justin Dolske [:Dolske]]

Comment on attachment 464655 [details] [diff] [review]
super diff -U8 04c3ca395c10

>+++ b/browser/base/content/tabview/drag.js
...
>+var Drag = function(item, event, isResizing, isFauxDrag) {
>+  try {
...
>+  } catch(e) {
>+    Utils.log(e);
>+  }

Do you really want to suppress any errors from the constructor? Have the catch() rethrow the error unless that's deliberate? [This pattern is in a number of other places too.]

>+  snapBounds: function Drag_snapBounds(bounds, stationaryCorner, assumeConstantSize, keepProportional, checkItemStatus) {
...
>+    // OH SNAP!

\o/

>+    if ( // if we aren't holding down the meta key...
>+         !Keys.meta &&
>+         (!checkItemStatus || // don't check the item status...
>+         // OR we aren't a tab on top of something else, and there's no drop site...
>+         (!(this.item.isATabItem && this.item.overlapsWithOtherItems()) &&
>+             !iQ(".acceptsDrop").length))
>+        ) {

The inline comments and formatting make my eyes bleed a bit...

>+      newRect = Trenches.snap(bounds,stationaryCorner,assumeConstantSize,keepProportional);

Ditto for not having spaces after the commas.

>+      if (typeof trench == 'object') {
>+        trench.showGuide = true;
>+        trench.show();
>+      } else if (trench === 'edge') {
>+        // show the edge...?
>+      }

File a followup bug for deciding the |else| comment, or remove/clarify it?

>+  drag: function(event) {
...
>+      if (/* now - this.startTime > 500 ||  */distance > 100) {

Nuke the commented out code. |var now| above also then becomes unneeded.


>+++ b/browser/base/content/tabview/groupitems.js
...
>+window.GroupItem = function GroupItem(listOfEls, options) {

This file is already being loaded into the iframe's global scope, so I think just "var GroupItem = function..." is sufficient here.

>+  $container
>+    .css({zIndex: -100})
>+    .appendTo("body");
>+/*     .dequeue(); */

Nuke commented-out code.

>+  // ___ Resizer
>+  this.$resizer = iQ("<div>")
>+    .addClass('resizer')
>+    .css({
>+      position: "absolute",
>+      width: 16, height: 16,
>+      bottom: 0, right: 0,
>+    })

Specifying this style inline is a themeing issue (ie, the 16x16 here is referring to a resizer image, which the theme might replace). You've already got CSS files and a classname on the div, so just specify these there? If it's transient style, control it with an attribute selector?

>+  // ___ Titlebar
>+  var html =
>+    "<div class='title-container'>" +
>+      "<input class='name' value='" + (options.title || "") + "'/>" +
>+      "<div class='title-shield' />" +
>+    "</div>";

Mentioned this on IRC, this is a must-fix for landing, since there's a big risk of XSS / privilege escalation from this.

This, and any other places where HTML is built up like this, must not slurp in variables unless it's clear they have limited, locally-controlled values.

r- for that, thanks for already fixing this in Hg. :)

>+  // ___ Stack Expander
>+  this.$expander = iQ("<img/>")
>+    .attr("src", "chrome://browser/skin/tabview/stack-expander.png")
>+    .addClass("stackExpander")

Another example of something that should just be in static /theme/* CSS files.


>+  // ----------
>+  // Function: adjustTitleSize
>+  // Used to adjust the width of the title box depending on groupItem width and title size.
>+  adjustTitleSize: function() {
>+    Utils.assert(this.bounds, 'bounds needs to have been set');
>+    var w = Math.min(this.bounds.width - 35, Math.max(150, this.getTitle().length * 6));

Could do with at least a comment explaining where these numbers code from.

I wonder if the CSS3 calc() stuff would help here?

>+  getContentBounds: function() {
...
>+    box.inset(6, 6);
>+
>+    box.height -= 33; // For new tab button

Ditto.

If some of this really needs to be computed on-the-fly, a more themer-compatible solution would be to use getComputedStyle() to look at the width/padding/whatever on elements that you're adjusting for. Then you pick up theme changes without hardcoding numbers.


>+  setBounds: function(rect, immediately, options) {
...
>+    rect.width = Math.max(110, rect.width);
>+    rect.height = Math.max(125, rect.height);

Smells like CSS max-width / max-height?

>+  // ----------
>+  // Function: add
>+  // Adds an item to the groupItem.
>+  // Parameters:
>+  //
>+  //   a - The item to add. Can be an <Item>, a DOM element or a jQuery object.

jQuery?! :)

>+      // TODO: You should be allowed to drop in the white space at the bottom and have it go to the end
>+      // (right now it can match the thumbnail above it and go there)

File bug?

>+        .css({
>+          opacity: .2,
>+          top: dT + childBB.height + Math.min(7, (this.getBounds().bottom-childBB.bottom)/2),
>+          // TODO: Why the magic -6? because the childBB.width seems to be over-sizing itself.
>+          // But who can blame an object for being a bit optimistic when self-reporting size.
>+          // It has to impress the ladies somehow.
>+          left: dL + childBB.width/2 - this.$expander.width()/2 - 6,
>+        });

*snicker*. Worth looking into / filing this.

>+        // TODO: does this change for rtl?

Check with Ehsan? Probably will need some RTL love overall, but that can happen well after landing.

>+    var Pi = Math.acos(-1);

Math.PI!

I would also have given bonus points for using Euler's identity instead of -1. :D

>+  childHit: function(child) {
...
>+    // ___ we're stacked, but command isn't held down
>+    /*if (!Keys.meta) {
>+      GroupItems.setActiveGroupItem(self);
>+      return { shouldZoom: true };
>+    }*/
>+
>+    GroupItems.setActiveGroupItem(self);
>+    return { shouldZoom: true };
>+
>+    /*this.expand();
>+    return {};*/

Removed commented-out code.

>+    if (pos.top < 0)  pos.top = 20;
>+    if (pos.left < 0) pos.left = 20;
>+    if (pos.top+overlayHeight > window.innerHeight) pos.top = window.innerHeight-overlayHeight-20;
>+    if (pos.left+overlayWidth > window.innerWidth)  pos.left = window.innerWidth-overlayWidth-20;

Couple of style nits that I've ignored up till now... :)

* |if (foo) bar;| should be on two lines, especially when the |bar| part is as long as it is here.
* |a+b-c| --> |a + b - c|

>+    var $shield = iQ('<div>')
>+      .css({
>+        left: 0,
>+        top: 0,
>+        width: window.innerWidth,
>+        height: window.innerHeight,

"width: 100%" and "height: 100%"?


>+                // TODO: This is probably a terrible hack that sets up a race
>+                // condition. We need a better solution.

File bug and reference here.

>+    // TODO: Because this happens as a callback, there is
>+    // sometimes a long delay before the animation occurs.
>+    // We need to fix this--immediate response to a users
>+    // actions is necessary for a good user experience.

File bug and reference here.

[This is style Brendan has been recommending -- no one can keep track of TODOs scattered through the vast amounts of source code, so they're essentially IGNOREMEs. We're much better about tracking bugs, so that's preferred. The comment+reference can then even go away entirely, unless it's helpful to understanding why code is doing something unexpected.]

>+    // ToDo: optimisation is needed to further reduce the tab move.

Again! :)

>+  init: function() {
>+/*
>+    Utils.log("hello");
>+    Utils.log("Groups", Groups);
>+*/
>+  },

Utils.log("goodbye -- remove this")

>+  save: function() {
>+    if (!this._inited) // too soon to save now
>+      return;

This is done in a few spots -- is there risk of dataloss from this? Instead of ignoring the call, defer/queue it? Or does it end up being harmless in the worst case, and usually taken care of by some later action triggering a store?


>+  groupItemStorageSanity: function(groupItemData) {
>+    // TODO: check everything

You know the spiel.

>+  // TODO: what if there are multiple groupItems with the same title??
>+  //       Right now, looks like it'll return the last one.

And again.

I think I'm just going to stop noting these -- do a global grep.

Comment 61

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #59)
> (In reply to comment #58)
> > http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/04f695c1e7db
> 
> Indentation in tabbrowser.xml is still off, as the superdiff will reveal.

Can you clarify what indentation is off? It is not clear to me.

This part (below)? Anything else?

>              if (closeWindow &&
>                 aCloseWindowFastpath &&
>                 this._removingTabs.length == 0 &&
>                 (this._windowIsClosing = window.closeWindow(true)))

Comment 62

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #60)
> Comment on attachment 464655 [details] [diff] [review]

Dolske, all your recommendations to drag.js + groupitems.js (and friends) were implemented (though some in the form of new bug reports + references in the comments).

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/63d1e97a5676

Comment 63

[Ian Gilman [:iangilman]]

(In reply to comment #59)
> What's the plan for content tabview.css vs. themes tabview.css?

That's been filed as a follow-up, Bug 586455.

Comment 64

[Ed Lee :Mardak]

Attachment: super diff -U8 11ef4a86497d

Comment 65

[Ed Lee :Mardak]

Attachment: inter diff -r [m-c/04c3ca395c10 merge]:11ef4a86497d

Comment 66

[Justin Dolske [:Dolske]]

Comment on attachment 465127 [details] [diff] [review]
super diff -U8 11ef4a86497d

>+++ b/browser/base/content/tabview/ui.js
...
>+        // ___ make info item
>+        var html =
>+          "<div class='intro'>"
>+            + "<h1>Welcome to Firefox Tab Sets</h1>" // TODO: This needs to be localized if it's kept in
>+            + "<div>(more goes here)</div><br>"
>+            + "<video src='http://people.mozilla.org/~araskin/movies/tabcandy_howto.webm' "
>+            + "width='100%' preload controls>"
>+          + "</div>";

This ought to be localized for landing, otherwise the localizers (hi Pike!) will freak out and yell at you. :)

Either create a DTD w/string entities to load into your .html, or open a string bundle from your JS code and pull strings from that.


>+  _addTabActionHandlers: function() {
...
>+          // ToDo: When running unit tests, everything happens so quick so
>+          // new tabs might be added after a tab is closing. Therefore, this
>+          // hack is used. We should look for a better solution.

Eww, indeed. This might end up being ok if the need for this just ends up being that the event loop needs to spin once before you handle it; setTimeout(..., 0) can be a necessary evil in such cases.

>+          setTimeout(function() { // Marshal event from chrome thread to DOM thread

I'm not sure what that comment means. Everything is running on one thread.


>+    AllTabs.register("select", function(tab) {
>+      if (tab.ownerDocument.defaultView != gWindow)
>+        return;

This check is in various tab listeners, why's it needed? How would you get events for tabs in other windows?


>+  // Called when the user switches from one tab to another outside of the TabView UI.
>+  tabOnFocus: function(tab) {

This seems like a surprising amount of code to be running on each tab selection change... Is there an opportunity for an early return / skipping work when not needed (eg, set a dirty bit and handle it later)?

[Nit: call this tabOnSelect, because it really doesn't have anything to do with _focus_.]


>+  _setTabViewFrameKeyHandlers: function() {

Instead of the various magic numbers in this function, you really ought to be using KeyEvent.DOM_VK_BLAH (.DOM_VK_TAB, .DOM_VK_RIGHT, etc).


>+  // Function: _addDevMenu
>+  // Fills out the "dev menu" in the TabView UI.
>+  _addDevMenu: function() {

Ought to kill this for landing.

Comment 67

[Justin Dolske [:Dolske]]

Comment on attachment 465127 [details] [diff] [review]
super diff -U8 11ef4a86497d


>+++ b/browser/base/content/test/Makefile.in
...
>                  browser_visibleTabs.js \
>+                 browser_visibleTabs_contextMenu.js \
>+                 browser_visibleTabs_bookmarkAllPages.js \
>+                 browser_visibleTabs_tabPreview.js \

Looks like you missed a test here:

>+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js

("AllPages" added, "AllTabs" not)



>+++ b/browser/base/content/test/tabview/Makefile.in
...
>+$(warning All TabView tests are disabled for now while we iterate on the UI)
>+
>+_BROWSER_FILES = \
>+                 browser_tabview_launch.js \
>+    $(NULL)
>+#                 browser_tabview_dragdrop.js \
>+#                 browser_tabview_group.js \

Hmm. Thats kind of sadmaking.

How far are these tests from working? Seems like these should go ahead and be enabled, since they're not testing terribly involved bits of TabCandy that iteration will quickly change.

Comment 68

[Justin Dolske [:Dolske]]

Comment on attachment 465127 [details] [diff] [review]
super diff -U8 11ef4a86497d

Marking r+, with a few notes and conditions, for the record:

0) All these worlds are yours, except mozilla-central. Attempt no landing there until try/branch builds indicate clean test runs with no leaks.

1A) I've only lightly reviewed the parts in browser/base/content/tabview/. This code is largely independent from the rest of the browser, so any FAIL here is contained. [There are a few bit that interact with the rest of the browser, and I did look at those more closely.] This is being allowed as a special case so that TabCandy can benefit from wider user feedback in the next Firefox beta, without blocking on a long nitpicky review.

1B) The expectation is that the TabCandy team will continue to iterate on design and bugfixing, and that there will effectively be an additional, fuller postfacto-review as the other points here and previously filed followup bugs are addressed in, err, followup bugs.

2) In addition to the specific followup bugs filed so far, there are a number of common, general issues that need to be addressed. All minor, but should be fixed ASAP.
  - straightening out themeing issues (move JS-driven CSS to static files, and remaining /content vs /theme splits)
  - removal of commented out code, "TODO"s without bugs on file
  - various minor style conventions and formatting nits. No a huge deal, but these stick out like a sore thumb and should be trivial to fix.
  - possibly a few other things mentioned in previous comments that I'm too tired to go diffing for at the moment. :)

3) As discussed on IRC, need to add a plan B #ifdef so that this can be turned off after landing without having to deal with a giant backout. Please make sure this has been tested to build and pass tests, lest invoking Plan B makes things even worse!

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 465127 [details] [diff] [review]
super diff -U8 11ef4a86497d

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

> 
>               // Closing the tab and replacing it with a blank one is notably slower
>               // than closing the window right away. If the caller opts in, take
>               // the fast path.
>               if (closeWindow &&
>-                  aCloseWindowFastpath &&
>-                  this._removingTabs.length == 0 &&
>-                  (this._windowIsClosing = window.closeWindow(true)))
>+                 aCloseWindowFastpath &&
>+                 this._removingTabs.length == 0 &&
>+                 (this._windowIsClosing = window.closeWindow(true)))
>                 return null;

Don't do this.

Comment 70

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

Not sure which patch I'm meant to approve here; also:

 -  need we be concerned with recently reported bug 586595 prior to landing this on mozilla-central?
 - is the superdiff updated to include the leak fixes from bug 586522?
 - have we done a test run (I think we have) on the latest superdiff?

Comment 71

[Dão Gottwald [:dao]]

Comment on attachment 465127 [details] [diff] [review]
super diff -U8 11ef4a86497d

>       <method name="showOnlyTheseTabs">
>         <parameter name="aTabs"/>
>         <body>
>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>             tab.hidden = aTabs.indexOf(tab) == -1 && !tab.pinned && !tab.selected;
>           });
>+          Services.obs.notifyObservers(window, "tab-visibility-change", null);
>         ]]>

As noted in bug 585855, this isn't quite sane. If nothing else, please just don't deal with it at all in this bug.

(Even if this was sane, a window-specific event would be better than a global notification.)

Comment 72

[Ed Lee :Mardak]

(In reply to comment #71)
> >+          Services.obs.notifyObservers(window, "tab-visibility-change", null);
> As noted in bug 585855, this isn't quite sane.
Nod. It's already been backed out. I can post a new superdiff.

Comment 73

[Ed Lee :Mardak]

Attachment: super diff -U8 874090a047df

Includes raymond's leak fixes and blackness workaround and backs out tab-visibility notification.

Comment 74

[Ed Lee :Mardak]

Attachment: inter diff -U8 [m-c/11ef4a86497d]:874090a047df

Comment 75

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

Comment on attachment 465286 [details] [diff] [review]
super diff -U8 874090a047df

a=beltzner; mardak tells me that the maple results look solid test and performance wise. Mardak, please co-ordinate with sheriff to get a landing spot on m-c; we don't want this to land with other things that might mask/confuse test results on that tree.

Comment 76

[Ian Gilman [:iangilman]]

(In reply to comment #66)
(In reply to comment #68)

My reading of comment 68, as well as the content of 66 and 67, is that we're good for initial landing, but that the items in 66 and 67 need to followed up on shortly. Detailed responses to 66 (67 coming shortly): 

> This ought to be localized for landing, otherwise the localizers (hi Pike!)
> will freak out and yell at you. :)
> 
> Either create a DTD w/string entities to load into your .html, or open a string
> bundle from your JS code and pull strings from that.

Filed follow-up, bug 586685.
	
> Eww, indeed. This might end up being ok if the need for this just ends up being
> that the event loop needs to spin once before you handle it; setTimeout(..., 0)
> can be a necessary evil in such cases.

Filed follow-up, bug 586689. 

> >+          setTimeout(function() { // Marshal event from chrome thread to DOM thread
> 
> I'm not sure what that comment means. Everything is running on one thread.

Things may have changed since I started this pattern, but at the time (a few months ago), events about xul:tabs (such as TabSelect) were in fact arriving on a different thread than the main UI thread of the Tab Candy frame. This was readily apparent with some logging; two of our routines were running simultaneously in parallel. This caused all sorts of extremely unpleasant, unpredictable bugs. Since we've added those setTimeouts to event end points, all of those bugs have gone away. 

Filed a follow-up to investigate if this is still necessary, bug 586693.

> >+    AllTabs.register("select", function(tab) {
> >+      if (tab.ownerDocument.defaultView != gWindow)
> >+        return;
> 
> This check is in various tab listeners, why's it needed? How would you get
> events for tabs in other windows?

Magic! AllTabs is a js module that grabs events for all tabs from all windows. This is going to important once Tab Candy is able to manage tabs across windows, but for now we just filter out the other windows.

> This seems like a surprising amount of code to be running on each tab selection
> change... Is there an opportunity for an early return / skipping work when not
> needed (eg, set a dirty bit and handle it later)?
> 
> [Nit: call this tabOnSelect, because it really doesn't have anything to do with
> _focus_.]

Filed a follow up, bug 586712.

> Instead of the various magic numbers in this function, you really ought to be
> using KeyEvent.DOM_VK_BLAH (.DOM_VK_TAB, .DOM_VK_RIGHT, etc).

Filed a follow up, bug 586719.

> >+  // Function: _addDevMenu
> Ought to kill this for landing.

Filed a follow up, bug 586721.

Comment 77

[Ian Gilman [:iangilman]]

(In reply to comment #67)

> Looks like you missed a test here:
> 
> >+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js

This is bug 585855, not yet resolved. 

> >+#                 browser_tabview_dragdrop.js \
> >+#                 browser_tabview_group.js \
> 
> Hmm. Thats kind of sadmaking.
> 
> How far are these tests from working? Seems like these should go ahead and be
> enabled, since they're not testing terribly involved bits of TabCandy that
> iteration will quickly change.

These tests are tracked with bug 584627 and bug 582677. 


I believe this brings us up to date on this bug.

Comment 78

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/5da28c582cc7

With revision history:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=5da28c582cc7

Sorry dao, I tried landing tabbrowser.xml changes separately like how I landed bug 582116. But after merging in tabcandy-central, it would still have an ancestor reference to these other revisions. So even what I did for bug 582116 would potentially show the /other/ ancestor revision instead of the mozilla-central ancestor.

I checked with mercurial guys and they said which parent it picks for giving blame is somewhat undefined.

Comment 79

[Martijn Wargers (dead)]

Shouldn't TabCandy have it's own bugzilla component? Currently, they are getting filed under Firefox->Tabbed Browser, but it seems to me TabCandy is something completely different.

Comment 80

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

(In reply to comment #79)
> Shouldn't TabCandy have it's own bugzilla component? Currently, they are
> getting filed under Firefox->Tabbed Browser, but it seems to me TabCandy is
> something completely different.

I'm not a fan of over-componentizing, to be honest; feel like that's more of a decision for the Firefox module owners and peers, isn't it?

Comment 81

[Dão Gottwald [:dao]]

It's separate code wise and feels big enough to justify a separate component. A convenient side-effect would be a separate QA contact for people dedicated to Tab Candy to watch.

Comment 82

[Steffen Wilberg]

Instead of creating a new component, we could move the existing "Mozilla Labs::TabCandy" component to Firefox.
How should it be named? TabCandy? TabView?
Need to file a bug in "mozilla.org::Bugzilla: Keywords & Components" in any case.

Comment 83

[Martijn Wargers (dead)]

Ok, thanks Steffen, I filed bug 587034 on doing that.

Comment 84

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I pushed additional changeset http://hg.mozilla.org/mozilla-central/rev/28f97576f0b4 on Dao's request

Comment 85

[Ian Gilman [:iangilman]]

(In reply to comment #84)
> I pushed additional changeset
> http://hg.mozilla.org/mozilla-central/rev/28f97576f0b4 on Dao's request

Does this change break anything? Why was the TabView check in there to begin with? Raymond, do you know?

Comment 86

[Dão Gottwald [:dao]]

(In reply to comment #85)
> (In reply to comment #84)
> > I pushed additional changeset
> > http://hg.mozilla.org/mozilla-central/rev/28f97576f0b4 on Dao's request
> 
> Does this change break anything? Why was the TabView check in there to begin
> with? Raymond, do you know?

It was introduced in bug 576427. We'll need to find a better solution for this, like letting tab candy not close all tabs in the first place, or opening a blank tab when closing the last tab.