602555 - Refactor CSR code

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

While working on bug 601024, I got stuck reusing the same code too much and making it overly complicated with nested condition after nested condition. It was getting to be overly complicated for me to follow and I just wrote it.

So I decided to wipe the slate and do things over. It started as a rewrite, but it's not really. It took everything into account that I learned about CSR (and the different edge cases not initially designed for). I also applied a suggestion Dietrich gave while reviewing previously (use a single state attribute instead of multiple state = true steps).

So by doing this I've fixed bug 601024 (and will include the additional test here), should fix bug 600007 (I haven't actually reproduced that, but design-wise the changes here should account for that). I've taken into account how I would go about fixing bug 598267.

I haven't fixed bug 599909 nor bug 598221 here. I'll do those in their respective bugs after this. Neither should be affected by the changes here.

I'm trying to keep churn small for easier review, and because we really shouldn't do any rewriting now (that's why it's a "refactor").

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

It makes things better.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 481754 [details] [diff] [review]
Patch v0.1

>@@ -876,7 +879,12 @@ SessionStoreService.prototype = {
> 
>     // If this tab was in the middle of restoring, we want to restore the next
>     // tab. If the tab hasn't been restored, we want to remove it from the array.
>-    this._resetTabRestoringState(aTab, true, false);
>+    let previousState;
>+    if (previousState = browser.__SS_restoreState) {
>+      this._resetTabRestoringState(aTab);
>+      if (previousState == TAB_STATE_RESTORING)
>+        this.restoreNextTab();
>+    }

update the comment, since you're not visibly removing anything from any array in this chunk of code.

>@@ -2219,8 +2227,10 @@ SessionStoreService.prototype = {
> 
>     // If overwriting tabs, we want to remove __SS_restoring from the browser.
>     if (aOverwriteTabs) {
>-      for (let i = 0; i < tabbrowser.tabs.length; i++)
>-        this._resetTabRestoringState(tabbrowser.tabs[i], false, false);
>+      for (let i = 0; i < tabbrowser.tabs.length; i++) {
>+        if (tabbrowser.browsers[i].__SS_restoreState)
>+          this._resetTabRestoringState(tabbrowser.tabs[i]);
>+      }
>     }

update the comment to describe the intent, instead of the not-visible-here implementation detail.

>-  restoreTab: function(aTab) {
>+  /**
>+   * Restores the specified tab. If the tab can't be restored (eg, no history or
>+   * calling gotoIndex fails), then state changes will be rolled back.
>+   * This method will check if gTabsProgressListener is attached to the tab's
>+   * window, ensuring that we don't get caught without one.
>+   * This method removes the session history listener right before starting to
>+   * attempt a load. This will prevent cases of "stuck" listeners.
>+   *
>+   * @param aTab
>+   *        the tab to restore
>+   *
>+   * @returns true/false indicating whether or not a load actually happened

can you also describe the caller, and how the flow changes if this returns false?

>-    if (shouldRestoreNextTab)
>-      this.restoreNextTab(true);
>+    // If we didn't start a load, then we won't reset this tab through the usual
>+    // chanel (via the progress listener), so reset the tab ourselves.
>+    if (!didStartLoad)
>+      this._resetTabRestoringState(aTab);
>+
>+    return didStartLoad;

not a chanel guy myself, so would prefer channel here.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> >-    if (shouldRestoreNextTab)
> >-      this.restoreNextTab(true);
> >+    // If we didn't start a load, then we won't reset this tab through the usual
> >+    // chanel (via the progress listener), so reset the tab ourselves.
> >+    if (!didStartLoad)
> >+      this._resetTabRestoringState(aTab);
> >+
> >+    return didStartLoad;
> 
> not a chanel guy myself, so would prefer channel here.

But the tab won't smell good anymore!

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v1.0

Updated patch fixes review comments. I also found a couple additional things I missed in the patch (small things, don't need more review)

I could go for the "this sort of blocks a blocker, so this blocks" but I feel like that's stretching that rule a bit here, so I'm asking for explicit approval or blocking.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/bfa9a991f78e