664653 - Preferences::RegisterCallback should use PREF_RegisterCallback instead of owns hash tables

Status: NEW

(Core :: Preferences: Backend, defect)

Description

[Makoto Kato [:m_kato]]

This API was introduced by bug 182954.  At that time, it was implemented in nsContentUtils.

Now, it is moved to Preferenece (libpref). So I think that we can remove ValueObserver and hash tableｓ to replace with calling PREF_* function directly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that last I checked the coalescing that this API does (that is, NOT actually registering a function anew each time an observer is added) was key for good performance.  Will that still be the case with direct PREF_* usage?

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, see bug 561076.

Comment 3

[Makoto Kato [:m_kato]]

PREF_* is used into libpref only.

When observer callback register using Preference::RegisterCallback, That callback info is stored into ValueObserver, nsPrefBranch.mObservers and gCallbacks into prefapi.cpp.  If Preference::RegisterCallback calls PREF_RegisterCallback directly, it is stored into gCallbacks only.

Originally, Preference::RegisterCallback is the replacement of nsIPref::RegisterCallback that is removed by bug 175193 and bug 524449.  This XPCOM method was calling PREF_RegisterCallback only.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> If Preference::RegisterCallback calls PREF_RegisterCallback directly, it is
> stored into gCallbacks only.

That doesn't answer my question.  I understand where things are stored, but note that right now there is ONE entry in gCallbacks for each set of calls to Preference::RegisterCallback with the same callback info.  You're proposing changing that to storing lots of entries in gCallbacks, but gCallbacks manipulation is ridiculously slow because its design is crappy (it does linear searches with string compares on every item, for example).  In fact, as far as I can tell from reading PREF_UnregisterCallback making this change would in fact regress bug 561076.  And these exact issues were clearly described in the bug 561076 comments...

Comment 5

[Boris Zbarsky [:bzbarsky]]

To be clear, we need to either redesign how gCallbacks works before fixing this bug or wontfix this bug.  In my opinion, of course.