1292095 - ctrlTab._initRecentlyUsedTabs is called way too often

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

We rebuild the _recentlyUsedTabs too often from scratch. This is because we depend on the SSWindowStateReady event which fires on various occasions that aren't of interest for our purposes. I'm not even sure what exactly those occasions are. E.g. loading a page from about:newtab seems to do this.

We only want to rebuild _recentlyUsedTabs when a whole window has been restored. It looks like there's currently no event specifically for this, only an nsIObserver notification.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

This implements SSWindowRestored (event name inspired by SSTabRestored).

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #0)
> We rebuild the _recentlyUsedTabs too often from scratch. This is because we
> depend on the SSWindowStateReady event which fires on various occasions that
> aren't of interest for our purposes. I'm not even sure what exactly those
> occasions are. E.g. loading a page from about:newtab seems to do this.

Huh? Quel oddness. Can you file a bug about that? I'll need to look at that someday...

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8777770 [details] [diff] [review]
patch

Review of attachment 8777770 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionStore.jsm
@@ +3072,5 @@
>      TelemetryStopwatch.finish("FX_SESSION_RESTORE_RESTORE_WINDOW_MS");
>  
>      this._setWindowStateReady(aWindow);
>  
> +    this._sendWindowRestoredNotification(aWindow);

Why not `this._sendWindowStateEvent(aWindow, "Restored");`?

Comment 4

[Dão Gottwald [:dao]]

Because Restored isn't one of the window states... it's either Busy or Ready. The event name should be SSWindowRestored, not SSWindowStateRestored.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> (In reply to Dão Gottwald [:dao] from comment #0)
> > We rebuild the _recentlyUsedTabs too often from scratch. This is because we
> > depend on the SSWindowStateReady event which fires on various occasions that
> > aren't of interest for our purposes. I'm not even sure what exactly those
> > occasions are. E.g. loading a page from about:newtab seems to do this.
> 
> Huh? Quel oddness. Can you file a bug about that? I'll need to look at that
> someday...

Well, I don't know that it's a bug...

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8777770 [details] [diff] [review]
patch

Review of attachment 8777770 [details] [diff] [review]:
-----------------------------------------------------------------

OK, convinced by comment 4. :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bbca4bc3ccd8
Implement SSWindowRestored event and use it instead of SSWindowStateReady for the Ctrl+Tab panel. r=mdeboer

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbca4bc3ccd8

Comment 9

[Dão Gottwald [:dao]]

We should put SSWindowRestored on devmo since the other sessionstore events are there too.