[FUEL] consider changing the PreferenceBranch.events API

RESOLVED WONTFIX

Status

()

Firefox
General
RESOLVED WONTFIX
11 years ago
9 years ago

People

(Reporter: Nickolay_Ponomarev, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

11 years ago
Given that Firefox code will be converted to fuel in the near term, I think we should get APIs for it right.

Among others, I have a few issues with the PreferenceBranch.events API. I think the older nsIPrefBranch2 interface was a better solution for the problem (letting consumers listen for pref changes) for the following reasons:

1. nsIPB2 didn't expose a generic way to add event listeners. I.e. once you got an nsIPrefBranch2, you could do this:
 prefBranch2.addObserver("subpref", listener, false);

whereas in FUEL you use:
 prefBranch.events.addListener("change", listener);

The issue is the need to specify the "change" topic here. Are any other topics possible/planned with a preference branch? If not, why can't we have the addListener directly on the fuelIPreferenceBranch (with the listener topic implied)?

I suppose the idea was to have a single way to register a callback throughout the API, but I don't think it's better for the users of the API.

1a. Another issue is the "events." part. What is the reason you didn't mimic the DOM EventTarget interface here? Something like:
 interface fuelIPreferenceBranch : fuelIEventTarget {
   ...
 }

2. The Events object wastes cycles checking that the listener in question isn't already registered. Why is this needed? I think this is modeled after addEventListener DOM API, but as noted in bug 343416 comment 14 it's a quirky API (see that comment for details).

Please consider changing this. FWIW, I think prefBrach(.events).addListener could just forward to nsIPB2.addObserver without any loss for usability.

I realize you might not have time for this, so in the case anything of this is agreed on, I can create the patch.

Comment 1

11 years ago
(In reply to comment #0)
> The issue is the need to specify the "change" topic here. Are any other topics
> possible/planned with a preference branch?

Yes, we're planning on adding 'reset' and 'add' events. This is definitely a case where this flexibility comes at an advantage.

> 1a. Another issue is the "events." part. What is the reason you didn't mimic
> the DOM EventTarget interface here? Something like:
>  interface fuelIPreferenceBranch : fuelIEventTarget {
>    ...
>  }

We chose not to do this to avoid cluttering up the namespaces for each object (Application, Preference, etc.). When planning this out, we considering being able to do prefs.addEvent or prefs.addEventListener but instead opted to keep the API simple (letting there only be one way to do things, avoiding confusion).

This would be especially confusing if Preference was the only object to have this extra set of functions.

Additionally, watching for preference events is a really minor aspect of this whole conversion. According to Attachment 201996 [details] [diff], this would only occur about 5 times, total; meaning that this shouldn't really be a big concern.

> 2. The Events object wastes cycles checking that the listener in question isn't
> already registered. Why is this needed? I think this is modeled after
> addEventListener DOM API, but as noted in bug 343416 comment 14 it's a quirky
> API (see that comment for details).

I guess I don't see how this is an issue. A unique function should only be bound to a listener once. A single loop to verify this seems appropriate; and since we can't use addObserver to manage this (see below), it's an acceptable cost.

> Please consider changing this. FWIW, I think prefBrach(.events).addListener
> could just forward to nsIPB2.addObserver without any loss for usability.

Unfortunately, the loss would be in the Event object that's being passed back in to the function itself. Currently we standardize how that object looks and the information that's being passed back to it (like the name of the preference, for example). Defaulting to addObserver would lose us that functionality.
(Reporter)

Comment 2

11 years ago
Thanks for your reply.

> (In reply to comment #0)
> > The issue is the need to specify the "change" topic here. Are any other topics
> > possible/planned with a preference branch?
> 
> Yes, we're planning on adding 'reset' and 'add' events. This is definitely a
> case where this flexibility comes at an advantage.
> 
Did you think of any use cases for these additional events? In my experience writing extensions, reading and tweaking the core code I never had a need to find out when a preference was "added" (effectively changed from its default value) or "reset"  (effectively changed back to the default value).

Prefs is a very simple thing conceptually, IMO, and should have a simple interface.

> > 1a. Another issue is the "events." part. What is the reason you didn't mimic
> > the DOM EventTarget interface here? Something like:
> >  interface fuelIPreferenceBranch : fuelIEventTarget {
> >    ...
> >  }
> 
> We chose not to do this to avoid cluttering up the namespaces for each object
> (Application, Preference, etc.). When planning this out, we considering being
> able to do prefs.addEvent or prefs.addEventListener but instead opted to keep
> the API simple

There's always a single Events object per Application/Preferences/etc, so it makes sense to combine the Events interface with the main interface - you save users some typing and save on the implementation complexity as well.

So what is the reason for keeping it separate? You mentioned a single one - to avoid cluttering up Application, Preference, and other interfaces. But there are two members on Events, so you don't save much "name space" by replacing those with a single "events" member in Application and other interfaces.

> (letting there only be one way to do things, avoiding
> confusion).
> 
Er, the alternative I suggested was to _replace_ the .events member with the add/remove methods (making Application/Preferences et al implement fuelIEvents, perhaps renamed to something like EventTarget). There's still one, easier, way to do things.

> This would be especially confusing if Preference was the only object to have
> this extra set of functions.
> 
I guess I confused matters a little bit here. There were several suggestions in the comment 0 - one was prefs-specific and two were about the Events api in general:
1. Don't use fuel events for preferences, as that provides unneeded flexibility (making people specify an event to listen for, when there's only one event).
1a. Failing that, keep using fuel events for prefs, but change the way event listeners are registered - fuel-wide.
2. An unrelated notice about the fuelIEvents API wrt duplicate listeners.

> Additionally, watching for preference events is a really minor aspect of this
> whole conversion. According to Attachment 201996 [details] [diff], this would only occur about 5
> times, total; meaning that this shouldn't really be a big concern.
> 
My numbers were as follows, but I think it's important to get the Event APIs in general right, since they would be more widely used than just pref observers alone.
Nickolay@ /s/mozilla/toolkit
$ egrep -ri "pref.*addobser" * | egrep "js|xml" | wc -l
      8

Nickolay@ /s/mozilla/browser
$ egrep -ri "pref.*addobser" * | egrep "js|xml" | wc -l
     46

> > 2. The Events object wastes cycles checking that the listener in question isn't
> > already registered. Why is this needed? I think this is modeled after
> > addEventListener DOM API, but as noted in bug 343416 comment 14 it's a quirky
> > API (see that comment for details).
> 
> I guess I don't see how this is an issue. A unique function should only be
> bound to a listener once.

Should be bound _by the user of FUEL_ once. If some code binds the same function twice (knowing that FUEL ignores the second listener), that code has a logic error.

That's because normally people wouldn't write code that sets up two identical listeners in a coordinated way. The duplicate listeners happen because there are separate pieces of code set up the same listener twice. The problem here is that they are also required to remove the listener they added, but don't have a way to unregister just their listener. They will also likely do it in an uncoordinated way, because otherwise they could just set up a single listener in a coordinated way.

So if the two removeListener calls for the same listener do not happen simultaneously, there's a problem, that is quite hard to figure out.

What is the utility of ignoring identical listeners?

> > Please consider changing this. FWIW, I think prefBrach(.events).addListener
> > could just forward to nsIPB2.addObserver without any loss for usability.
> 
> Unfortunately, the loss would be in the Event object that's being passed back
> in to the function itself. Currently we standardize how that object looks and
> the information that's being passed back to it (like the name of the
> preference, for example). Defaulting to addObserver would lose us that
> functionality.
> 
It wouldn't actually. You're given the name of the preference in the callback, as well as the branch the callback was registered with (unlike the fuelIPreferenceBranch case).

But I can see how you might want to consistently use your own interfaces for callbacks and such.


To sum up:
1. I see you're opposed to using a custom methods to register pref observers, in particular to forwarding those calls to nsIPB2. This is very unlikely to change and I'm not planning to argue about this more.
2. There are some changes to the Events interface I'd still like to see implemented (1a and 2 in comment 0). I think I addressed your concerns about those changes. I can file different bugs on these changes if we agree on any of them.
(Reporter)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.