Closed Bug 928335 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Session Restore, defect)

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox24 --- wontfix
firefox25 - wontfix
firefox26 - affected
firefox27 - verified

People

(Reporter: mihai.morar, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
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
Blocks: 819274
Version: 21 Branch → 20 Branch
This is an old regression that doesn't appear to be critical to our users (this is our first report). No need to track.
QA Contact: mihai.morar
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #820152 - Flags: review?(ehsan)
Attachment #820152 - Flags: review?(dao)
Hardware: x86_64 → All
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...
Attachment #820152 - Flags: review?(dao) → review-
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.
Attachment #820152 - Flags: review?(ehsan)
(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 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.
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?
(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 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: ...
}
Attachment #821486 - Flags: review?(dao) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/18ccb2ac1b15
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Verified as fixed on latest Aurora (build ID: 20131111004004) and on latest Nightly (build ID: 20131110030205).
QA Contact: mihai.morar → cornel.ionce
You need to log in before you can comment on or make changes to this bug.