597584 - Port changes to session store from Bug 586068 and Bug 596806 to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Ian Neal]

Bug 586068 and Bug 596806 have recently landed on Firefox trunk and we should probably port them to SeaMonkey.

Comment 1

[Misak Khachatryan]

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]

Attachment: 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]

Attachment: v 0.2

Incorporates important fixes from bug 602555

Comment 4

[[e-mail]]

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

[[e-mail]]

Hmm, I guess I need a profile with a large number of tabs to test this...

Comment 6

[[e-mail]]

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]

Attachment: 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]

(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

[[e-mail]]

(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

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

(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]

(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]

(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]

(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

[[e-mail]]

(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]

(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]

Attachment: v 0.4

Comment 19

[[e-mail]]

(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]

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

[[e-mail]]

I guess I can solve my problem by setting max_concurrent_tabs to 6.

Comment 22

[[e-mail]]

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]

Attachment: what actually checked in

I fixed all nits and also added another try/catch block on removeProgressListener previously missed.

Comment 24

[Misak Khachatryan]

Pushed: http://hg.mozilla.org/comm-central/rev/c786f8559bec

Comment 25

[Serge Gautherie (:sgautherie)]

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]

I have fix for that in mq, but it depends from other bug, they all in Neil's review queue.

Comment 27

[[e-mail]]

(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]

I planning to implement _tPos, i'll fix it there. Or should i fix it here ?

Comment 29

[[e-mail]]

(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]

There isn't any _tPos. in suite code :P

Comment 31

[Misak Khachatryan]

Attachment: bustage fix

Comment 32

[Misak Khachatryan]

Now i got what You mean ...

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