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

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
Session Restore
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Simon Bünzli, Assigned: Simon Bünzli)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Needed for bug 298571 and bug 393716.
(Assignee)

Comment 1

11 years ago
Created attachment 291898 [details] [diff] [review]
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.
Attachment #291898 - Flags: review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Attachment #291898 - Flags: review?(gavin.sharp) → review?(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
Attachment #291898 - Flags: review?(dietrich) → review+
(Assignee)

Comment 3

11 years ago
Created attachment 292063 [details] [diff] [review]
nits fixed

Drivers: This refactoring is needed to fix the "wanted-firefox3" bug 298571.
Attachment #291898 - Attachment is obsolete: true
Attachment #292063 - Flags: approval1.9?
Comment on attachment 292063 [details] [diff] [review]
nits fixed

a=mconnor on behalf of drivers for 1.9
Attachment #292063 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 5

11 years ago
Note to self: Will need to unbitrot the patch for check-in once bug 407166 is fixed.
Target Milestone: --- → Firefox 3 M11
Keywords: checkin-needed
Created attachment 292564 [details] [diff] [review]
unbitrotten patch

Simon, will this work? I _think_ got everything merged properly from bug 407166, but I'm not sure.
Attachment #292063 - Attachment is obsolete: true
Attachment #292564 - Flags: review?(zeniko)
Keywords: checkin-needed
(Assignee)

Comment 7

11 years ago
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!
Attachment #292564 - Flags: review?(zeniko) → review+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.