633190 - Inconsistent animation behavior for creating new tab in Panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre

Reproducible: Always

STR:
1. Enter Panorama
2. Create new tab using plus icon from group and observe animation
3. Enter Panorama
4. Create new tab using CTRL+T

Actual results:
 - no animation when using CTRL+T

Expected results:
 - the same animation as before is present when using CTRL+T

Comment 1

[Raymond Lee [:raymondlee]]

We don't handle ctrl/cmd + T in Panorama.  There is code to hide the Panorama UI on tab switch without zoom in animation (create a new blank tab would trigger a tab switch).  

Ian: do we want to add code to handle ctrl/cmd + T and show the zoom in animation?

Comment 2

[Kevin Hanes]

We want a consistent "new tab" experience. We want to handle the ctrl/cmd + t case in a way that looks like opening a new tab any other way in Panorama.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: WIP v1

Ian: please give some feedback about this patch.  The test runs fine by itself but  it breaks some other tests.  Will look into that next.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 512213 [details] [diff] [review]
WIP v1

The approach looks sound, though I'll want feedback from a browser peer on the browser.js change. Also, although we certainly want this, I wouldn't say it's high-priority. 

>+  // On move to group pop showing.
>+  openNewTab: function TabView_openNewTab() {

That comment is just copied from the routine above it; please put a real comment there. 

>+    if (this._window)
>+      this._window.UI.openNewTab();

This needs to do something sensible if this._window doesn't exist (I realize it's never called that way in the current patch, but once the routine exists, you never know how it will get called in the future). The routine name says it'll open a new tab; it shouldn't just silently fail to do so under certain circumstances.

>+  // Function: openNewTab
>+  // Open new tab using ctr/cmd+T or File > New Tab menu.

For clarity, I think the comment should read "in response to" instead of "using"; this routine itself doesn't actually use those commands. 

>+      // GroupItems_newTab() would handle this.

I'm not sure what this commment is trying to say; seems superfluous?

>+    routine(function() {
>+      test2();
>+    });

This can just be: 

  routine(test2);

>+function setup(callback) {
>+  ok(!TabView.isVisible(), "Tab View is not visible");
>+
>+  // show tabview
>+  let onTabViewShown = function() {
>+    window.removeEventListener("tabviewshown", onTabViewShown, false);
>+    callback();
>+  }
>+  window.addEventListener("tabviewshown", onTabViewShown, false);
>+  TabView.toggle();
>+}

Use showTabView() from head.js instead.

>+function routine(callback) {

This is an awfully generic name. How about "testCreateTabAndThen()"?

Thanks!

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #4)
> Comment on attachment 512213 [details] [diff] [review]
> WIP v1
> 
> The approach looks sound, though I'll want feedback from a browser peer on the
> browser.js change. Also, although we certainly want this, I wouldn't say it's
> high-priority. 
> 
> >+  // On move to group pop showing.
> >+  openNewTab: function TabView_openNewTab() {
> 
> That comment is just copied from the routine above it; please put a real
> comment there. 

Fixed

> 
> >+    if (this._window)
> >+      this._window.UI.openNewTab();
> 
> This needs to do something sensible if this._window doesn't exist (I realize
> it's never called that way in the current patch, but once the routine exists,
> you never know how it will get called in the future). The routine name says
> it'll open a new tab; it shouldn't just silently fail to do so under certain
> circumstances.
> 

Fixed

> >+  // Function: openNewTab
> >+  // Open new tab using ctr/cmd+T or File > New Tab menu.
> 
> For clarity, I think the comment should read "in response to" instead of
> "using"; this routine itself doesn't actually use those commands. 
> 

Fixed

> >+      // GroupItems_newTab() would handle this.
> 
> I'm not sure what this commment is trying to say; seems superfluous?
> 

Fixed

> >+    routine(function() {
> >+      test2();
> >+    });
> 
> This can just be: 
> 
>   routine(test2);
> 

Fixed

> >+function setup(callback) {
> >+  ok(!TabView.isVisible(), "Tab View is not visible");
> >+
> >+  // show tabview
> >+  let onTabViewShown = function() {
> >+    window.removeEventListener("tabviewshown", onTabViewShown, false);
> >+    callback();
> >+  }
> >+  window.addEventListener("tabviewshown", onTabViewShown, false);
> >+  TabView.toggle();
> >+}
> 
> Use showTabView() from head.js instead.
> 

Fixed

> >+function routine(callback) {
> 
> This is an awfully generic name. How about "testCreateTabAndThen()"?
> 

Fixed

Comment 6

[Raymond Lee [:raymondlee]]

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

This patch requires the patch for bug 635668

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

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

Overall: this is nice, but I'm afraid it will not get approved at this point in time, particularly as it touches browser code too. (Adding Dāo for feedback as well to weigh in on this point.) I think we should probably go ahead and punt on it. Here's my feedback, though:

>+  openNewTab: function TabView_openNewTab() {
>+    if (this._window && this.isVisible())
>+      this._window.UI.openNewTab();
>+    else
>+      gBrowser.loadOneTab("about:blank", {inBackground: false});

I don't see why this if-check is necessary. The only place where TabView_openNewTab is called is from BrowserOpenTab, which already checks that TabView is visible.

>+    if (activeTabItem) {
>+      if (activeTabItem.parent) {
>+       activeTabItem.parent.newTab();

nit: spacing is off.

>+        // set the active orphan tab item so GroupItems_newTab() would create a
>+        // group item which contains the orphan tab item and new tab item.
>+        GroupItems.setActiveGroupItem(null);
>+        GroupItems.setActiveOrphanTab(activeTabItem);

Why do we need to do this? If we have an active tab item, and it doesn't have a parent, it should already be the activeOrphanTab, no?

>+        let newTab =
>+          gBrowser.loadOneTab("about:blank", { inBackground: true });
>+        // give a delay so the group item and tab item are set and shown
>+        setTimeout(function() { newTab._tabViewTabItem.zoomIn(true); }, 0);

Why is this delay required? Can we use an observer notification or something instead? Or an event listener or progress notifier on newTab itself?

Also, BrowserOpenTab calls focusAndSelectUrlBar after the new tab is created. We aren't doing this here. Is it possible to do that after the zoomIn in this case?

>+      // after the new tab is created, the tabitem would be created and then 
>+      // GroupItems_newTab() would be called to put the tabitem into the right
>+      // group.
>+      let newTab =
>+        gBrowser.loadOneTab("about:blank", { inBackground: true });
>+      // zoom into the tab item.
>+      newTab._tabViewTabItem.zoomIn(true);

This code as it stands looks redundant, between when we start with an activeTabItem or not. Can we simplify the code path?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #7)
> >+        let newTab =
> >+          gBrowser.loadOneTab("about:blank", { inBackground: true });
> >+        // give a delay so the group item and tab item are set and shown
> >+        setTimeout(function() { newTab._tabViewTabItem.zoomIn(true); }, 0);
> 
> Also, BrowserOpenTab calls focusAndSelectUrlBar after the new tab is created.
> We aren't doing this here. Is it possible to do that after the zoomIn in this
> case?

tabItem.zoomIn(true) does this already :)

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to comment #7)
> Comment on attachment 514142 [details] [diff] [review]
> v2
> 
> Overall: this is nice, but I'm afraid it will not get approved at this point in
> time, particularly as it touches browser code too. (Adding Dāo for feedback as
> well to weigh in on this point.) I think we should probably go ahead and punt
> on it. Here's my feedback, though:
> 
> >+  openNewTab: function TabView_openNewTab() {
> >+    if (this._window && this.isVisible())
> >+      this._window.UI.openNewTab();
> >+    else
> >+      gBrowser.loadOneTab("about:blank", {inBackground: false});
> 
> I don't see why this if-check is necessary. The only place where
> TabView_openNewTab is called is from BrowserOpenTab, which already checks that
> TabView is visible.

Ian mentioned in comment 4 that you never know how it will get called in the future so it seems if-check is necessary.  However, I've removed the if-else blocks and changed to a more appropriate name to indicate what it does.

> 
> >+    if (activeTabItem) {
> >+      if (activeTabItem.parent) {
> >+       activeTabItem.parent.newTab();
> 
> nit: spacing is off.

Fixed

> 
> >+        // set the active orphan tab item so GroupItems_newTab() would create a
> >+        // group item which contains the orphan tab item and new tab item.
> >+        GroupItems.setActiveGroupItem(null);
> >+        GroupItems.setActiveOrphanTab(activeTabItem);
> 
> Why do we need to do this? If we have an active tab item, and it doesn't have a
> parent, it should already be the activeOrphanTab, no?

No. We are getting the highlighted tabitem using UI.getActiveTab() but it doesn't mean that the tabitem is the same from the GroupItems.getActiveTabItem()/GroupItems.getActiveOrphanTab().  When you use keyboard to move the hightlight, the active tab in UI object gets updated but not in the GroupItems object.

I think bug 608407 is filed for that reason.

> 
> >+        let newTab =
> >+          gBrowser.loadOneTab("about:blank", { inBackground: true });
> >+        // give a delay so the group item and tab item are set and shown
> >+        setTimeout(function() { newTab._tabViewTabItem.zoomIn(true); }, 0);
> 
> Why is this delay required? Can we use an observer notification or something
> instead? Or an event listener or progress notifier on newTab itself?

The new group isn't set to the correct position before the zoom in animation starts, therefore, the animation starts from the bottom right.  I've updated the code to use the "immediately"option so the group would be created and set immediately, hence the animation starts from the right position without using the delay.

> 
> Also, BrowserOpenTab calls focusAndSelectUrlBar after the new tab is created.
> We aren't doing this here. Is it possible to do that after the zoomIn in this
> case?

Tim mentioned that in comment 7

> 
> >+      // after the new tab is created, the tabitem would be created and then 
> >+      // GroupItems_newTab() would be called to put the tabitem into the right
> >+      // group.
> >+      let newTab =
> >+        gBrowser.loadOneTab("about:blank", { inBackground: true });
> >+      // zoom into the tab item.
> >+      newTab._tabViewTabItem.zoomIn(true);
> 
> This code as it stands looks redundant, between when we start with an
> activeTabItem or not. Can we simplify the code path?

Simplified it

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

Fixed a typo

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

[bugspam: betaN -> future]

Comment 12

[Raymond Lee [:raymondlee]]

This is marked for future and just for record, it passed try.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=a022965ca90b

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> Ian mentioned in comment 4 that you never know how it will get called in the
> future so it seems if-check is necessary.  However, I've removed the if-else
> blocks and changed to a more appropriate name to indicate what it does.

Ok. Ian's comment makes sense too... ultimately which is correct here would be up to the browser-side reviewer (Dão)

> When you use
> keyboard to move the hightlight, the active tab in UI object gets updated but
> not in the GroupItems object.
> 
> I think bug 608407 is filed for that reason.

Oh... that's very very sad. :( In my mind the right way to do this, then, is to fix bug 608407 and keep the code very clean here. Unless the plan is to try to review this quick and try to beg for approval for 4 right now, I think that's what we should do, post-fx4.

> The new group isn't set to the correct position before the zoom in animation
> starts, therefore, the animation starts from the bottom right.  I've updated
> the code to use the "immediately"option so the group would be created and set
> immediately, hence the animation starts from the right position without using
> the delay.

Great.

f-, but only because I think we should fix bug 608407 (or whatever that "active"-tracking issue is) first. Otherwise looking pretty clean.

Comment 14

[Kevin Hanes]

bugspam

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v4

Updated the patch

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 530991 [details] [diff] [review]
v4

Do we really want those changes to browser.js? Why not just do something like this in ui.js:

> case self._browserKeys.newNavigator:
>   preventDefault = false;
>   break;
> case self._browserKeys.newNavigatorTab:
>   let newTab = gBrowser.loadOneTab("about:blank", {inBackground: true});
>   newTab._tabViewTabItem.zoomIn(true);
>   break;

We have a key handler already installed for that. That seems less intrusive to me. What do you think?

Comment 17

[Raymond Lee [:raymondlee]]

(In reply to comment #16)
> Comment on attachment 530991 [details] [diff] [review] [review]
> v4
> 
> Do we really want those changes to browser.js? Why not just do something
> like this in ui.js:
> 
> > case self._browserKeys.newNavigator:
> >   preventDefault = false;
> >   break;
> > case self._browserKeys.newNavigatorTab:
> >   let newTab = gBrowser.loadOneTab("about:blank", {inBackground: true});
> >   newTab._tabViewTabItem.zoomIn(true);
> >   break;
> 
> We have a key handler already installed for that. That seems less intrusive
> to me. What do you think?

If we just do what you suggested and user goes to File > new Tab, he wouldn't see the animation.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #17)
> If we just do what you suggested and user goes to File > new Tab, he
> wouldn't see the animation.

True. Forgot about that :/

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 530991 [details] [diff] [review]
v4

Review of attachment 530991 [details] [diff] [review]:
-----------------------------------------------------------------

Approach looks good. We'd need a browser peer to look at the browser.js changes.

::: browser/base/content/tabview/groupitems.js
@@ +2343,5 @@
>        tabItems = [tabItem];
>      }
>  
>      newGroupItemBounds.inset(-40,-40);
> +    let immediately = (options && options.immediately) ? true : false;

Nit: We don't need "true : false" here.

::: browser/base/content/tabview/ui.js
@@ +1598,5 @@
> +    } else {
> +      let newTab =
> +        gBrowser.loadOneTab("about:blank", { inBackground: true });
> +      newTab._tabViewTabItem.zoomIn(true);
> +    }

The code for handling orphan tabs is (almost) exactly the same as GroupItem.newTab(). Can't we just always use:

openNewTab: function UI_openNewTab(url) {
  let newTab = gBrowser.loadOneTab(url || "about:blank", { inBackground: true });
  newTab._tabViewTabItem.zoomIn(!url);
}

here and call UI.openNewTab(url) from GroupItem.newTab() like:

newTab: function GroupItem_newTab(url) {
  UI.setActive(this, { dontSetActiveTabInGroup: true });
  UI.openNewTab(url);
}

::: browser/base/content/test/tabview/browser_tabview_bug633190.js
@@ +15,5 @@
> +function test1() {
> +  showTabView(function() {
> +    ok(origTab._tabViewTabItem.parent, "The original tab belongs to a group");
> +
> +    contentWindow = document.getElementById("tab-view").contentWindow;

Please use TabView.getContentWindow() here.

@@ +51,5 @@
> +    testCreateTabAndThen(function() {
> +      newTabs.forEach(function(tab) {
> +        gBrowser.removeTab(tab);
> +      });
> +       hideTabView(finish);

Nit: indentation is off.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: v5

Updated the patch based on comment 19

Comment 21

[Dão Gottwald [:dao]]

> If we just do what you suggested and user goes to File > new Tab, he wouldn't
> see the animation.

Sounds like you want onTabSelect to handle this.

Comment 22

[Raymond Lee [:raymondlee]]

(In reply to comment #21)
> > If we just do what you suggested and user goes to File > new Tab, he wouldn't
> > see the animation.
> 
> Sounds like you want onTabSelect to handle this.

In the Panorama UI, creating a new tab or removing a tab would trigger onTabSelect so we can't easily determine when to display the zoom-in animation.  Any suggestions?

Comment 23

[Dão Gottwald [:dao]]

The UI knows beforehand when it creates or removes a tab and can pass that information around, right?

Comment 24

[Dão Gottwald [:dao]]

I'm not sure I understand comment 22, though.

Comment 25

[Ian Gilman [:iangilman]]

Comment on attachment 531985 [details] [diff] [review]
v5

Review of attachment 531985 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me; flag me for review again when Dao's happy.

::: browser/base/content/tabview/groupitems.js
@@ +1787,3 @@
>  
>      // TabItems will have handled the new tab and added the tabItem property.
>      // We don't have to check if it's an app tab (and therefore wouldn't have a

This comment no longer belongs here; it should go into openNewTab

@@ +2345,5 @@
>      newGroupItemBounds.inset(-40,-40);
> +    let immediately = options && options.immediately
> +    let newGroupItem = 
> +      new GroupItem(tabItems, 
> +                    { bounds: newGroupItemBounds, immediately: immediately });

Looks like this change is no longer necessary? I suppose it doesn't hurt, but it's not really relevant to the patch.

Comment 26

[Ian Gilman [:iangilman]]

(In reply to comment #24)
> I'm not sure I understand comment 22, though.

I think Raymond is saying that since onTabSelect fires under a number of circumstances, BrowserOpenTab is a better match for our needs. 

I assume there's no event for tab creation?

I guess if we can make it work cleanly with onTabSelect, that saves us from having to modify browser.js. If it's cleaner to modify browser.js, though, we should do that; whether it means hooking directly in or adding a new event.

Comment 27

[Raymond Lee [:raymondlee]]

(In reply to comment #26)
> (In reply to comment #24)
> > I'm not sure I understand comment 22, though.
> 
> I think Raymond is saying that since onTabSelect fires under a number of
> circumstances, BrowserOpenTab is a better match for our needs. 
> 
> I assume there's no event for tab creation?
>
> I guess if we can make it work cleanly with onTabSelect, that saves us from
> having to modify browser.js. If it's cleaner to modify browser.js, though,
> we should do that; whether it means hooking directly in or adding a new
> event.


There is a tab creation event.  I am working to see whether we can modify the panorama code without touching browser.js.

Comment 28

[Raymond Lee [:raymondlee]]

Attachment: v6

Updated the patch and it doesn't touch the browser.js

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 532608 [details] [diff] [review]
v6

Review of attachment 532608 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +775,5 @@
>        let emptyGroups = GroupItems.groupItems.filter(function (groupItem) {
>          return (groupItem != self && !groupItem.getChildren().length);
>        });
>        let group = (emptyGroups.length ? emptyGroups[0] : GroupItems.newGroup());
> +      group.newTab(null, { noVisibleTabs:true });

Nit: space between "noVisibleTabs: true".

@@ +1781,5 @@
>  
>    // ----------
>    // Function: newTab
>    // Creates a new tab within this groupItem.
> +  newTab: function GroupItem_newTab(url, options) {

Please describe the arguments and options here in the comment above.

@@ +1789,2 @@
>      UI.setActive(this, { dontSetActiveTabInGroup: true });
> +    let newTab = gBrowser.loadOneTab(url || "about:blank", { inBackground: false });

Nit: We don't need this variable anymore.

::: browser/base/content/tabview/ui.js
@@ +139,5 @@
>    // Variable: ignoreKeypressForSearch
>    // Used to prevent keypress being handled after quitting search mode.
>    ignoreKeypressForSearch: false,
>  
> +  creatingNewOrphanTab: false,

Nit: Please add a comment describing what this variable does.

@@ +724,5 @@
>        // if it's an app tab, add it to all the group items
>        if (tab.pinned)
>          GroupItems.addAppTab(tab);
> +
> +      if (self.isTabViewVisible())

Let's make that "else if (self.isTabViewVisible())" and we can remove the check for _tabViewTabItem on line 869.

@@ +725,5 @@
>        if (tab.pinned)
>          GroupItems.addAppTab(tab);
> +
> +      if (self.isTabViewVisible())
> +        self.openedNewTab = tab;

We should name that "_openedNewTab" at least. A better name would be something like "_lastOpenedTab" and we should set an initial value at the top.

@@ +865,5 @@
>      this._currentTab = tab;
>  
> +    if (this.isTabViewVisible()) {
> +      if (!this.restoredClosedTab && this.openedNewTab && 
> +          this.openedNewTab == tab && tab._tabViewTabItem) {

Nit: we don't need to check for (this.openedNewTab) because we do (this.openedNewTab == tab).

::: browser/base/content/test/tabview/browser_tabview_bug633190.js
@@ +50,5 @@
> +
> +    testCreateTabAndThen(function() {
> +      newTabs.forEach(function(tab) {
> +        gBrowser.removeTab(tab);
> +      });

(see below)

@@ +61,5 @@
> +  ok(TabView.isVisible(), "Tab View is visible");
> +
> +  // detect tab open and zoomed in event.
> +  gBrowser.tabContainer.addEventListener("TabOpen", function (event) {
> +    gBrowser.tabContainer.removeEventListener("TabOpen", arguments.callee, false);

Please dont' use arguments.callee as that'll throw in ES5 strict mode. You can name the original callback function and reference that instead.

@@ +65,5 @@
> +    gBrowser.tabContainer.removeEventListener("TabOpen", arguments.callee, false);
> +
> +    let tab = event.target;
> +    tabItem = tab._tabViewTabItem;
> +    ok(tabItem, "Tab item is available after tab open");

I guess that could lead to an intermittent orange if we rely on the test listener being called after the default tabview listener. Please add executeSoon() here (with a comment) to make sure ._tabViewTabItem exists.

@@ +72,5 @@
> +       tabItem.removeSubscriber(tabItem, "zoomedIn");
> +
> +       is(gBrowser.selectedTab, tab,
> +         "The selected tab is the same as the newly opened tab");
> +       newTabs.push(tab);

You could just use "registerCleanupFunction(function () gBrowser.removeTab(tab))" here. So you wouldn't need newTabs[] at all and these tabs get removed even when timing out.

Comment 32

[Raymond Lee [:raymondlee]]

Attachment: v7

Comment 33

[Raymond Lee [:raymondlee]]

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=74390e25bf32

Comment 34

[Ian Gilman [:iangilman]]

Comment on attachment 537719 [details] [diff] [review]
v7

Review of attachment 537719 [details] [diff] [review]:
-----------------------------------------------------------------

This has become a complicated patch! Can you write up a brief description of what it's doing?

::: browser/base/content/tabview/groupitems.js
@@ +1785,5 @@
> +  // Parameters:
> +  //  url - the new tab should open this url as well
> +  //  options - the options object
> +  //    noVisibleTabs - boolean indicates whether ther aer no visible tabs 
> +  //      on the tabstrip

Typo: ther aer

::: browser/base/content/test/tabview/browser_tabview_bug633190.js
@@ +33,5 @@
> +
> +// Open a new tab when the active tab item is an orphan tab.
> +function test3() {
> +  registerCleanupFunction(function () TabView.hide());
> +

Shouldn't this clean-up be registered in test1?

Comment 35

[Raymond Lee [:raymondlee]]

(In reply to comment #34)
> Comment on attachment 537719 [details] [diff] [review] [review]
> v7
> 
> Review of attachment 537719 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> This has become a complicated patch! Can you write up a brief description of
> what it's doing?

This patch changes the behavior so that when a new tab is created in Panorama, the tab would be selected.  This triggers the UI.onTabSelect() and then calls the zoom in animation.

Comment 36

[Raymond Lee [:raymondlee]]

Attachment: v8

(In reply to comment #34)
> ::: browser/base/content/tabview/groupitems.js
> @@ +1785,5 @@
> > +  // Parameters:
> > +  //  url - the new tab should open this url as well
> > +  //  options - the options object
> > +  //    noVisibleTabs - boolean indicates whether ther aer no visible tabs 
> > +  //      on the tabstrip
> 
> Typo: ther aer

Updated the comment

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug633190.js
> @@ +33,5 @@
> > +
> > +// Open a new tab when the active tab item is an orphan tab.
> > +function test3() {
> > +  registerCleanupFunction(function () TabView.hide());
> > +
> 
> Shouldn't this clean-up be registered in test1?

Moved the clean-up code to test1

Comment 37

[Ian Gilman [:iangilman]]

Comment on attachment 537719 [details] [diff] [review]
v7

Review of attachment 537719 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 38

[Ian Gilman [:iangilman]]

Comment on attachment 537944 [details] [diff] [review]
v8

Review of attachment 537944 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the description. Looks good to me.

::: browser/base/content/tabview/groupitems.js
@@ +1788,5 @@
> +  //    noVisibleTabs - boolean indicates no visible tabs exist in Panorama
> +  newTab: function GroupItem_newTab(url, options) {
> +    if (options && options.noVisibleTabs)
> +      UI.closedLastTabInTabView = true;
> +

closedLastTabInTabView sounds like there are no more tabs in Panorama at all... I think you mean closedLastVisibleTab or some such.

Comment 39

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to comment #38)
> Comment on attachment 537944 [details] [diff] [review] [review]
> v8
> 
> Review of attachment 537944 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the description. Looks good to me.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1788,5 @@
> > +  //    noVisibleTabs - boolean indicates no visible tabs exist in Panorama
> > +  newTab: function GroupItem_newTab(url, options) {
> > +    if (options && options.noVisibleTabs)
> > +      UI.closedLastTabInTabView = true;
> > +
> 
> closedLastTabInTabView sounds like there are no more tabs in Panorama at
> all... I think you mean closedLastVisibleTab or some such.

It actually means there are no more tabs in Panorama.  options.noVisibleTabs is a bit confusing so I've changed that to options.closedLastTab.  When a user closes the last tab item in panorama, it would create a new tab in a group and does the zoom in animation.  options.closedLastTab and UI.closedLastTabInTabView are for keeping this behavior after applying this patch.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/33bcd05554f3

Comment 41

[Ian Gilman [:iangilman]]

(In reply to comment #39)
> It actually means there are no more tabs in Panorama.  options.noVisibleTabs
> is a bit confusing so I've changed that to options.closedLastTab.  When a
> user closes the last tab item in panorama, it would create a new tab in a
> group and does the zoom in animation.  options.closedLastTab and
> UI.closedLastTabInTabView are for keeping this behavior after applying this
> patch.

I see; thanks for clarifying!

Comment 42

[Boris Zbarsky [:bzbarsky]]

Backed out due to mochitest-other orange.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

This was the reason the patches got backed out. It actually didn't pass try:

http://tbpl.mozilla.org/?tree=Try&rev=74390e25bf32

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is no longer visible

This is the same failure that we saw on m-i:

http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f0472d3bf28a
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0fd6f88a7c1b

Comment 44

[Raymond Lee [:raymondlee]]

(In reply to comment #43)
> This was the reason the patches got backed out. It actually didn't pass try:
> 
> http://tbpl.mozilla.org/?tree=Try&rev=74390e25bf32
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/tabview/
> browser_tabview_privatebrowsing.js | Tab View is no longer visible
> 
> This is the same failure that we saw on m-i:
> 
> http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f0472d3bf28a
> http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0fd6f88a7c1b


It's odd that it passed with this.
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33bcd05554f3

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #44)
> It's odd that it passed with this.
> http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33bcd05554f3

Totally, yeah. This is definitely intermittent and I have no real idea what's happening there (yet).

Comment 46

[Raymond Lee [:raymondlee]]

(In reply to comment #45)
> (In reply to comment #44)
> > It's odd that it passed with this.
> > http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33bcd05554f3
> 
> Totally, yeah. This is definitely intermittent and I have no real idea
> what's happening there (yet).

I've sent this patch to try again and lets see how it goes.

Comment 47

[Raymond Lee [:raymondlee]]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #44)
> > > It's odd that it passed with this.
> > > http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=33bcd05554f3
> > 
> > Totally, yeah. This is definitely intermittent and I have no real idea
> > what's happening there (yet).
> 
> I've sent this patch to try again and lets see how it goes.

http://tbpl.mozilla.org/?tree=Try&rev=b180a38e1587

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

Happened again on win opt:

http://tinderbox.mozilla.org/showlog.cgi?log=Try/1307716271.1307717962.13443.gz

Comment 49

[Raymond Lee [:raymondlee]]

Attachment: v9

Updated the patch and it passed try twice.

http://tbpl.mozilla.org/?tree=Try&rev=9a05ca751d0f
http://tbpl.mozilla.org/?tree=Try&rev=f5d5f90d678b

Comment 50

[Ian Gilman [:iangilman]]

Comment on attachment 538761 [details] [diff] [review]
v9

Review of attachment 538761 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 51

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/623b1da3c382

Comment 53

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/623b1da3c382

Comment 54

[Matt Brubeck (:mbrubeck)]

Backed out because it might have caused a new intermittent failure in browser_tabview_privatebrowsing.js (bug 664492):
http://hg.mozilla.org/integration/mozilla-inbound/rev/4f59f296bc30

Comment 55

[Raymond Lee [:raymondlee]]

Attachment: v10

When starting/stopping private browsing mode, the zoom in/out animation would happen.  We didn't change the properly so the animation might not complete because we do the check which causes intermittent failure in browser_tabview_privatebrowsing.js. The patch should fix this.

Comment 56

[Ian Gilman [:iangilman]]

Comment on attachment 539990 [details] [diff] [review]
v10

Review of attachment 539990 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 57

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Pushed to try twice and there is an random orange which doesn't relate to this patch.
http://tbpl.mozilla.org/?tree=Try&rev=0b9a6e2e40bf
http://tbpl.mozilla.org/?tree=Try&rev=7705eb0064cf

Comment 58

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dd5e2794cd16

Comment 59

[(no longer active)]

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

Comment 60

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110626 Firefox/7.0a1

Verified issue on Ubuntu 11.04 x86, Mac OS X 10.6, WinXP, Win 7 x86  and it's no longer reproducible.

Setting status to Verified Fixed.