Closed
Bug 819274
Opened 10 years ago
Closed 10 years ago
Disable the Restore Previous Session command in private windows
Categories
(Firefox :: Private Browsing, defect)
Firefox
Private Browsing
Tracking
()
RESOLVED
FIXED
Firefox 20
People
(Reporter: mihaelav, Assigned: ehsan.akhgari)
References
Details
(Whiteboard: [testday-20121207])
Attachments
(1 file, 3 obsolete files)
4.86 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Whiteboard: [testday-20121207]
Assignee | ||
Comment 1•10 years ago
|
||
Hmm, should we just disable the Restore Previous Session menu item in private windows altogether?
Comment 2•10 years ago
|
||
That seems like a logical choice to me.
Assignee | ||
Updated•10 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•10 years ago
|
||
Attachment #690272 -
Flags: review?(dao)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
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•10 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•10 years ago
|
||
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 6•10 years ago
|
||
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-
Comment 7•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6) > >+ restoreItem.removeAttribute("disabled"); > > goSetCommandEnabled("Browser:RestoreLastSession", false); Err, s/false/true/
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #690361 -
Attachment is obsolete: true
Attachment #690407 -
Flags: review?(dao)
Comment 9•10 years ago
|
||
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•10 years ago
|
||
Attachment #690407 -
Attachment is obsolete: true
Attachment #690407 -
Flags: review?(dao)
Attachment #690448 -
Flags: review?(gavin.sharp)
Updated•10 years ago
|
Attachment #690448 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Updated•10 years ago
|
Attachment #690448 -
Flags: review?(gavin.sharp)
Comment 14•10 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•