The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.1b3

Status

SeaMonkey
Session Restore
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ian Neal, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1b3
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

(seamonkey2.1 wanted)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
status-seamonkey2.1: --- → wanted
(Assignee)

Comment 1

6 years ago
Sorry, i didn't noticed this bug and opened bug 617987 for 596806. Work for Bug 586068 will be done here.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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).
Assignee: nobody → misak.bugzilla
Attachment #496792 - Flags: review?(neil)
(Assignee)

Comment 3

6 years ago
Created attachment 496793 [details] [diff] [review]
v 0.2

Incorporates important fixes from bug 602555
Attachment #496792 - Attachment is obsolete: true
Attachment #496793 - Flags: review?(neil)
Attachment #496792 - Flags: review?(neil)

Comment 4

6 years ago
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

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

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #496793 - Attachment is obsolete: true
Attachment #501797 - Flags: review?(neil)
Attachment #496793 - Flags: review?(neil)
(Assignee)

Comment 8

6 years ago
(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

6 years ago
(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 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 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 :-(
(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?
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
(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.
(Assignee)

Comment 15

6 years ago
(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.
(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.
(Assignee)

Comment 17

6 years ago
(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.
Target Milestone: --- → seamonkey2.1b1
(Assignee)

Comment 18

6 years ago
Created attachment 511325 [details] [diff] [review]
v 0.4
Attachment #501797 - Attachment is obsolete: true
Attachment #511325 - Flags: review?(neil)
Attachment #501797 - Flags: review?(neil)
(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 :-(
(Assignee)

Comment 20

6 years ago
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.
I guess I can solve my problem by setting max_concurrent_tabs to 6.
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.
Attachment #511325 - Flags: review?(neil) → review+
(Assignee)

Comment 23

6 years ago
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.
Attachment #511325 - Attachment is obsolete: true
Attachment #511933 - Flags: review+
(Assignee)

Comment 24

6 years ago
Pushed: http://hg.mozilla.org/comm-central/rev/c786f8559bec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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?
Flags: in-testsuite+
Target Milestone: seamonkey2.1b1 → seamonkey2.1b3
(Assignee)

Comment 26

6 years ago
I have fix for that in mq, but it depends from other bug, they all in Neil's review queue.
(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 :-(
(Assignee)

Updated

6 years ago
Depends on: 634528
(Assignee)

Comment 28

6 years ago
I planning to implement _tPos, i'll fix it there. Or should i fix it here ?
Depends on: 630140
(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" ;-)
(Assignee)

Comment 30

6 years ago
There isn't any _tPos. in suite code :P
(Assignee)

Comment 31

6 years ago
Created attachment 521849 [details] [diff] [review]
bustage fix
(Assignee)

Comment 32

6 years ago
Now i got what You mean ...

http://hg.mozilla.org/comm-central/rev/751318dd73aa
You need to log in before you can comment on or make changes to this bug.