Last Comment Bug 701377 - setTabState() always unhides the tab
: setTabState() always unhides the tab
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks: 675539
  Show dependency treegraph
 
Reported: 2011-11-10 07:53 PST by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2012-03-29 02:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (2.01 KB, patch)
2011-11-10 07:58 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
paul: review+
Details | Diff | Splinter Review
patch v2 (with test) (5.08 KB, patch)
2011-12-08 20:37 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
paul: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-11-10 07:53:22 PST
When I have a hidden blank tab (in another group for example) and call ss.setTabState(blankTab) I expect it not to be restored but be ready for restoration when the corresponding group gets shown again. That's because of setTabState() calls:

> this.restoreHistoryPrecursor(window, [aTab], [tabState], 0, 0, 0);

And restoreHistoryPrecursor contains these lines:

> let unhiddenTabs = aTabData.filter(function (aData) !aData.hidden).length;
> 
> // if all tabs to be restored are hidden, make the first one visible
> if (unhiddenTabs == 0) {
>   aTabData[0].hidden = false;
> } else if (aTabs.length > 1) {
>   ...
> }

This results in very strange results (especially in combination with Panorama) as the tab gets shown, loads and is hidden again.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-11-10 07:58:05 PST
Created attachment 573511 [details] [diff] [review]
patch v1

This patch moves the code that unhides the first tab (to have at least one shown tab) to restoreWindow().
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-21 16:08:12 PST
Comment on attachment 573511 [details] [diff] [review]
patch v1

Review of attachment 573511 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine by me, just add a test before landing. Thanks Tim!
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-12-08 20:37:13 PST
Created attachment 580303 [details] [diff] [review]
patch v2 (with test)

Added a sessionstore test. Alas, there are intermittent try failures which need to be investigated:

browser_tabview_bug627288.js times out because the tab passed to ss.restoreTab() seems to be removed from its window. I think there's some issue with setTimeout() and the tab being removed in the meantime...
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-22 22:04:44 PST
I know you're a busy man Tim, but any update on those oranges?
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-01-25 06:59:01 PST
It's been in my todo list for weeks and I've always wanted to look at this again. Hope I can find the time soon. Don't hesitate to steal this in the meantime if anyone feels like it :)
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-27 04:33:17 PDT
Comment on attachment 580303 [details] [diff] [review]
patch v2 (with test)

So I pushed it to try again and had one strange intermittent error that didn't seem to recur:

https://tbpl.mozilla.org/?tree=Try&rev=ad05cc963b54

A second try push with some debug output didn't show it, too:

https://tbpl.mozilla.org/?tree=Try&rev=8ebc88495609
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-28 00:26:33 PDT
https://hg.mozilla.org/integration/fx-team/rev/9f166123a74f
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-29 02:53:56 PDT
https://hg.mozilla.org/mozilla-central/rev/9f166123a74f

Note You need to log in before you can comment on or make changes to this bug.