801823 - Figure out the UI for per-window private browsing autostart mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Comment 1

[(no longer active)]

I chatted with shorlander about this today.  He said that we can allow requiring a restart for this pref to be set, but this panel needs a re-design.  He's going to come up with mock-ups for re-designing it.

Comment 2

[u428464]

I don't really understand what this bug is about ?

Comment 3

[(no longer active)]

(In reply to comment #2)
> I don't really understand what this bug is about ?

It's about figuring out what user interface we want to use for the Privacy pane of the Preferences dialog from which you can set the autostart private browsing mode pref.

Comment 4

[u428464]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> (In reply to comment #2)
> > I don't really understand what this bug is about ?
> 
> It's about figuring out what user interface we want to use for the Privacy
> pane of the Preferences dialog from which you can set the autostart private
> browsing mode pref.

Ok now I perfectly see what this is about. Thanks.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Prompt to restart Firefox when changing the private browsing autostart preference.

Here's a patch that appears to do the right thing. I haven't checked how it runs under tests yet.

Comment 6

[(no longer active)]

Comment on attachment 694578 [details] [diff] [review]
Prompt to restart Firefox when changing the private browsing autostart preference.

Review of attachment 694578 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/privacy.js
@@ +234,5 @@
> +        return;
> +      }
> +
> +      const Cc = Components.classes, Ci = Components.interfaces;
> +      let shouldProceed = confirm("Firefox must restart to enable this feature.");

You need to add this string to a properties file and read it through the bundle service so that it would be localizable.  Also, you don't want to hard-code the brand name.

@@ +244,5 @@
> +          shouldProceed = !cancelQuit.data;
> +
> +          let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> +                             .getService(Ci.nsIAppStartup);
> +          appStartup.quit(Ci.nsIAppStartup.eAttemptQuit |  Ci.nsIAppStartup.eRestart);

You're ignoring shouldProceed here...

Comment 7

[Josh Matthews [:jdm]]

Attachment: Prompt to restart Firefox when changing the private browsing autostart preference.

All tests pass for in-content and not.

Comment 8

[(no longer active)]

Comment on attachment 696395 [details] [diff] [review]
Prompt to restart Firefox when changing the private browsing autostart preference.

Review of attachment 696395 [details] [diff] [review]:
-----------------------------------------------------------------

Most of these comments are trivial, I'll address them myself and land the patch.  Thanks a lot for working on this!

::: browser/components/preferences/in-content/tests/browser_privacypane_1.js
@@ +14,5 @@
> +  try {
> +    loader.loadSubScript(rootDir + "privacypane_tests_perwindow.js", this);
> +  } catch(x) {
> +    loader.loadSubScript(rootDir + "privacypane_tests.js", this);
> +  }

I think we should do this for all of these tests, even the ones which do not use PB routines, so that later on we can safely remove privacypane_tests.js and all of these hacks altogether.

::: browser/components/preferences/privacy.js
@@ +14,5 @@
>  
>    /**
> +   * Whether the prompt to restart Firefox should appear when changing the autostart pref.
> +   */
> +  _shouldPromptForRestart: true,

The same changes in this file should be applied to the in-content equivalent as well.

@@ +244,5 @@
> +                                          "featureEnableRequiresRestart" : "featureDisableRequiresRestart",
> +                                          [brandName]);
> +      let title = bundle.getFormattedString("shouldRestartTitle", [brandName]);
> +      let prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"].getService(Ci.nsIPromptService);
> +      let shouldProceed = prompts.confirm(null, title, msg)

Nit: you should pass |window| as the parent window.

@@ +255,5 @@
> +
> +        if (shouldProceed) {
> +          let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> +                             .getService(Ci.nsIAppStartup);
> +          appStartup.quit(Ci.nsIAppStartup.eAttemptQuit |  Ci.nsIAppStartup.eRestart);

Adding a return statement here would make this code a bit more clear, and will also make the next |if (!shouldProceed)| unnecessary.

@@ +268,5 @@
> +            mode.selectedIndex = pref.value ? 1 : 0;
> +            mode.doCommand();
> +        } else {
> +            let rememberHistoryCheckbox = document.getElementById("rememberHistory");
> +            rememberHistory.checked = pref.value;

Nit: indentation.

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33d3e7354bf9

Comment 10

[u428464]

No redesign needed ?

Comment 11

[(no longer active)]

(In reply to comment #10)
> No redesign needed ?

No.

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/33d3e7354bf9

Comment 14

[Michael Kraft [:morac]]

This isn't working correctly (at least not under Windows).  I filed bug 826371 documenting the problem.