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

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #481705 - Flags: review?(mark.finkle)
Comment on attachment 481705 [details] [diff] [review]
Patch

Watch your "if(" usage :)
Attachment #481705 - Flags: review?(mark.finkle) → review+
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 on attachment 481705 [details] [diff] [review]
Patch

pulling r+ until we discuss the delayed delete change
Attachment #481705 - Flags: review+ → review?(mark.finkle)
Also, we need some tests for this
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 7

7 years ago
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.
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+
pushed:
http://hg.mozilla.org/mobile-browser/rev/fbccbe691b6a
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 7 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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.