Disable the Restore Previous Session command in private windows

RESOLVED FIXED in Firefox 20

Status

()

--
major
RESOLVED FIXED
6 years ago
5 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)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [testday-20121207]

Updated

6 years ago
Blocks: 797256
(Assignee)

Comment 1

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

Comment 2

6 years ago
That seems like a logical choice to me.
(Assignee)

Updated

6 years ago
Assignee: nobody → ehsan
Blocks: 463027
(Assignee)

Updated

6 years ago
Summary: Previous session is restored in Private Browsing window → Disable the Restore Previous Session command in per-window PB builds
(Assignee)

Comment 3

6 years ago
Created attachment 690272 [details] [diff] [review]
Patch (v1)
Attachment #690272 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
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-

Updated

6 years ago
Summary: Disable the Restore Previous Session command in per-window PB builds → Disable the Restore Previous Session command in private windows
(Assignee)

Comment 5

6 years ago
Created attachment 690361 [details] [diff] [review]
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.
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/
(Assignee)

Comment 8

6 years ago
Created attachment 690407 [details] [diff] [review]
Patch (v2)
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".
(Assignee)

Comment 10

6 years ago
Created attachment 690448 [details] [diff] [review]
Patch (v3)
Attachment #690407 - Attachment is obsolete: true
Attachment #690407 - Flags: review?(dao)
Attachment #690448 - Flags: review?(gavin.sharp)

Updated

6 years ago
Attachment #690448 - Flags: review+
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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

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