24612 - need way to register pref callback from JS

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

Lots of JS code is using preferences now, and some of them are going to need a
way to register for change notifications.

It's probably really easy if the scriptable interface doesn't permit a closure
for the callback, and a little harder if it does.

Comment 1

[leger]

Moving all libPref component bugs to new Preferences: Backend component.  
libPref component will be deleted.

Comment 2

[neeti]

Reassigning to Alec as per discussion

Comment 3

[Alec Flett]

moving to m16, dependant on the libpref tracking bug

Comment 4

[Alec Flett]

moving non-critical bugs to M19

Comment 5

[Alec Flett]

nobody needs this rightnow, futuring

Comment 6

[Mike Shaver (:shaver emeritus)]

Anyone writing a significant component or feature in JS needs this; I'm amazed
that rginda hasn't been bugging you for it yet for chatzilla.

(People on #mozilla have been asking for it of late.)

Comment 7

[Rann Bar-On]

agreed, this would apply to any company adding anything to the prefs dialog
(which is important, since we're pushing customisation).  Also, I encountered
this problem while writing Dino stuff: http://sites.netscape.net/dinoproject/

Dino, btw, is a project aimed at providing UI modifications to mozilla that
users have been asking for and devs futuring or ignoring.

Nominating nsbeta3

Comment 8

[Alec Flett]

people are only blocked by this if they are doing any of:
1) caching prefs in JS (i.e. retaining the value of GetIntPref/etc in some 
global place)
2) relying on the value of a pref triggering an action

So I claim that there aren't actually that many cases where this is needed...

Furthermore, for 1) GetXXXPref is really not expensive, so unless you're calling 
this in a loop where the value can't be cached then you should just not cache

There's really no workaround for 2) :(

I'd like to fix this, but I have other pref stuff that I'd like to fix before 
this goes in.

As for the closure, one is required based on the C API - I'd have to require it 
to be an nsISupports, since void* isn't scriptable

Comment 9

[Mike Shaver (:shaver emeritus)]

Dude, let's just get ugly and staple a bag on the side.

[scriptable,uuid(0xdeadbeef)]
interface nsIJSPrefObserver
{
  void onChange(in string pref);
};

[scriptable,uuid(0xfee1dead)]
interface nsIJSPrefObserverService
{
   void addObserver(in string pref, in nsIJSPrefObserver observer);
   void removeObserver(in string pref, in nsIJSPrefObserver observer);
};

The implementation of addObserver just slams the observer in as the
pref-notification closure, and adds it to an nsSupportsHashtable. 
removeObserver is obvious.

Whaddya think?  I can try to hack it up today, but I might be short on time.

(FWIW, people are asking about ``2'' in #mozilla.)

Comment 10

[selmer (gone)]

Mike, I think you should take ownership of this bug.  We would minus it otherwise.

Comment 11

[Mike Shaver (:shaver emeritus)]

Sure, I guess.

Comment 12

[Michael La Guardia]

Marking nsbeta3- per pdt review.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would really like this to be able configure some stuff in the navigator 
chrome. I.e. I want that 2) feature.

Comment 14

[(not reading, please use [e-mail] instead)]

adding alecf back to the cc list.

bugscape bug #2817 also depends on this.

Comment 15

[Mike Shaver (:shaver emeritus)]

Attachment: Add nsIPref.{add,remove}Observer, and misc cleanup

Comment 16

[Mike Shaver (:shaver emeritus)]

Attachment: Test for pref observers (JS)

Comment 17

[Mike Shaver (:shaver emeritus)]

This seems to work.

(I also DEBUG_silenced some printf noise, and fixed indentation/line-wrapping in
nsIPref.idl.  It's The Right Thing.)

Are we ambitious enough to want this for RTM?  (I'm not privy to bugscape #2817.)

Comment 18

[(not reading, please use [e-mail] instead)]

this is no something we need for rtm.  (that bugscape bug is a minor ui bug that
having pref callbacks from js would fix.)

how about for mozilla 1.0?

I bet the folks who write XUL / JS for a living will find good uses for this.

Comment 19

[Mike Shaver (:shaver emeritus)]

Mozilla 1.0 for sure: I want this on the trunk RSN.

I crave review!

Comment 20

[timeless]

Adding Review keyword.
Also adding mozilla0.9 and mozilla1.0, per shaver moving to mozilla1.0 
milestone. As per shaver it would be nice if we got it in sooner (ie mozilla0.9)
Adding testcase keyword.

could we get the module owner to review? who is it?

Comment 21

[(not reading, please use [e-mail] instead)]

alecf would be the right person to review it.

Comment 22

[Alec Flett]

Sorry for the silence, I've been away all weekend....
Everything looks great - I'm glad you hooked into the existing pref callback 
mechanism, and the XPCOM notification mechanism!
r/sr=alecf

The one thing about the #ifdef debug stuff - if you feel like it, you can 
actually remove all the #ifdef DEBUG_tao stuff, and some of the related blocks 
of code (like the GetLocalizedUnicharPref("browser.startup.homepage") stuff as 
well? I can't believe it's in there, and it's just slowing us down.

Comment 23

[Mike Shaver (:shaver emeritus)]

In the tree, though I didn't rip out the localized pref stuff.

Comment 24

[sairuh (rarely reading bugmail)]

rubberstamp vrfy [checked nsIPref.idl in lxr, and the changes are there].