602708 - Closing a tab earlier than the current one results in wrong tab being shown

Status: VERIFIED FIXED

Description

[Wesley Johnston (:wesj)]

Created attachment 481705 [details] [diff] [review]
Patch

When we moved to a deck, I assumed that the deck could handle elements being deleted inside it. It looks like that isn't true. If you delete an element prior to the currently selected one, selectedIndex isn't updated, and the deck will show the wrong element.

Patch forces selectedPanel to be updated, even if it is the same as the current one. Also has to fire it for the delayed deletion of the old tab.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481705 [details] [diff] [review]
Patch

Watch your "if(" usage :)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481705 [details] [diff] [review]
Patch


>     tab.destroy();
>     this._tabs.splice(tabIndex, 1);
>+
>+    this.selectedTab = nextTab;

>       Util.executeSoon(function() {
>         Elements.browsers.removeChild(browser);
>+        Elements.browsers.selectedPanel = Browser.selectedTab.browser;

Are you sure we need this last change? We only there by calling | tab.destroy() | in the top block. And you update the selectedPanel, via selectedTab, after the call.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481705 [details] [diff] [review]
Patch

pulling r+ until we discuss the delayed delete change

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Also, we need some tests for this

Comment 5

[Wesley Johnston (:wesj)]

Util.executeSoon means that the browser.destroy method doesn't fire until after set selectedTab() is called. That means that the index doesn't actually change until some time later.

So, to be honest, the original call to set selectedTab() doesn't fix anything. Only the delayed one does. We could probably move the entire fix into the delayed delete code I think, but this seemed cleaner.

I probably should have added a comment to the changes in set selectedTab too, indicating what is going on. Seems like something I would be confused about and delete later.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I talked to Wes on IRC, asking him to see if we can remove the "delayed" destroy of the browser element. We should be able to just destroy it right away now.

Comment 7

[Wesley Johnston (:wesj)]

Created attachment 481899 [details] [diff] [review]
Patch v2

Get rid of the delayed removal of the browser. I tested this for a bit with a debug build on Android and things seemed fine, but my non-debug builds seem hosed. I'm going to clean and rebuild.

Also, added a comment pointing to this bug, and some browser-chrome tests. The tests will have to be updated when bug 598331 lands.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481899 [details] [diff] [review]
Patch v2

tested on my N900 and it seems to have no performance problems while closing a tab. Browser-chrome tests are all green.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/fbccbe691b6a

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Crash bug reported based on regression: Bug 603680

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Simple test cases are verified:

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre)Gecko/20101012 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101012 Firefox/4.0b8pre Fennec/4.0b2pre