Open Bug 1240198 Opened 8 years ago Updated 2 years ago

Indication to the user if a preference is live or it needs a restart

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Platform:
Unspecified
All
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox46 --- affected

People

(Reporter: milan, Unassigned)

References

Details

Attachments

(4 files, 20 obsolete files)

Part 2. Handle file I/O (mostly I) for preference flags live/restart and set a few graphics preferences to the correct state. r=bsmedberg
19.93 KB, patch
Details | Diff | Splinter Review
Part 1. Add methods to set and get the live/restart and session modified valuues,through idl, Preferences, gfxPref, as appropriate. Previously r=bgirard, r=bsmedberg
14.15 KB, patch
Details | Diff | Splinter Review
Part 3. Show preference live/restart type in about:config, in nightly only. Previous review by pchang. r=bsmedberg
7.79 KB, patch
Details | Diff | Splinter Review
Part 4. Track which preferences have changed during the session, and would need restart to apply. Nightly only. Increases the size of the pref hash table entry back to 40 bytes (from 32.) r=bsmedberg
6.65 KB, patch
Details | Diff | Splinter Review
Attached image An example of a prototype (obsolete) — 
We have preferences that are only evaluated when first used or even at start up (call these "Once"), and we have preferences that are immediately changing Firefox behaviour (call these "Live" - see bug 1158428 for some background.)

This bug is to see if we want to expose this to the users (of about:config), giving them a hint whether they have to restart Firefox or not when changing those values.
Some thoughts from prototyping:
* We'll want a Preferences API to explicitly set the preference type ("Once", "Live" or "Not specified", but please ignore the names) from C++ code.
* We'll want preference read/write API to set the preference type from JS, when the values are set (e.g., instead of user_pref and sticky_pref, either allow an extra flag, or a separate type restart_pref and norestart_pref, for example. Although sticky makes it a bit more complicated.)
* Right now I'm thinking of an extra column in the about:config page, but only because there seems to be a lot of room there.  It could easily be a tooltip, or just a message once the value is modified.
restart_pref and norestart_pref is not enough, even ignoring sticky - we would need the regular and user versions of both.  And if sticky is in the picture, perhaps the sticky version as well.
One ugliness is that the parser basically makes a decision on what particular command it is only based on the first character.  We have "pref", "user_pref" and "sticky_pref".  We can add "restart_pref" and "norestart_pref", but we couldn't also have "restart_user_pref" and "norestart_user_pref" as the first characters match.  At least, not without modifying the parser.  And if the parser was getting modified, then we may be better of adding a (trailing) flag for any extra attributes and future proof it.  Otherwise we're going from originally two, through recently three into nine different versions (assuming restart_sticky_pref and norestart_sticky_pref.)
Attached image Screen grab of the current WIP. (obsolete) — 
With some feedback from UX, don't tag the "live" preferences (except when debugging) and simplify the wording somewhat.
Attachment #8708588 - Attachment is obsolete: true

        Attachment #8728332 -
        Attachment is obsolete: true

        Attachment #8728334 -
        Attachment is obsolete: true

        Attachment #8728335 -
        Attachment is obsolete: true

        Attachment #8728336 -
        Attachment is obsolete: true

    
    

    
  
I'm going to start asking for sanity check reviews, but will ask for a preferences peer review once the first wave is done.
Assignee: nobody → milan

    
    

    
  
Comment on attachment 8739573 [details] [diff] [review]
Part 1. Add methods to set and get the live/restart and session modified valuues,through idl, Preferences, gfxPref, as appropriate.

First pass at reviews, will also get one from the peer once it passes.

        Attachment #8739573 -
        Flags: review?(bgirard)

    
    

    
  
Comment on attachment 8739574 [details] [diff] [review]
Part 2. Handle pref I/O (mostly I) and get rid of sticky_pref

First pass at reviews, will also get one from the peer once it passes.

        Attachment #8739574 -
        Flags: review?(mchang)

    
    

    
  
Comment on attachment 8739575 [details] [diff] [review]
Part 3. Show preference live/restart type in about:config.

First pass at reviews, will also get one from the peer once it passes.

        Attachment #8739575 -
        Flags: review?(howareyou322)

    
    

    
  
Comment on attachment 8739576 [details] [diff] [review]
Part 4. Track what has changed and needs restart.  Increases hash table entry size back to 40 bytes (recently optimized from 40 to 32) on some platforms. Nightly only.

First pass at reviews, will also get one from the peer once it passes.

        Attachment #8739576 -
        Flags: review?(dvander)

    
    

    
  
Comment on attachment 8739573 [details] [diff] [review]
Part 1. Add methods to set and get the live/restart and session modified valuues,through idl, Preferences, gfxPref, as appropriate.

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

r+ with these minor changes made.

::: modules/libpref/nsIPrefBranch.idl
@@ +239,5 @@
> +   *               1  The preference would need restart if modified to take effect.
> +   *               0  The preference takes effect immediately.
> +   *              -1  The preference is undeclared as to what it does.
> +   */
> +  long prefRestartRequired(in string aPrefName);

Assuming that there's no enum in idl, we should probably do something similar to nsIGfxInfo and have const long values for these return codes.

::: modules/libpref/prefapi.cpp
@@ +469,5 @@
> +
> +int32_t PREF_RestartRequired(const char *pref_name)
> +{
> +    if (!gHashTable)
> +        return -1;

these should also be replaced to use the const from the IDL. It would make this code much more readable.

@@ +472,5 @@
> +    if (!gHashTable)
> +        return -1;
> +
> +    PrefHashEntry *pref = pref_HashTableLookup(pref_name);
> +    if (!pref) return -1;

I'm not a fan of single line if. In general the guidelines say that they should also always be braced which I agree with it.

        Attachment #8739573 -
        Flags: review?(bgirard) → review+

    
    

    
  
Comment on attachment 8739576 [details] [diff] [review]
Part 4. Track what has changed and needs restart.  Increases hash table entry size back to 40 bytes (recently optimized from 40 to 32) on some platforms. Nightly only.

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

Looks good, but why nightly only?

::: modules/libpref/prefapi.cpp
@@ +824,5 @@
> +#ifdef NIGHTLY_BUILD
> +    if (flags & kPrefFromFile) {
> +        pref_SetValue(&pref->sessionPref, pref->prefFlags, value, type);
> +    } else {
> +        bool toWhat = pref_ValueChanged(pref->sessionPref, value, type);

s/toWhat/changed/ would make the return value clearer.

        Attachment #8739576 -
        Flags: review?(dvander) → review+

    
    

    
  
Comment on attachment 8739575 [details] [diff] [review]
Part 3. Show preference live/restart type in about:config.

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

LGTM.

::: toolkit/locales/en-US/chrome/global/config.properties
@@ +19,5 @@
>  modify_title=Enter %S value
> +modify_title_restart=Enter %S value (restart required)
> +
> +modify_type_restart=requires restart
> +modify_type_live=

Suggest we can define the string for live type, then we can confirm this preference is live not undeclared type. So we can know which preference is still undeclared and update it if needs.

        Attachment #8739575 -
        Flags: review?(howareyou322) → review+

    
    

    
  
Comment on attachment 8739574 [details] [diff] [review]
Part 2. Handle pref I/O (mostly I) and get rid of sticky_pref

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

::: modules/libpref/prefread.cpp
@@ +323,5 @@
>          case PREF_PARSE_INT_VALUE:
>              /* grow line buffer if necessary... */
>              if (ps->lbcur == ps->lbend && !pref_GrowBuf(ps))
>                  return false; /* out of memory */
> +            if (isdigit(c)) 

nit: whitespace

::: modules/libpref/prefread.h
@@ +60,5 @@
> +
> +    bool        fflagdefault:1; /* true if (default) pref    */
> +    bool        fflagsticky:1;  /* true if (sticky) pref     */
> +    bool        fflaglive:1;    /* true if (live) pref       */
> +    bool        fflagrestart:1; /* true if (live) pref       */

nit: comment should say true if (restart) pref?

        Attachment #8739574 -
        Flags: review?(mchang) → review+

    
    

    
  
(In reply to peter chang[:pchang][:peter] from comment #23)
> Comment on attachment 8739575 [details] [diff] [review]
> Part 3. Show preference live/restart type in about:config.
> 
> Review of attachment 8739575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: toolkit/locales/en-US/chrome/global/config.properties
> @@ +19,5 @@
> >  modify_title=Enter %S value
> > +modify_title_restart=Enter %S value (restart required)
> > +
> > +modify_type_restart=requires restart
> > +modify_type_live=
> 
> Suggest we can define the string for live type, then we can confirm this
> preference is live not undeclared type. So we can know which preference is
> still undeclared and update it if needs.

Exactly!  Something like that is a comment in part 4 - UX wants "live" to say nothing, so once this settles, the "unknown" can be set to something like "*** fix this ... ***" when people are working on defining the type.  And eventually in nightly.

    
    

    
  
(In reply to David Anderson [:dvander] from comment #22)
> ...
> Looks good, but why nightly only?

Letting UX comment on it more before going further.

    
    

    
  
Comment on attachment 8740603 [details] [diff] [review]
Part 2. Handle pref I/O (mostly I) and get rid of sticky_pref. Carry r=mchang

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

Mark, would you mind also reviewing this, since it changes how sticky preferences are stored in the file, and you landed bug 1098343?

        Attachment #8740603 -
        Flags: feedback?(markh)

    
    

    
  
I'd be surprised if Benjamin doesn't have an opinion on this.

    
    

    
  
Comment on attachment 8740603 [details] [diff] [review]
Part 2. Handle pref I/O (mostly I) and get rid of sticky_pref. Carry r=mchang

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

Unless I'm missing something, it doesn't make sense to me that sticky be treated the same as these new flags - a sticky pref could still be either "live" or "restart" (but probably not unknown once the default prefs files have annotations for every pref.)

::: modules/libpref/prefapi.cpp
@@ +325,5 @@
>          auto pref = static_cast<PrefHashEntry*>(iter.Get());
>  
>          nsAutoCString prefValue;
>          nsAutoCString prefPrefix;
> +        nsAutoCString optFlags;

I don't see how optFlags is set? But that's good - I'd be concerned if pref saving writes these flags, as we'd have problems bouncing around channels. So why this change?

::: modules/libpref/prefread.cpp
@@ +50,4 @@
>  static const char kTrue[] = "true";
>  static const char kFalse[] = "false";
>  
> +static const char kFlagSticky = 'S';

Without considering the performance or storage implications, it seems using strings (eg, "sticky", "live", "restart") would be friendlier and avoid needing explicit comments each time they are used (ie, I noticed you felt it necessary to document that "S" means "sticky", which is a bit of a red flag)

@@ +165,3 @@
>   * pref-name     = quoted-string
>   * pref-value    = quoted-string | "true" | "false" | integer-value
> + * pref-flags    = (optional) any combination of S L R (sticky, live, restart)

I'd expect "live" and "restart" to be mutually exclusive (ie, that a single pref couldn't have both flags)? And looking at the parsing code below, it seems *all* are mutually exclusive - it seems a sticky pref should also be annotated as live or restart - the fact a pref is sticky says nothing about these semantics.

        Attachment #8740603 -
        Flags: feedback?(markh) → feedback-

    
    

    
  
(In reply to Mark Hammond [:markh] from comment #32)
> I'd be surprised if Benjamin doesn't have an opinion on this.

Oh, he knows what I'm up to, I'm just leaving him as a second wave :)

    
    

    
  
(In reply to Mark Hammond [:markh] from comment #33)
> Comment on attachment 8740603 [details] [diff] [review]
> Part 2. Handle pref I/O (mostly I) and get rid of sticky_pref. Carry r=mchang
> 
> Review of attachment 8740603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unless I'm missing something, it doesn't make sense to me that sticky be
> treated the same as these new flags - a sticky pref could still be either
> "live" or "restart" (but probably not unknown once the default prefs files
> have annotations for every pref.)

Right.  You're right, it happens that live/restart/unknown are describing one particular property, and sticky is describing something completely orthogonal.  I could have instead gone with the syntax that adds an extra parameter for live/restart/unknown (the last one probably just no value at all), and left sticky the way it was.

It seemed better to roll everything into the "extra flags" concept, but perhaps that's a secondary conversation to be had, in a separate bug.

> 
> ::: modules/libpref/prefapi.cpp
> @@ +325,5 @@
> >          auto pref = static_cast<PrefHashEntry*>(iter.Get());
> >  
> >          nsAutoCString prefValue;
> >          nsAutoCString prefPrefix;
> > +        nsAutoCString optFlags;
> 
> I don't see how optFlags is set? But that's good - I'd be concerned if pref
> saving writes these flags, as we'd have problems bouncing around channels.
> So why this change?

Left over code, I'll remove it.  It's exactly what you were saying.  I was thinking we needed to save these flags, put the code in, realized we don't actually need it, but also that if we did, we'd have trouble with different versions, as you mention.

> 
> ::: modules/libpref/prefread.cpp
> @@ +50,4 @@
> >  static const char kTrue[] = "true";
> >  static const char kFalse[] = "false";
> >  
> > +static const char kFlagSticky = 'S';
> 
> Without considering the performance or storage implications, it seems using
> strings (eg, "sticky", "live", "restart") would be friendlier and avoid
> needing explicit comments each time they are used (ie, I noticed you felt it
> necessary to document that "S" means "sticky", which is a bit of a red flag)
> 
> @@ +165,3 @@
> >   * pref-name     = quoted-string
> >   * pref-value    = quoted-string | "true" | "false" | integer-value
> > + * pref-flags    = (optional) any combination of S L R (sticky, live, restart)

This really should have been written as S L|R, to show that we want either live or restart, though I'm not sure that I'd go beyond a warning if both were specified.  So, it shouldn't have both, but I can't stop people from putting both down - it's just a question of whether that's a warning or "stop parsing the file" type of error.

> 
> I'd expect "live" and "restart" to be mutually exclusive (ie, that a single
> pref couldn't have both flags)? And looking at the parsing code below, it
> seems *all* are mutually exclusive - it seems a sticky pref should also be
> annotated as live or restart - the fact a pref is sticky says nothing about
> these semantics.

Correct - sticky is orthogonal.  It could actually be left as sticky_pref.

All of the ugliness comes from not complicating the parser, but it makes it now obvious that it isn't a JS anymore.  That's probably the biggest question.

    
    

    
  
Benjamin, a quick question for you before going further, to get the syntax out of the way.  There are two (orthogonal) decisions to make for now:

1. Which of these two; the first one (a) is easier to read, larger (is this important?), and somewhat more complicated parsing code.  The second one (b) is smaller, simpler parser, harder to read.

a) pref("some.pref", "some.value", "live");
   pref("other.pref", "other.value", "live", "sticky");

b) pref("some.pref", "some.value", L);
   pref("other.pref", "other.value", LS);

2. Should we make sticky a flag, or leave it as sticky_pref "command"?  Does the decision depend on the answer to question 1? (e.g., sticky as a flag if doing 1a, but sticky_pref if doing 1b.)

If size is not important, I'd probably go with 1a and sticky as a flag, even though that complicates the code, but I don't know if size is important.  Of course, we may eventually get rid of all of these if the type of preference is specified somewhere else (like the graphics preferences are in gfxPref) and this may all be irrelevant :)
Flags: needinfo?(benjamin)

    
    

    
  
(In reply to Milan Sreckovic [:milan] from comment #35)
> All of the ugliness comes from not complicating the parser, but it makes it
> now obvious that it isn't a JS anymore.  That's probably the biggest
> question.

I don't see why this is any less JS than it was before - eg, |user_pref("foo", "bar")| is JS if we assume |user_pref| exists. |user_pref("foo", "bar", live)| is also JS if we also assume |live| exists - ditto for |LS|, although it's more subtle as all possible combinations of the flags would need to be taken into account.

FWIW, my tastes still prefer keeping sticky_pref(), and using |live| or |restart| as valid flags (identifiers, not strings), and that the flags can't be combined - that would allow cowboys to continue treating it as JS just by adding a couple of additional identifiers to the environment.

    
    

    
  
I prefer A) as long as it doesn't make parsing take more time. We need to decide what the behavior will be when we encounter an unknown flag.

But yeah, we should also just get rid of all this since it really sucks.
Flags: needinfo?(benjamin)

    
    

    
  


  
Addressing comment 33 and comment 38.  Left the sticky_pref alone, just have "live" or "restart".

        Attachment #8740603 -
        Attachment is obsolete: true

        Attachment #8747111 -
        Flags: review?(benjamin)

    
    

    
  


  
Mostly rebased from previous review.

        Attachment #8740605 -
        Attachment is obsolete: true

        Attachment #8747112 -
        Flags: review?(benjamin)

    
    

    
  
Updated - had the enums, but wasn't using them everywhere.

    
    

    
  
I will get to this Friday.

    
    

    
  
Comment on attachment 8747111 [details] [diff] [review]
Part 2. Handle file I/O (mostly I) for preference flags live/restart and set a few graphics preferences to the correct state. r=bsmedberg

FWIW, now that we've gone through some of the related pref changes, I don't think we should do it like this. If we're going to flag live/restart prefs, we should do it with manifests.

        Attachment #8747111 -
        Flags: review?(benjamin)

    
  

        Attachment #8747226 -
        Flags: review?(benjamin)

    
  

        Attachment #8747227 -
        Flags: review?(benjamin)

    
  

        Attachment #8747228 -
        Flags: review?(benjamin)
Assignee: milaninbugzilla → nobody

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: