462041 - Refresh the Privacy preference pane

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

There are a few bugs that touch the privacy preference pane, so I figure it is worth filing a tracking bug to aggregate them all.  Here is a mockup of the proposed UI:

http://people.mozilla.com/~faaborg/files/shiretoko/privacy_i2.png

Bugs related to the prefpane:

bug 460346 - Privacy pref for "Always on" Private Browsing Mode
bug 460343 - Add privacy-section prefs to control awesomebar behaviour

This bug can also cover the remaining general UI changes (adding the drop down to the top of the preference pane, regrouping items into a history groupbox, etc.)

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

note: this bug should also cover landing the new icon

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Is this in scope for beta 3?

Also note bug 464204 for using consistent terminology

Comment 3

[(no longer active)]

One part of the work here is being done in bug 469158 (that is, removing the privacy.sanitize.promptOnSanitize preference, and making sure that clear recent history on shutdown never triggers the UI).

Comment 4

[(no longer active)]

Stealing this from Johnath.  :-)

Alex, is the mockup in comment 0 the latest one?

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

At the bottom the "unless bookmarks are tagged with" feature should be removed.  Otherwise this looks up to date.

Comment 6

[(no longer active)]

Attachment: Strings

Strings patch to land before the string freeze.  I'm not removing any old strings yet.

Comment 7

[(no longer active)]

Attachment: Latest Mock-up

Latest Mock-up provided by Alex, attached here for reference.

Comment 8

[(no longer active)]

By the way, if we decide to show a notification bar in the preferences dialog for the always-on private browsing mode, we need to add the appropriate strings now as well.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368208 [details] [diff] [review]
Strings

uir+a191=beltzner

Comment 10

[Vlado Valastiak [:wladow]]

Comment on attachment 368208 [details] [diff] [review]
Strings

>+<!ENTITY  rememberDownloads.label        "Remember download history">
>+<!ENTITY  rememberDownloads.accesskey    "d">

You need to choose another id
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#24

Comment 11

[(no longer active)]

Attachment: String ID fix

(In reply to comment #10)
> (From update of attachment 368208 [details] [diff] [review])
> >+<!ENTITY  rememberDownloads.label        "Remember download history">
> >+<!ENTITY  rememberDownloads.accesskey    "d">
> 
> You need to choose another id
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#24

Oh, sorry I missed that.  Simple patch to fix this.

Comment 12

[Nochum Sossonko [:Natch]]

Why don't you just remove it? Are the accesskeys conflicting? Why make localizers translate twice?

Comment 13

[(no longer active)]

String ID fix landed as <http://hg.mozilla.org/mozilla-central/rev/76201f08710a>

(In reply to comment #12)
> Why don't you just remove it? Are the accesskeys conflicting? Why make
> localizers translate twice?

Same IDs with different text cause localizers not to detect the string change using the current automated notification system (l10n tinderbox), which is bad.  I think the policy is that for any non-minor change to a string, the ID needs to be changed as well.

The problem here is not related to accesskeys, as the new strings are not yet used anywhere in the UI.  We also don't make localizers translate twice, because on my final patch I'll remove the unused strings (not from 3.5 though, but localizers have already translated the old strings there).

Comment 14

[Nochum Sossonko [:Natch]]

Oh, I hadn't realized that the old strings were to be removed later, in that case just ignore my comment.

Comment 15

[(no longer active)]

Oh, I forgot to mention that the strings patch itself was landed <http://hg.mozilla.org/mozilla-central/rev/054195131304> :-)

Comment 16

[(no longer active)]

Strings (with the fixed ID) landed on 1.9.1 as well: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/693b2a3d6ca6>

Comment 17

[(no longer active)]

Attachment: Left over string

I missed one string in my previous patch :(

Comment 18

[(no longer active)]

It would be great if someone can land attachment 368506 [details] [diff] [review] on trunk and 1.9.1 once it's reviewed and approved, as I may not be around in time in order not to miss the string freeze.

Comment 19

[(no longer active)]

Attachment: WIP 1

This patch implements the new design of the privacy prefpane, and hooks up various pieces of the UI.  It also changes the empty text for the location bar based on the user settings.  I am submitting this patch in order for others to give it a try (and hopefully find possible bugs).

The things which are left to do is making the private browsing auto-start pref take effect right away instead of at the next restart, a number of tests, and small clean-ups for it to be ready for review.

I also went ahead and used my limited GIMP experience to incorporate the existing privacy icons into the new UI, just for fun.  :-)

I've submitted a try server build with this patch (and attachment 368506 [details] [diff] [review] of course), and I'll post a link to it when it's available.

Comment 20

[(no longer active)]

Try server build for the WIP1 patch available at: <https://build.mozilla.org/tryserver-builds/2009-03-21_07:[e-mail]-try-886d0be4351/>

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368506 [details] [diff] [review]
Left over string

uir+a=beltzner

Comment 22

[(no longer active)]

The left-over string was landed on both trunk and 1.9.1.

http://hg.mozilla.org/mozilla-central/rev/8d2c566f3256
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f5340979aee1

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> Try server build for the WIP1 patch available at:
> <https://build.mozilla.org/tryserver-builds/2009-03-21_07:[e-mail]-try-886d0be4351/>

I've taken a look at this build and have some comments:

* Why we use "private browsing session" here? In about:privatebrowsing we have "In a Private Browsing session, Minefield". Shouldn't we have capital letters here too?

* When having "Remembering History" selected there is displayed "Web sites". Why there is a capital letter? Similar for "Web" with the "Never remember history" selection.

* The keep until dropdown is initially disabled until I uncheck/check the third party Cookies checkbox

* For Cookies the autostart checkbox hides the state for "keep until" in non-private browsing mode. Means until the browser is restarted to current value is not shown.

* Selecting "suggest History" and using the dropdown on the right of the location bar only shows the domains but not the visited pages. When selecting bookmarks the autocomplete popup isn't opened at all. (Probably another bug)

Comment 24

[(no longer active)]

(In reply to comment #23)
> * The keep until dropdown is initially disabled until I uncheck/check the third
> party Cookies checkbox

I'm not sure why this was not the case in the mock-up, but I think that the "accept cookies" and "accept third-party cookies" checkboxes should also be disabled when the "always in private browsing mode" checkbox is checked.  If that happens, then this won't be a problem.

> * For Cookies the autostart checkbox hides the state for "keep until" in
> non-private browsing mode. Means until the browser is restarted to current
> value is not shown.

When the "always in private browsing mode" checkbox is checked, the value of keep until is shown as "I close Minefield", however the underlying pref is not shown.  As soon as you uncheck that checkbox, the value of this control is filled from the underlying pref.  If the above problem is fixed, then this won't be inconsistent.

I think the "clear history when minefield exits" checkbox should also be disabled in "always in private browsing" mode, because it doesn't make sense to be in private browsing mode and leave that box unchecked (from the user's point of view, the history is always cleared in that mode).

> * Selecting "suggest History" and using the dropdown on the right of the
> location bar only shows the domains but not the visited pages. When selecting
> bookmarks the autocomplete popup isn't opened at all. (Probably another bug)

I can't reproduce this, for me they all work as expected.  But yeah that should be another bug, because I have not touched the code related to those settings at all.

Comment 25

[(no longer active)]

This try server build reworks the dependent controls in the UI as I described in comment 24: <https://build.mozilla.org/tryserver-builds/2009-03-23_00:[e-mail]-try-b2044217865/>

Comment 26

[(no longer active)]

Attachment: WIP 2

The patch used to create the try server build I mentioned in the previous comment.  Asking ui-review on the UI controls.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #25)
> This try server build reworks the dependent controls in the UI as I described
> in comment 24:
> <https://build.mozilla.org/tryserver-builds/2009-03-23_00:[e-mail]-try-b2044217865/>

Ehsan, it looks much better. Here two more small things I have noticed:

* The vertical space between the checkboxes are different.

* Further the first checkbox should even be shift down some pixels. So it isn't glued to the dropdown.

Comment 28

[(no longer active)]

(In reply to comment #27)
> Ehsan, it looks much better. Here two more small things I have noticed:

Thanks for testing this.

> * The vertical space between the checkboxes are different.

The reason is that some of them are wrapped inside <hbox> elements which contain taller elements (such as a text box or a button) as well...  I'm not sure how once can fix this though, suggestions welcome.

> * Further the first checkbox should even be shift down some pixels. So it isn't
> glued to the dropdown.

I fixed this locally using a <separator class="thin"/> element.  It will be included in the next iteration of the patch.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #28)
> > * The vertical space between the checkboxes are different.
> 
> The reason is that some of them are wrapped inside <hbox> elements which
> contain taller elements (such as a text box or a button) as well...  I'm not
> sure how once can fix this though, suggestions welcome.

Dao, would you have an idea?

Comment 30

[(no longer active)]

Attachment: Patch (v1)

OK, I think this is ready for review now.

I added handling of the private browsing mode switch from the privacy prefpane.  This needed a number of changes to the browser.js UI code as well as the service code.  I have also added a comprehensive automated test which I think catches all the interesting properties which we would like to test here.  Of course this could use a few Litmus tests as well.

I have pushed a try server build, which should be ready in a few hours.  Setting the ui-review flag request based on that try server build.

Comment 31

[Alex Faaborg [:faaborg] (Firefox UX)]

>> * The keep until dropdown is initially disabled until I uncheck/check the third
>> party Cookies checkbox
>
>I'm not sure why this was not the case in the mock-up, but I think that the
>"accept cookies" and "accept third-party cookies" checkboxes should also be
>disabled when the "always in private browsing mode" checkbox is checked.  If
>that happens, then this won't be a problem.

I might be confused about how private browsing mode works.  My impression was that while in private browsing mode cookies are still accepted (both from sites and third parties), but they are stored in memory instead of on the hard drive.  So, assuming that is the case, it seems like we would need to enable the controls since going into private browsing mode and also rejecting all cookies (or just third party cookies) is even more private than the default behavior.  The one part of the cookie area that is effected by the private browsing check box is when they are going to be cleared however.  Since the cookies are just stored in memory, this needs to be set to "On Close" and then disabled to match the behavior of private browsing mode.

It isn't great to have only 1 of 3 controls effected by the higher level check box, but I couldn't think of a better way around the problem.

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

>I think the "clear history when minefield exits" checkbox should also be
>disabled in "always in private browsing" mode, because it doesn't make sense to
>be in private browsing mode and leave that box unchecked (from the user's point
>of view, the history is always cleared in that mode).

Perhaps disabled and unchecked? it isn't really possible to clear something that was never collected in the first place.  The annoying outlier is cookies, as mentioend above (I think).

Also, when the user checks automatically start minefield in a private browsing session, I think we should uncheck and disable history, download history, and search form history, since these things won't be collected.

Comment 33

[Alex Faaborg [:faaborg] (Firefox UX)]

>> * Selecting "suggest History" and using the dropdown on the right of the
>> location bar only shows the domains but not the visited pages. When selecting
>> bookmarks the autocomplete popup isn't opened at all. (Probably another bug)
>
>I can't reproduce this, for me they all work as expected.  But yeah that should
>be another bug, because I have not touched the code related to those settings
>at all.

I couldn't reproduce that either, but seeing bookmarks appear after selecting only history (because they are in history) feels a little odd, filed follow up bug 485122

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 369142 [details] [diff] [review]
Patch (v1)

overall awesome work!  ui-r+ once we figure out how the cookies interface should work, and we both uncheck and disable items when "always start in a private browsing session" is turned on.

Comment 35

[(no longer active)]

(In reply to comment #31)
> I might be confused about how private browsing mode works.  My impression was
> that while in private browsing mode cookies are still accepted (both from sites
> and third parties), but they are stored in memory instead of on the hard drive.

Yes, that's exactly what happens.

>  So, assuming that is the case, it seems like we would need to enable the
> controls since going into private browsing mode and also rejecting all cookies
> (or just third party cookies) is even more private than the default behavior. 
> The one part of the cookie area that is effected by the private browsing check
> box is when they are going to be cleared however.  Since the cookies are just
> stored in memory, this needs to be set to "On Close" and then disabled to match
> the behavior of private browsing mode.

I think this would make sense.

> It isn't great to have only 1 of 3 controls effected by the higher level check
> box, but I couldn't think of a better way around the problem.

Let's do this here, and file a bug to change the behavior if someone has a better idea in the future.  I'll change this in my next patch.

(In reply to comment #32)
> Perhaps disabled and unchecked? it isn't really possible to clear something
> that was never collected in the first place.  The annoying outlier is cookies,
> as mentioend above (I think).

Will do.

> Also, when the user checks automatically start minefield in a private browsing
> session, I think we should uncheck and disable history, download history, and
> search form history, since these things won't be collected.

Yes, makes sense.  Will do this as well.

BTW, the try server build with the patch in attachment 369142 [details] [diff] [review] is available at <https://build.mozilla.org/tryserver-builds/2009-03-24_13:[e-mail]-try-1289bd7773d/>.

Comment 36

[(no longer active)]

Attachment: Patch (v1.1)

New patch with the changes requested by Alex implemented, and added to the automated tests.

Comment 37

[(no longer active)]

Latest try server build: <https://build.mozilla.org/tryserver-builds/2009-03-25_07:[e-mail]-try-21aa5cc63da/> (Mac only, as the try server seems to have a lot of issues right now)

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

One thing I thought of today: if someone's already altered their settings from the default, will this dialog notice it and in the migrating-user case set their drop-down to be "Use custom settings"?

Comment 39

[(no longer active)]

(In reply to comment #38)
> One thing I thought of today: if someone's already altered their settings from
> the default, will this dialog notice it and in the migrating-user case set
> their drop-down to be "Use custom settings"?

Yes.  Of course the PB autostart pref takes precedence here, so if it's set, the drop-down box will default to "don't remember".

Comment 40

[Kadir Topal [:atopal]]

As a localizer I'm not sure how to translate "browsing history". IIRC we did't use that term before. What does it actually mean? Is it just visited pages or more? 

Also as a user I would be left wondering, so maybe the wording could be changed?

Comment 41

[Alex Faaborg [:faaborg] (Firefox UX)]

>As a localizer I'm not sure how to translate "browsing history". IIRC we did't
>use that term before. What does it actually mean? Is it just visited pages or
>more? 
>
>Also as a user I would be left wondering, so maybe the wording could be
>changed?

The term "browsing history" or just "history" is meant to encompass all of the things that are implicitly collected while you use Firefox (visited pages, cache, cookies, searches, form entries, etc.)  This is a modified definition from previous releases (it used to just mean visited pages), but simply localizing "Browser history" to a translation of "history" is fine.

The goal is to be able to say "history will not be collected" or "clear recent history" and the user won't have to worry about the details of what that includes.  Or, if they are curious, they can open up the privacy prefpane, set the collection option to Custom, and see that all of this stuff is now in a group box called "History."

In the case of the preference for how much time Firefox should remember history (with a default of 90 days), we are planning to use this time range for the full set of things encompassed by the term "history." (although I'm not sure if all of the sub-components have available time stamps in place yet).

Comment 42

[Kadir Topal [:atopal]]

I guess you already had a big discussion about this and I don't want to start it all over again, but wouldn't the term "information" or "data" be more telling than just "browsing history"? 
"no information/data will be collected during this session" or 
"clear all collected data"

Even if you don't agree, do you see a problem in translating it like this?

Comment 43

[Alex Faaborg [:faaborg] (Firefox UX)]

>I guess you already had a big discussion about this and I don't want to start
>it all over again, but wouldn't the term "information" or "data" be more
>telling than just "browsing history"? 
>"no information/data will be collected during this session" or 
>"clear all collected data"
>
>Even if you don't agree, do you see a problem in translating it like this?

We are worried that the term data might overlap in the user's mind to include things that are explicitly created, like bookmarks or saved passwords.

Comment 44

[u60234]

(In reply to comment #41)

> The term "browsing history" or just "history" is meant to encompass all of the
> things that are implicitly collected while you use Firefox (visited pages,
> cache, cookies, searches, form entries, etc.)

I'm confused. The mock-ups and patches here and in bug 480169 all use the term "Browsing history" to describe a subset of "History", namely visited pages. This is also consistent with your list of terms in bug 464204. Are you now saying that "Browsing history" also includes downloads, form and search history, and cookies?

Comment 45

[(no longer active)]

Attachment: Patch (v1.1)

Unbitrotted patch after the landing of bug 418687.

Comment 46

[Mike Connor [:mconnor]]

Comment on attachment 371317 [details] [diff] [review]
Patch (v1.1)

I haven't reviewed the tests yet, but here's the code pieces:

>+    // Enable the menu item in after exiting the auto-start mode
>+    let pbMenuItem = document.getElementById("privateBrowsingItem");
>+    if (pbMenuItem && pbMenuItem.hasAttribute("disabled"))
>+      pbMenuItem.removeAttribute("disabled");
>+    let pbCommand = document.getElementById("Tools:PrivateBrowsing");
>+    if (pbCommand.hasAttribute("disabled"))
>+      pbCommand.removeAttribute("disabled");

So, why do you need to null-check pbMenuItem but not pbCommand?  I don't think you need to null-check either, since short of extensions explicitly removing them, they should always be there.  It's also unnecessary to check hasAttribute here, we should just call removeAttribute.

>diff --git a/browser/components/preferences/privacy.js b/browser/components/preferences/privacy.js

> var gPrivacyPane = {
>+
>+  /**
>+   * Whether the keepUntil cookie controls should be disabled because of the
>+   * auto-start private browsing mode.
>+   */
>+  keepUntilDisabled: false,
>+
>+  /**
>+   * Whether the history expiration days pref should be skipped when updating
>+   * the user interface for that pref because of the auto-start private browsing
>+   * mode.
>+   */
>+  ignoreHistoryExpirePref: false,

Why are these separate booleans, and why aren't they private vars?

>+    if (getVal("browser.privatebrowsing.autostart"))
>+      mode = "dontremember";
>+    else if (getVal("browser.history_expire_days") > 0 &&
>+             getVal("browser.download.manager.retention") == 2 &&
>+             getVal("browser.formfill.enable") &&
>+             getVal("network.cookie.cookieBehavior") == 0 &&
>+             getVal("network.cookie.lifetimePolicy") == 0 &&
>+             !getVal("privacy.sanitize.sanitizeOnShutdown"))

This is going to be fragile, since anyone changing a default value will have to change here as well.  Also, I think that the history_expire_days check is wrong, if you're using a non-default value I think we're into custom mode.

I'd also recommend making it an array of prefs to check for custom values, and loop over that.  Then an extension that added/exposed more prefs to this panel can easily be added.

basically then you have something like this:

prefsForDefault: [...],
_checkDefaultValues: function (aPrefs) {
  for (let i = 0; i < aPrefs,length;i++) {
    if (document.getElementById(aPrefs[i]).value != 
        document.getElementById(aPrefs[i]).defaultValue)
       return false;
  }
  return true;
}

>+    else
>+      mode = "custom";
>+    document.getElementById("historyMode").value = mode;

nit: space before this line plase

>+  updateHistoryModePrefs: function PPP_updateHistoryModePrefs()
>+  {
>+      // select the remember downloads option if needed
>+      if (!document.getElementById("rememberDownloads").checked) {
>+        document.getElementById("browser.download.manager.retention").value = 2;
>+      }

no brackets around single lines please

>+  updatePrivacyMicroControls: function PPP_updatePrivacyMicroControls()
>+  {
>+    if (document.getElementById("historyMode").value == "custom") {
>+      let disabled = document.getElementById("privateBrowsingAutoStart").checked;
>+      [
>+        "rememberHistoryDays",
>+        "rememberAfter",
>+        "rememberDownloads",
>+        "rememberForms",
>+        "keepUntil",
>+        "keepCookiesUntil",
>+        "alwaysClear",
>+        "clearDataSettings"
>+      ].forEach(function (aElement)
>+                document.getElementById(aElement).disabled = disabled);

Can you make this an array that extensions can append to?

Comment 47

[(no longer active)]

Attachment: Patch (v1.2)

(In reply to comment #46)
> >+    // Enable the menu item in after exiting the auto-start mode
> >+    let pbMenuItem = document.getElementById("privateBrowsingItem");
> >+    if (pbMenuItem && pbMenuItem.hasAttribute("disabled"))
> >+      pbMenuItem.removeAttribute("disabled");
> >+    let pbCommand = document.getElementById("Tools:PrivateBrowsing");
> >+    if (pbCommand.hasAttribute("disabled"))
> >+      pbCommand.removeAttribute("disabled");
> 
> So, why do you need to null-check pbMenuItem but not pbCommand?  I don't think
> you need to null-check either, since short of extensions explicitly removing
> them, they should always be there.  It's also unnecessary to check hasAttribute
> here, we should just call removeAttribute.

You're right.  I was confused by the same checks in other places in this file as I thought not doing that will cause Mac problems, but since the Tools menu is always shown on Mac I figured that I'm wrong.  I removed the same checks from other places in the private browsing UI code.

> >+  /**
> >+   * Whether the keepUntil cookie controls should be disabled because of the
> >+   * auto-start private browsing mode.
> >+   */
> >+  keepUntilDisabled: false,
> >+
> >+  /**
> >+   * Whether the history expiration days pref should be skipped when updating
> >+   * the user interface for that pref because of the auto-start private browsing
> >+   * mode.
> >+   */
> >+  ignoreHistoryExpirePref: false,
> 
> Why are these separate booleans, and why aren't they private vars?

The reason that these were separate booleans is that I didn't notice that they're representing the same thing.  :-)  But I'm not sure what you mean by them being private...  They are accessed from multiple methods.  I added a leading underscore to their name FWIW.

> >+    if (getVal("browser.privatebrowsing.autostart"))
> >+      mode = "dontremember";
> >+    else if (getVal("browser.history_expire_days") > 0 &&
> >+             getVal("browser.download.manager.retention") == 2 &&
> >+             getVal("browser.formfill.enable") &&
> >+             getVal("network.cookie.cookieBehavior") == 0 &&
> >+             getVal("network.cookie.lifetimePolicy") == 0 &&
> >+             !getVal("privacy.sanitize.sanitizeOnShutdown"))
> 
> This is going to be fragile, since anyone changing a default value will have to
> change here as well.

You're right, I changed the code to use the default value from the preferences service.

>  Also, I think that the history_expire_days check is
> wrong, if you're using a non-default value I think we're into custom mode.

I also added a check for history_expire_days_min there as well.  I realized that I'm not testing whether we remain in custom mode if one of those controls are actually changed, so I added the needed tests to the automated test.

> I'd also recommend making it an array of prefs to check for custom values, and
> loop over that.  Then an extension that added/exposed more prefs to this panel
> can easily be added.

Done.

> >+    else
> >+      mode = "custom";
> >+    document.getElementById("historyMode").value = mode;
> 
> nit: space before this line plase

Done.

> >+  updateHistoryModePrefs: function PPP_updateHistoryModePrefs()
> >+  {
> >+      // select the remember downloads option if needed
> >+      if (!document.getElementById("rememberDownloads").checked) {
> >+        document.getElementById("browser.download.manager.retention").value = 2;
> >+      }
> 
> no brackets around single lines please

Done.

> >+  updatePrivacyMicroControls: function PPP_updatePrivacyMicroControls()
> >+  {
> >+    if (document.getElementById("historyMode").value == "custom") {
> >+      let disabled = document.getElementById("privateBrowsingAutoStart").checked;
> >+      [
> >+        "rememberHistoryDays",
> >+        "rememberAfter",
> >+        "rememberDownloads",
> >+        "rememberForms",
> >+        "keepUntil",
> >+        "keepCookiesUntil",
> >+        "alwaysClear",
> >+        "clearDataSettings"
> >+      ].forEach(function (aElement)
> >+                document.getElementById(aElement).disabled = disabled);
> 
> Can you make this an array that extensions can append to?

Done.

Comment 48

[Kadir Topal [:atopal]]

(In reply to comment #43)
> We are worried that the term data might overlap in the user's mind to include
> things that are explicitly created, like bookmarks or saved passwords.

Alex, but shouldn't "collected data" be specific enough? And as Hasse said we use browsing history next to search history, form history and download history. So I'm confused as well. L10 freeze is in 4 days, it would be great if we could resolve this before that date.

Comment 49

[(no longer active)]

mconnor: ping?

Comment 50

[Mike Connor [:mconnor]]

Comment on attachment 371858 [details] [diff] [review]
Patch (v1.2)

looks good, tests look pretty good too, thanks!

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371858 [details] [diff] [review]
Patch (v1.2)

a=191 please land on branch after you've got a clean build cycle on trunk

Comment 52

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'm confused. The mock-ups and patches here and in bug 480169 all use the term
>"Browsing history" to describe a subset of "History", namely visited pages.
>This is also consistent with your list of terms in bug 464204. Are you now
>saying that "Browsing history" also includes downloads, form and search
>history, and cookies?

In the case of the privacy preference pane, the phrase "remember my browsing history for at least 90 days" should apply (or will eventually apply) to the full set of things under history (like download history, form and search history, cookies, cache, etc).  When this setting controls everything we will want to make sure it says "history" instead of the more specific "browsing history."  Right now I think it is visited pages + some (but not all) other parts.

In the case of the the clear recent history dialogbox, the first item should probably be be "Pages visited" so there isn't any subset/superset confusion, although we missed the string freeze.  I'll add a note over in bug 464204.

Comment 53

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/398bc50386b7

Comment 54

[(no longer active)]

Backed out due to Mac test failures:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239702125.1239708675.29430.gz>

Comment 55

[Robert Kaiser]

Sorry if this might sound like bikeshedding, but for the whole confusion over history, I would very much have preferred to see us use "browsing activities" or "logged activities" or something like that, as all Mozilla and many other browsers have exclusively used the word "history" for visited pages only, and we are now blurring this term quite a lot (apart from the term itself being hard to localize in some languages already to fit with a meaning of something like archives of historic activities/happenings).

Comment 56

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan - do you know what caused these test failures? Is it that we didn't update existing tests properly?

Comment 57

[(no longer active)]

(In reply to comment #56)
> Ehsan - do you know what caused these test failures? Is it that we didn't
> update existing tests properly?

No, I don't think that's the reason.  On Mac, all of the browser chrome tests that I added passed, but the tests failed to finish and timed out.  I suspect that something threw at the end of the test, though I have no idea what.  I'm still trying to figure out the exact details, but it's hard given that I don't have a Mac to test it on, and I can't reproduce the failure on Linux/Windows.  Please note that the second time that the Mac tests were run, all tests passed (I'll talk about the leak in a moment), so this failure is intermittent.

Another failure observed on all three platforms were leaks in mochitest-plain.  This is intermittent at least on Windows (because on a second cycle the leaks were gone, and since it happened in mochitest-plain and my patch neither touches a mochitest nor changes anything which concerns a mochitest, I suspect the leaks may not be related to my patch.

After these failures, I submitted the same patch to the try server.  Windows browser chrome tests passed, mochitest-plain tests failed because of seemingly randomorange problems, and Linux and Mac haven't cycled yet.

Now I had run these tests quite a few times on Windows/Linux, and they'd all been passing merrily... :/

Comment 58

[Shawn Wilsher :sdwilsh]

This appears to have caused a Txul regression at least on XP:
http://graphs.mozilla.org/#show=395014,395042,395002&sel=1239529145,1239874745

Comment 59

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: New privacy prefpane icon (Luna)

This file needs to replace the file at:

/source/browser/themes/winstripe/browser/preferences/Options.png

Comment 60

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: New privacy prefpane icon (Aero)

This file needs to replace the file at:

/source/browser/themes/winstripe/browser/preferences/Options-aero.png

Comment 61

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: New privacy prefpane icon (Tango)

This should replace the file at:

/source/browser/themes/gnomestripe/browser/preferences/Options.png

Comment 62

[(no longer active)]

Alex: any plans for attaching an OS X icon set?

Comment 63

[Alex Faaborg [:faaborg] (Firefox UX)]

Sorry, I for some reason thought we had already landed that.  I'll put it together when I am back online tomorrow.

Comment 64

[klonos]

Looking at the proposed mock-up... all looks nice and I see there's been a lot of thought and effort to improve things.

Small suggestion/observation for the 'Micromanaging All Settings' aka 'Use custom settings for history' mode:

There already exists a limit (in days) for the remembered browsing history, but not for downloads or search/forms. Shouldn't there be one for them as well (perhaps for downloads not in days but in number items instead)?

Also next to the number-of-days(or items) limiting control there should be a 'Exceptions...' button for each one of the browsing, downloads and search/forms history. This should be following the idea of the mechanism used by the 'Exceptions...' button for the cookies, white/blacklisting domains.

I am really interested in knowing what people think about all these and also if my ideas are relatively easily implementable since *I guess* most of the code is already there (I cannot code myself, so wouldn't know for sure).

Anyways... my point is I think it would be nice if these features/improvements were added at some point, but I am not the right person to say if, when or how.

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #58)
> This appears to have caused a Txul regression at least on XP:
> http://graphs.mozilla.org/#show=395014,395042,395002&sel=1239529145,1239874745

Apparently unrelated as the backout of this patch didn't bring it down.

Comment 66

[(no longer active)]

OK, got a lead on why the tests are failing.

I managed to reproduce this on a slow machine on debug builds.  The test would just time out in random places at near the end of its run.  So, this occured to me that the timeouts are, well, timeouts!  The test is simply too large to run within the default 30 seconds!  :-)

So, I'm splitting the test into multiple files, and will attach a patch shortly.

Comment 67

[(no longer active)]

Attachment: Split the test

This patch is identical to the previous one except that I splitted the test into six files, and also incorporated new images attached here by Alex.

mconnor, can you do a quick review here?  Thanks! :-)

Beltzner, waiting for your approval to get this on 1.9.1.

Comment 68

[Mike Connor [:mconnor]]

Comment on attachment 372913 [details] [diff] [review]
Split the test

re-org stuff like this I trust you to do

carrying over a=

Comment 69

[(no longer active)]

Landed the new patch: <http://hg.mozilla.org/mozilla-central/rev/6b6d75293757>

Hopefully it'll stick this time.  :-)

Comment 70

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex: any plans for attaching an OS X icon set?

looks like we have it up on trunk, which is probably why I was confused:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/preferences/Options.png

Comment 71

[(no longer active)]

(In reply to comment #70)
> >Alex: any plans for attaching an OS X icon set?
> 
> looks like we have it up on trunk, which is probably why I was confused:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/preferences/Options.png

That is just the dummy thing I mashed up in Gimp!  Do we really want to ship with that?  ;-)

Comment 72

[(no longer active)]

Landed on 1.9.1 without the string changes: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ffcfe2147342>

Comment 73

[Axel Hecht [:Pike]]

This is pending l10n testing by the l10n drivers team, and should block Beta 4 until we're done.

Comment 74

[Axel Hecht [:Pike]]

L10n testing passed, the resulting bugs are either fixed or not blocking.

Comment 75

[Alex Faaborg [:faaborg] (Firefox UX)]

>That is just the dummy thing I mashed up in Gimp!  Do we really want to ship
>with that?  ;-)

I need to check with Stephen Horlander about the exact settings for the greyed out row, and then I'll attach a new file.  We can ship this file for b4 though, since the change will be nearly imperceptible to users.

Comment 76

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre

Windows Vista equivalent nightly

Icons are fine for all builds except Mac, which is noted in Comment 75. Perhaps we should file a separate bug to track that work?

Comment 77

[(no longer active)]

(In reply to comment #76)
> Icons are fine for all builds except Mac, which is noted in Comment 75. Perhaps
> we should file a separate bug to track that work?

Filed bug 489574.

Comment 78

[Marcia Knous [:marcia]]

Adding verified1.9.1 keyword as it got missed in Comment 76.

Comment 79

[Chris Ilias [:cilias]]

user-doc-complete
<https://support.mozilla.com/kb/Options+window+-+Privacy+panel?bl=n>

Comment 80

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk too on all three major platforms with builds like Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090514 Minefield/3.6a1pre ID:20090514031229

Comment 81

[Ton [:Tonnes]]

dontrememberDescription.label  "&brandShortName; will use the same settings as private browsing, …" -> as (in/with) Private Browsing ?

Comment 82

[(no longer active)]

(In reply to comment #81)
> dontrememberDescription.label  "&brandShortName; will use the same settings as
> private browsing, …" -> as (in/with) Private Browsing ?

Alex, do we want this?

Comment 83

[Alex Faaborg [:faaborg] (Firefox UX)]

yeah, we could use an editor on staff.  Can anyone confirm that the current text is specifically a grammatical error?  In terms of readability it seems like introducing "in" means we would need to refer to private browsing specifically as a state that &brandShortName can be "in"

Shiretoko will use the same settings as when it is in private browsing

or

Shiretoko will use the same settings as in private browsing mode.

saying:

"Shiretoko will use the same settings as in private browsing." makes it sound like "as in" is another way to express "use the same settings" and not that the settings themselves are the same. 

I think the current form text is ok, and since it is simple, we should continue to use it (unless someone knows for sure that it's grammatically unacceptable).

Comment 84

[Alex Faaborg [:faaborg] (Firefox UX)]

I guess if I had a chance to rewrite this, I would have opted for something more like:

Shiretoko will use the same settings as private browsing all of the time, and will not remember any history as you browse the Web.

or 

Shiretoko will always use the same settings as private browsing, and will not remember any history as you browse the Web.

Comment 85

[Ton [:Tonnes]]

Sorry, my ‘in/with’ suggestion above needed ‘mode’ as well.

I’m not a native English speaker, but my thoughts on its current form were that ‘private browsing mode’ could be interpreted as this line’s subject. I.o.w.

"Shiretoko will use the same settings as private browsing (does)"

This seems incorrect (even though Fx and the mode both use settings, we can’t compare a browser to a mode), but I could be wrong.

Considering that private browsing mode is not written in capitals, I presume it is not the subject here (the user is), which may lead to the need of using e.g. ‘when’:

"Shiretoko will use the same settings as when private browsing"

or, when specifically referring to the mode indeed:

"Shiretoko will use the same settings as in Private Browsing mode"

Comment 86

[Alex Faaborg [:faaborg] (Firefox UX)]

>we can’t compare a browser to a mode

yeah, that part doesn't work incredibly well, but the full disambiguation turns into:

"Shiretoko will always use the same settings as it does when it is in private browsing mode."

which reads really clunky.

Comment 87

[Ton [:Tonnes]]

How about ‘… as when using Private Browsing’, or (looking at privateBrowsingAutoStart.label) ‘…as when in a private browsing session’?

(I’ll stop commenting on this and will see what comes out ;) )

Comment 88

[Marcia Knous [:marcia]]

I have updated the test cases in Litmus that pertain to the Privacy preferences so setting this to in-litmus+.