1098343 - Default preference differences between devedition and non-devedition in shared-profile usecase break people wanting non-default settings

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[kud]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141113004001

Steps to reproduce:

- Go to dev edition
- Choose light theme for devtools
- Exit dev edition
- Go to nightly or stable (with the same profile)
- Exit nightly or stable
- Go to dev edition


Actual results:

black theme is coming back.


Expected results:

it should stay on light theme.

Comment 1

[:Gijs (out for now; he/him)]

This is because the defaults are different:

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1479

and saving a pref to its default value is a no-op, meaning nothing ends up in prefs.js, and then it'll pick the (other) default value when you start in the other channel.

Solution: different pref to override this for devedition (or switch the default).

Comment 2

[Brian Grinstead [:bgrins]]

In general, I've noticed that switching between channels on the same profile doesn't have very good test coverage and can cause weird things like this to happen.

(In reply to :Gijs Kruitbosch from comment #1)
> This is because the defaults are different:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js#1479
> 
> and saving a pref to its default value is a no-op, meaning nothing ends up
> in prefs.js, and then it'll pick the (other) default value when you start in
> the other channel.
> 

I'm trying to make sure I understand exactly what happens here:

1. Go to dev edition - devtools.theme is the default, dark
2. Choose light theme for devtools - devtools.theme is user set, light
3. Exit dev edition - devtools.theme is user set, light
4. Go to nightly or stable with the same profile - devtools.theme is user set, light. Does it become un-user set at this step since it matches with the default?  That's the only thing that makes sense to me that would explain the results when back in dev edition
5. Exit nightly or stable - I guess it's un-user set at this point?
6. Go to dev edition - devtools.theme is back to dark theme

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> In general, I've noticed that switching between channels on the same profile
> doesn't have very good test coverage and can cause weird things like this to
> happen.
> 
> (In reply to :Gijs Kruitbosch from comment #1)
> > This is because the defaults are different:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> > js#1479
> > 
> > and saving a pref to its default value is a no-op, meaning nothing ends up
> > in prefs.js, and then it'll pick the (other) default value when you start in
> > the other channel.
> > 
> 
> I'm trying to make sure I understand exactly what happens here:
> 
> 1. Go to dev edition - devtools.theme is the default, dark
> 2. Choose light theme for devtools - devtools.theme is user set, light
> 3. Exit dev edition - devtools.theme is user set, light
> 4. Go to nightly or stable with the same profile - devtools.theme is user
> set, light. Does it become un-user set at this step since it matches with
> the default?  That's the only thing that makes sense to me that would
> explain the results when back in dev edition

Yes. I think we force-flush prefs on exit (maybe? or only if they're dirty, except I'm fairly sure I read recently that there are bugs in that detection...), and re-saving the pref file will cause this to be un-set. I dunno. Maybe it requires actually doing something which causes a pref flush.

Comment 4

[:Gijs (out for now; he/him)]

(either way, we could fix by making devtools also check a devedition.override pref or something, and read/set that only when devedition is defined - layers of complexity!)

Comment 5

[Brian Grinstead [:bgrins]]

This is also going to be a problem when someone disables the devedition theme on devedition, then opens up stable, and reopens devedition.  The theme will be applied again.  I'm not sure how deeply we want to support this use case (we do have an option now to run the channels alongside each other and sync between them).  It sounds like the solution you are proposing in Comment 4 would work, I'm just concerned about adding extra complexity / differences between DE and other channels for things like bug 1097937.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

I don't think using dev-edition on a profile shared with release should be supported. It was a product-specific decision to use a separate profile, with well-understood tradeoffs. People can always do whatever they want with it of course, but we shouldn't go out of our way to engineer solutions for such cases.

Comment 7

[kud]

So why there's an option "use different profile" where you can disable it? you'll remove it?

Comment 8

[kud]

Plus, what do you do with people like me who only use one profile / one browser. Dev edition is perfect for me as user and developer. Plus it seems to be faster than original.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #6)
> I don't think using dev-edition on a profile shared with release should be
> supported. It was a product-specific decision to use a separate profile,
> with well-understood tradeoffs. People can always do whatever they want with
> it of course, but we shouldn't go out of our way to engineer solutions for
> such cases.

People have done this with aurora for the longest of times. I don't think just because it's now called devedition and we're trying to actively encourage a certain target usergroup to use it, we should make it quite as separate as that - it's also still part of the trains system, and non-target-usergroup people need to be able to use it normally, which includes being able to run it easily with profiles used on other update trains.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Erwann Mest from comment #7)
> So why there's an option "use different profile" where you can disable it?
> you'll remove it?

That option allows you to use another, presumably aurora-specific, profile and it's provided for long-time aurora users who would like to opt-out of the new profile and do things their own way. There are no plans to remove it, but it's not something that we expect anyone other than very experienced users to use. If one belongs in that category, great, but he is expected to know how to deal with the rough edges.

(In reply to Erwann Mest from comment #8)
> Plus, what do you do with people like me who only use one profile / one
> browser. Dev edition is perfect for me as user and developer. Plus it seems
> to be faster than original.

If all you use is Developer Edition, then there shouldn't be any issues. Switching to Release and then back *on the same profile*, is what is causing the problem we are discussing here.

(In reply to :Gijs Kruitbosch from comment #9)
> People have done this with aurora for the longest of times. I don't think
> just because it's now called devedition and we're trying to actively
> encourage a certain target usergroup to use it, we should make it quite as
> separate as that - it's also still part of the trains system, and
> non-target-usergroup people need to be able to use it normally, which
> includes being able to run it easily with profiles used on other update
> trains.

In Developer Edition we are doing experimental stuff, like enabling features that are not quite ready for the vast majority of users out there. Today it's new tools like the timeline, tomorrow it could be e10s or some new tool for asm.js. When we enable a feature in Developer Edition we feel confident that the code *in that branch* doesn't explode in normal use. It could perfectly well crash and burn older releases and sharing the profile is a surefire way to make that happen.

Remember how users who tried e10s on Nightly a few months ago found it unusable and then went back to Release *on the same profile*? That was a very bad experience. Try using Developer Edition today on a profile shared with Nightly and then you see that some devtools no longer work (we are still landing e10s-related fixes).

This is the kind of bad experience we are trying to protect the user from.

Comment 11

[kud]

Thank you for the explanation. ;)

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > People have done this with aurora for the longest of times. I don't think
> > just because it's now called devedition and we're trying to actively
> > encourage a certain target usergroup to use it, we should make it quite as
> > separate as that - it's also still part of the trains system, and
> > non-target-usergroup people need to be able to use it normally, which
> > includes being able to run it easily with profiles used on other update
> > trains.
> 
> In Developer Edition we are doing experimental stuff, like enabling features
> that are not quite ready for the vast majority of users out there. Today
> it's new tools like the timeline, tomorrow it could be e10s or some new tool
> for asm.js. When we enable a feature in Developer Edition we feel confident
> that the code *in that branch* doesn't explode in normal use. It could
> perfectly well crash and burn older releases and sharing the profile is a
> surefire way to make that happen.
> 
> Remember how users who tried e10s on Nightly a few months ago found it
> unusable and then went back to Release *on the same profile*? That was a
> very bad experience. Try using Developer Edition today on a profile shared
> with Nightly and then you see that some devtools no longer work (we are
> still landing e10s-related fixes).
> 
> This is the kind of bad experience we are trying to protect the user from.

And protecting users is noble and a fine thing, and we should do that. FWIW, when I posted my initial reply, I thought this was bug 1098986 (blame bugmail and me not re-reading every single bug top to bottom - sorry). I do think we should fix that, and it's probably higher priority than this bug.

For this particular bug, I think we're basically running into a limitation of the pref system. While I agree that we should protect users from themselves, so to speak, I don't think we should do that for things like these themes.

comment #0 and comment #5 are somewhat different cases, though, I think.

1) I think it makes sense that people expect the devtools theme to be in sync if they use the profile in different branches.

2) I think it currently makes less sense for the devedition theme to be in sync - but actually, it's quite possible people will feel differently about this once it's available everywhere - plenty of people want us to support a "global dark mode" kind of thing, and I'm sure we'll see some kind of "hey, if you go into about:config and do this and that, you get this other theme even in release/beta Firefox" blogposts when 35 hits beta/release.


The simplest way to achieve (1) would be to have the defaults be the same everywhere. Failing that, we could use a dual pref system (one the default, the other the user setting).

For (2)... it depends on what we want. I've been writing this comment for 10 minutes now because I can't come up with any solution that deals with all the cases:

1) user wants devedition theme everywhere
2) user wants devedition theme on devedition, but not elsewhere
3) user wants to disable devedition theme everywhere

I guess we could make it a tristate pref internally, but I don't know how that would work gui-wise... although I suppose the default could be 2, and toggling it on devedition could set it between 2 and 3, while toggling on non-devedition would toggle between 1 and 2?

Whether the complexity is worth it though... I don't know. I do think that the current state is confusing for some people, and I don't know how to fix this without putting in significant effort.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to :Gijs Kruitbosch from comment #12)
...
> 1) user wants devedition theme everywhere
> 2) user wants devedition theme on devedition, but not elsewhere
> 3) user wants to disable devedition theme everywhere
> 
> I guess we could make it a tristate pref internally, but I don't know how
> that would work gui-wise... although I suppose the default could be 2, and
> toggling it on devedition could set it between 2 and 3, while toggling on
> non-devedition would toggle between 1 and 2?
> 
> Whether the complexity is worth it though... I don't know. I do think that
> the current state is confusing for some people, and I don't know how to fix
> this without putting in significant effort.

I don't think the complexity is worth it, I would much rather drop case 1 for the *product*, and if people love the theme so much they can pull it out of the codebase and apply it as a 3rd party complete theme. I suspect (with no evidence to back me up) someone is already working on that.

Comment 15

[:Gijs (out for now; he/him)]

Gavin, fwiw, I expect that this is related to e.g. https://twitter.com/gavinsharp/status/542090255600349186 .

Unfortunately, that doesn't make this any easier to fix.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

"me too" - I would be sad if I was told it's OK for me to bounce between nightly and beta/release, but I should expect a really poor experience if I want to continue including Aurora/DE.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

We talked about this in our weekly dev-edition meeting and the consensus was that we should wontfix this. The problems described in comment 10 and comment 12 are not easily solvable and are only affecting experienced, old Aurora users, who are vastly outnumbered by the new users we attracted to this channel. 

Furthermore, the workaround for those who face this issue is simple and well-documented: use the default devedition-specific profile and Sync to copy your data over to it. Unfortunately, I don't believe we have the resources to spend into making this corner case work as expected.

Gavin, what's your take on it?

Comment 18

[:Gijs (out for now; he/him)]

FWIW, one other idea I had about this after looking at markh's report was that we could do what e10s did and use separate prefs for devedition vs. the other channels, where we change the defaults for devedition. This at least fixes the counterintuitive "it doesn't remember what I told it" issue, without affecting new users. That seems like it'd be a simple enough patch.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I agree with Gijs - using different default prefs across channels shouldn't be hard. If it somehow gets too complicated in ways I don't understand yet, I am fine with WONTFIX.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

To be clear, this is about using separate prefs for just the theme, right? The proposal is not to use separate prefs for every experimental devtools feature that we intend to enable on dev-edition only in the future? Because that would obviously not scale.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Panos Astithas [:past] from comment #20)
> To be clear, this is about using separate prefs for just the theme, right?
> The proposal is not to use separate prefs for every experimental devtools
> feature that we intend to enable on dev-edition only in the future? Because
> that would obviously not scale.

How many prefs does dev-edition currently have that are different? I thought it was still less than ~15

Depending on how we implement it, it wouldn't actually be that hard to scale if we used a consistent pref prefix/suffix and a helper function, for instance.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Panos Astithas [:past] from comment #20)
> > To be clear, this is about using separate prefs for just the theme, right?
> > The proposal is not to use separate prefs for every experimental devtools
> > feature that we intend to enable on dev-edition only in the future? Because
> > that would obviously not scale.
> 
> How many prefs does dev-edition currently have that are different? I thought
> it was still less than ~15

Yeah, probably around that.  We will also be adding others in the future.

> Depending on how we implement it, it wouldn't actually be that hard to scale
> if we used a consistent pref prefix/suffix and a helper function, for
> instance.

Some prefs will be cleaner / easier than others.  For an example, here are all the references of devtools.theme: http://dxr.mozilla.org/mozilla-central/search?q=devtools.theme.   It (and other prefs) includes a reference in the options panel xul file, so we would we would need an additional preprocessor directive there as well depending on how it's implemented.

I'm not sure the best way to handle this in JS.  A couple of ideas:
1. Have a helper function that takes in a string and just returns something like (str + ".devedition-default") when in the DE channel.  Then make sure that is called every time one of these prefs is used.
2. Have a custom get*Pref / set*Pref / clearUserPref implementation that we use just for *all* references to prefs within devtools and it keeps a whitelist of preferences that need prefixing.

The downside to 1 is that we would have to remember exactly which prefs are overridden whenever writing / reviewing patches.  Would be hard to keep track of because things will be working fine in development.  When we change an existing pref's default on devedition, we have to also remember to find/replace all occurrences of that pref.

The downside to 2 is that it would require a lot of changed lines and would be hard to remember / enforce usage of this function in place of the default functionality.

The downside to both is that we have to keep track of which defaults have changed in more than one place (firefox.js, anywhere the prefs are accessed in JS, and anywhere the prefs are stored in markup).

I would probably approach this using something like option 1 but only for the devtools.theme and browser.devedition.theme.enabled prefs since they are the most likely to change by the user and be broken in the most spectacular ways.  Of course, then you could end up with things that are less obviously broken which could be very frustrating.  But at least this would cover the most common bugs.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

(In reply to Panos Astithas [:past] from comment #20)
> To be clear, this is about using separate prefs for just the theme, right?
> The proposal is not to use separate prefs for every experimental devtools
> feature that we intend to enable on dev-edition only in the future? Because
> that would obviously not scale.

I hope it would include other prefs such as "use chrome debugging" and "allow remote connections" - the fact these are reset in the same scenario is, for me, as big a problem as the theme - and almost ironic :)

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #3)
> Yes. I think we force-flush prefs on exit (maybe? or only if they're dirty,
> except I'm fairly sure I read recently that there are bugs in that
> detection...), and re-saving the pref file will cause this to be un-set. I
> dunno. Maybe it requires actually doing something which causes a pref flush.

(In reply to :Gijs Kruitbosch from comment #12)
> For this particular bug, I think we're basically running into a limitation
> of the pref system. While I agree that we should protect users from
> themselves, so to speak, I don't think we should do that for things like
> these themes.

What do you think about whitelisting a set of prefs to *not* do this behavior with?  So, basically on shutdown don't force clear these prefs even if it matches the default value?  I don't know if this is reasonable (or where/how to do that), but it would be easier to maintain if we changed this behavior.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Yes. I think we force-flush prefs on exit (maybe? or only if they're dirty,
> > except I'm fairly sure I read recently that there are bugs in that
> > detection...), and re-saving the pref file will cause this to be un-set. I
> > dunno. Maybe it requires actually doing something which causes a pref flush.
> 
> (In reply to :Gijs Kruitbosch from comment #12)
> > For this particular bug, I think we're basically running into a limitation
> > of the pref system. While I agree that we should protect users from
> > themselves, so to speak, I don't think we should do that for things like
> > these themes.
> 
> What do you think about whitelisting a set of prefs to *not* do this
> behavior with?  So, basically on shutdown don't force clear these prefs even
> if it matches the default value?  I don't know if this is reasonable (or
> where/how to do that), but it would be easier to maintain if we changed this
> behavior.

I think that'd require updating libpref to support such a thing, and it'd break a whole host of assumptions (such as prefHasUserValue being a good predictor for whether the user branch has a value and/or whether the value is the same as it is on the default branch).

It sounds like a substantial amount of work that might be tricky to get right, but I am not an expert on our prefs system. Benjamin is. :-)

Comment 26

[Benjamin Smedberg]

A whitelist doesn't sound like a huge amount of work. If we were redesigning the pref impl from scratch, I suspect we'd be a lot more aggressive about saving some user prefs even if they match the default.

It's unlikely that anyone on my team would be able to do anything about it this year, though. So we'd only accept patches if they had thorough unit tests and were relatively easy to review.

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #26)
> A whitelist doesn't sound like a huge amount of work. If we were redesigning
> the pref impl from scratch, I suspect we'd be a lot more aggressive about
> saving some user prefs even if they match the default.
> 
> It's unlikely that anyone on my team would be able to do anything about it
> this year, though. So we'd only accept patches if they had thorough unit
> tests and were relatively easy to review.

OK great, I'll look into how to do this.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0006-Bug-1098343-channel_pref.patch

Here is a strawman:

* firefox.js or similar can do:

pref("foo", "bar")
#ifdef NIGHTLY
channel_pref("something", "value1");
#else
channel_pref("something", "value2");
#endif

The pref "something" will *always* be written to the user prefs file.  The name "channel_pref" is simply to reflect that the main use-case is for different channels wanting different default values - a better/different name might be good.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #26)
> A whitelist doesn't sound like a huge amount of work. If we were redesigning
> the pref impl from scratch, I suspect we'd be a lot more aggressive about
> saving some user prefs even if they match the default.

In effect, the whitelist is any pref declared with channel_pref().  There's no additional API to adjust a pref to this behaviour.  Includes a simple test.

Benjamin, do you think this is a reasonable first-cut?

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8585329 [details] [diff] [review]
0006-Bug-1098343-channel_pref.patch

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

::: modules/libpref/prefread.h
@@ +25,5 @@
>   * @param type
>   *        preference type (PREF_STRING, PREF_INT, or PREF_BOOL)
>   * @param defPref
>   *        preference type (true: default, false: user preference)
> + * @param defPref

oops - should be channelPref

Comment 30

[Benjamin Smedberg]

Comment on attachment 8585329 [details] [diff] [review]
0006-Bug-1098343-channel_pref.patch

Couple notes here:

I don't like the name "channel pref". I think that the name should make it clear that once the user sets the pref, it's always written out to prefs.js. Perhaps "persistent pref"?

I know we're light on docs for prefs in general, but I want documented somewhere:
* that these always-recorded pref values are only written out if the user explicitly sets them
* what files can contain the "channel_pref" instruction? Is it only default pref files, or the user prefs.js file, or some combination?

I'm assuming that we don't expect these to show up in user prefs.js files, right? That would break backwards-compat of the file format, which is not a good idea.

It doesn't appear that you're exposing the ability to set or get this new flag from the pref API anywhere. Is that because you don't expect script to ever need to know about this? I bet we'll want to end up exposing something about this to about:config, so we'll need something like nsIPrefBranch.isPersistentPref("name") at a minimum.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)

> Perhaps "persistent pref"?

Sounds good to me! I'll use persistent_pref() in the examples below.

> make it clear that once the user sets the pref, it's always written

That's subtly different from what the patch does (and I should have made it clearer). The patch is setting a bit that says "absolutely always write the current pref value as a user_pref()". So in the context of Firefox it means the first time a persistent_pref() is seen it is "frozen" with that value until something (ie, the user) explicitly changes it.

With "only write when changed", it's an almost identical outcome, except the value isn't frozen until the first time it is *changed*.  But it's only going to be *changed* when the current value is "wrong" - and it's only going to be wrong due to this "different channels have different default prefs" issue (eg, e10s, devtools).

Avoiding "only write when changed" also means a much simpler patch and much simpler semantics - ie, "persistent_pref() == always write a user_pref()". It avoids tracking another pref value (firefox.js default, prefs.js default, current in-memory value) and only costs 1 bit per pref.

> Perhaps "persistent pref"?

That's exactly the kind of word my brain insists ends in "ant" - but I think that's just me :) Maybe sticky or frozen? I'll take your favorite (even if I *am* still leaning a little towards 'channel' after re-explaining use-cases :)

> I know we're light on docs for prefs in general, but I want documented
> somewhere:
> * that these always-recorded pref values are only written out if the user
> explicitly sets them

See above - always written.

> * what files can contain the "channel_pref" instruction? Is it only default
> pref files, or the user prefs.js file, or some combination?

This should be documented next to "pref()" and would read something like "Similar to pref(), but as well as marking the preference as a 'default' pref, also marks it as 'persistent', meaning the preference will be written even if the value specified is identical to the channel's current default value'

(Obviously, "pref()" would be documented as something like "marks as default, saving prefs only writes it if a user_pref differs from the default")

> I'm assuming that we don't expect these to show up in user prefs.js files,
> right? That would break backwards-compat of the file format, which is not a
> good idea.

Yep, exactly - it has the exact same semantics as "pref()" but sets an additional bit at read-time forcing the pref value to *always* be written as a "user_pref()" at write-time.

> It doesn't appear that you're exposing the ability to set or get this new
> flag from the pref API anywhere. Is that because you don't expect script to
> ever need to know about this? I bet we'll want to end up exposing something
> about this to about:config, so we'll need something like
> nsIPrefBranch.isPersistentPref("name") at a minimum.

I'm not even sure about that - it just means that in some profiles the pref will be marked as being the default value while in others it will not. You can reset it, which will just toggle the set of Firefox channels in which it is considered the default and in which it isn't.

Benjamin, any thoughts on the above?

Comment 32

[Benjamin Smedberg]

I think the behavior "absolutely always write the current pref value as a user_pref" is wrong. If a user is switching back and forth between channels and hasn't made any explicit decisions in the pref pane, their default *should* change.

Only in the case where the user has explicitly made a change should we remember that change.

This is not hard to do in the current code, and still only requires a single flag bit.

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0008-Bug-1098343-sticky_pref.patch

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)

> This is not hard to do in the current code, and still only requires a single
> flag bit.

OK - this version work as requested. I did misunderstand initially (but it took 2 bits ;) Once bit to flag it when the default is loaded and another to flag it once the value changes.

Note this patch uses sticky_pref() - the parser really wants each function to have a unique first char ;) I think "sticky" is quite fine, but if you really want persistent_pref (and the "look at the next char too" changes this would mean to the parser) let me know.

The basic idea is:
* sticky_pref() sets the flag to say "hey, I've got sticky semantics".
* user_pref() and any of the runtime setPref() functions set the "hey, I've also got a sticky value".
* saving prefs only writes ones with the "sticky value" flag. So a pref that sees sticky_pref() followed by user_pref() is always going to be subsequently written.

A slight downside is that this means that if we manage to process:

user_pref("somepref", ...);
sticky_pref("somepref", ...);

We will not work correctly - we expect to have seen the sticky_pref() first. We could probably warn/throw in this case if the user_pref() has a different value to the default, but I'm not sure that's worthwhile and shouldn't be possible in practice.

Comment 34

[Benjamin Smedberg]

I still don't understand why it takes two bits.

We already know whether a pref has a user value. I believe that the only thing we need to know is what do to when the value gets set by the user back to the default.

Currently in that case we remove the user value: http://hg.mozilla.org/mozilla-central/annotate/2c9708e6b54d/modules/libpref/prefapi.cpp#l790

But with the new single bitflag, we should instead keep storing the user value in this case and write it out to disk like any other user pref.

Comment 35

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1098343-sticky_pref.patch

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #34)
> But with the new single bitflag, we should instead keep storing the user
> value in this case and write it out to disk like any other user pref.

Right! Thanks for spelling it out for me :) This patch does exactly that.

Comment 36

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0008-sticky-prefs-use-stick_pref-to-define-some.patch

This patch adjusts some of the relevant prefs to be sticky_prefs.  I tested this on a local nightly and a locally built aurora, and it works as expected:

* A profile without (say) "devtools.chrome.enabled" defined in prefs.js continues to use the channel default and the value isn't written to prefs.js.

* Once the value has been set in any channel it's written to prefs.js and sticks - so swapping between channels keeps the preference value set in the other channel.

Not formally requesting feedback yet while the "part 1" is in flux, but feedback from any of the devtools peers would be welcome!

Comment 37

[Benjamin Smedberg]

Comment on attachment 8593206 [details] [diff] [review]
0007-Bug-1098343-sticky_pref.patch

Overall this looks correct. I think you need more test coverage, especially for these cases:

* prefHasUserValue for the sticky pref case
* pref observers still fire correctly for sticky prefs:
** in particular if set*Pref("name", defaultValue), should pref observers fire? I can see both sides, but we need to specify and test the behavior.

Comment 38

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1098343-sticky_pref.patch

> * prefHasUserValue for the sticky pref case

Done - prefHasUserValue always returns true once it has a sticky value, even when it's the default. Test has comments justifying this based on it giving sane about:config behaviour.

> * pref observers still fire correctly for sticky prefs:
> ** in particular if set*Pref("name", defaultValue), should pref observers
> fire? I can see both sides, but we need to specify and test the behavior.

Done - pref always fires even when changed to the default. test attempts to justify this behaviour too (but really I would have justified the other way around if necessary ;)

Also tests for clearUserPref (which fully resets as if the pref had never been changed.)

So I think this is looking nearly done! Benjamin, is there somewhere other than https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences I would need to add docs once this gets review?

Comment 39

[Benjamin Smedberg]

I don't know of another place, no.

Are you looking for feedback or a final code review on this patch?

Comment 40

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1098343-sticky_pref.patch

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #39)

> Are you looking for feedback or a final code review on this patch?

I was mainly looking for feedback that the described semantics are desired - but sure, happy for review! This version has a few trivial cosmetic changes to the tests.

Comment 41

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8593207 [details] [diff] [review]
0008-sticky-prefs-use-stick_pref-to-define-some.patch

Brian,
 If you aren't the correct person to look at this, can you please nominate someone else?

Comment 42

[Benjamin Smedberg]

Comment on attachment 8597834 [details] [diff] [review]
0003-Bug-1098343-sticky_pref.patch

I'd like to see the commit message also. marking r+ but please set a NEEDINFO for that.

Comment 43

[Mark Hammond [:markh] [:mhammond]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #42)
> I'd like to see the commit message also. marking r+ but please set a
> NEEDINFO for that.

I was thinking "Bug 1098343 - support 'sticky' preferences, meaning a user value is retained even when it matches the default. r=bsmedberg"

I'm also struggling to find anywhere other than https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences to document this, but that doesn't have detailed information about any of the pref functions - so I think I'll add that sticky_prefs() exists and a paragraph near the end about how and why they work.

Comment 44

[Brian Grinstead [:bgrins]]

(In reply to Mark Hammond [:markh] from comment #41)
> Comment on attachment 8593207 [details] [diff] [review]
> 0008-sticky-prefs-use-stick_pref-to-define-some.patch
> 
> Brian,
>  If you aren't the correct person to look at this, can you please nominate
> someone else?

Thanks, I'm going to take a look at this today.

Comment 45

[Brian Grinstead [:bgrins]]

Comment on attachment 8593207 [details] [diff] [review]
0008-sticky-prefs-use-stick_pref-to-define-some.patch

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

This looks good, I've tested with aurora / nightly builds and confirmed the prefs are sticking as expected.

Comment 46

[Benjamin Smedberg]

LGTM

Comment 47

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8593207 [details] [diff] [review]
0008-sticky-prefs-use-stick_pref-to-define-some.patch

Awesome! Once this gets r+ I'll land both patches. For the commit message I'll say "Use sticky_pref to define devtools preferences that use different defaults on different channels."

Comment 48

[Brian Grinstead [:bgrins]]

Comment on attachment 8593207 [details] [diff] [review]
0008-sticky-prefs-use-stick_pref-to-define-some.patch

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

This looks good, I've tested with aurora / nightly builds and confirmed the prefs are sticking as expected.

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/99afe078d602
https://hg.mozilla.org/integration/fx-team/rev/c20dd193b5df

Comment 50

[Mark Hammond [:markh] [:mhammond]]

Thanks! needinfo me to update MDN once it gets to central.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99afe078d602
https://hg.mozilla.org/mozilla-central/rev/c20dd193b5df

Comment 52

[Mark Hammond [:markh] [:mhammond]]

Added some tweaks to https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences