Last Comment Bug 597584 - Port changes to session store from Bug 586068 and Bug 596806 to SeaMonkey
: Port changes to session store from Bug 586068 and Bug 596806 to SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 586068 596806 630140 634528
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-17 16:22 PDT by Ian Neal
Modified: 2011-03-25 10:19 PDT (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
v 0.1 (70.11 KB, patch)
2010-12-10 05:04 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
v 0.2 (74.49 KB, patch)
2010-12-10 05:57 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
v 0.3 (90.90 KB, patch)
2011-01-06 13:33 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
v 0.4 (91.14 KB, patch)
2011-02-10 01:55 PST, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
what actually checked in (89.78 KB, patch)
2011-02-12 01:25 PST, Misak Khachatryan
misak.bugzilla: review+
Details | Diff | Splinter Review
bustage fix (1.23 KB, patch)
2011-03-25 10:18 PDT, Misak Khachatryan
no flags Details | Diff | Splinter Review

Description Ian Neal 2010-09-17 16:22:22 PDT
Bug 586068 and Bug 596806 have recently landed on Firefox trunk and we should probably port them to SeaMonkey.
Comment 1 Misak Khachatryan 2010-12-09 09:53:13 PST
Sorry, i didn't noticed this bug and opened bug 617987 for 596806. Work for Bug 586068 will be done here.
Comment 2 Misak Khachatryan 2010-12-10 05:04:16 PST
Created attachment 496792 [details] [diff] [review]
v 0.1

This patch works, no error on Error console. I noticed some tests are failing, investigating now. This patch incorporates all fixes from Bugs 586211, 596806, 597716, 597757, 597807, 597901, 597933, 598011, 598020, 598221, 600007, 604551, 607016 (the dependent bugs, that in moment of writing this patch are fixed).
Comment 3 Misak Khachatryan 2010-12-10 05:57:25 PST
Created attachment 496793 [details] [diff] [review]
v 0.2

Incorporates important fixes from bug 602555
Comment 4 neil@parkwaycc.co.uk 2010-12-10 06:57:07 PST
Comment on attachment 496793 [details] [diff] [review]
v 0.2

>+const TAB_EVENTS = ["TabOpen", "TabClose", "TabSelect", "TabShow", "TabHide"];
I take it TabShow and TabHide are related to Panorama?

>-        Array.forEach(aWindow.getBrowser().browsers, function(aBrowser) {
>-          delete aBrowser.__SS_data;
>-        });
>+        Array.forEach(aWindow.gBrowser.tabs, function(aTab) {
>+          delete aTab.linkedBrowser.__SS_data;
>+          if (aTab.linkedBrowser.__SS_restoreState)
>+            this._resetTabRestoringState(aTab);
>+       });
Nit: indentation of the last line changed by mistake

>+    let previousState;
>+    if (previousState = browser.__SS_restoreState) {
Nit: let previousState = browser.__SS_restoreState; if (previousState)

>+      this._tabsToRestore.hidden.splice(this._tabsToRestore.hidden.indexOf(aTab));
Should be .splice(..., 1) like in the other cases.
(Also applies to .visible of course.)

>+    if (aSelectTab-- && aTabs[aSelectTab]) {
>+        aTabs.unshift(aTabs.splice(aSelectTab, 1)[0]);
>+        aTabData.unshift(aTabData.splice(aSelectTab, 1)[0]);
>+        tabbrowser.selectedTab = aTabs[0];
>+    }
Nit: wrong indentation (should be 2 spaces, not 4)

>+  _getTabForBrowser: function sss_getTabForBrowser(aBrowser) {
>+    let window = aBrowser.ownerDocument.defaultView;
>+    for (let i = 0; i < window.gBrowser.tabs.length; i++) {
>+      let tab = window.gBrowser.tabs[i];
>+      if (tab.linkedBrowser == aBrowser)
>+        return tab;
>+    }
>+  },
No return value, but I guess the loop will never fail ;-)
An alternative idea would be to do something with indexOf e.g.
let tabbrowser = aBrowser.ownerDocument.getBindingParent(aBrowser);
return tabbrowser.tabs[tabbrowser.browsers.indexOf(aBrowser)];

>+  _resetRestoringState: function sss__initRestoringState() {
Nit: sss_initRestoringState etc.

>+}
>+SessionStoreSHistoryListener.prototype = {
Nit: needs a blank line
Comment 5 neil@parkwaycc.co.uk 2010-12-10 07:10:02 PST
Hmm, I guess I need a profile with a large number of tabs to test this...
Comment 6 neil@parkwaycc.co.uk 2010-12-31 13:10:48 PST
OK, so I tried this out, and I was sorta confused because it looked like the tabs were already loaded and then they reloaded themselves in order. Is this intentional? I guess the other possibility would be to create all the tabs with the "Loading..." title and busy icon.
Comment 7 Misak Khachatryan 2011-01-06 13:33:05 PST
Created attachment 501797 [details] [diff] [review]
v 0.3

>>+const TAB_EVENTS = ["TabOpen", "TabClose", "TabSelect", "TabShow", "TabHide"];
>I take it TabShow and TabHide are related to Panorama?

Yes, i decided to take all changes to make Panorama porter life easier.

This patch picks up even more fixes and followups, with nits fixed.
Comment 8 Misak Khachatryan 2011-01-06 13:37:08 PST
(In reply to comment #6)
> OK, so I tried this out, and I was sorta confused because it looked like the
> tabs were already loaded and then they reloaded themselves in order. Is this
> intentional? I guess the other possibility would be to create all the tabs with
> the "Loading..." title and busy icon.

Yes, that's intentional. We simulate that all tabs loaded, but actually loadind few pref controlled number of them. As tab finishes loading, we starting to load next in queue, until all are loaded. This reduces startup time and CPU load. It's very like BarTab extension for Firefox.
Comment 9 neil@parkwaycc.co.uk 2011-01-08 15:46:09 PST
(In reply to comment #8)
> (In reply to comment #6)
> > OK, so I tried this out, and I was sorta confused because it looked like the
> > tabs were already loaded and then they reloaded themselves in order. Is this
> > intentional? I guess the other possibility would be to create all the tabs with
> > the "Loading..." title and busy icon.
> 
> Yes, that's intentional. We simulate that all tabs loaded, but actually loadind
> few pref controlled number of them. As tab finishes loading, we starting to
> load next in queue, until all are loaded. This reduces startup time and CPU
> load. It's very like BarTab extension for Firefox.

Yes, but it's not clear that some tabs are queued while others have loaded.
Comment 10 neil@parkwaycc.co.uk 2011-01-08 15:57:56 PST
Comment on attachment 501797 [details] [diff] [review]
v 0.3

>-    if (aWinData.isPopup)
>+    if (aWinData.isPopup) {
>       this._windows[aWindow.__SSi].isPopup = true;
>-    else
>+      if (aWindow.gURLBar) {
>+        aWindow.gURLBar.readOnly = true;
>+        aWindow.gURLBar.setAttribute("enablehistory", "false");
>+      }
>+    }
>+    else {
>       delete this._windows[aWindow.__SSi].isPopup;
>+      if (aWindow.gURLBar) {
>+        aWindow.gURLBar.readOnly = false;
>+        aWindow.gURLBar.setAttribute("enablehistory", "true");
>+      }
>+    }
We don't disable the URLbar for popups.

>+  _getTabForBrowser: function sss_getTabForBrowser(aBrowser) {
>+    let window = aBrowser.ownerDocument.defaultView;
>+    for (let i = 0; i < window.getBrowser().tabs.length; i++) {
>+      let tab = window.getBrowser().tabs[i];
Nit: aBrowser.ownerDocument.defaultView.getBrowser().tabs should be a local.

>-        Services.obs.notifyObservers(this.windowToFocus,
>+        Services.obs.notifyObservers(null,
Erroneous change?

>     while (normalWindowIndex < this._closedWindows.length &&
>-           this._closedWindows[normalWindowIndex].isPopup)
>+           !!this._closedWindows[normalWindowIndex].isPopup)
Meaningless change?

>+  _ensureTabsProgressListener: function sss_ensureTabsProgressListener(aWindow) {
>+    let tabbrowser = aWindow.getBrowser();
>+    if (tabbrowser.mTabsProgressListeners.indexOf(gRestoreTabsProgressListener) == -1)
>+      tabbrowser.addTabsProgressListener(gRestoreTabsProgressListener);
[Could use try/catch because tabbrowser already does the check]
Comment 11 neil@parkwaycc.co.uk 2011-01-08 16:11:52 PST
Comment on attachment 501797 [details] [diff] [review]
v 0.3

>+    if (aOverwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount)
&& tabbrowser.tabContainer.selectedIndex >=
[we don't have _tPos]

Session store really needs something to protect your .json if session store fails to restore your session. Otherwise it likes to throw it away :-(
Comment 12 neil@parkwaycc.co.uk 2011-01-09 08:01:33 PST
(In reply to comment #9)
> it's not clear that some tabs are queued while others have loaded.

And it's not easy to change this, because restoreHistory restores the tab state immediately (thus overwriting the title and icon) but the actual tab contents are only cascaded.

On the other hand, is there a bug to force all the currently visible tabs to restore first without cascading?
Comment 13 Misak Khachatryan 2011-01-16 05:05:30 PST
(In reply to comment #10)
> Comment on attachment 501797 [details] [diff] [review]
> v 0.3
> 
> We don't disable the URLbar for popups.
> 

Fixed.

> >-        Services.obs.notifyObservers(this.windowToFocus,
> >+        Services.obs.notifyObservers(null,
> Erroneous change?
> 
This i picked from firefox, and ss should notify all observers of topic.

> >     while (normalWindowIndex < this._closedWindows.length &&
> >-           this._closedWindows[normalWindowIndex].isPopup)
> >+           !!this._closedWindows[normalWindowIndex].isPopup)
> Meaningless change?
> 
Sorry, fixed.

> >+  _ensureTabsProgressListener: function sss_ensureTabsProgressListener(aWindow) {
> >+    let tabbrowser = aWindow.getBrowser();
> >+    if (tabbrowser.mTabsProgressListeners.indexOf(gRestoreTabsProgressListener) == -1)
> >+      tabbrowser.addTabsProgressListener(gRestoreTabsProgressListener);
> [Could use try/catch because tabbrowser already does the check]

Done.
Comment 14 Misak Khachatryan 2011-01-16 05:16:53 PST
(In reply to comment #11)
> Comment on attachment 501797 [details] [diff] [review]
> v 0.3
> 
> >+    if (aOverwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount)
> && tabbrowser.tabContainer.selectedIndex >=
> [we don't have _tPos]
> 

IMHO it's worth to introduce it, if You don't mind. That should improve our tabbrowser compatibility. If you like this idea, i'll file a blocker bug. It's a one line patch.
Comment 15 Misak Khachatryan 2011-01-16 05:20:20 PST
(In reply to comment #12)
> (In reply to comment #9)
> > it's not clear that some tabs are queued while others have loaded.
> 
> And it's not easy to change this, because restoreHistory restores the tab state
> immediately (thus overwriting the title and icon) but the actual tab contents
> are only cascaded.
> 

Personally i like Bartab approach here. It didn't load tab until you didn't clicked on it. But from the other hand You should wait until tab loaded to use it. 

> On the other hand, is there a bug to force all the currently visible tabs to
> restore first without cascading?

No, AFAIK. You can completely disable cascading though.
Comment 16 neil@parkwaycc.co.uk 2011-01-20 10:05:28 PST
(In reply to comment #13)
> (In reply to comment #10)
> > (From update of attachment 501797 [details] [diff] [review])
> > >-        Services.obs.notifyObservers(this.windowToFocus,
> > >+        Services.obs.notifyObservers(null,
> > Erroneous change?
> This i picked from firefox, and ss should notify all observers of topic.
The parameter is not the observer to notifiy, but the subject of the notification. In particular nsSuiteGlue.js needs to know which browser to attach the rights/places locked/update notifications to.

(In reply to comment #14)
> (In reply to comment #11)
> > (From update of attachment 501797 [details] [diff] [review])
> > >+    if (aOverwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount)
> > && tabbrowser.tabContainer.selectedIndex >=
> > [we don't have _tPos]
> IMHO it's worth to introduce it, if You don't mind. It's a one line patch.
No it's not, because there's all sorts of code to maintain _tPos.

(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > it's not clear that some tabs are queued while others have loaded.
> > 
> > And it's not easy to change this, because restoreHistory restores the tab state
> > immediately (thus overwriting the title and icon) but the actual tab contents
> > are only cascaded.
> Personally i like Bartab approach here. It didn't load tab until you didn't
> clicked on it. But from the other hand You should wait until tab loaded to use
> it. 
Right, but it's not quite obvious which tabs have loaded. And I was thinking that the focused browser's visible tabs should be ready as quickly as possible, while the other tabs aren't so important and it's good to cascade them.
Comment 17 Misak Khachatryan 2011-01-23 02:30:51 PST
(In reply to comment #16)
> (In reply to comment #13)
> The parameter is not the observer to notifiy, but the subject of the
> notification. In particular nsSuiteGlue.js needs to know which browser to
> attach the rights/places locked/update notifications to.

Fixed

> 
> (In reply to comment #14)
> > (In reply to comment #11)
> > > (From update of attachment 501797 [details] [diff] [review])
> > > >+    if (aOverwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount)
> > > && tabbrowser.tabContainer.selectedIndex >=
> > > [we don't have _tPos]
> > IMHO it's worth to introduce it, if You don't mind. It's a one line patch.
> No it's not, because there's all sorts of code to maintain _tPos.
> 

Well, I'll file new bug for that, using workaround now.

> (In reply to comment #15)
> Right, but it's not quite obvious which tabs have loaded. And I was thinking
> that the focused browser's visible tabs should be ready as quickly as possible,
> while the other tabs aren't so important and it's good to cascade them. 

What about having special boolean pref that will load tabstrip visible tabs first? Or even without any new pref, if browser.sessionstore.max_concurrent_tabs < 0, which assumes that we will always cascade invisible tabs. But personally I'll prefer fast browser startup, which means less tab loading. For example in my typical session having 2 windows with more than 20 tabs each i'll get responsive browser much faster when loading 3 tabs and cascading others than loading 10-12 visible tabs. If i need not yet loaded tab, clicking on it and wait for it's load also faster too that waiting browser to load all that visible ones.
Comment 18 Misak Khachatryan 2011-02-10 01:55:41 PST
Created attachment 511325 [details] [diff] [review]
v 0.4
Comment 19 neil@parkwaycc.co.uk 2011-02-10 08:27:52 PST
(In reply to comment #17)
> But personally I'll prefer fast browser startup, which means less tab loading.
> For example in my typical session having 2 windows with more than 20 tabs each
> i'll get responsive browser much faster when loading 3 tabs and cascading
> others than loading 10-12 visible tabs.
10-12? How wide is your tabstrip? I only get 6 tabs before it overflows. Here was me thinking that you could live with 6 tabs loading on startup :-(
Comment 20 Misak Khachatryan 2011-02-10 22:17:15 PST
I just counted: on my smallest monitor i have 15 visible tabs and 65 overall in one window :P And it's going to be typical for many users.
Comment 21 neil@parkwaycc.co.uk 2011-02-11 08:17:28 PST
I guess I can solve my problem by setting max_concurrent_tabs to 6.
Comment 22 neil@parkwaycc.co.uk 2011-02-11 08:41:47 PST
Comment on attachment 511325 [details] [diff] [review]
v 0.4

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
No changes here please.

>+  _sendTabRestoredNotification: function sss_sendTabRestoredNotification(aTab) {
>+      let event = aTab.ownerDocument.createEvent("Events");
>+      event.initEvent("SSTabRestored", true, false);
>+      aTab.dispatchEvent(event);
Nit: incorrect indentation.

>+// This is used to help meter the number of restoring tabs. This is the control
>+// point for telling the next tab to restore. It gets attached to each gBrowser
>+// via gBrowser.addTabsProgressListener
[I wonder why they use a separate object for this...]

>+function SessionStoreSHistoryListener(ss, aTab) {
>+  this.tab = aTab;
>+  this.ss = ss;
>+}
>+
>+SessionStoreSHistoryListener.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsISHistoryListener,
>+                                         Components.interfaces.nsISupportsWeakReference]),
>+  browser: null,
tab: null, no? r=me with this fixed.
Comment 23 Misak Khachatryan 2011-02-12 01:25:35 PST
Created attachment 511933 [details] [diff] [review]
what actually checked in

I fixed all nits and also added another try/catch block on removeProgressListener previously missed.
Comment 24 Misak Khachatryan 2011-02-12 01:27:52 PST
Pushed: http://hg.mozilla.org/comm-central/rev/c786f8559bec
Comment 25 Serge Gautherie (:sgautherie) 2011-02-13 05:04:06 PST
It looks like this changeset changed previously random failures (iirc) into (near-)permanent ones on all platforms:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_522545.js | sessionstore got correct userTypedValue - Got undefined, expected mozilla.org
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_522545.js | sessionstore got correct userTypedClear - Got undefined, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_bug510890.js | The window correctly restored the URL - Got about:blank, expected about:config
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_bug510890.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_bug510890.js | Found a browser window after previous test timed out
}

Could you check that?
Comment 26 Misak Khachatryan 2011-02-13 10:42:41 PST
I have fix for that in mq, but it depends from other bug, they all in Neil's review queue.
Comment 27 neil@parkwaycc.co.uk 2011-02-15 08:36:54 PST
(In reply to comment #11)
>(From update of attachment 501797 [details] [diff] [review])
>>+    if (aOverwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount)
>&& tabbrowser.tabContainer.selectedIndex >=
I've only just notice that the final patch actually has tabbrowser.tabs.selectedIndex :-(
Comment 28 Misak Khachatryan 2011-02-15 22:45:38 PST
I planning to implement _tPos, i'll fix it there. Or should i fix it here ?
Comment 29 neil@parkwaycc.co.uk 2011-03-25 09:57:36 PDT
(In reply to comment #28)
> I planning to implement _tPos, i'll fix it there. Or should i fix it here ?
I'm not planning on supporting _tPos. Just land a "bustage fix" ;-)
Comment 30 Misak Khachatryan 2011-03-25 10:05:53 PDT
There isn't any _tPos. in suite code :P
Comment 31 Misak Khachatryan 2011-03-25 10:18:22 PDT
Created attachment 521849 [details] [diff] [review]
bustage fix
Comment 32 Misak Khachatryan 2011-03-25 10:19:55 PDT
Now i got what You mean ...

http://hg.mozilla.org/comm-central/rev/751318dd73aa

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