819274 - Disable the Restore Previous Session command in private windows

Status: RESOLVED FIXED

Description

[Mihaela Velimiroviciu (:mihaelav)]

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.

Comment 1

[:Ehsan Akhgari]

Hmm, should we just disable the Restore Previous Session menu item in private windows altogether?

Comment 2

[Josh Matthews [:jdm]]

That seems like a logical choice to me.

Comment 3

[:Ehsan Akhgari]

Attachment: Patch (v1)

Comment 4

[Dão Gottwald [::dao]]

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.

Comment 5

[:Ehsan Akhgari]

Attachment: Patch (v1)

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.

Comment 6

[Dão Gottwald [::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);

Comment 7

[Dão Gottwald [::dao]]

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

Err, s/false/true/

Comment 8

[:Ehsan Akhgari]

Attachment: Patch (v2)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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".

Comment 10

[:Ehsan Akhgari]

Attachment: Patch (v3)

Comment 11

[:Ehsan Akhgari]

http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d

Comment 12

[Ioana (away)]

Is this already covered by an automated test?

Comment 13

[Josh Matthews [:jdm]]

It does not appear to be.

Comment 14

[Ioana (away)]

(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.