Closed Bug 602708 Opened 13 years ago Closed 13 years ago

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


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: wesj, Assigned: wesj)




(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #481705 - Flags: review?(mark.finkle)
Comment on attachment 481705 [details] [diff] [review]

Watch your "if(" usage :)
Attachment #481705 - Flags: review?(mark.finkle) → review+
Comment on attachment 481705 [details] [diff] [review]

>     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 on attachment 481705 [details] [diff] [review]

pulling r+ until we discuss the delayed delete change
Attachment #481705 - Flags: review+ → review?(mark.finkle)
Also, we need some tests for this
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.
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.
Attached patch Patch v2Splinter Review
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.
Attachment #481705 - Attachment is obsolete: true
Attachment #481705 - Flags: review?(mark.finkle)
Blocks: 603375
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.
Attachment #481899 - Flags: review+
Assignee: nobody → wjohnston
Closed: 13 years ago
Resolution: --- → FIXED
Crash bug reported based on regression: Bug 603680
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
You need to log in before you can comment on or make changes to this bug.