Callback instead of a global gDirty for preferences

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

Right now, we have a global gDirty in preferences; bug 789945 needs this as a callback instead, but even without that requirement, it would be a good change.
(In reply to Milan Sreckovic [:milan] from comment #1)
> Created attachment 8771566 [details]
> Bug 1287215: Replace preferences gDirty global with a callback. +bugs
> 
> Review commit: https://reviewboard.mozilla.org/r/64678/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/64678/

This is really Gijs' patch, which I reviewed, extracted this part, modified a bit, then back to Gijs for review.
Attachment #8771566 - Flags: review?(gijskruitbosch+bugs)
Version: 38 Branch → Trunk
Attachment #8771566 - Flags: review?(aklotz)
Comment on attachment 8771566 [details]
Bug 1287215: Replace preferences gDirty global with a callback.

https://reviewboard.mozilla.org/r/64678/#review61756

Thanks for taking this back up again. I keep not really finding time for it, and am probably a little out of my depth on this set of patches anyway, so I'm very grateful someone else is pushing changes here.

In general, this change looks good. The only thing that I'm somewhat concerned about is: is it currently possible for gDirty to be set to true before we call Preferences::Init ? You're nullchecking `gDirtyCallback`, which makes some degree of sense in any case because nullptr crashes are bad, but if there really is a case where it would be null and we would otherwise have set gDirty to true, this patch changes behaviour.

Otherwise, I think it would be wise for someone with a bit more gecko/c++/core background to have a peek at this before we land, so I flagged Aaron. I'm afraid not many people know this code anyway, besides maybe bsmedberg (but even then, it doesn't materially change very often...).
Attachment #8771566 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee: nobody → milan
Status: NEW → ASSIGNED
Comment on attachment 8771566 [details]
Bug 1287215: Replace preferences gDirty global with a callback.

Good question about dirty getting set before Preferences is initialized.  Let me check.
Attachment #8771566 - Flags: review?(aklotz)
Some more reasoning behind this change. Right now, we set the global "dirty" flag inside of prefapi utilities file. Preferences class is the only place where we use it - to decide if we need to save the preferences to the file or not.  Once the preferences are saved to a file, Preferences resets the global dirty flag.  Moving this dirty flag into Preferences class instead cleans things up somewhat.
Comment on attachment 8771566 [details]
Bug 1287215: Replace preferences gDirty global with a callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64678/diff/1-2/
Attachment #8771566 - Attachment description: Bug 1287215: Replace preferences gDirty global with a callback. +bugs → Bug 1287215: Replace preferences gDirty global with a callback.
Attachment #8771566 - Flags: review+ → review?(aklotz)
Attachment #8771566 - Flags: review?(aklotz) → review+
Comment on attachment 8771566 [details]
Bug 1287215: Replace preferences gDirty global with a callback.

https://reviewboard.mozilla.org/r/64678/#review62628

OK, just a few style nits.

::: modules/libpref/Preferences.h:399
(Diff revision 2)
>  
>    static Preferences*      sPreferences;
>    static nsIPrefBranch*    sRootBranch;
>    static nsIPrefBranch*    sDefaultRootBranch;
>    static bool              sShutdown;
> -
> + 

Nit: I think there's whitespace here.

::: modules/libpref/Preferences.cpp:491
(Diff revision 2)
>   * Constructor/Destructor
>   */
>  
>  Preferences::Preferences()
> -{
> +  : mDirty(false)
> +{ 

Nit: trailing whitespace (at least I think that's what reviewboard is trying to tell me)

::: modules/libpref/Preferences.cpp:542
(Diff revision 2)
>  {
>    nsresult rv;
>  
> +  PREF_SetDirtyCallback(&DirtyCallback);
>    PREF_Init();
> -
> +  

Nit: whitespace

::: modules/libpref/Preferences.cpp:929
(Diff revision 2)
>  Preferences::SavePrefFileInternal(nsIFile *aFile)
>  {
>    if (nullptr == aFile) {
> -    // the gDirty flag tells us if we should write to mCurrentFile
> +    // the mDirty flag tells us if we should write to mCurrentFile
>      // we only check this flag when the caller wants to write to the default
> -    if (!gDirty)
> +    if (!mDirty)

Since we're already touching it, let's add curlies around this if block

::: modules/libpref/prefapi.cpp:117
(Diff revision 2)
>          memcpy(mem, str, len+1);
>      return static_cast<char*>(mem);
>  }
>  
> +static PrefsDirtyFunc gDirtyCallback = nullptr;
> +inline void MakeDirtyCallback()

Nit: Blank line separating this function from the above declaration of gDirtyCallback.

::: modules/libpref/prefapi.cpp:128
(Diff revision 2)
> +    MOZ_ASSERT(gDirtyCallback);
> +    if (gDirtyCallback) {
> +        gDirtyCallback();
> +    }
> +}
> +void PREF_SetDirtyCallback(PrefsDirtyFunc aFunc)

Nit: Blank line separating this function from the one above it.
Comment on attachment 8771566 [details]
Bug 1287215: Replace preferences gDirty global with a callback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64678/diff/2-3/
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c3eda5a65a4
Replace preferences gDirty global with a callback. r=aklotz,Gijs
https://hg.mozilla.org/mozilla-central/rev/5c3eda5a65a4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.