407162 - Refactor SessionStore so that it could retrieve data for a single tab

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Needed for bug 298571 and bug 393716.

Comment 1

[Simon Bünzli]

Attachment: method body shuffling

This patch just moves two loops into new methods so that we can retrieve history and form/scroll data for a single tab without having to iterate over all tabs of one window.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 291898 [details] [diff] [review]
method body shuffling

>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v
>retrieving revision 1.84
>diff -u -8 -d -p -r1.84 nsSessionStore.js
>--- browser/components/sessionstore/src/nsSessionStore.js	5 Dec 2007 01:02:57 -0000	1.84
>+++ browser/components/sessionstore/src/nsSessionStore.js	6 Dec 2007 16:50:35 -0000
>@@ -542,23 +542,21 @@ SessionStoreService.prototype = {
>   onTabClose: function sss_onTabClose(aWindow, aTab) {
>     var maxTabsUndo = this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>     // don't update our internal state if we don't have to
>     if (maxTabsUndo == 0) {
>       return;
>     }
>     
>     // make sure that the tab related data is up-to-date
>-    this._saveWindowHistory(aWindow);
>-    this._updateTextAndScrollData(aWindow);
>+    var tabState = this._collectTabData(aTab, aTab.linkedBrowser);

if we can always get the browser via tab.linkedBrowser then we should simplify by just passing the tab in.


>-      else {
>-        tabData.entries[0] = { url: browser.currentURI.spec };
>-        tabData.index = 1;
>+      if (browsers[i] == tabbrowser.selectedBrowser) {
>+        this._windows[aWindow.__SSi].selected = i + 1;
>       }

nit: could drop the braces here now

>+  _collectTabData: function sss_collectTabData(aTab, aBrowser) {
>+    var tabData = { entries: [], index: 0 };
>+    
>+    if (!aBrowser || !aBrowser.currentURI)
>+      // can happen when calling this function right after .addTab()
>+      return tabData;
>+    else if (aBrowser.parentNode.__SS_data && aBrowser.parentNode.__SS_data._tab)
>+      // use the data to be restored when the tab hasn't been completely loaded
>+      return aBrowser.parentNode.__SS_data;
>+    
>+    var history = null;
>+    try {
>+      history = aBrowser.sessionHistory;
>+    }
>+    catch (ex) { } // this could happen if we catch a tab during (de)initialization
>+    
>+    if (history && aBrowser.parentNode.__SS_data && aBrowser.parentNode.__SS_data.entries[history.index]) {

nit: can you fix the line length here?

>+  _updateTextAndScrollDataForTab:
>+    function sss_updateTextAndScrollDataForTab(aWindow, aBrowser, aTabData) {
>+    var text = [];
>+    if (aBrowser.parentNode.__SS_text && this._checkPrivacyLevel(aBrowser.currentURI.schemeIs("https"))) {
>+      for (var ix = aBrowser.parentNode.__SS_text.length - 1; ix >= 0; ix--) {
>+        var data = aBrowser.parentNode.__SS_text[ix];
>+        if (!data.cache)
>+          // update the text element's value before adding it to the data structure
>+          data.cache = encodeURI(data.element.value);
>+        text.push(data.id + "=" + data.cache);
>+      }
>+    }
>+    if (aBrowser.currentURI.spec == "about:config")
>+      text = ["#textbox=" + encodeURI(aBrowser.contentDocument.getElementById("textbox").wrappedJSObject.value)];

nit: line length

r=me w/ nits fixed

Comment 3

[Simon Bünzli]

Attachment: nits fixed

Drivers: This refactoring is needed to fix the "wanted-firefox3" bug 298571.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 292063 [details] [diff] [review]
nits fixed

a=mconnor on behalf of drivers for 1.9

Comment 5

[Simon Bünzli]

Note to self: Will need to unbitrot the patch for check-in once bug 407166 is fixed.

Comment 6

[Reed Loden [:reed]]

Attachment: unbitrotten patch

Simon, will this work? I _think_ got everything merged properly from bug 407166, but I'm not sure.

Comment 7

[Simon Bünzli]

Comment on attachment 292564 [details] [diff] [review]
unbitrotten patch

I've diff'd the patches and the only differences are the expected changes from bug 407166 - so this looks perfectly fine. Thanks for unbitrotting and the quick landing!

Comment 8

[Reed Loden [:reed]]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.86; previous revision: 1.85
done