When restoring a session, we should prioritize the tabs that are visible on the tabstrip over the ones that are in the background. While this won't necessarily make start-up any faster, it should feel faster.

Note that we already restore the selected tab first - and that shouldn't be changed. So the place to change the tab restoration order should probably be right after <http://hg.mozilla.org/mozilla-central/annotate/5354b94d250f/browser/components/sessionstore/src/nsSessionStore.js#l1758>

First pass - no tests, not sure if we can automate the testing for this anyway. Simon, Deitrich - want to take a quick look at it and let me know what you think. I can probably get this all into the initial if block, but wanted to get something up ASAP for the sprint

Comment on attachment 364465 [details] [diff] [review] Patch v0.1 >+ // XXXzpao is there an API to see how many tabs we're showing instead of >+ // making up a number? You should be able to generalize the code used to determine whether a single tab is visible in tabbrowser.xml's _notifyBackgroundTab: <http://hg.mozilla.org/mozilla-central/annotate/52110c34b6b4/browser/base/content/tabbrowser.xml#l3203> >+ let first = false, last = false; Nit: Even though JavaScript isn't statically typed, please still code as if it were and either assign numbers or nothing at all here. >+ // if first == 1, then we dont need to change any ordering Caution: JavaScript arrays are zero-based. Just for fun, aSelectTab is one-based up until where we currently move the first tab to the start of the list (hence the |aSelectTab--| which evaluates to true if aSelectTab > 0 and then decrements it to get it zero-based for actual use)...

Now with more simple logic & cleaner code. Like the comment in there, I think this is only handling LTR, so that should be addressed before a final version. Simon, looking better now? Also, thoughts on how we should test this? Addresses... > You should be able to generalize the code used to determine whether a single > tab is visible in tabbrowser.xml's _notifyBackgroundTab: > <http://hg.mozilla.org/mozilla-central/annotate/52110c34b6b4/browser/base/content/tabbrowser.xml#l3203> I took the 2 lines that I could use. I don't think it made sense to do any of this stuff there or generalize that into another function in tabbrowser.xml - that function actually needs the values calculated & we do here too (to a certain extent), but not all of them. So it made more sense to duplicate that code just a little bit. > Nit: Even though JavaScript isn't statically typed, please still code as if it > were and either assign numbers or nothing at all here. Fair enough. I changed things so it doesn't rely on that check anyway. > Caution: JavaScript arrays are zero-based. Just for fun, aSelectTab is > one-based up until where we currently move the first tab to the start of the > list (hence the |aSelectTab--| which evaluates to true if aSelectTab > 0 and > then decrements it to get it zero-based for actual use)... I did notice that and adjustments are done accordingly - I made sure the arrays stay in order & everything

Comment on attachment 365327 [details] [diff] [review] Patch v0.2 >+ // better to round up in the case of 7.1 (for example) >+ let maxTabs = Math.round(tsbo.width / ctbo.width + 0.49); Any reason not to use Math.ceil? And please rename tsbo and ctbo to something comprehensible. >+ // XXXzpao I think this only handles LTR locales AFAICT it should handle RTL locales fine, already. >+ if (aTabs.length - maxTabs > aSelectTab) // aSelectTab is leftmost >+ first = aSelectTab; aSelectTab is guaranteed to be leftmost in this case due to how we scroll it into view when restoring, right? Might be worth commenting on that, just in case we decide to correctly scroll the tabstrip during restoration (which could lead to the selected tab being somewhere in the middle at this point). >+ else if (aTabs.length == aSelectTab) // aSelectTab is rightmost >+ first = aTabs.length - maxTabs; >+ else // aSelectTab is in the middle somewhere >+ first = aTabs.length - maxTabs; Nit: Code duplication. (In reply to comment #4) > Also, thoughts on how we should test this? Maybe by restoring a 50 tab session with the 25th tab selected into a new window (through setWindowState) and ensuring that SSTabRestoring is fired first for the 25th to 30th tab (in this order). See e.g. the test for bug 464199 for the first bits.

Addresses previous comments & adds tests.

Comment on attachment 366421 [details] [diff] [review] Patch v0.3 >+ let maxTabs = Math.ceil(tabScrollBoxObject.width / tabBoxObject.width); Nit: s/maxTabs/maxVisibleTabs/ >+ if (aTabs.length - maxTabs > aSelectTab) >+ // aSelectTab is leftmost since we scroll to it when possible >+ first = aSelectTab - 1; Nit: We do use braces even for single statements when they span more than a single line (or are prepended with a comment line as in this case). >+ { "entries": [{"url": "http://example.com/", "title": "Example Web Page"}], "index": 1 } Nit: You could do without title and index. And please don't write JSON if you don't have to: there's no need to quote key names. >+ let tabbrowser = this.window.getBrowser(); Nit: Please use gBrowser instead of getBrowser() as the latter has been deprecated. >+ let test1 = generateTest(1, 13, 1, 6, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]); >+ test1.run(); Is there any reason you can't do runTest(...) instead of generateTest(...).run() ? Otherwise this is r+=me with the above nits fixed (and provided that the tests actually pass ;-)). Thanks for this nice enhancement, Paul!

Just a little note. There was a little problem with this. If user has pages which require authorization, browser will switch to that tabs on restore, so visible pages will dynamically change on restore process.

(In reply to comment #8) > Just a little note. There was a little problem with this. If user has pages > which require authorization, browser will switch to that tabs on restore, so > visible pages will dynamically change on restore process. That's true, but as far as I know, we already don't do that correction. I also think it's a bit outside of the scope of sessionstore to do that correction. Even if a tab does steal focus for authorization, we can't detect that until the data is loaded back into the tab, so it's kind of a moot point anyway. All this patch does is reorder the array of data coming from disk (before that data gets loaded into tabs). It's really just a small optimization to make it seem like we're starting up faster. Maybe Simon or Dietrich can weigh in with more authority on the matter.

Simon - fixes the things from your previous comments. Yes the tests actually pass and I enjoy making these enhancements, especially since I work here now :) Dietrich - you wanted to look at this too, so you're getting the r?.

Comment on attachment 366604 [details] [diff] [review] Patch v0.4 talked over irc, r=me

Not sure if this is the right place to comment on this, but in general it may be beneficial for session store to set smoothScroll to false (and then reset it) while closing/moving tabs around (there may be a perf gain there).

With fix from Dietrich's comments. I don't have commit access yet so if somebody could do that, awesome.

(In reply to comment #12) > Not sure if this is the right place to comment on this, but in general it may > be beneficial for session store to set smoothScroll to false (and then reset > it) while closing/moving tabs around (there may be a perf gain there). That's something that did get talked about a little bit during discussion around this bug. I think it's something to take separately though since it would involve touch more than just window opening. For this, we're not actually reordering any of the tabs, just the order we load data into them.

Could anyone familiar with this patch hazard a guess as to its potential to be affect or be affected by Tab Mix Plus? With its landing in Trunk today (20090314), tabs will not load on (re)start. Disable TMP and restart. The tabs will restore as expected. If you all think this patch is the likely cause of this, I will pass it on to TMP's authors (Gary and onemen). Thx.

(In reply to comment #16) > Could anyone familiar with this patch hazard a guess as to its potential to be > affect or be affected by Tab Mix Plus? Bug 476928 touched nsSessionStore.js as well. Should be pretty easy for the TMP devs to figure out which change it was.

(In reply to comment #17) > (In reply to comment #16) > > Could anyone familiar with this patch hazard a guess as to its potential to be > > affect or be affected by Tab Mix Plus? > > Bug 476928 touched nsSessionStore.js as well. Should be pretty easy for the TMP > devs to figure out which change it was. Thanks. I'll pass all this on to the boys.

This seems to be the cause of bug 483382. I noticed that this chrome test is run just before the browser_420786.js test.

Looks like if finish() is called from executeSoon that prevents the shell test from failing. // if we're all done, explicitly finish if (++completedTests == numTests) { executeSoon(function () { finish(); }); }

I disabled the test from this bug.

Comment on attachment 367250 [details] [diff] [review] Patch v0.5 (for checkin) a191=beltzner

Would it be possible to land this on branch with the tests disabled like we did for mozilla-central? Then we can just use bug 483382 to fix in both places? I can put up a clean branch patch...

Yep.

clean patch for 1.9.1; test is disabled (see bug 483382)

It looks very well. However, it would be more useful if Firefox caches titles of the webpages and favicons. If Firefox does it, I'd be able to find the tab that I want to use when it is loading. This requirement may be in a separated bug.

After this bug landed restoreHistoryPrecursor throws exceptions if aTabs is null or aTabs = [] can you add to restoreWindow test if there is tabs to restore before calling restoreHistoryPrecursor. maybe something like this: + if (tabs.length) { this.restoreHistoryPrecursor(aWindow, tabs, winData.tabs, (aOverwriteTabs ? (parseInt(winData.selected) || 1) : 0), 0, 0); + }

(In reply to comment #28) > After this bug landed restoreHistoryPrecursor throws exceptions if aTabs is > null or aTabs = [] > > can you add to restoreWindow test if there is tabs to restore before calling > restoreHistoryPrecursor. > > maybe something like this: > > + if (tabs.length) { > this.restoreHistoryPrecursor(aWindow, tabs, winData.tabs, > (aOverwriteTabs ? (parseInt(winData.selected) || 1) : 0), 0, 0); > + } Out of curiosity, how is a window being restored with no tabs? I guess it's legitimate to say that's a bug, but it should be spun off into a new bug.

In Tabmix we use setWindowState to remkove closed tab from the list without restore it first we call setWindowState with aOverwrite set to false. also setWindowState check if there is cookies in cookies winData before calling restoreCookies and if there is extData in winData before updating extData i think that the same should apply to tabs. we can workaround the problem by wrapping the call to setWindowState with try-catch. try {ss.setWindowState(window,state, false);} catch (e) {} but that not look right to me. i would appropriate if you can take car of open the new bug and applying the patch.

(In reply to comment #30) While I'm not arguing the validity of this problem, what I am going to argue is your assessment of them problem on your end. > In Tabmix we use setWindowState to remkove closed tab from the list without > restore it first > we call setWindowState with aOverwrite set to false. That's fine and dandy. But if you remove the tab before the window is restored, shouldn't you also be checking if there are actually any tabs to restore first. Something like this: for each (window in state.windows) { if (!window.tabs) { delete window; } } Or only check the windows when you remove tabs. That would be a workaround for you that is both safe & sensible. > try {ss.setWindowState(window,state, false);} catch (e) {} > but that not look right to me. This has the same net effect as sample code above, but is in my opinion less elegant. Simon - Should we spin this off & open a new bug for this case & if so, what's the best way to handle it?

(In reply to comment #31) > Simon - Should we spin this off & open a new bug for this case & if so, what's > the best way to handle it? The best way is to fix bug 461634 which will be the proper way to remove a tab from the list of closed tabs. Hopefully it'll make Firefox 3.5.

verified FIXED on builds: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031443 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031310

More improvements coming in bug 496458.