Content Preferences - Architecture/API

The content preferences code currently has a four piece architecture:
  1. a service that manages the database of prefs;
  2. a controller for each window that watches the browser's location and notifies pref handlers when it changes;
  3. pref handlers that tweak the browser according to the user's prefs;
  4. a sidebar housing controls for twiddling prefs.
In the first version of the extension, I designed these to support preferences like the text zoom pref.  In v2 I redesigned them to improve that support and tried to generalize the API to work for other prefs.

But a single caller is a very limited perspective for designing an API (indeed, I've already found issues while getting a second pref up and running on it), so I'm keen to get feedback on how to make this work well for other prefs (in both core code and extensions).

Here's how it works at the moment:

The Service (mozISitePrefService)

The service manages the database of prefs.  Its API is pretty simple.  It has four core methods, getPref, setPref, hasPref, and removePref:

mozISitePref getPref(in AUTF8String site, in AUTF8String key);
void setPref(in AUTF8String site, in AUTF8String key, in AUTF8String value);
boolean hasPref(in AUTF8String site, in AUTF8String key);
void removePref(in AUTF8String site, in AUTF8String key);

It also has a method, getPrefs, that gets all prefs for a site, and it has another method, getSiteForURI, that extracts a site identifier from a URI:

void getPrefs(in AUTF8String site, 
out unsigned long prefCount,
[retval, array, size_is(prefCount)] out mozISitePref prefs);

AUTF8String getSiteForURI(in nsIURI uri);

Site identifiers are full domains by default (f.e. www.mozilla.org), but you can configure the extension to use base domains (f.e. mozilla.org), and perhaps we'll cook up other options in the future.

The interface to a preference (mozISitePref) is very basic:

readonly attribute AUTF8String site;
readonly attribute AUTF8String key;
readonly attribute AUTF8String value;

The Controller (SitePref)

The controller's job is to notify pref handlers when the browser location changes so those handlers can apply the user's preferences for the new location.  It has addObserver and removeObserver methods that handlers can use to register/unregister themselves for notifications:

addObserver(in AUTF8String key, in nsISupports observer)
removeObserver(in AUTF8String key, in nsISupports observer)

The methods take a pref key so the controller know what setting the handler cares about.  When the location changes, the controller grabs the set of prefs for the new location and calls each observer's onLocationChange method with the pref it cares about (if any):

onLocationChange(in nsISitePref pref) 

The Pref Handlers

The pref handlers implement the aforementioned onLocationChange method and then do whatever they need to do to apply user preferences to the browser.

The Sidebar (chrome://cpref/content/sidebar.xul)

The sidebar provides a central location for site-specific preference controls.  It's an experiment in exposing these preferences in a place that is location-specific (unlike the Preferences dialog) and visible alongside the page being affected (unlike the View menu).

It doesn't do much besides provide a place for pref controls to overlay themselves onto.  Controls are responsible for updating themselves when the location changes.  But the sidebar does define a gMainWindow shortcut for getting the browser window that the sidebar is embedded in.


I'm interested in feedback on all aspects of this API.  Here are the questions I know I should ask:
  1. Should pref values be typed?

    We can't store them typed in the database given the EAV model we're using to store them (although we could change the model if it mattered enough), but we could convert them on their way in and out of the database and have type-specific methods for getting and setting them (à la nsIPrefBranch).

  2. Should pref handlers listen for location changes themselves and get their prefs directly from the service instead of having that work be mediated by the controller?

    It's faster for the controller to query once for all prefs, and it's simpler on the pref handlers not to have to implement nsIWebProgressListener themselves, but it makes the architecture more complex, since it adds an extra layer between the service and the handlers.

  3. Is AUTF8String the right string type for site identifiers and keys?
And then there are all those questions I don't know I should be asking (along with their answers), so clue me in!


Daniel Glazman said...

Myk, to be honest, I don't like addObserver/removeObserver not because it's bad but because of the leaks we'll have with sites forgetting to remove the observers. And I'm unfortunately sure this is not a silly hypothesis :-(

Myk said...

Hey Daniel, I'm not sure what you meant by "sites" forgetting to remove the observers, as it's actually extensions (and core code) which register themselves as observers, not web sites (although having a set of preferences that sites can access, like your preferred number of items per page, would be useful).

Also, virtually all handlers will want to observe preferences for the life of each browser window, so it's only important for them to remove themselves when the window closes, and then presumably only if they hold a reference to the controller (which wouldn't be very useful since the controller is already available in the global scope).

But to be certain we don't leak memory, perhaps the controller could automatically remove all observers when the browser window closes.

john p baker said...

As you know, I have been thinking aloud, publishing various random thoughts on the newsgroup - My colleagues hate this, they ask me a simple question and I throw out five or six methods and they have to wait until the following day for a recommended course of action.
Anyway it has given me an idea of how it fits together as I constructed a use case.

Anyway now for the consolidated feedback...

I now like the controller - I think it takes the complexity and repetition of handling the prefs and defaults away from the consumers.

I would definitely want typed fields - especially after a string "000169" got converted to "169" in the database ("00016a" is OK), however I now use "#000169".

A consistent way is needed to indicate that a preference should be deleted from the database.

I have changed HashColouredTabs+ to use a 3-slider palette selector and the current development version (0.4.10a) can be downloaded from my home page.


john.p.baker said...

On further thought, it is going to be nearly impossible to cover all the typed data that might be wanted so perhaps consumers just have to code the strings up themselves using something like JSON; In my problem example I could have used '"000169"'.


Myk said...

I would definitely want typed fields - especially after a string "000169" got converted to "169" in the database ("00016a" is OK), however I now use "#000169".

Turns out that conversion is because of a bug in mozISitePrefService. I defined the prefs.value column as type STRING, which SQLite doesn't recognize, and when you define a column as a type SQLite doesn't recognize it creates the column as type NUMERIC, which means it tries to convert string values inserted into the column into numbers.

I think the best solution will be to update the column to be type NONE in the next version of the extension, which will result in values getting stored in the database without any modification whatsoever.

And to better support multiple datatypes, I'll change pref values in the mozISitePref* interfaces to be nsIVariants instead of AUTF8Strings.

john p baker said...

Left over from the newsgroup...
When I said use proper observer I was more thinking of keeping your dispatcher but, instead of it callingonLocationChange(pref), using observe(pref, 'sitePref:locationChange', null) for instance.

But I am not sure that is any real advantage over your current mechanism!


Anonymous said...

Hey Myk --

Any plans to update Content Preferences to work with Firefox 3?

I'd love to have it.


Myk said...

Jordan: yes, I took a look at it last weekend, but I wasn't able to finish updating it then. I'll take another look next weekend.

Karri said...

Hi Myk - any chance you'll be updating Content Preferences for firefox 3.0? Its the only reason I'm not upgrading firefox on my personal machine to 3.0 ;)

- Karri

Myk said...

Karri: I am planning to update the extension. I won't have time to work on it this weekend, unfortunately, but I'm going to work on it next weekend.

Karri said...

Hi Myk - great to hear that its in the pipeline. I'll be waiting like a kid for christmas :)

- Karri