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
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: wesj, Assigned: wesj)
References
Details
Attachments
(1 file, 1 obsolete file)
4.76 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | 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.
Assignee | ||
Updated•13 years ago
|
Attachment #481705 -
Flags: review?(mark.finkle)
Comment 1•13 years ago
|
||
Comment on attachment 481705 [details] [diff] [review] Patch Watch your "if(" usage :)
Attachment #481705 -
Flags: review?(mark.finkle) → review+
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 481705 [details] [diff] [review] Patch pulling r+ until we discuss the delayed delete change
Attachment #481705 -
Flags: review+ → review?(mark.finkle)
Comment 4•13 years ago
|
||
Also, we need some tests for this
Assignee | ||
Comment 5•13 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.
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 8•13 years ago
|
||
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+
Comment 9•13 years ago
|
||
pushed: http://hg.mozilla.org/mobile-browser/rev/fbccbe691b6a
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•