Closed Bug 1292095 Opened 5 years ago Closed 5 years ago

ctrlTab._initRecentlyUsedTabs is called way too often

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, perf)

Attachments

(1 file)

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.
Attached patch patchSplinter Review
This implements SSWindowRestored (event name inspired by SSTabRestored).
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8777770 - Flags: review?(mdeboer)
(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 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");`?
Attachment #8777770 - Flags: review?(mdeboer)
Because Restored isn't one of the window states... it's either Busy or Ready. The event name should be SSWindowRestored, not SSWindowStateRestored.
(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 on attachment 8777770 [details] [diff] [review]
patch

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

OK, convinced by comment 4. :)
Attachment #8777770 - Flags: review+
Pushed by dgottwald@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/bbca4bc3ccd8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
We should put SSWindowRestored on devmo since the other sessionstore events are there too.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.