1203531 - Use custom settings for history should not be based on querying prefs

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Mike Kaply [:mkaply]]

Currently the preferences window only switches to "Use custom settings for history" if a change to a preference is detected.

This creates a problem.

If you change the default values to NOT remember history (via CCK2 or Autoconfig), the preference dialog still defaults to Never remember history (even though it's technically wrong at this point).

To fix this, the "Use Custom settings for history" menu choice should be based on a preference, it should not query preferences to decide if it is enabled.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Simple patch that allows a pref to set the mode

Thinking about this more, I think that a simple preference to allow for this edge case is sufficient.

If an enterprise is changing the default preferences, they can also set this preference so that the custom mode is displayed by default.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8663825 [details] [diff] [review]
Simple patch that allows a pref to set the mode

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

I don't think we should be adding a hidden pref for this. Can you rewrite the code here to not assume what the default values are, instead just checking the prefs for the various values?

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Alternative fix

Are you thinking something like this?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8668002 [details] [diff] [review]
Alternative fix

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

Yeah, but we should add a comment to those prefs within firefox.js or all.js and mention that their default value is duplicated in privacy.js and it will need to be updated there as well if it changes. I'm curious to hear what Gijs says about this though.

Comment 5

[Mike Kaply [:mkaply]]

> Yeah, but we should add a comment to those prefs within firefox.js or all.js and mention that their default value is duplicated in privacy.js and it will need to be updated there as well if it changes.

I was thinking the same thing.

Comment 6

[:Gijs (he/him)]

I mean, yeah, comments. Also, this UI code is kind of sad.

Comment 7

[Mike Kaply [:mkaply]]

Better patch coming. I was just validating the idea. I'll clean up the existing comments and add comments to the JS file.

Comment 8

[Mike Kaply [:mkaply]]

> I mean, yeah, comments. Also, this UI code is kind of sad.

I'm starting to wonder if this is the really the right thing.

If a user changes the preference page to be "Use custom settings for history", shouldn't we just preserve that choice in a pref?

Why do we use the pref values at all to decide where the user wants to be? Maybe they want to see the "Use custom settings" every time?

I'm thinking we should just preserve the users choice in a pref...

Comment 9

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #8)
> > I mean, yeah, comments. Also, this UI code is kind of sad.
> 
> I'm starting to wonder if this is the really the right thing.

This sounds fair enough.

> If a user changes the preference page to be "Use custom settings for
> history", shouldn't we just preserve that choice in a pref?

I would prefer not to save that UI choice in a separate pref, because it'll allow things to get out of sync, right? I could have that pref set to "always save history" or whatever, and all the "real" prefs set to "never save anything", which is obviously going to cause issues. "But that should never happen" - sure, but realistically, it will - people already complain about this set of prefs and their clarity.

> Why do we use the pref values at all to decide where the user wants to be?
> Maybe they want to see the "Use custom settings" every time?

I think we're using them in order to simplify the choices here. 90% of users don't know what "third-party cookies" means, and giving people options they don't understand generally leads to confusion because those users will toggle the options "to see what it does", then forget, then be confused because stuff is broken. "Even" add-on authors get into situations like this (cf. bug 1191535).

IOW, I don't think we want people to see the "custom setting" unless they actively choose it one way or another.


Re-reading this bug, I guess I'm actually confused here:

(In reply to Mike Kaply [:mkaply] from comment #0)
> If you change the default values to NOT remember history (via CCK2 or
> Autoconfig), the preference dialog still defaults to Never remember history
> (even though it's technically wrong at this point).

I don't understand this statement. You're changing the prefs to not remember anything, and then the prefs pane shows that... Firefox never remembers anything? That sounds correct. Can you give more detail as to what's going wrong here?

Comment 10

[Mike Kaply [:mkaply]]

The problem is that the pref pane uses the concept of default values to determine if "Use Custom Settings" or "Remember history" should be shown.

So if a company changes the default values (for instance, turn off "remember browsing history"), the pref dialog opens to "Remember history" even though it is NOT remembering history.

The preference dialog needs a way to reflect the actual state it is in and it shouldn't rely on default values because those default values might not match reality.

That's where the idea came in to put the default values into the .js file as opposed to relying on the preference default values (which could change).

But now I'm wondering if we should just allow people to select this option in their preferences.

If the last thing I selected in preferences was "Use custom settings", shouldn't it be preserved even if I didn't change anything?

Comment 11

[Mike Kaply [:mkaply]]

Just reread your comment and it answers my question.

I'll do a new patch.

Comment 12

[Mike Kaply [:mkaply]]

Attachment: New patch with comments

So as I thought about this more, I think the use of the term "default" here is really screwing things up.

This has nothing to do with the default value of the prefs. It has to do with whether or not the given prefs are set to keep history.

So this patch renames the values.

This also means that there are no comments in the .js files because if those values were changes, they would no longer be  keeping history anyway.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8675043 [details] [diff] [review]
New patch with comments

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

With the below addressed, r=me

::: browser/components/preferences/in-content/privacy.js
@@ +163,3 @@
>     *
>     * @param aPrefs an array of pref names to check for
> +   * @returns boolean true if all of the prefs are set keep history,

'to' is missing

@@ +166,4 @@
>     *                  false otherwise
>     */
> +  _checkHistoryValues: function(aPrefs) {
> +    for (pref in aPrefs) {

You need to declare pref, and please use for...of instead of for...in, so:

for (let pref of Object.keys(aPrefs))

Comment 14

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b031fc40cf45213e29c085292568a22ef9d5621c
Bug 1203531 - Don't use default prefs for determining Custom History, use actual values. r=Gijs

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b031fc40cf45