Closed Bug 597584 Opened 10 years ago Closed 10 years ago

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

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1b3
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: iann_bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 586068 and Bug 596806 have recently landed on Firefox trunk and we should probably port them to SeaMonkey.
Sorry, i didn't noticed this bug and opened bug 617987 for 596806. Work for Bug 586068 will be done here.
Status: NEW → ASSIGNED
Attached patch v 0.1 (obsolete) — Splinter Review
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)
Attached patch v 0.2 (obsolete) — Splinter Review
Incorporates important fixes from bug 602555
Attachment #496792 - Attachment is obsolete: true
Attachment #496793 - Flags: review?(neil)
Attachment #496792 - Flags: review?(neil)
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
Hmm, I guess I need a profile with a large number of tabs to test this...
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.
Attached patch v 0.3 (obsolete) — Splinter Review
>>+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)
(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.
(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?
(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.
(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.
(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.
(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
Attached patch v 0.4 (obsolete) — Splinter Review
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 :-(
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+
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+
Pushed: http://hg.mozilla.org/comm-central/rev/c786f8559bec
Status: ASSIGNED → RESOLVED
Closed: 10 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
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 :-(
Depends on: 634528
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" ;-)
There isn't any _tPos. in suite code :P
Attached patch bustage fixSplinter Review
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.