Closed Bug 1413811 Opened 3 years ago Closed 3 years ago

Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback()

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files)

This will simplify things a bit.
Comment on attachment 8924436 [details]
Bug 1413811 (part 1) - Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback(). .

https://reviewboard.mozilla.org/r/195724/#review201214

::: modules/libpref/Preferences.cpp:5072
(Diff revision 1)
> -  nsresult rv = AddStrongObserver(observer, aPref);
> -  NS_ENSURE_SUCCESS(rv, rv);
> -
> +  PREF_RegisterCallback(aPref,
> +                        NotifyObserver,
> +                        static_cast<nsIObserver*>(observer),
> +                        /* isPriority */ false);

That looks very much like RegisterPriorityCallback now, you may want to refactor that.
Attachment #8924436 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8924437 [details]
Bug 1413811 (part 2) - Factor out similarities between RegisterPriorityCallback() and Preferences::RegisterCallback(). .

https://reviewboard.mozilla.org/r/195726/#review201216

::: modules/libpref/Preferences.cpp:5029
(Diff revision 1)
>                      NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
>                      NS_ConvertASCIItoUTF16(aPref).get());
>  }
>  
>  static void
> -RegisterPriorityCallback(PrefChangedFunc aCallback,
> +RegisterCallbackHelper(PrefChangedFunc aCallback,

Heh, you just did.

::: modules/libpref/Preferences.cpp:5055
(Diff revision 1)
> +static void
> +RegisterPriorityCallback(PrefChangedFunc aCallback,
> +                         const char* aPref,
> +                         void* aClosure)
> +{
> +  MOZ_ASSERT(Preferences::IsServiceAvailable());

It seems to me this assert, as well as the one in RegisterCallback, are just as valid for both cases and should just be in the helper.
Attachment #8924437 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8924438 [details]
Bug 1413811 (part 3) - Rename RegisterPriorityCallback(). .

https://reviewboard.mozilla.org/r/195728/#review201218
Attachment #8924438 - Flags: review?(mh+mozilla) → review+
> > +RegisterPriorityCallback(PrefChangedFunc aCallback,
> > +                         const char* aPref,
> > +                         void* aClosure)
> > +{
> > +  MOZ_ASSERT(Preferences::IsServiceAvailable());
> 
> It seems to me this assert, as well as the one in RegisterCallback, are just
> as valid for both cases and should just be in the helper.

RegisterPriorityCallback() is a local function with five uses, and the calling context ensures that prefs are initialized and that the callback is non-null. But Preferences::RegisterCallback() is externally visible and could be called before prefs have been enabled and with a null callback. So I will leave it as is.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ade96f752be032b1de492c2520c68d242d04ec
Bug 1413811 (part 1) - Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/044253938e1c21d20026e88be2ff9a4f4f3beb27
Bug 1413811 (part 2) - Factor out similarities between RegisterPriorityCallback() and Preferences::RegisterCallback(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4a51bb3b210376c58239968e37d930d76f51fc
Bug 1413811 (part 3) - Rename RegisterPriorityCallback(). r=glandium.
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.