No sync preferences defined for some Privacy & Security settings

RESOLVED FIXED in seamonkey2.26

Status

SeaMonkey
Sync UI
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

Trunk
seamonkey2.26
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.23 wontfix, seamonkey2.24 fixed, seamonkey2.25 fixed, seamonkey2.26 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
I've noticed that some services.sync.prefs.sync.{privacy,security}.* default preferences are missing in browser-prefs.js; one of them I forgot myself as part of bug 835134, the other may have been omitted on purpose in bug 576970.
(Assignee)

Comment 1

5 years ago
Created attachment 833388 [details] [diff] [review]
Add missing sync prefs

This adds services.sync.prefs.sync.privacy.donottrackheader.value for which the UI was introduced in bug 835134; the second preference missing from sync is services.sync.prefs.sync.privacy.sanitize.promptOnSanitize, where bug 576970 only introduced services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown.

Jens, if the latter preference setting isn't supposed to be synced on purpose, please let me know and I'll remove it from the patch
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #833388 - Flags: review?(iann_bugzilla)
Flags: needinfo?(jh)
(In reply to rsx11m from comment #1)
> the second preference missing from sync
> is services.sync.prefs.sync.privacy.sanitize.promptOnSanitize, where bug
> 576970 only introduced
> services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown.
> 
> Jens, if the latter preference setting isn't supposed to be synced on
> purpose, please let me know and I'll remove it from the patch

I think we only left it out because the initial set of preferences was based on Firefox's set, and promptOnSanitize is SM-only.

Anyway, from my POV there are only two checks needed to decide whether we should configure a pref for syncing by default or not:
1) Whether the pref name is static. Dynamic ones, like used in some parts of MailNews, are currently not supported by the Preferences Sync engine.
2) Whether we expect users to have the pref value equal on multiple computers. Ones that might for example have different values on different platforms (with equal pref name) should not be added since Sync works cross-platform.

Of course, the usual review chain applies, so the reviewer might disagree, or me (as the SM Sync owner), or the SM Council (unlikely, but for the sake of completeness...).
Flags: needinfo?(jh)
(Assignee)

Comment 3

5 years ago
Oh, I figured that preferences imply Ian's review without realizing that you are actually the module owner for Sync, sorry about that (Ian is listed as a peer, thus I hope that I'm still fine).

Bug 765398 introduced privacy.donottrackheader.value and also added it to the Sync prefs for Firefox, so I'd assume that there is no question whether or not to add it given that privacy.donottrackheader.enabled is already synced and the two together determine the configuration of the DNT feature.

Similarly, I'd think that privacy.sanitize.promptOnSanitize should be synced given that it's sibling privacy.sanitize.sanitizeOnShutdown already is (why would you set one the same on all installations but keep the others different?).
(In reply to rsx11m from comment #3)
> Oh, I figured that preferences imply Ian's review without realizing that you
> are actually the module owner for Sync, sorry about that (Ian is listed as a
> peer, thus I hope that I'm still fine).

Hehe, choosing a peer is perfectly fine, and especially a Preferences expert. :-)

Regarding which prefs to sync, I'd like to point out that it doesn't matter much what Firefox does or which similar/related prefs are already synced. I'm not implying that your judgment as described in comment 3 falls into either category; just wanted to make sure you get the idea that "what makes sense for the average/targeted user" is all that should count.

That said, I agree that the prefs you cited should be synced (according to your description; don't take this as "I checked those pref names").
Flags: needinfo?(jh)
(Assignee)

Comment 5

5 years ago
(Weird, bugzilla seems to be loosing track of the flags again. I didn't request another needinfo?...)

Comment 6

5 years ago
Comment on attachment 833388 [details] [diff] [review]
Add missing sync prefs

[Triage Comment]

r=me, a=me for c-a/c-b too
Attachment #833388 - Flags: review?(iann_bugzilla)
Attachment #833388 - Flags: review+
Attachment #833388 - Flags: approval-comm-beta+
Attachment #833388 - Flags: approval-comm-aurora+
(Assignee)

Comment 7

5 years ago
Created attachment 8345273 [details] [diff] [review]
Final patch for checkin [r+a=IanN]

> pref("services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown", true);
>+pref("services.sync.prefs.sync.privacy.sanitize.promptOnSanitize", true);

As mentioned on IRC, these needed to be switched to retain alphabetical order.
(Assignee)

Updated

5 years ago
status-seamonkey2.23: --- → wontfix
status-seamonkey2.24: --- → affected
status-seamonkey2.25: --- → affected
status-seamonkey2.26: --- → affected
Keywords: checkin-needed
Whiteboard: [c-n: attachment 8345273 for comm-central/aurora/beta]
Attachment #833388 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/a958e35a8119
https://hg.mozilla.org/releases/comm-aurora/rev/e7cc5e3c989a
https://hg.mozilla.org/releases/comm-beta/rev/b3fbfd93e668
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: attachment 8345273 for comm-central/aurora/beta]
Target Milestone: --- → seamonkey2.26
status-seamonkey2.24: affected → fixed
status-seamonkey2.25: affected → fixed
status-seamonkey2.26: affected → fixed
(Assignee)

Comment 9

5 years ago
Comment on attachment 8345273 [details] [diff] [review]
Final patch for checkin [r+a=IanN]

Thanks. I can't carry forward the a+ flags, but for reference let's set the r+ from attachment 833388 [details] [diff] [review] at least.
Attachment #8345273 - Attachment description: Final patch for checkin → Final patch for checkin [r+a=IanN]
Attachment #8345273 - Flags: review+
You need to log in before you can comment on or make changes to this bug.