Geoff Lankow (:darktrojan) Assignee
	Description • 15 years ago

The content pref service is leaving group names behind because it's calling _deleteGroupIfUnused with group names instead of IDs. Working on a patch/test now.

	Geoff Lankow (:darktrojan) Assignee
	Comment 2 • 15 years ago

Forgot to hg add before hg diff. How come I don't notice these things until I upload them and make a fool of myself?

	Geoff Lankow (:darktrojan) Assignee
	Comment 3 • 15 years ago

Seems removeGroupedPrefs leaves unused setting names behind too.

canOfWorms.open();

	Myk Melez [:myk] [@mykmelez]
	Comment 4 • 15 years ago

Comment on attachment 419514 [details] [diff] [review]
two fixes and tests

Just back from vacation... nice catches!


>+      this._dbConnection.executeSimpleSQL(
>+        "DELETE FROM settings " +
>+        "WHERE id NOT IN (SELECT DISTINCT settingID FROM prefs)"
>+      );

This has a potential performance implication, since prefs.settingID is not indexed by itself, so this does a full table scan of the prefs table.  We're dealing with relatively small datasets, so it may not matter, but we could make sure it doesn't by adding an index on the prefs.settingID column.

(Either way, SQLite will still do a full scan of the settings table, since it can't use an index to resolve a term using a NOT IN operator.)


>     this._dbConnection.executeSimpleSQL("DELETE FROM prefs WHERE settingID = " + settingID);

FWIW, the performance implication of the query added above applies to this existing query as well.


>+    for (var i = 0; i < groupNames.length; i++) {
>+      this._notifyPrefRemoved(groupNames[i], aName);
>+      if (groupNames[i]) // ie. not null, which will be last (and i == groupIDs.length)
>+        this._deleteGroupIfUnused(groupIDs[i]);

It's unclear whether _deleteGroupIfUnused is more performant than a single |DELETE FROM groups WHERE id NOT IN (SELECT groupID FROM prefs)| after the loop would be.  _deleteGroupIfUnused uses indexes on both the groups and prefs tables, but we run it multiple times, whereas the single query only runs once, but it does a full table scan of the groups table (it still uses an index for the prefs table).  Barring determinative data, let's err on the side of multiple queries using indexes (i.e. the current implementation).

	Geoff Lankow (:darktrojan) Assignee
	Comment 5 • 15 years ago

Comment on attachment 419514 [details] [diff] [review]
two fixes and tests

Hopefully this will make it to 1.9.2 where removePrefsByName appears for the first time. Might as well not be buggy on debut, if we can avoid it.

	Geoff Lankow (:darktrojan) Assignee
	Comment 6 • 15 years ago

I'm not too concerned about performance of this as I expect most users will have a very small dataset. Perhaps we should get some metrics (Test Pilot?) on CPS usage.

	Nochum Sossonko [:Natch]
	Comment 7 • 15 years ago

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/80aca4165c6c.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 8 • 14 years ago

Comment on attachment 419514 [details] [diff] [review]
two fixes and tests

Sorry that we missed this for the 1.9.2.2 cycle; had some more pressing regressions to get to. We'll be back to approve this (great) change for 1.9.2.3

	Myk Melez [:myk] [@mykmelez]
	Comment 9 • 14 years ago

If I remember correctly, this fix has a privacy implication, since ContentPrefs::removeGroupedPrefs is used by the data sanitizer to remove site-specific preferences.  Since 1.9.2 is going to be around for a while longer, it's worth getting this fix onto the branch.

	LegNeato
	Comment 10 • 14 years ago

Comment on attachment 419514 [details] [diff] [review]
two fixes and tests

a=LegNeato for 1.9.2.9.

	Daniel Veditz [:dveditz]
	Comment 11 • 14 years ago

Comment on attachment 419514 [details] [diff] [review]
two fixes and tests

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/427a3286223e