1965508 - Replace uiStateInitialized with a more robust way to check if backup state is needed

Status: NEW

(Firefox :: Sidebar, task, P3)

Description

[Jonathan Sudiaman [:jsudiaman]]

This is the existing logic that checks whether backup state should be loaded:

https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/browser/components/sidebar/browser-sidebar.js#456-471

requestIdleCallback(() => {
  const windowPrivacyMatches =
    !window.opener || this.windowPrivacyMatches(window.opener, window);
  // If other sources (like session store or source window) haven't set the
  // UI state at this point, load the backup state. (Do not load the backup
  // state if this is a popup, or we are coming from a window of a different
  // privacy level.)
  if (
    !this.uiStateInitialized &&
    !this.inSingleTabWindow &&
    (this.sidebarRevampEnabled || windowPrivacyMatches)
  ) {
    const backupState = this.SidebarManager.getBackupState();
    this.initializeUIState(backupState);
  }
});

Rather than relying on this.uiStateInitialized and an idle period from requestIdleCallback(), it would be more useful to check the conditions that determine whether backup state is needed in the first place.

Comment 1

[Sarah Clements [:sclements]]

Jonathan, how are you feeling about importance and priority for this? (What's the risk if we don't do this anytime soon).

Comment 2

[Jonathan Sudiaman [:jsudiaman]]

I don't believe there is really any risk here.

Digging up an old Phabricator comment that most likely inspired this bug:

I think this is fine, but its one of those gnarly cases where we want to use the backup when something hasn't happened. Do we know the idlecallback won't get in a race with SessionStore's restoreWindowFeatures, where it restores the sidebar inside that setTimeout?

The possible cases are:

• SessionStore calls restoreSidebar() before this happens, thus we bail out of this condition.
• SessionStore will not call restoreSidebar() because session store data was purged, so we indeed want to load the backup state.
• SessionStore hasn't called restoreSidebar() yet, so we load the backup state. Once restoreSidebar() is called, the backup state will be overwritten.

The third case is not ideal, but it at least guarantees we will not be throwing away per-window data if we have it. I also have not seen the third case happen, usually restoreSidebar() will finish first. Maybe SessionStore could have a promise that resolves whenever either sidebar is restored, or we know that we will not have to restore it - but it seems kinda overkill to me, because we'd have to make it per-window and all that.