ctrlTab._initRecentlyUsedTabs is called way too often

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug, {dev-doc-needed, perf})

Trunk
Firefox 51
dev-doc-needed, perf
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8777770 [details] [diff] [review]
patch

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)
(Assignee)

Comment 4

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

Comment 5

2 years ago
(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+

Comment 7

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

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbca4bc3ccd8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Comment 9

2 years ago
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.