928335 - Restore Previous Session menu option is still enabled after Restoring Previous Session

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mihai Morar, (:MihaiMorar)]

STR:
1. Open FF using a new profile
2. Open 3 tabs and pin one of them
3. Restart Firefox and go to History->Restore Previous Session
4. Repeat step 3

Actual Results:
At step 4 History->Restpre Previous Session button is still enabled.

Expecred Results:
At step 4 History->Restpre Previous Session button must be disabled.

This is reproducible on all OS's starting with FF 21.0. I will provide the real causing changeset ASAP

Comment 1

[Alice0775 White]

Regressed window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/00e26dece6ea
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121210 Firefox/20.0 ID:20121210105701
Bad:
http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121210 Firefox/20.0 ID:20121210134422
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00e26dece6ea&tochange=4dfe323a663d

Regressed by:
4dfe323a663d	Ehsan Akhgari — Bug 819274 - Disable the Restore Previous Session command for private windows in per-window PB builds; r=dao

Comment 2

[Alex Keybl [:akeybl]]

This is an old regression that doesn't appear to be critical to our users (this is our first report). No need to track.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Disable 'Restore Last Session' menuitem after restoring the last session

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 820152 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session

This looks like it won't disable the command across multiple windows...

Comment 5

[(no longer active)]

Comment on attachment 820152 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session

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

Dao's review here should be enough.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #4)
> This looks like it won't disable the command across multiple windows...

Right, sorry. I totally missed that we need to handle all windows.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Disable 'Restore Last Session' menuitem after restoring the last session, v2

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 820843 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v2

Can SessionStore.jsm's restoreLastSession send a restoring-last-session notification? It's not clear to me why the _lastSessionState setter should notify. Seems somewhat obscure.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

The problem is that external callers can reset the last session state by setting ss.canRestoreLastSession = false. When purging history we throw away the last session state as well.

I could copy the send notification code to all those places where _lastSessionState might be reset. I could also introduce a tiny object that holds the last session state and we could call something like LastSessionState.clear() in SessionStore internally, that might be more obvious than having a "private" getter/setter do it?

Comment 10

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> I could also introduce a tiny object that
> holds the last session state and we could call something like
> LastSessionState.clear() in SessionStore internally, that might be more
> obvious than having a "private" getter/setter do it?

I guess so, especially since _lastSessionState is already a private property of the private SessionStoreInternal object, so _lastSessionStateInternal would add a third layer...

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Disable 'Restore Last Session' menuitem after restoring the last session, v3

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 821486 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v3

>   _tabsRestoringCount: 0,
> 
>-  // The state from the previous session (after restoring pinned tabs). This
>-  // state is persisted and passed through to the next session during an app
>-  // restart to make the third party add-on warning not trash the deferred
>-  // session
>-  _lastSessionState: null,
> 

nit: remove one of the two empty lines

>+let LastSessionState = {
>+  _state: null,
>+
>+  get canRestore() {
>+    return !!this._state;
>+  },
>+
>+  get: function () {
>+    return this._state;
>+  },
>+
>+  set: function (state) {
>+    this._state = state;
>+  },
>+
>+  clear: function () {
>+    if (this._state) {
>+      this._state = null;
>+      Services.obs.notifyObservers(null, NOTIFY_LAST_SESSION_CLEARED, null);
>+    }
>+  }
>+};

I wonder if this would be a better naming scheme:

let LastSession = {
  get canRestore()...
  getState: ...
  setState: ...
  clear: ...
}

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #12)
> I wonder if this would be a better naming scheme:
> 
> let LastSession = {
>   get canRestore()...
>   getState: ...
>   setState: ...
>   clear: ...
> }

Yup, I like that more. Thanks!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/18ccb2ac1b15

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18ccb2ac1b15

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Aurora (build ID: 20131111004004) and on latest Nightly (build ID: 20131110030205).