need way to register pref callback from JS

VERIFIED FIXED in mozilla1.0

Status

()

P3
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: shaver, Assigned: shaver)

Tracking

({arch, testcase})

Trunk
mozilla1.0
x86
All
arch, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-])

Attachments

(2 attachments)

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.

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M15

Comment 1

19 years ago
Moving all libPref component bugs to new Preferences: Backend component.  
libPref component will be deleted.
Component: libPref → Preferences: Backend

Updated

19 years ago
Target Milestone: M15 → M16

Comment 2

19 years ago
Reassigning to Alec as per discussion
Assignee: neeti → alecf
Status: ASSIGNED → NEW

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M16 → M15

Updated

19 years ago
Depends on: 21135

Comment 3

19 years ago
moving to m16, dependant on the libpref tracking bug
Target Milestone: M15 → M16

Comment 4

19 years ago
moving non-critical bugs to M19
Target Milestone: M16 → M19

Comment 5

19 years ago
nobody needs this rightnow, futuring
Target Milestone: M19 → Future
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

18 years ago
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
Keywords: nsbeta3

Comment 8

18 years ago
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
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

18 years ago
Mike, I think you should take ownership of this bug.  We would minus it otherwise.
Assignee: alecf → shaver
Status: ASSIGNED → NEW
Sure, I guess.
Status: NEW → ASSIGNED
Keywords: arch

Comment 12

18 years ago
Marking nsbeta3- per pdt review.
Whiteboard: [nsbeta3-]
I would really like this to be able configure some stuff in the navigator 
chrome. I.e. I want that 2) feature.
adding alecf back to the cc list.

bugscape bug #2817 also depends on this.
OS: Linux → All
Created attachment 17128 [details] [diff] [review]
Add nsIPref.{add,remove}Observer, and misc cleanup
Created attachment 17129 [details]
Test for pref observers (JS)
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.)
Keywords: patch
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.
Mozilla 1.0 for sure: I want this on the trunk RSN.

I crave review!

Comment 20

18 years ago
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?
Keywords: mozilla0.9, mozilla1.0, review, testcase
Target Milestone: Future → mozilla1.0
alecf would be the right person to review it.

Comment 22

18 years ago
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.
In the tree, though I didn't rip out the localized pref stuff.

Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
rubberstamp vrfy [checked nsIPref.idl in lxr, and the changes are there].
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.