335101 - search service should not use prefs for engine metadata (alias, hidden)

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

Comment 1

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

One option is mozstorage, the other is just an extra xml file loaded at startup. I won't be able to get to this, but I can probably help out if needed.

Comment 2

[Mike Connor [:mconnor]]

I get that this is ugly, what's a ballpark estimate for doing it differently?  What's the risk in leaving it the way it is?

Comment 3

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

Doing it differently shouldn't be that complicated (mozStorage should make it pretty easy, I think), and doing it for Firefox 2 would the need to do any tricky conversions from the old way to the new way in subsequent versions.

Comment 4

[Mike Connor [:mconnor]]

Ok.  Joey, please get some estimate around this, and we'll get load balanced by the end of the weekend.

Comment 5

[Joey Minta]

If we're all agreed on using mozStorage for this, I would put a swag around 4-5d.  Are we all confident enough in mozStorage at this point to use it for this?

Comment 6

[Mike Connor [:mconnor]]

I don't think we are.  As much as this would be nice to have, there are bigger fish to fry right now.

Dropping this off blocking radar, if someone finds cycles to do this, cool, but we just don't have the resources to do this right now.

Comment 7

[Joey Minta]

Attachment: move to mozStorage

This ended up being a lot easier than I thought.  It's based off of calendar/ code that is used for something similar (meta-data for calendars).  If possible I'd like to land this sooner as it will make the renaming/duplicate name bugs easier to do.

Comment 8

[Joey Minta]

re-requesting blocking, after talk with mconnor.

Comment 9

[Axel Hecht [:Pike]]

There is at least one getEnginePref(foo, bar, val), that should be a 
setEnginePref in set hidden(). Can't do a real review, but that just popped up to 
my eyes.

Comment 10

[Joey Minta]

Attachment: move to mozStorage v2

Doh.  Patch fixes the typo pointed out by Axel.  'hidden' now works fine.  I'm not sure the best way to actually test 'alias' changes.

Comment 11

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

Comment on attachment 225308 [details] [diff] [review]
move to mozStorage v2

I'm really not familiar with the storage APIs, so I don't really feel comfortable reviewing that part of the patch, but that part was already reviewed for calendar, so I guess that's not a big problem.

Just for my information, where does this data end up being stored on disk? Is there a single "profile" .sqlite file? I noticed other users of storage have specific files (like formhistory.sqlite), but I don't know what openSpecialDatabase("profile") ends up doing.

Here are some comments about the other parts.

>+const kStorageServiceContractID = "@mozilla.org/storage/service;1";
>+const kStorageServiceIID = Components.interfaces.mozIStorageService;
>+
>+const kMozStorageStatementWrapperContractID = "@mozilla.org/storage/statement-wrapper;1";
>+const kMozStorageStatementWrapperIID = Components.interfaces.mozIStorageStatementWrapper;

These are only used once, so there's no need for them to be global, right? just put them before where they're used.

>+function createStatement (dbconn, sql) {
>+    var stmt = dbconn.createStatement(sql);
>+    var wrapper = MozStorageStatementWrapper();
>+
>+    wrapper.initialize(stmt);
>+    return wrapper;
>+}

Can you add just a bit of documentation for this method (similar to the other methods above it)?

>-      this._alias = getLocalizedPref(this._pref + "alias", "");
>+      this._alias = enginePrefService.getEnginePref(this, "alias");

The "pref" object/function names are kind of misleading, since this has nothing to do with prefs. Maybe call the object "engineMetaData", with "getAttr" and "setAttr" methods? Maybe you can think of something better.

>+  _convertOldPrefs: function SRCH_SVC_convertOrder() {

One of the reasons I wanted to make sure this made the 2.0 release was to avoid having to do this conversion, and never putting it in means we don't have to worry about getting rid of it at some point in the future. None of this data is really critical, and I don't think reconfiguring engines once is too much of a burden to put on nightly/alpha testers. Having the extra prefs lying around for those that used the pre-release versions isn't really a big deal either. I guess mconnor should make the call as to whether this should be included or not.

>+	if ("getProfileStorage" in dbService) {
>+	  // 1.8 branch
>+	  this.mDB = dbService.getProfileStorage("profile");
>+	} else {
>+	  // trunk 
>+	  this.mDB = dbService.openSpecialDatabase("profile");
>+	}

Looks like it's now openSpecialDatabase for both branch and trunk, so I think this check can be removed.

>+    try {
>+      this.mDB.createTable("engine_prefs", enginePrefTable);
>+    } catch (ex) {
>+      dump("error creating search engine pref table -- probably already exists\n");
>+    }

Use LOG() instead of dump(). Or can this dump() just be removed? Won't the table always already exist, other than on first startup? That wouldn't really be a useful thing to dump.

>+  getEnginePref: function epsGetPref(engine, name) {
>+      // pref names must be lower case
>+      name = name.toLowerCase();
>+
>+      var stmt = this.mGetPref;
>+      stmt.reset();
>+      var pp = stmt.params;
>+      pp.location = engine._location;

So _location wasn't really meant for this... I added it so that I could dump() something useful when debugging, and for error messages in the case where the engine being loaded didn't have a file yet. I think you should create an _id property, and have it ENSURE_WARN(engine._file), and then return (ioService.newFileURI(engine._file).spec).

Also, need to fix the wacky 2 space/4 space indentation combo. :)

Comment 12

[Joey Minta]

Attachment: move to mozStorage v3

Patch updated to reflect Gavin's previous comments.  We've decided to go ahead with the migration, since the ordering migration is necessary for new profiles anyway.  For reference, openSpecialDatabase("profile") uses storage.sdb as its database.

Comment 13

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

Comment on attachment 225348 [details] [diff] [review]
move to mozStorage v3

General nits: Need to use Cc/Ci throughout the patch, spaces around operators (there are a few squished "+"s), and need to lose the extra braces around single-line then blocks. Also, be sure not to be adding any trailing whitespace.

>   get hidden() {
>     if (this._hidden === null) {
>-      // Initialize the hidden property from a pref
>-      this._hidden = getBoolPref(this._pref + "hidden", false);
>+      // Initialize the hidden property from data
>+      this._hidden = engineMetadataService.getAttr(this, "hidden");

I'd say just lose the comment (and the braces), I'm not sure why I added this comment (it's pretty useless).

>   set hidden(val) {
>     var value = !!val;
>     if (value != this._hidden) {
>-      setBoolPref(this._pref + "hidden", value);
>       this._hidden = value;
>+      engineMetadataService.setAttr(this, "hidden", val);

s/val/value/, or get rid of value and just use val everywhere (I'm also not sure why this is here :()

>+  // The file that the plugin is loaded from is a unique identifier for it.  We
>+  // use this as the identifier to store data in the sqlite database
>+  get _id() {
>+    ENSURE_WARN(this, this._file, Cr.NS_ERROR_FAILURE);
>+    return this._file.path;
>+  },

The ENSURE_WARN here is incorrect, but you mentioned you fixed this on IRC. Does the fact that .path can contain spaces not matter? What about characters in strange encodings? That's why I recommended using a file:// URI from |ioService.newFileURI(this._file).spec|.

>   _buildSortedEngineList: function SRCH_SVC_buildSortedEngineList() {
>     var addedEngines = { };
>-    this._sortedEngines = [];
>+    this._sortedEngines = Array(this._engines.length);

Convention is to use "new Array", I didn't know that this works. Same with MozStorageStatementWrapper ("new MozStorageStatementWrapper()").

>     var engineName, engine;

>+    for each (engine in this._engines) {
>+      var orderNumber = engineMetadataService.getAttr(engine, "order");
>+      if (orderNumber) {
>+        this._sortedEngines[orderNumber-1] = engine;
>+        addedEngines[engine.name] = engine;
>+      }
>     }
> 
>     // Array for the remaining engines, alphabetically sorted
>     var alphaEngines = [];
> 
>     for (engineName in this._engines) {
>       engine = this._engines[engineName];

Can you make this a "for each" loop too, so you can get rid of the engineName variable?

>+      if (hidden)
>+        engineMetadataService(engine, "hidden", hidden);

This is missing a |.setAttr|. Don't you just want to set hidden unconditionally? Same with alias?

>+      var engineBranch = prefService.getBranch(basePref);
>+      engineBranch.deleteBranch("");

Hmm, does deleteBranch do what we want here? The comment in the IDL about it operating on both default and user prefbranches kinda scares me.

>+    this.mDB = dbService.openSpecialDatabase("profile");
>+    this.mGetData = createStatement (
>+    this.mDeleteData = createStatement (
>+    this.mInsertData = createStatement (

Convention is to use "_" instead of "m" as a member prefix, but I guess this doesn't need to be changed.

Comment 14

[Axel Hecht [:Pike]]

We need to bootstrap some of this metadata, at least browser.search.order.
That doesn't necessarily have to be backwards compatible, but we need to port 
this.

Currently this is done in
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/chrome/browser-region/region.properties#12,
I wouldn't be sad if this was dealt with in a single localizable value, though.

Do we need to be able to reset the order to defaults?

Comment 15

[Mike Connor [:mconnor]]

(In reply to comment #14)
> Do we need to be able to reset the order to defaults?

yes, definitely.

Comment 16

[Joey Minta]

(In reply to comment #14)
> We need to bootstrap some of this metadata, at least browser.search.order.
> That doesn't necessarily have to be backwards compatible, but we need to port 
> this.
The migration code in this patch takes care of this.

> 
> Do we need to be able to reset the order to defaults?
> 
This is also pretty easy to do with the new code.  Should we spin off another bug or do you want me to include that functionality here?

Comment 17

[Axel Hecht [:Pike]]

IMHO, the migration code should be called when the database didn't exist.
Not on each startup, switched off by an additional pref.

I'd really like to see us not having to create legacy prefs to migrate from for
new profiles, though.

Do we need to migrate browser.search.param. + engine?

On a general note, should the metadata support localized settings, like the
prefs do? The localized setting can change if you change the language pack.
That's not too well supported, but it still works currently.

Comment 18

[Joey Minta]

(In reply to comment #17)
> IMHO, the migration code should be called when the database didn't exist.
> Not on each startup, switched off by an additional pref.
Makes sense.

> 
> I'd really like to see us not having to create legacy prefs to migrate from for
> new profiles, though.
> 
> Do we need to migrate browser.search.param. + engine?
I can find where we get/set these prefs.  Since they weren't used in the search engine code I was touching, I didn't mess with them.  Can someone get me an lxr link?

> 
> On a general note, should the metadata support localized settings, like the
> prefs do? The localized setting can change if you change the language pack.
> That's not too well supported, but it still works currently.
As far as I know, mozStorage has no problem handling data in other languages.  (It's been doing it for Sunbird/Lightning event data already.)  Is that the only concern or am I missing something?

Comment 19

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

(In reply to comment #17)
> Do we need to migrate browser.search.param. + engine?

No, that will be handled in the parameter bug (bug 335460) (I'm hoping I can make those prefs go away, since they're not used at all for Yahoo, and mostly not used for Google).

> On a general note, should the metadata support localized settings, like the
> prefs do? The localized setting can change if you change the language pack.
> That's not too well supported, but it still works currently.

I don't think that's worth supporting, especially given that there is now UI for reordering the engines.

Comment 20

[Joey Minta]

Attachment: move to mozStorage v4

Patch addresses the previous comments, as well as the suggestion about basing pref-conversion on whether or not the table was created.  I think deleteBranch() is the correct way to go, since there are no defaults for alias or hidden, and those are the only prefs that might be in the branches being deleted, so the end result would be the same.

Comment 21

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

Comment on attachment 225462 [details] [diff] [review]
move to mozStorage v4

This patch removes the only callers of setBoolPref, so it can be removed. |var preferences| in saveSortedEngineList is also now unused (along with the brackets around the loop). Can you add a comment above _convertOldPrefs that mentions how it also initializes the DB on the first startup with a new profile?

I think you should explicitly declare newTable on the engineMetadataService object, with a short comment describing what it means.

>+      var hidden = getBoolPref(basePref + "hidden", false);
>+      engineMetadataService(engine, "hidden", hidden);

Still missing |.setAttr| here, this makes this patch fail with a new profile.

Otherwise, this is great stuff :). One thing I forgot about was that we need to remove the stored meta data when a profile engine is removed, as it is that data will just hang around forever. That's not really a huge issue, and can be fixed separately (prefs also had this problem, except worse because it would apply to any future identically named engine, where this will only apply to engines with the same file name in the same location). Would adding a "remove all DB entries for <engine>" method to engineMetadataService be easy to do? That doesn't have to happen now, I'm just wondering. I'll file a bug on that if it isn't already filed.

Comment 22

[Joey Minta]

Patch checked in.  Please do file the bug for removing metadata after removing engines if it doesn't already exist.

Comment 23

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

(In reply to comment #22)
> Patch checked in.  Please do file the bug for removing metadata after removing
> engines if it doesn't already exist.

I filed bug 341833.

Comment 24

[Régis Caspar]

I found this bug after looking what's into the mysterious "storage.sdb" I saw in my profile and I have 2 suggestions:
 - "storage.sdb" should IMHO be renamed to something more explicit "search_data.sqlite"?
 - in "storage.sdb", absolute path to each installed search plug-in is stored why not (to minimize storage) using some kind of TAG (i.e. "[Profile]" + PATH_SEPARATOR + "foo.xml").

Comment 25

[Adam Guthrie]

Regis, please feel free to file another follow-up bug for the issue you mentioned. The issue described in this bug is fixed.

Comment 26

[Régis Caspar]

(In reply to comment #25)
> Regis, please feel free to file another follow-up bug for the issue you
> mentioned. The issue described in this bug is fixed.
OK, I opened bug 341975 (for the file naming) and bug 341976 for the storage optimization. I should stop having hesitations opening bugs :|
Thanks (and sorry for the bugspam)