458299 - nsIContentPrefService should provide an interface for listing stored prefs.

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Justin Dolske [:Dolske]]

I was working on an extension to add some site-specific preferences, and nsIContentPrefService was a natural fit for this. It made setting and checking for a preference nice and easy.

But I also wanted to add some management UI to allow the user to see what preferences they had set, and delete/modify them if they don't want them stored any more. There's no API to do anything like this, so I had to use cps.DBConnection to query the DB directly.

It would be nice to be able to do something like:

var cps = Cc["..."].getService(Ci.nsICPS);
var prefs = cps.getAllSitePrefs("my.pref.name")
// prefs is an array
for (i=0; i<prefs.length; i++)
  dump("Host: " + prefs[i].hostname + " set to " + prefs[i].value)

I suppose a getAllPrefNames() API might be useful to have too.

A secondary issue is that the hostname retrieved from the DB this way is not a proper URL (just "www.host.com"), but all the nsICPS interfaces require a nsIURI to modify or delete the pref. This can be fudged by prepending "http://" to the string and running it though ioService.newURI(), not sure if there's a better way to do it.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #0)
> It would be nice to be able to do something like:
> 
> var cps = Cc["..."].getService(Ci.nsICPS);
> var prefs = cps.getAllSitePrefs("my.pref.name")
> // prefs is an array
> for (i=0; i<prefs.length; i++)
>   dump("Host: " + prefs[i].hostname + " set to " + prefs[i].value)

Yup, we should definitely make this happen.


> I suppose a getAllPrefNames() API might be useful to have too.

Indeed!


> A secondary issue is that the hostname retrieved from the DB this way is not a
> proper URL (just "www.host.com"), but all the nsICPS interfaces require a
> nsIURI to modify or delete the pref. This can be fudged by prepending "http://"
> to the string and running it though ioService.newURI(), not sure if there's a
> better way to do it.

That's a good point.  Perhaps we can make the service's methods polymorphically accept either a URI or a string representing the site.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: patch v1

Here's a patch which adds some useful methods to nsIContentPrefService. Not complete by any means.

Comments?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 385973 [details] [diff] [review]
patch v1

[I'm just back from vacation and looking at this now...]

> Here's a patch which adds some useful methods to nsIContentPrefService. Not
> complete by any means.
> 
> Comments?

I like it!  It implements the principal feature requested in the original description for this bug report (getting the set of preferences for a given setting).

The only big thing missing from this patch are some unit tests for toolkit/components/contentprefs/tests/unit/test_contentPrefs.js that validate its behavior.


>+  void removeNamedPrefs(in AString aName);
>+  nsIPropertyBag2 getNamedPrefs(in AString aName);

Minor concern: I wonder if these names will imply that there are unnamed prefs, just as removeGroupedPrefs implies there are ungrouped prefs (which actually do exist, whereas there aren't any unnamed prefs).  Perhaps something like getPrefsByName and removePrefsByName, although clunkier, would be clearer.


>+  removeNamedPrefs: function ContentPrefService_removeNamedPrefs(aName) {
>+    var settingID = this._selectSettingID(aName);
>+
>+    var s = this._dbConnection.createStatement("SELECT groupID FROM prefs WHERE settingID = " + settingID);
>+    var groups = Array();
>+    while (s.step()) {
>+      groups.push(s.row["groupID"]);
>+    }
>+
>+    this._dbConnection.executeSimpleSQL("DELETE FROM prefs WHERE settingID = " + settingID);
>+    this._dbConnection.executeSimpleSQL("DELETE FROM settings WHERE id = " + settingID);
>+
>+    for (var i = 0; i < groups.length; i++) {
>+/*      for each (var observer in this._getObservers(aName)) {
>+        try {
>+          observer.onContentPrefRemoved(groups [i], aName);
>+        }
>+        catch(ex) {
>+          Cu.reportError(ex);
>+        }
>+      } */
>+      this._deleteGroupIfUnused(groups [i]);

This'll need to be uncommented so that onContentPrefRemoved gets called when these prefs are removed (ideally we'd send a batch notification, but that's another bug).  Also, note that onContentPrefRemoved's first parameter should be the name of the group, whereas this code is retrieving and passing the ID of the group.

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to comment #3)
> >+  void removeNamedPrefs(in AString aName);
> >+  nsIPropertyBag2 getNamedPrefs(in AString aName);
> 
> Minor concern: I wonder if these names will imply that there are unnamed prefs,
> just as removeGroupedPrefs implies there are ungrouped prefs (which actually do
> exist, whereas there aren't any unnamed prefs).  Perhaps something like
> getPrefsByName and removePrefsByName, although clunkier, would be clearer.

I had a similar thought, so I've renamed them.

> This'll need to be uncommented so that onContentPrefRemoved gets called when
> these prefs are removed (ideally we'd send a batch notification, but that's
> another bug).  Also, note that onContentPrefRemoved's first parameter should be
> the name of the group, whereas this code is retrieving and passing the ID of
> the group.

I'm not sure what to do about observers. Should nsIContentPrefObserver be modified to handle a bulk notification (given that it observes specifically named preferences), or should we tell it of each removal individually? Or both?

New patch on the way, just building it now...

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

Now with added tests and better names! :)

Comment 6

[John P Baker]

Comment on attachment 387132 [details] [diff] [review]
patch v2

Just passing by...

>diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/toolkit/components/contentprefs/public/nsIContentPrefService.idl
>--- a/toolkit/components/contentprefs/public/nsIContentPrefService.idl
>+++ b/toolkit/components/contentprefs/public/nsIContentPrefService.idl

May need to change the uuid here (I may be wrong however)

>diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js
>--- a/toolkit/components/contentprefs/src/nsContentPrefService.js
>+++ b/toolkit/components/contentprefs/src/nsContentPrefService.js
>@@ -232,24 +232,56 @@ ContentPrefService.prototype = {

>+  removePrefsByName: function ContentPrefService_removePrefsByName(aName) {
>+    var settingID = this._selectSettingID(aName);

I think settingID can be undefined here if aName was not in database.

>+    var groups = Array();

var groups = []; ?

>+          observer.onContentPrefRemoved(groups [i], aName);

groups[i] (and others)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 387132 [details] [diff] [review]
patch v2

(In reply to comment #4)
> I'm not sure what to do about observers. Should nsIContentPrefObserver be
> modified to handle a bulk notification (given that it observes specifically
> named preferences), or should we tell it of each removal individually? Or both?

For now, let's tell observers about each removal individually, as you've done, but factor out the code for doing so from removePref and removePrefByName into a new method that both those methods call.

(In the long run, it'd be good to provide a batch notification, but we should probably switch to using the Observer service for these notifications rather than building out the custom nsIContentPrefObserver interface.)

John's review comments all seem right to me.  In addition, here are a couple other minor issues:


>+  removePrefsByName: function ContentPrefService_removePrefsByName(aName) {
>+    var settingID = this._selectSettingID(aName);
>+
>+    var s = this._dbConnection.createStatement("SELECT groups.name AS groupName " +
>+	  "FROM prefs " +
>+	  "JOIN groups ON prefs.groupID = groups.id " +
>+	  "WHERE prefs.settingID = " + settingID);

For consistency, use the _dbCreateStatement method, put a newline after the opening and before the closing parentheses, use spaces instead of tabs to indent, and indent the query strings two spaces, i.e.:

    var s = this._dbCreateStatement(
      "SELECT groups.name AS groupName " +
      "FROM prefs " +
      "JOIN groups ON prefs.groupID = groups.id " +
      "WHERE prefs.settingID = " + settingID
    );


>+    var groups = Array();
>+    while (s.step()) {
>+      groups.push(s.row["groupName"]);
>+    }

This statement should get reset after execution using a try/finally block (like in _selectPrefs).


>+      this._deleteGroupIfUnused(groups [i]);

Hmm, I wonder if it would be more performant to delete all unused groups in a single query after removing all the prefs.


Otherwise this looks great!

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: patch v3 + a bit

added tests and corrected some code to handle global prefs correctly

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 387430 [details] [diff] [review]
patch v3 + a bit

Just one issue:

>+    var global = this._selectGlobalPref(aName);
>+    if (global) {
>+      prefs.setProperty(null, global);
>+    }

The value _selectGlobalPref returns could evaluate to false, so this test needs to be |if (typeof global != "undefined")|.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: patch v4

Changed UUIDs, fixed up what you said in your last comment, added a few tests to cover things

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 387573 [details] [diff] [review]
patch v4

This looks good and passes tests.  One minor nit: some of the code has Windows-style endings (\r\n).  Make sure you use all Unix-style line endings (\n).

With that fix, this'll be ready for official review by a toolkit peer (perhaps Gavin?).

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: patch v4 again

... and now without Windows line endings (hopefully)

Comment 14

[John P Baker]

Comment on attachment 387777 [details] [diff] [review]
patch v4 again

I am not sure if it matters but I think something odd has happened with the uuid:

>-[scriptable, uuid(5047e359-dfda-4858-abec-d145c7463250)]
>+[scriptable, uuid(e6a3f533-4ffa-4615-8eb4-d4e72d883fa7)]
...

>   classDescription: "Content Pref Service",
>-  classID:          Components.ID("{e6a3f533-4ffa-4615-8eb4-d4e72d883fa7}"),
>+  classID:          Components.ID("{f98daa2b-b9f9-4897-9137-ea1b8b56a7f6}"),
>   contractID:       "@mozilla.org/content-pref/service;1",

you seem to have transferred the old classID to the new iid.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 387777 [details] [diff] [review]
patch v4 again

John is right, you should avoid re-using UUIDs!

I'm not extremely familiar with this schema or even with SQL in general, but I'm assuming Myk's reviewed those areas given his comments, but marking r? just to make sure :)

>diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/toolkit/components/contentprefs/public/nsIContentPrefService.idl

>   /**
>+   * Get the prefs with the given name.
>+   *
>+   * @param    aName        the setting name for which to remove prefs
>+   */
>+  void removePrefsByName(in AString aName);

>+  /**
>+   * Get the prefs with the given name.
>+   *
>+   * @param    aName        the setting name for which to retrieve prefs
>+   * 
>+   * @returns  a property bag of prefs
>+   */
>+  nsIPropertyBag2 getPrefsByName(in AString aName);

Do these handle null to mean "all prefs" as some other methods do? Seems like they do, would be worth documenting.

>diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js

>+  removePrefsByName: function ContentPrefService_removePrefsByName(aName) {

>+    for (var i = 0; i < groups.length; i++) {

>+      this._deleteGroupIfUnused(groups[i]);

How does this handle a null group name? I guess stmtDeleteGroupIfUnused just returns no results?

>diff --git a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js b/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js

Test looks good. Would be nice to also test the property names in addition to the value from the enumerated properties (e.g. to ensure that ungrouped prefs are handled correctly). Also could  check name/value in onContentPrefRemoved (e.g. using an array of expected values, and popping them off the array when notified, then checking that the array length == 0 at the end of the test rather than just using a counter).

Is it worth testing the nsIPropertyBag2 accessor methods of the enumerator? I guess xpconnect handles all that conversion automatically.

Comment 16

[Geoff Lankow (:darktrojan)]

(In reply to comment #14)
> (From update of attachment 387777 [details] [diff] [review])
> you seem to have transferred the old classID to the new iid.

Looks like I've had some clipboard issues... that's a little embarrassing!

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: patch v5

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 387998 [details] [diff] [review]
patch v5

(In reply to comment #15)
> I'm not extremely familiar with this schema or even with SQL in general, but
> I'm assuming Myk's reviewed those areas given his comments, but marking r? just
> to make sure :)

Yup, I've reviewed the patch, but here's a review of the latest version taking your comments into consideration...


> Do these handle null to mean "all prefs" as some other methods do? Seems like
> they do, would be worth documenting.

A null group has meaning (it specifies a pref that applies to all groups), but a null setting name doesn't, so the settings.name column has a NOT NULL constraint, and there cannot be a setting in the database whose name is null.

Passing null to getPrefsByName currently returns an empty enumerator, while passing null to removePrefsByName doesn't remove any prefs.  Ideally, these methods would throw an exception in this case, but none of the existing methods throw on a null setting name, so I'm inclined to treat this as a separate bug that should be addressed for all methods at once. I have filed it as bug 503971.


> >diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js
> 
> >+  removePrefsByName: function ContentPrefService_removePrefsByName(aName) {
> 
> >+    for (var i = 0; i < groups.length; i++) {
> 
> >+      this._deleteGroupIfUnused(groups[i]);
> 
> How does this handle a null group name? I guess stmtDeleteGroupIfUnused just
> returns no results?

It looks like the latest version of the patch no longer calls _deleteGroupIfUnused on a null group ID, which is the right thing to do:

      if (groups[i])
        this._deleteGroupIfUnused(groups[i]);


> Is it worth testing the nsIPropertyBag2 accessor methods of the enumerator? I
> guess xpconnect handles all that conversion automatically.

I suspect nsIPropertyBag2 accessor methods are best tested in an nsIPropertyBag2-centric test.


>diff --git a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js b/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
>--- a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
>+++ b/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
>@@ -345,16 +345,114 @@ function run_test() {
>+        var temp = [];
>+        for (var i = 0; i < this.expectedDomains.length; i++) {
>+          if (i != index)
>+            temp.push(this.expectedDomains[i]);
>+        }
>+        this.expectedDomains = temp;

Nit: here and below, you can do this more simply via Array::splice:

  this.expectedDomains.splice(index, 1);


One last question: I know you have to rev an interface ID when you change it, but I'm not sure you actually have to rev a component ID when the component changes.  Gavin: can you confirm?


In any case, this looks good and works well, r=myk.

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: patch v6 [Checkin: Comment 20]

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 390677 [details] [diff] [review]
patch v6
[Checkin: Comment 20]


http://hg.mozilla.org/mozilla-central/rev/8e3f77b2c264

Comment 21

[Shawn Wilsher :sdwilsh]

Why didn't this get sr per the new sr requirement on new API additions?

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 390677 [details] [diff] [review]
patch v6
[Checkin: Comment 20]

post-facto sr=mconnor, but we should fix bug 503971 ASAP, since removePrefsByName should definitely throw if we pass it an unknown string, IMO.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> One last question: I know you have to rev an interface ID when you change it,
> but I'm not sure you actually have to rev a component ID when the component
> changes.  Gavin: can you confirm?

That's correct - there's no need to rev the CID.

Comment 24

[Eric Shepherd [:sheppy]]

Now documented here: https://developer.mozilla.org/en/nsIContentPrefService

With a mention on Firefox 3.6 for developers.