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

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

This will simplify things a bit.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1413811 (part 1) - Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback(). .

Pref callbacks registered via Preferences::Add*VarCache() are wrapped in a
ValueObserver -- which groups all callback requests that have the same prefname
and callback function -- and the ValueObserver is put into gObserverTable. This
is reasonable.

Observers registered via nsPrefBranch::Add{Weak,Strong}Observer() are wrapped
in a PrefCallback, and the PrefCallback is put into sRootBranch->mObservers.
This is also reasonable.

Pref callbacks registered via Preferences::RegisterCallback() are conceptually
similar to those registered via Preferences::Add*VarCache(). However, they are
implemented by using *both* of the above mechanisms: they are wrapped in a
ValueObserver which is put into gObserverTable, *and* that ValueObserver is then
wrapped in a PrefCallback which is put into sRootBranch->mObservers.

Using both mechanisms isn't necessary, so this patch removes the
PrefCallback/mObservers part. This makes Preferences::RegisterCallback() work
in much the same way as Preferences::Add*VarCache().

Specifically:

- Preferences::RegisterCallback() now calls PREF_RegisterCallback() instead of
  Preferences::AddStrongObserver(). This makes it more similar to
  RegisterPriorityCallback().

- Preferences::UnregisterCallback() now explicitly calls
  PREF_UnregisterCallback() instead of Preferences::RemoveObserver (which
  previously happened via ~ValueObserver() when the ValueObserver was removed
  from gObserverTable and its refcount dropped to zerod).

Review commit: https://reviewboard.mozilla.org/r/195724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195724/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1413811 (part 2) - Factor out similarities between RegisterPriorityCallback() and Preferences::RegisterCallback(). .

Review commit: https://reviewboard.mozilla.org/r/195726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195726/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1413811 (part 3) - Rename RegisterPriorityCallback(). .

Review commit: https://reviewboard.mozilla.org/r/195728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195728/

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8924438 [details]
Bug 1413811 (part 3) - Rename RegisterPriorityCallback(). .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195728/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8924438 [details]
Bug 1413811 (part 3) - Rename RegisterPriorityCallback(). .

https://reviewboard.mozilla.org/r/195728/#review201218

Comment 8

[Nicholas Nethercote [inactive]]

> > +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.

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b7ade96f752b
https://hg.mozilla.org/mozilla-central/rev/044253938e1c
https://hg.mozilla.org/mozilla-central/rev/3d4a51bb3b21