Disable the Restore Previous Session command in private windows

RESOLVED FIXED in Firefox 20

Status

()

defect
--
major
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mihaelav, Assigned: Ehsan)

Tracking

unspecified
Firefox 20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20121207])

Attachments

(1 attachment, 3 obsolete attachments)

Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-birch/

Since Per-Window Private Browsing, Restore previous session option is available in Private Browsing window and session can be restored into this window.

Steps:
1. Open some tabs in normal browsing (and make sure "Show my home page" is selected as startup option)
2. Restart Firefox
3. Open a private browsing window
4. While in private browsing window, restore previous session (from about:home page or from History menu)

Expected result: Previous session is restored in normal browsing window

Actual result: Previous session is restored in Private Browsing window. After that, you cannot restore that session in normal browsing mode.
Whiteboard: [testday-20121207]
Blocks: fxPBnGen
Hmm, should we just disable the Restore Previous Session menu item in private windows altogether?
That seems like a logical choice to me.
Assignee: nobody → ehsan
Blocks: PBnGen
Summary: Previous session is restored in Private Browsing window → Disable the Restore Previous Session command in per-window PB builds
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #690272 - Flags: review?(dao)
Status: NEW → ASSIGNED
Comment on attachment 690272 [details] [diff] [review]
Patch (v1)

You really should enable/disable the Browser:RestoreLastSession command as the window opens rather than while opening a menu containing such a menu item.
Attachment #690272 - Flags: review?(dao) → review-
Summary: Disable the Restore Previous Session command in per-window PB builds → Disable the Restore Previous Session command in private windows
Posted patch Patch (v1) (obsolete) — Splinter Review
I do this in delayedStartup because canRestoreLastSession doesn't have the correct value before the onload handler for the first window has been run, apparently.
Attachment #690272 - Attachment is obsolete: true
Attachment #690361 - Flags: review?(dao)
Comment on attachment 690361 [details] [diff] [review]
Patch (v1)

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> I do this in delayedStartup because canRestoreLastSession doesn't have the
> correct value before the onload handler for the first window has been run,
> apparently.

This is because the sessionstore service is initialized in delayedStartup. Please move your block up there such that we don't get the service twice.

>+    let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+        getService(Ci.nsISessionStore);

Indentation seems random here. I'll also lobby against the trailing dot and suggest this:

    let ss = Cc["@mozilla.org/browser/sessionstore;1"]
               .getService(Ci.nsISessionStore);

However, this also looks like it fits well on one line. Although we do avoid overly long lines, my feeling is that we're not pedantic about enforcing an 80 characters limit in most browser code.

>+    let restoreItem = document.getElementById("Browser:RestoreLastSession");
>+    if (ss.canRestoreLastSession
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+        && !PrivateBrowsingUtils.isWindowPrivate(window)
>+#endif
>+        )
>+      restoreItem.removeAttribute("disabled");

goSetCommandEnabled("Browser:RestoreLastSession", false);
Attachment #690361 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #6)
> >+      restoreItem.removeAttribute("disabled");
> 
> goSetCommandEnabled("Browser:RestoreLastSession", false);

Err, s/false/true/
Posted patch Patch (v2) (obsolete) — Splinter Review
Attachment #690361 - Attachment is obsolete: true
Attachment #690407 - Flags: review?(dao)
I don't see any valid reason why the getService() call would fail unless things are otherwise seriously broken. I would leave it out of the try/catch and remove the null check for "ss".
Posted patch Patch (v3)Splinter Review
Attachment #690407 - Attachment is obsolete: true
Attachment #690407 - Flags: review?(dao)
Attachment #690448 - Flags: review?(gavin.sharp)
Attachment #690448 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Attachment #690448 - Flags: review?(gavin.sharp)

Comment 12

6 years ago
Is this already covered by an automated test?
Flags: in-testsuite?
It does not appear to be.
Flags: in-testsuite? → in-testsuite-

Comment 14

6 years ago
(In reply to Josh Matthews [:jdm] from comment #13)
> It does not appear to be.

Thanks! The creation of such a test is now tracked in bug 856043.

Updated

6 years ago
Depends on: 928335
You need to log in before you can comment on or make changes to this bug.