Current behavior: The undo button fades away after 15 seconds, and then the Firefox window closes somewhat unexpectedly. Desired behavior: Only for the last group, the undo button should stay indefinitely (meaning until the browser window or close button is manually closed).

Pushed to try and waiting for the results

Passed try

Comment on attachment 474398 [details] [diff] [review] v1 I'd like to suggest a couple of design refinements: * This behavior should not be based on whether the group in question is the last group; It should be based on whether the group in question contains all of the remaining tabs in the window. For instance, if there are orphan tabs, it's okay to close all groups. Likewise, if there are other groups, but they are all empty and there are no orphans, you'd invoke this behavior on the last group with tabs. * If, after keeping the "undo" state indefinitely, new tabs are added, either as orphans or to other groups, the 15 second count down should resume on the group that's being held in stasis. Aza, would you agree?

Also, this test has a 15 second time out in it; that's unacceptable. Right now the browser chrome mochitests take a little over a minute (on my computer); adding an additional 15 seconds for a single test is way too much. On the other hand, how would one test this feature without a 15 second delay? Ehsan, any thoughts on this?

(In reply to comment #3) > Comment on attachment 474398 [details] [diff] [review] > v1 > > I'd like to suggest a couple of design refinements: > > * This behavior should not be based on whether the group in question is the > last group; It should be based on whether the group in question contains all of > the remaining tabs in the window. For instance, if there are orphan tabs, it's > okay to close all groups. Likewise, if there are other groups, but they are all > empty and there are no orphans, you'd invoke this behavior on the last group > with tabs. Done. > > * If, after keeping the "undo" state indefinitely, new tabs are added, either > as orphans or to other groups, the 15 second count down should resume on the > group that's being held in stasis. > When a new tab is added, the zoom in would happen so it is not necessary to resume the count down on the group as it would be removed when exiting the Panorama UI. Note: we still need to address the 15 timeout check in the test.

Waiting 15 seconds in a test is a horrible idea! I think we should add a mechanism inside the code to override the timeout completely, and in the test use that mechanism to override the timeout, test the behavior without using a timeout, and then restore the timeout. Alternatively, if the timeout is crucial here, we should get a Litmus test for this. And we can always have both a Litmus test and an automated test. Caveat: when I say Litmus, I might mean Mozmill... ;-)

I think the easiest thing to do would be to make a property for the timeout length, and set that down to 1 ms in the test.

Raymond, can you finish this one up? Let me know if you still have questions about it.

Comment on attachment 482186 [details] [diff] [review] v3 Looks good. Looks like this patch and bug 599626 have a lot of overlap; you'll have to clean up some conflicts. I like what you've done with this patch, though. A few small items: >+ this._undoButtonTimeoutId = setTimeout(function() { >+ self._fadeAwayUndoButton(); }, this._fadeAwayUndoButtonDelay); Please break that second line like so: this._undoButtonTimeoutId = setTimeout(function() { self._fadeAwayUndoButton(); }, this._fadeAwayUndoButtonDelay); >+ // Removes the group item, it's children and it's container. Should be "its" in both cases. >+ let endGame = function() { >+ window.removeEventListener("tabviewhidden", endGame, false); >+ ok(!TabView.isVisible(), "Tab View is hidden"); >+ groupItem.setFadeAwayUndoButtonDelay(15000); >+ groupItem.setFadeAwayUndoButtonDuration(300); Rather than hard-coding those numbers, save the original values before setting new values, and then set them back to the saved values at the end. f+ with those items.

(In reply to comment #11) > Comment on attachment 482186 [details] [diff] [review] > v3 > > Looks good. > > Looks like this patch and bug 599626 have a lot of overlap; you'll have to > clean up some conflicts. I like what you've done with this patch, though. > > A few small items: > > >+ this._undoButtonTimeoutId = setTimeout(function() { > >+ self._fadeAwayUndoButton(); }, this._fadeAwayUndoButtonDelay); > > Please break that second line like so: > > this._undoButtonTimeoutId = setTimeout(function() { > self._fadeAwayUndoButton(); > }, this._fadeAwayUndoButtonDelay); > Done > >+ // Removes the group item, it's children and it's container. > Done > Should be "its" in both cases. > > >+ let endGame = function() { > >+ window.removeEventListener("tabviewhidden", endGame, false); > >+ ok(!TabView.isVisible(), "Tab View is hidden"); > >+ groupItem.setFadeAwayUndoButtonDelay(15000); > >+ groupItem.setFadeAwayUndoButtonDuration(300); > > Rather than hard-coding those numbers, save the original values before setting > new values, and then set them back to the saved values at the end. > Done

Comment on attachment 482500 [details] [diff] [review] v3 Switching review to dolske, as dietrich says he's not reviewing non-blockers for the time being.

Comment on attachment 482500 [details] [diff] [review] v3 >+ // Function: setFadeAwayUndoButtonDelay >+ // Sets the delay time for undo button to fade away. >+ setFadeAwayUndoButtonDelay: function GroupItem_setFadeAwayUndoButtonDelay(milliseconds) { >+ this._fadeAwayUndoButtonDelay = milliseconds; >+ }, >+ >+ // ---------- >+ // Function: getFadeAwayUndoButtonDelay >+ // Gets the delay time for undo button to fade away. >+ getFadeAwayUndoButtonDelay: function GroupItem_getFadeAwayUndoButtonDelay() { >+ return this._fadeAwayUndoButtonDelay; >+ }, >+ >+ // ---------- >+ // Function: setFadeAwayUndoButtonDuration >+ // Sets the duration for the undo button to fade away. >+ setFadeAwayUndoButtonDuration: function GroupItem_setFadeAwayUndoButtonDuration(milliseconds) { >+ this._fadeAwayUndoButtonDuration = milliseconds; >+ }, >+ >+ // ---------- >+ // Function: getFadeAwayUndoButtonDuration >+ // Gets the duration for the undo button to fade away. >+ getFadeAwayUndoButtonDuration: function GroupItem_getFadeAwayUndoButtonDuration() { >+ return this._fadeAwayUndoButtonDuration; >+ }, Drop the get/set functions, and just use the property directly (maybe removing "_" prefix to indicate it's not just used internally, but since it's basically just for the test I don't care about that too much.) r+ with that.

(In reply to comment #14) > Comment on attachment 482500 [details] [diff] [review] > v3 > > Drop the get/set functions, and just use the property directly (maybe removing > "_" prefix to indicate it's not just used internally, but since it's basically > just for the test I don't care about that too much.) > > r+ with that. Done

Request blocking2.0 because Bug 597188 has blocking2.0=betaN+ which depends on this bug.

Updated the patch for latest code in trunk

Comment on attachment 486847 [details] [diff] [review] Patch for check-in This bug is now blocking, so removing the approval request. Raymond, please package for check in.

(In reply to comment #18) > Comment on attachment 486847 [details] [diff] [review] > Patch for check-in > > This bug is now blocking, so removing the approval request. Raymond, please > package for check in. Nevermind... I see you've already done that. Landing on panorama-central.

Landed on panorama-central: http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/f6ddea7a3ed7 (sorry for the mixed up communication, Raymond)

(In reply to comment #21) > Landed on panorama-central: > > http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/f6ddea7a3ed7 > > (sorry for the mixed up communication, Raymond) That's ok ;)

Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/03a3ee4ece35

Verified on minefield nightly build of 20101112