Marco Bonardo [:mak] Reporter
	Description • 13 years ago

nsSearchService.js should stop using synchronous Storage APIs.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 13 years ago

It should really stop using Storage APIs entirely (bug 335101 was not a great decision, in retrospect). We could probably live with a JSON flat file for this stuff. The only complication with that is that we'll need some migration code.

	Marco Bonardo [:mak] Reporter
	Comment 2 • 13 years ago

yeah, I filed these bugs to find a solution, that may even be to drop sqlite. It makes a lot of sense in all those cases where we have to handle less than 100 records or complex queries are not needed.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 3 • 13 years ago

I'm going to use a JSON flat file. However, doing this async will require a background IO thread, which is now impossible without ctypes access because of bug 649537. So, I'm adding bug 563742 as a blocker.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 4 • 13 years ago

CC-ing vladan who is working on a patch that may conflict.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> I'm going to use a JSON flat file. However, doing this async will require a
> background IO thread, which is now impossible without ctypes access

Async IO doesn't require using workers, you can use e.g. NetUtil.asyncCopy from NetUtil.jsm.

	(dormant account)
	Comment 6 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > I'm going to use a JSON flat file. However, doing this async will require a
> > background IO thread, which is now impossible without ctypes access
> 
> Async IO doesn't require using workers, you can use e.g. NetUtil.asyncCopy
> from NetUtil.jsm.

which is a rather terribly performing thing.

	Dietrich Ayala (:dietrich)
	Comment 7 • 13 years ago

(In reply to Taras Glek (:taras) from comment #6)
> > Async IO doesn't require using workers, you can use e.g. NetUtil.asyncCopy
> > from NetUtil.jsm.
> 
> which is a rather terribly performing thing.

What is your recommendation?

	(dormant account)
	Comment 8 • 13 years ago

(In reply to Dietrich Ayala (:dietrich) from comment #7)

> > which is a rather terribly performing thing.
> 
> What is your recommendation?

We don't have anything i can recommend. This is why I asked David to get us a new api

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 13 years ago

I don't think waiting on a new API is worthwhile. We need to fix asyncCopy anyways, so just use that for now and we can revisit all its users in the future when a newer API is available. The important pieces here are getting rid of the sync Storage API use, and handling the migration.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 10 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> I don't think waiting on a new API is worthwhile. We need to fix asyncCopy
> anyways, so just use that for now and we can revisit all its users in the
> future when a newer API is available. The important pieces here are getting
> rid of the sync Storage API use, and handling the migration.

Yes, this is what I am doing for the moment, as a first step and to avoid having to test too many changes at once. I will try and make the first few features of the JSFileAPI available asap, so as to be able to move to the following step.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 11 • 13 years ago

I attach a preview version. This version uses a json backend, implements migration and _some_ asynchronicity, with a synchronous fallback for getters. Unfortunately, experimenting with it shows that, in almost all cases, the synchronous fallback is used and makes asynchronicity useless. I will see what I can do to increase asynchronicity.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 12 • 13 years ago

Comment on attachment 575461 [details] [diff] [review]
Early preview

This looks like the wrong patch.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 13 • 13 years ago

My bad about that. This is the early preview.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 14 • 13 years ago

Ok, I will clean up and add a few more tests, but it now seems to be working – and perform loading asynchronously.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 15 • 13 years ago

gavin, I notice that there is a second database in this service ("search.json") and that it is loaded synchronously. Do you want me to take a look at this? Or should this go into another bug?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 16 • 13 years ago

I attach an async version, along with a few additional tests. This version still contains some debugging code, as I am waiting for gavin's answer to my earlier comment before deciding to either finalize the patch or pursue work on making the code more asynchronous.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 17 • 13 years ago

Comment on attachment 576145 [details] [diff] [review]
Async version (contains debugging code)

I noticed a lot of style issues, like stray tabs being introduced, unnecessarily added brackets, "if(" instead of "if (", etc. I know this is just a work in progress, but those will need to be fixed before this can land.

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>-[scriptable, uuid(8307b8f2-08ea-45b8-96bf-b1dc7688fe3b)]
>+[scriptable, uuid(550D27A2-AC28-4055-9876-9E4D97C5DD42)]

FWIW you don't need to rev IIDs unless you're making changes to the interface itself (as opposed to just constants).

>  * The observer topic to listen to for actions performed on installed
>- * search engines.
>+ * search engines or on the search service itself.
>  */
> #define SEARCH_ENGINE_TOPIC "browser-search-engine-modified"

It's not a good idea to re-use this topic, IMO. I think it would be nicer to have the search service have an explicit init(in nsIObserver callback).

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+AsyncUtils = {

>+    later:     function AsyncUtils_setTimeout(aCallback) {
>+	var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+	timer.initWithCallback(new this.TimerCallback(aCallback), 0, timer.TYPE_ONE_SHOT);

Using nsIThreadManager (as in executeSoon) for this would be more efficient.

>+    forEachInArray: function AsyncUtils_forEachInArray(aArray, aItemCallback, aCompleteCallback) {

Is this actually useful? I can't really think of any cases where you'd want to iterate over items in an array asynchronously (at least not one at a time). It doesn't seem to be used, so let's not add it before there are use cases.

>+AsyncUtils.TimerCallback.prototype = {

Not relevant if you switch to nsIThreadManager, but nsITimerCallback is annotated with "function", so there's no need to pass an actual object with XPCOM goop - you can just pass a JS function object directly to initWithCallback().

>+ * Breakpointing from a C++ debugger is quite useful e.g. to debug
>+ * calls from XUL to XPCOM through XPConnect.
>+ *
>+ * To use it, breakpoint upon `nsXPCComponents_Utils::EvalInSandbox`
>+ */
>+var BREAKPOINT = function() {

I know this is just debugging code, but isn't it usually easier to just breakpoint on the actual XPCOM method implementation you care about?

> function bytesToString(aBytes, aCharset) {

>-    return converter.convertFromByteArray(aBytes, aBytes.length);
>-  } catch (ex) {}
>+
>+    //FIXME: Once bug 699156 is fixed, we will be able to simply write
>+    //return converter.convertFromByteArray(aBytes, aBytes.length);

I don't understand why you're making this change. The current code works fine, AFAIK, and there's no need to switch this to a typed array as part of this bug.

>+  _alias: {/**@type {string|null}*/value: undefined},

I don't understand this change to using an object either.

>   _initFromURI: function SRCH_ENG_initFromURI() {

This method already fetches data asynchronously, so I'm not sure why you're switching it to use an XHR. I think that's less efficient and might also regress some functionality (e.g. ignoring bad certs).

Overall, I think the approach might need tweaking: we should store all the relevant metadata in-memory, so that the getters/setters can remain synchronous, and have the overall service initialization and persisting to disk be async.

search.json is a JSON cache of basically all of the search service state - it allows us to avoid hitting SQLite or parsing XML entirely for the common cases (or at least that's the idea), but it can be blown away frequently (on any upgrade, e.g.). It's probably simpler as a first step to leave all of that logic intact, and instead focus on moving metadata storage to search-metadata.json or somesuch.

The tests are great, search service is in dire need of more tests :( (bug 458810)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 18 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> Comment on attachment 576145 [details] [diff] [review] [diff] [details] [review]
> Async version (contains debugging code)
> 
> I noticed a lot of style issues, like stray tabs being introduced,
> unnecessarily added brackets, "if(" instead of "if (", etc. I know this is
> just a work in progress, but those will need to be fixed before this can
> land.

Absolutely. I am still adapting to the Mozilla Coding Guidelines.

> 
> >diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl
> 
> >-[scriptable, uuid(8307b8f2-08ea-45b8-96bf-b1dc7688fe3b)]
> >+[scriptable, uuid(550D27A2-AC28-4055-9876-9E4D97C5DD42)]
> 
> FWIW you don't need to rev IIDs unless you're making changes to the
> interface itself (as opposed to just constants).

Ok.

> 
> >  * The observer topic to listen to for actions performed on installed
> >- * search engines.
> >+ * search engines or on the search service itself.
> >  */
> > #define SEARCH_ENGINE_TOPIC "browser-search-engine-modified"
> 
> It's not a good idea to re-use this topic, IMO. I think it would be nicer to
> have the search service have an explicit init(in nsIObserver callback).

Ok.

> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+AsyncUtils = {
> 
> >+    later:     function AsyncUtils_setTimeout(aCallback) {
> >+	var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> >+	timer.initWithCallback(new this.TimerCallback(aCallback), 0, timer.TYPE_ONE_SHOT);
> 
> Using nsIThreadManager (as in executeSoon) for this would be more efficient.
> 
> >+    forEachInArray: function AsyncUtils_forEachInArray(aArray, aItemCallback, aCompleteCallback) {
> 
> Is this actually useful? I can't really think of any cases where you'd want
> to iterate over items in an array asynchronously (at least not one at a
> time). It doesn't seem to be used, so let's not add it before there are use
> cases.

Well, it is definitely the wrong place. But yes, it can be useful for properly handling the list of closures waiting for initialization (storage._initQueue). As is, we have a subtle race condition that could be triggered by an add-on attempting to access the service late in its initialization. This async iterator would be necessary to fix things properly.

As it is, I need to decide whether we want to fix the race condition, or simply to throw an exception if it is encountered.

> >+AsyncUtils.TimerCallback.prototype = {
> 
> Not relevant if you switch to nsIThreadManager, but nsITimerCallback is
> annotated with "function", so there's no need to pass an actual object with
> XPCOM goop - you can just pass a JS function object directly to
> initWithCallback().

Good to know.

> 
> >+ * Breakpointing from a C++ debugger is quite useful e.g. to debug
> >+ * calls from XUL to XPCOM through XPConnect.
> >+ *
> >+ * To use it, breakpoint upon `nsXPCComponents_Utils::EvalInSandbox`
> >+ */
> >+var BREAKPOINT = function() {
> 
> I know this is just debugging code, but isn't it usually easier to just
> breakpoint on the actual XPCOM method implementation you care about?

Certainly easier, but this serves a different end. I used it to trace race conditions back to the original (XPCOM) implementation.

> 
> > function bytesToString(aBytes, aCharset) {
> 
> >-    return converter.convertFromByteArray(aBytes, aBytes.length);
> >-  } catch (ex) {}
> >+
> >+    //FIXME: Once bug 699156 is fixed, we will be able to simply write
> >+    //return converter.convertFromByteArray(aBytes, aBytes.length);
> 
> I don't understand why you're making this change. The current code works
> fine, AFAIK, and there's no need to switch this to a typed array as part of
> this bug.

The general idea is that:
- typed arrays are much faster than regular arrays;
- with a proper XHR, we can avoid array concatenations.

But yes, definitely, this optimization could (should) have waited for its own bug.

> 
> >+  _alias: {/**@type {string|null}*/value: undefined},
> 
> I don't understand this change to using an object either.

It is probably useless, actually.

> >   _initFromURI: function SRCH_ENG_initFromURI() {
> 
> This method already fetches data asynchronously, so I'm not sure why you're
> switching it to use an XHR. I think that's less efficient and might also
> regress some functionality (e.g. ignoring bad certs).

Good to know.


> Overall, I think the approach might need tweaking: we should store all the
> relevant metadata in-memory, so that the getters/setters can remain
> synchronous, and have the overall service initialization and persisting to
> disk be async.

Well, all relevant metadata is in-memory (hum, all 130 bytes of it), and I provide synchronous getters/setters, with async persisting, so the change should be easy. The only reason for which we have a weird asynchronous path was a manner of avoiding changing the public interface, as I have no clue which add-ons can be using it.

However, if we decide to change the public interface, the code can become reasonably simpler.

> search.json is a JSON cache of basically all of the search service state -
> it allows us to avoid hitting SQLite or parsing XML entirely for the common
> cases (or at least that's the idea), but it can be blown away frequently (on
> any upgrade, e.g.). It's probably simpler as a first step to leave all of
> that logic intact, and instead focus on moving metadata storage to
> search-metadata.json or somesuch.

Should I open a second bug on loading it asynchronously? It is much larger than search-metadata.json.

> The tests are great, search service is in dire need of more tests :( (bug
> 458810)

Ok, I will submit the tests separately.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 13 years ago

One more question: should I bump the contractid to ";2"?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 20 • 13 years ago

Applying suggestions, here is a new, less ambitious, version.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 22 • 13 years ago

Comment on attachment 577985 [details] [diff] [review]
The API.

Cancelling review, I have found a bug.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 23 • 13 years ago

Note: I have uploaded the test patch on a different bug.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 24 • 13 years ago

Bug fixed.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 25 • 13 years ago

Review ping?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 26 • 13 years ago

Comment on attachment 578194 [details] [diff] [review]
The API.

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>+ *   David Rajchenbach-Teller <dteller@mozilla.com> (async initializer)

nit: no need for the parenthetical, it gets a bit messy if everyone tries to list what exactly they changed (and these headers should be going away soon anyways).

>+  void init(in nsIObserver aObserver);

It's a little unpleasant to be using nsIObserver for this, since I don't think we'll really need any way to indicate failures to initialize. You could instead create your own nsIBrowserSearchServiceInitCallback, I suppose? Maybe overkill.

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>-function DO_LOG(aText) {

Please don't make these changes.

>+const AsyncUtils = {

>+  executeSoon: function AsyncUtils_executeSoon(func) {
>+    if (!this._tm) {
>+      this._tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
>+    }

You could make use of Services.tm (and file a followup to make the rest of this file take advantage of Services.jsm).

>+    this._tm.mainThread.dispatch({
>+      run: function() {
>+        func();
>+      }

You can just pass func directly to dispatch().

>+  init: function SRCH_SVC__init(aObserver) {

>+      for each(let q in queue) {

nit: prefer array.forEach() over |for each (a in array)|

>   _readCacheFile: function SRCH_SVC__readCacheFile(aFile) {

Is the idea to make this IO async in a followup?

>@@ -2910,25 +3026,30 @@ SearchService.prototype = {

>     for (var i = 0; i < engines.length; ++i) {
>-      names[i] = "order";
>+        name:   names[i],

This looks wrong (names is an empty array here).

> var engineMetadataService = {

>+  init: function eps_init(aCallback) {
>+    if (this._data) {//Ignore re-initializations
>+      aCallback.call(null);

should use executeSoon here (mixing sync/async callbacks is a recipe for trouble)

>+  _getJsonStore: function SRCH_SVC_EMS_getJsonStore() {
>+    LOG("SRCH_SVC_EMS_getJsonStore 1");
>+    if (!this._directoryCached) {
>+      return null;//No directory, this is certainly a special case without stored metadata
>+    }

I don't understand what this comment says, or why _directoryCached would be null. I think caching the nsIFile for the profile isn't worth the trouble, you can just create a new one every time (which you're doing anyways with .clone()).

>+  _backupStore: function SRCH_SVC_EMS_backupStore(aStore)

This seems like something that could be done in a followup.

>+  _migrateOldDb: function SRCH_SVC_EMS_migrate(aPath) {

>+      while (statement.executeStep()) {

Shouldn't we be using the async storage API here?

>+        var   val    = store[engine];
>+        if (!val) {
>+          val = {};
>+          store[engine] = val;
>+        }
>+        val[name] = value;

"val" just obfuscates the code here - just reference store[engine] directly.

>+  _commit: function SRCH_SVC_EMS_commit()

>+      ofile.append("search.tmp");

>+            ofile.moveTo(null, "search-metadata.json");

Doesn't the safe-file-output-stream take care of this for you automatically? That's it's purpose...

>-  deleteEngineData: function epsDelData(engine, name) {

Hmm, it's a bug that this doesn't have any callers (bug 341833, in fact!). We can fix that separately.

r- given the bug with the order saving - it would be really awesome to get a test that would cover that. As mentioned in email, it would also be nice to split this into smaller incremental changes, since there are a couple of different things changing at once here.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 27 • 13 years ago

Comment on attachment 577987 [details] [diff] [review]
The Client.

This is surprisingly simple! (nit: you should re-indent the observer to avoid the weird double-})

I think you'll need to update all the other users of nsIBrowserSearchService as well, though, since it's not OK to rely on the search bar initializing the search service (the search bar is removable, for example).

This has the potential to break addons that make use of the search service, but if we try to guarantee that the search service is initialized early enough (which should be true for users with the search bar enabled, at the very least), we will probably avoid most of the damage.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 28 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #26)
> Comment on attachment 578194 [details] [diff] [review]
> The API.
> 
> >diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl
> 
> >+ *   David Rajchenbach-Teller <dteller@mozilla.com> (async initializer)
> 
> nit: no need for the parenthetical, it gets a bit messy if everyone tries to
> list what exactly they changed (and these headers should be going away soon
> anyways).

ok.

> >+  void init(in nsIObserver aObserver);
> 
> It's a little unpleasant to be using nsIObserver for this, since I don't
> think we'll really need any way to indicate failures to initialize. You
> could instead create your own nsIBrowserSearchServiceInitCallback, I
> suppose? Maybe overkill.

I think it would be a bit overkill.
Also, in the future, I think it would be best to progressively introduce an API of Promises, if we can.

> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >-function DO_LOG(aText) {
> 
> Please don't make these changes.

Ok. What exactly is this function doing there, though? We should have a Debug.jsm or something such.

> >+const AsyncUtils = {

Note: It is my hope that I will be able to use Schedule.jsm (bug 692420) instead of AsyncUtils.

> >+  executeSoon: function AsyncUtils_executeSoon(func) {
> >+    if (!this._tm) {
> >+      this._tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
> >+    }
> 
> You could make use of Services.tm (and file a followup to make the rest of
> this file take advantage of Services.jsm).

Ok.

> 
> >+    this._tm.mainThread.dispatch({
> >+      run: function() {
> >+        func();
> >+      }
> 
> You can just pass func directly to dispatch().

Ok.

> >+  init: function SRCH_SVC__init(aObserver) {
> 
> >+      for each(let q in queue) {
> 
> nit: prefer array.forEach() over |for each (a in array)|

Well, I have no issue with higher-order programming, but I had the impression that |for each| was the recommended technique. Why do you suggest |array.forEach|?

Also, if this is the case, perhaps a pass of rewriting this module would be required, as I see quite a few uses of |for each|.

> >   _readCacheFile: function SRCH_SVC__readCacheFile(aFile) {
> 
> Is the idea to make this IO async in a followup?

Indeed. For the moment, I only concentrated on the database engine. Should I file a bug?

> 
> >@@ -2910,25 +3026,30 @@ SearchService.prototype = {
> 
> >     for (var i = 0; i < engines.length; ++i) {
> >-      names[i] = "order";
> >+        name:   names[i],
> 
> This looks wrong (names is an empty array here).

Indeed, this looks like a bug, thanks for spotting it.


> > var engineMetadataService = {
> 
> >+  init: function eps_init(aCallback) {
> >+    if (this._data) {//Ignore re-initializations
> >+      aCallback.call(null);
> 
> should use executeSoon here (mixing sync/async callbacks is a recipe for
> trouble)

ok

> I don't understand what this comment says, or why _directoryCached would be
> null. I think caching the nsIFile for the profile isn't worth the trouble,
> you can just create a new one every time (which you're doing anyways with
> .clone()).

Ok, removing the cache. And I confirm that I do not remember exactly what the comment meant, which is a bad sign.

> 
> >+  _backupStore: function SRCH_SVC_EMS_backupStore(aStore)
> 
> This seems like something that could be done in a followup.

That's part of error-handling, so I tend to believe that it should belong to the same patch.

> >+  _migrateOldDb: function SRCH_SVC_EMS_migrate(aPath) {
> 
> >+      while (statement.executeStep()) {
> 
> Shouldn't we be using the async storage API here?

I suppose that I could. However, I have the feeling that it may be a bit overkill for a function that will be executed exactly once, at first startup..

> 
> >+        var   val    = store[engine];
> >+        if (!val) {
> >+          val = {};
> >+          store[engine] = val;
> >+        }
> >+        val[name] = value;
> 
> "val" just obfuscates the code here - just reference store[engine] directly.

Well, it probably makes things just a little tiny bit faster, but ok, removed.

> >+  _commit: function SRCH_SVC_EMS_commit()
> 
> >+      ofile.append("search.tmp");
> 
> >+            ofile.moveTo(null, "search-metadata.json");
> 
> Doesn't the safe-file-output-stream take care of this for you automatically?
> That's it's purpose...

You are right, my bad.

> >-  deleteEngineData: function epsDelData(engine, name) {
> 
> Hmm, it's a bug that this doesn't have any callers (bug 341833, in fact!).
> We can fix that separately.
> 
> r- given the bug with the order saving - it would be really awesome to get a
> test that would cover that. As mentioned in email, it would also be nice to
> split this into smaller incremental changes, since there are a couple of
> different things changing at once here.

I'll try and write a two-parts test for order saving.

I am not sure how I could split this into smaller incremental changes, though. Any suggestion?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 29 • 13 years ago

Applied feedback.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 31 • 13 years ago

I forgot to add that I have not reverted DO_LOG to its previous state yet. I will do this once we are satisfied with the rest of the code, as it is much more useful than the previous version of DO_LOG for debugging.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 32 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #28)
> Well, I have no issue with higher-order programming, but I had the
> impression that |for each| was the recommended technique. Why do you suggest
> |array.forEach|?

I don't really know what this has to do with "higher-order programming". Array.forEach only iterates over "own properties", and avoids getting tripped up by additions to e.g. Array.prototype that can sometime occur:
Array.prototype.foo = "bar";
for each (a in []) print(a);

It's true that we don't follow this guideline consistently - that mostly isn't an issue in practice because it's rare for people to muck with Array.prototype in our chrome code, but it's still a pattern to be avoided in new code.

> I suppose that I could. However, I have the feeling that it may be a bit
> overkill for a function that will be executed exactly once, at first
> startup..

We're trying to eradicate all users of the synchronous storage API. Using the async version here isn't difficult, so we should.

> I am not sure how I could split this into smaller incremental changes,
> though. Any suggestion?

You could split up this in to two major pieces, at least:
1) making the search service initialization async (without touching engineMetaDataService)
2) modifications to engineMetaDataService

That's probably not worth it at this point since I've looked at the entire patch already anyways.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 33 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #32)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #28)
> > Well, I have no issue with higher-order programming, but I had the
> > impression that |for each| was the recommended technique. Why do you suggest
> > |array.forEach|?
> 
> I don't really know what this has to do with "higher-order programming".
> Array.forEach only iterates over "own properties", and avoids getting
> tripped up by additions to e.g.

My bad, I will fix this.

> > I suppose that I could. However, I have the feeling that it may be a bit
> > overkill for a function that will be executed exactly once, at first
> > startup.
> 
> We're trying to eradicate all users of the synchronous storage API. Using
> the async version here isn't difficult, so we should.

Ok.

> > I am not sure how I could split this into smaller incremental changes,
> > though. Any suggestion?
> 
> You could split up this in to two major pieces, at least:
> 1) making the search service initialization async (without touching
> engineMetaDataService)
> 2) modifications to engineMetaDataService
> 
> That's probably not worth it at this point since I've looked at the entire
> patch already anyways.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 34 • 13 years ago

Fixes applied.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 35 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #27)
> Comment on attachment 577987 [details] [diff] [review]
> The Client.
> 
> This is surprisingly simple! (nit: you should re-indent the observer to
> avoid the weird double-})
> 
> I think you'll need to update all the other users of nsIBrowserSearchService
> as well, though, since it's not OK to rely on the search bar initializing
> the search service (the search bar is removable, for example).
> 
> This has the potential to break addons that make use of the search service,
> but if we try to guarantee that the search service is initialized early
> enough (which should be true for users with the search bar enabled, at the
> very least), we will probably avoid most of the damage.

Indeed, I am aware of the add-on breakability issue. I see three possibilities:
1. add some form of guard to cleanly kill API clients that do not use the new API;
2. add a real fallback synchronous initializer that is triggered automatically whenever we call any of the synchronous functions before having called the asynchronous initializer (this is what I have done in the first version of the patch);
3. add a fake fallback synchronous initializer, triggered in the same conditions, using the asynchronous initializer and a nested event loop to wait until the async initializer has run to completion.

None of the 3 is fully satisfactory, but I tend to believe that 2. is the best. What do you think>

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 37 • 13 years ago

Comment on attachment 590177 [details] [diff] [review]
Use JSON instead of mozStorage

Splitting the task even further, as discussed by e-mail. Here's a patch that simply replaces mozStorage with JSON storage.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 39 • 13 years ago

As per our conversation by e-mail, here is a shallow patch that makes the API asynchronous without actually changing its implementation. It depends on bug JSScheduleAPI.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 41 • 13 years ago

Not sure who this belongs to. Gavin, perhaps you could suggest someone to review this patch?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 42 • 13 years ago

Not sure who this belongs to. Gavin, perhaps you could suggest someone to review this patch?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 46 • 13 years ago

Comment on attachment 591017 [details] [diff] [review]
Use JSON instead of mozStorage

This looks good overall as step 1, just a few issues:

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

> var engineMetadataService = {
>+  _mDB: null,

We've moved away from using the "m" prefix in new code (in favor of "_"), and avoiding "DB" would be nice - "_store"?

>   get mDB() {

>+    let store = FileUtils.getFile(NS_APP_USER_PROFILE_50_DIR,
>+                                  ["search-metadata.json"]);

I'd name this "jsonFile"

>   deleteEngineData: function epsDelData(engine, name) {

>+    let record = db[engine];
>+    if (!record) {
>+      delete record[name];

Looks like this check is wrong (should be if (record)).

>+  _migrateOldDB: function SRCH_SVC_EMS_migrate() {

>+    let dbService = Services.storage;

Don't need this temp variable, it's only used once.

>+  _commit: function epsCommit() {

>+     //Note: we are only commiting about 100-200 bytes, so no real need
>+     // to bufferize, background or optimize

I'm not sure this is true - the engine manager dialog can trigger many successive writes, I think, so I think it would be best to add a timer to coalesce subsequent commits.

This could also really use some tests. It should be doable to write some xpcshell tests for this - let me know if you want help with that.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 47 • 13 years ago

Comment on attachment 591036 [details] [diff] [review]
Asynchronous API (shallow changes)

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>+interface nsIObserver;

I think I mentioned this earlier, but I think you should add a new interface for this rather than re-using nsIObserver.

> interface nsIBrowserSearchService : nsISupports

>+  bool init([optional] in nsIObserver aObserver);

Let's not make this return bool, since the next step will be making initialization always-async, so there's no point.

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+Components.utils.import("resource://gre/modules/schedule.jsm");

What's the bug for this API?

>+Components.utils.import("resource:///modules/devtools/Promise.jsm");

I don't see this being used.

>+//Why exactly do we have a prototype for something that is a singleton?
> SearchService.prototype = {

Standard boilerplate. We could get rid of it but there isn't much benefit.

>+  init: function SRCH_SVC__init(aObserver) {
>+    let initialization = this._initialization;
>+    if (!initialization) {

I don't see this._initialization being set to null, where is that supposed to happen?

>   get currentEngine() {

>-    if (!this._currentEngine || this._currentEngine.hidden)
>+    if (!this._currentEngine || this._currentEngine.hidden) {
>       this._currentEngine = this.defaultEngine;
>+    }

omit this change

> var engineMetadataService = {

>   get mDB() {

>-      return this._mDB = this._migrateOldDB();
>+      this._mDB = this._migrateOldDB();
>+      this._commit();
>+      return this._mDB;

Couldn't _migrateOldDB just call _commit?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 48 • 13 years ago

It would probably be best to have this bug cover only part 1, and file followups for the other steps we talked about - it gets difficult to track multiple steps in a single bug.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 49 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> It would probably be best to have this bug cover only part 1, and file
> followups for the other steps we talked about - it gets difficult to track
> multiple steps in a single bug.

Done:

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 50 • 13 years ago

I meant: « Done: bug 699856 and bug 722332 »

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 51 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #46)
> Comment on attachment 591017 [details] [diff] [review]
> Use JSON instead of mozStorage
> 
> This looks good overall as step 1, just a few issues:
> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> > var engineMetadataService = {
> >+  _mDB: null,
> 
> We've moved away from using the "m" prefix in new code (in favor of "_"),
> and avoiding "DB" would be nice - "_store"?

Ok.

> >   get mDB() {
> 
> >+    let store = FileUtils.getFile(NS_APP_USER_PROFILE_50_DIR,
> >+                                  ["search-metadata.json"]);
> 
> I'd name this "jsonFile"

Ok.

> >   deleteEngineData: function epsDelData(engine, name) {
> 
> >+    let record = db[engine];
> >+    if (!record) {
> >+      delete record[name];
> 
> Looks like this check is wrong (should be if (record)).

Ah, you are right, thanks for the spot.

> 
> >+  _migrateOldDB: function SRCH_SVC_EMS_migrate() {
> 
> >+    let dbService = Services.storage;
> 
> Don't need this temp variable, it's only used once.

Fair enough.

> >+  _commit: function epsCommit() {
> 
> >+     //Note: we are only commiting about 100-200 bytes, so no real need
> >+     // to bufferize, background or optimize
> 
> I'm not sure this is true - the engine manager dialog can trigger many
> successive writes, I think, so I think it would be best to add a timer to
> coalesce subsequent commits.

Actually, I don't think that it can. Changing the order of engines, for instance, only triggers one write, during shutdown. Still, changed. If I have your approval on this chunk, I will factor the code between delayed commit of search-metadata.json and delayed commit of search.json .

> This could also really use some tests. It should be doable to write some
> xpcshell tests for this - let me know if you want help with that.

I will try and think of some tests, in addition to the ones I have already written (bug 458810, if you recall).

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 53 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #47)
> Comment on attachment 591036 [details] [diff] [review]
> Asynchronous API (shallow changes)

I will reply to this in the new bug.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 54 • 13 years ago

Comment on attachment 593758 [details] [diff] [review]
Use JSON instead of mozStorage

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+function Lazy(code, delay)

>+  let self = this;
>+  this._callback = {
>+    notify: function() {
>+      code();
>+      self._timer = null;
>+    }
>+  };

this._callback = (function () {
  func();
  this._timer = null;
}).bind(this);

>+  if (delay) {
>+    this._delay = delay;
>+  } else {
>+    this._delay = LAZY_SERIALIZE_DELAY;
>+  }

this._delay = delay || LAZY_SERIALIZE_DELAY;

>+Lazy.__defineGetter__(
>+  "_timerClass",

There's no need to make this a lazy getter. You could use Components.Constructor, but even that isn't necessary.

> var engineMetadataService = {

>+  get _store() {

>+      return this.__jsonFile = this._migrateOldDB();

Looks like this should be __store? We should have tests that cover every branch of this getter.

>   deleteEngineData: function epsDelData(engine, name) {

Doesn't this need to _commit?

>   closeDB: function epsCloseDB() {

Seems like this should be renamed. This looks like it wants to force an immediate write, since it occurs on quit, so if there are changes to commit you want to avoid the lazy in this case.

>+  _commit: function epsCommit() {

>+     if (!this._changes || !this.__store) {

Isn't it impossible for __store to be null?

>+  _writeCommit: function() {
>+    LOG("epsWriteCommit: start");
>+      let ofile = FileUtils.getFile(NS_APP_USER_PROFILE_50_DIR,
>+                                    ["search-metadata.json"]);

This file could be cached on the object.

>+    let ostream = FileUtils.
>+      openSafeFileOutputStream(ofile, 0x02 | 0x08 | 0x20);

Can you use named constants for this? I think they are defined already in this file.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 55 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #54)
> Comment on attachment 593758 [details] [diff] [review]
> Use JSON instead of mozStorage
> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+function Lazy(code, delay)
[...]

As you wish.

> >+  get _store() {
> 
> >+      return this.__jsonFile = this._migrateOldDB();
> 
> Looks like this should be __store? We should have tests that cover every
> branch of this getter.

Good catch. I'll try and write these tests.

> 
> >   deleteEngineData: function epsDelData(engine, name) {
> 
> Doesn't this need to _commit?

Difficult to be sure. As far as I can tell, this function is undocumented dead code, so writing it involves some guesswork. Let's say it does.

> >   closeDB: function epsCloseDB() {
> 
> Seems like this should be renamed. This looks like it wants to force an
> immediate write, since it occurs on quit, so if there are changes to commit
> you want to avoid the lazy in this case.

Good point, doing this.

> 
> >+  _commit: function epsCommit() {
> 
> >+     if (!this._changes || !this.__store) {
> 
> Isn't it impossible for __store to be null?

Actually, it could be null if closeDB was called before any call to the db getter. Which probably cannot happen, but as there is no protocol documentation to ensure that clients have to call that getter at least once, I figured that I might as well err on the safe side. Do you think I should do otherwise?

> 
> >+  _writeCommit: function() {
> >+    LOG("epsWriteCommit: start");
> >+      let ofile = FileUtils.getFile(NS_APP_USER_PROFILE_50_DIR,
> >+                                    ["search-metadata.json"]);
> 
> This file could be cached on the object.

Ok.

> >+    let ostream = FileUtils.
> >+      openSafeFileOutputStream(ofile, 0x02 | 0x08 | 0x20);
> 
> Can you use named constants for this? I think they are defined already in
> this file.

Good point.

Do I have your blessing to migrate |_lazySerializeToFile| to |Lazy|? Or should that go in a followup bug?

I upload a revised patch, which I'll mark r? once I have written the new tests.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 57 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #55)
> I figured that I might as well err on the safe side. Do you think I should
> do otherwise?

That's fine (but add a comment)

> Do I have your blessing to migrate |_lazySerializeToFile| to |Lazy|? Or
> should that go in a followup bug?

Followup bug.

> I upload a revised patch, which I'll mark r? once I have written the new
> tests.

Thanks!

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 60 • 13 years ago

Comment on attachment 594521 [details] [diff] [review]
Use JSON instead of mozStorage

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+__defineGetter__("Services", function() {

No need to make this a lazy getter, just import it directly (it's going to already be loaded anyways).

>+    LOG("_loadEngines: getting nsIXULAppInfo");

Not sure why this is useful, but if it somehow is, put it right before the actual call to nsIXULAppInfo.

> var engineMetadataService = {

>+  __jsonFile: null,

don't need this anymore.

>+  _changes: false,

It looks like _commit only ever gets called when this has been set to true, so is there any reason to keep it? I.e. can we just have _commit always commit.

>+  get _store() {

Can you make this a lazy getter?

get _store() {
  delete this._store;
  return this._store = this._loadStore();
}

That should also allow you to get rid of __store.

We should be trying to avoid creating the JSON file if there's nothing to save. This might not be possible at the moment since we might be always saving stuff, but I think it might be possible with a few tweaks, and it would be a nice win to avoid IO (even if we eventually make it async).

>+  _setAttr: function epsSetAttr(db, engine, name, value) {

This function is only ever passed this._store as "db", so you don't need the argument.

>   deleteEngineData: function epsDelData(engine, name) {

>+    let record = db[engine];

Hmm, doesn't this need to be using engine._id? You can just not add this method until we make use of it (bug 341833).

>+  _commit: function epsCommit() {
>+    LOG("epsCommit: start");
>+     if (!this._changes || !this.__store) {

Indentation looks off here.

This is close to landing. Let me know if you want me to pick this up and make these adjustments.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 61 • 13 years ago

Hmm, I think we should probably also delete search.sqlite on successful migration... though I guess that will mess up downgrades. How tolerant are we to a loss of all search metadata?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 62 • 13 years ago

Comment on attachment 594522 [details] [diff] [review]
Companion testsuite

>diff --git a/toolkit/components/search/tests/xpcshell/test_645970.js.rej b/toolkit/components/search/tests/xpcshell/test_645970.js.rej

Think you probably don't want to add this :)

>diff --git a/toolkit/components/search/tests/xpcshell/test_migratedb.js b/toolkit/components/search/tests/xpcshell/test_migratedb.js

>+//Call this function at start and at end
>+function run_test()

Not sure what this comment means.

>+  do_timeout(500,

This looks fragile. Why does this need a 500ms timeout - because of the Lazy()? You could add an observer service notification for this to avoid needing the timer.

>\ No newline at end of file

nit: fix this! :)

>diff --git a/toolkit/components/search/tests/xpcshell/test_nodb.js b/toolkit/components/search/tests/xpcshell/test_nodb.js

>+//Testing when search.sqlite exists but no search-metadata.json

Hmm, isn't that what test_migratedb does? This test seems to test the blank-profile case (neither JSON nor SQLite present).

>diff --git a/toolkit/components/search/tests/xpcshell/test_regulardb.js b/toolkit/components/search/tests/xpcshell/test_regulardb.js

>+//Testing when both search.sqlite and search-metadata.json exist

Hmm, this test doesn't seem that useful (just testing that a file that you copied at the start of the test is still around at the end). Is the idea to eventually make this actually test that the right data was used (JSON instead of SQLite)?

Seems like the tests should be expanded to cover ensuring that the right data was migrated, that the right data is used, and that data is saved correctly.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 63 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #60)
> Comment on attachment 594521 [details] [diff] [review]
> Use JSON instead of mozStorage
> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+__defineGetter__("Services", function() {
> 
> No need to make this a lazy getter, just import it directly (it's going to
> already be loaded anyways).

Ok.

> >+    LOG("_loadEngines: getting nsIXULAppInfo");
> 
> Not sure why this is useful, but if it somehow is, put it right before the
> actual call to nsIXULAppInfo.

No, you're right, I should remove it.
> > var engineMetadataService = {
> 
> >+  __jsonFile: null,
> 
> don't need this anymore.

True.

> >+  _changes: false,
> 
> It looks like _commit only ever gets called when this has been set to true,
> so is there any reason to keep it? I.e. can we just have _commit always
> commit.

Actually, that is not quite true: there are several paths in which |_commit| may be called with |_changes| holding |false|:
- changes to engine metadata by |setAttr| or |setAttrs| may be noop;
- the final flush may not have anything to flush.

> >+  get _store() {
> 
> Can you make this a lazy getter?
> 
> get _store() {
>   delete this._store;
>   return this._store = this._loadStore();
> }
> 
> That should also allow you to get rid of __store.

Ok.

> We should be trying to avoid creating the JSON file if there's nothing to
> save. This might not be possible at the moment since we might be always
> saving stuff, but I think it might be possible with a few tweaks, and it
> would be a nice win to avoid IO (even if we eventually make it async).

Well, that's one of the reasons for |_changes|. I have just added one more tweak to not write if the .sqlite file does not exist, the .json file does not exist and we have made no change.



> >+  _setAttr: function epsSetAttr(db, engine, name, value) {
> 
> This function is only ever passed this._store as "db", so you don't need the
> argument.
> 
> >   deleteEngineData: function epsDelData(engine, name) {
> 
> >+    let record = db[engine];
> 
> Hmm, doesn't this need to be using engine._id? You can just not add this
> method until we make use of it (bug 341833).

Ok. Sending it back to oblivion :)

> >+  _commit: function epsCommit() {
> >+    LOG("epsCommit: start");
> >+     if (!this._changes || !this.__store) {
> 
> Indentation looks off here.

Thanks.

> This is close to landing. Let me know if you want me to pick this up and
> make these adjustments.

No thanks, it's a matter of personal pride :)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 64 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #62)
> Comment on attachment 594522 [details] [diff] [review]
> Companion testsuite
> 
> >diff --git a/toolkit/components/search/tests/xpcshell/test_645970.js.rej b/toolkit/components/search/tests/xpcshell/test_645970.js.rej
> 
> Think you probably don't want to add this :)

Oops :)


> >diff --git a/toolkit/components/search/tests/xpcshell/test_migratedb.js b/toolkit/components/search/tests/xpcshell/test_migratedb.js
> 
> >+//Call this function at start and at end
> >+function run_test()
> 
> Not sure what this comment means.
Yeah, the comment should have been on the next line.

> 
> >+  do_timeout(500,
> 
> This looks fragile. Why does this need a 500ms timeout - because of the
> Lazy()? You could add an observer service notification for this to avoid
> needing the timer.

I think it may be overkill to add yet another observer service notification just for the sake of testing. Or perhaps only if we are in DEBUG mode?

> >\ No newline at end of file
> 
> nit: fix this! :)

Thanks.

> >diff --git a/toolkit/components/search/tests/xpcshell/test_nodb.js b/toolkit/components/search/tests/xpcshell/test_nodb.js
> 
> >+//Testing when search.sqlite exists but no search-metadata.json
> 
> Hmm, isn't that what test_migratedb does? This test seems to test the
> blank-profile case (neither JSON nor SQLite present).

Obviously, writing comments at 2am does not seem to be my best skill. I'll just rewrite all the comments.

> 
> >diff --git a/toolkit/components/search/tests/xpcshell/test_regulardb.js b/toolkit/components/search/tests/xpcshell/test_regulardb.js
> 
> >+//Testing when both search.sqlite and search-metadata.json exist
> 
> Hmm, this test doesn't seem that useful (just testing that a file that you
> copied at the start of the test is still around at the end). Is the idea to
> eventually make this actually test that the right data was used (JSON
> instead of SQLite)?
>
> Seems like the tests should be expanded to cover ensuring that the right
> data was migrated, that the right data is used, and that data is saved
> correctly.

Yes, my latest iteration on the testsuite expands this.

Migrating this conversation (and the patch) to bug 458810.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 66 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #64)
> Migrating this conversation (and the patch) to bug 458810.

Actually, on second thought, this is a bad idea. Let's keep conversation and testsuite patch here as they are both specific to the json version.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 69 • 13 years ago

Is there a reason you haven't requested review again?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 70 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #63)
> Actually, that is not quite true: there are several paths in which |_commit|
> may be called with |_changes| holding |false|:
> - changes to engine metadata by |setAttr| or |setAttrs| may be noop;

Seems like this case should just avoid calling commit (it would be easy to make _setAttr return bool, and let setAttr/setAttrs use that to determine whether they need to commit).

> - the final flush may not have anything to flush.

For this, you only need to be able to tell whether there's an already pending commit - you could do that by introducing a Lazy.isPending, or by just setting _lazyWriter to null in _writeCommit.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 71 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #64)
> I think it may be overkill to add yet another observer service notification
> just for the sake of testing. Or perhaps only if we are in DEBUG mode?

I disagree - it's the only reliable way to do the testing, and there isn't really any downside (chances of it being abused are rather low).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 72 • 13 years ago

I filed bug 726232 about the only really sub-optimal case of losing all engine meta data that I could find. For the moment, let's just avoid deleting search.sqlite (no worse than bug 341833), and file a followup to delete it at some later point.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 73 • 13 years ago

Comment on attachment 595766 [details] [diff] [review]
Use JSON instead of mozStorage

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+Components.utils.import("resource://gre/modules/Services.jsm");

Might be a good idea to file a followup on making wider use of this in this file.

>+ * @param {number=} delay An optional delay.

...in milliseconds

>+function Lazy(code, delay)
>+{

nit: I would prefer if you stuck to {-on-same-line style in this file (and in general), but you don't need to change all of these if you don't want to.

>+    this._callback.call(null);

you can just call it directly rather than using .call(), fwiw, since the function is bound.

> var engineMetadataService = {

>+  _loadStore: function() {

>+      LOG("loadStore cannot not parse file: "+x);// File may simply not exist

The exists() check earlier would catch that case, right? Looks like you should just remove the comment.

>+  getAttr: function epsGetAttr(engine, name) {

I thought about this earlier but forgot to mention it: since we're just a plain JS object, e.g. getAttr(engine, "toSource") will return unexpected results. To avoid this, _store can be a Dict.jsm (which can be constructed from an object). You can do this in a followup if you want.

With these and all my other comments addressed, I think this is ready to land, but I'll want to take another look at the final patch.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 75 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #73)
> Comment on attachment 595766 [details] [diff] [review]
> Use JSON instead of mozStorage
> 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+Components.utils.import("resource://gre/modules/Services.jsm");
> 
> Might be a good idea to file a followup on making wider use of this in this
> file.

Filed as bug 726279.

> >+  getAttr: function epsGetAttr(engine, name) {
> 
> I thought about this earlier but forgot to mention it: since we're just a
> plain JS object, e.g. getAttr(engine, "toSource") will return unexpected
> results. To avoid this, _store can be a Dict.jsm (which can be constructed
> from an object). You can do this in a followup if you want.

Ah, right. Filed as bug 727555.

> With these and all my other comments addressed, I think this is ready to
> land, but I'll want to take another look at the final patch.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #71)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #64)
> > I think it may be overkill to add yet another observer service notification
> > just for the sake of testing. Or perhaps only if we are in DEBUG mode?
> 
> I disagree - it's the only reliable way to do the testing, and there isn't
> really any downside (chances of it being abused are rather low).

Well, my inner concurrent programmer tends to scream at every use of a single point of failure/contention/synchronization, but I admit that the cost is negligible in that case.

Anyway, attaching updated patches.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 78 • 13 years ago

Comment on attachment 597602 [details] [diff] [review]
Companion testsuite

>diff --git a/toolkit/components/search/tests/xpcshell/data/engine.xml b/toolkit/components/search/tests/xpcshell/data/engine.xml

You should probably use a generic search plugin rather than just copying the Google plugin.

>diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js

>+  let obs = {
>+    observe: function(result, topic, verb) {

function obs(result, topic, verb) {
}

This occurs in several of the tests too.

>+  do_timeout(1000, function() {
>+      do_throw("Timeout");
>+    });

This shouldn't be necessary (the test harness will deal with timeouts - this also applies to some of the tests).

>diff --git a/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js b/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js

>+/* ***** BEGIN LICENSE BLOCK *****

We use the public domain license header for tests, generally.

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+Components.utils.import("resource://gre/modules/NetUtil.jsm");
>+Components.utils.import("resource://gre/modules/Services.jsm");

some of these are already loaded in head.js aren't they (this comment also applies to some other tests)?

>+var gPrefService = Services.prefs;
>+var IOService = Services.io;

These don't seem useful (they're not really shorter).

>+function run_test() {

>+  // The search service needs to be started after the jarURIs pref has been
>+  // set in order to initiate it correctly

Don't understand this comment - is it just copied from somewhere else?

>+  let searchService = Cc["@mozilla.org/browser/search-service;1"]
>+                       .getService(Ci.nsIBrowserSearchService);
>+  var observerService = Cc["@mozilla.org/observer-service;1"]
>+    .getService(Ci.nsIObserverService);

You can use the Services.* variants.

>diff --git a/toolkit/components/search/tests/xpcshell/test_migratedb.js b/toolkit/components/search/tests/xpcshell/test_migratedb.js

>+Services.prefs.setBoolPref("browser.search.useDBForOrder", true);

Why is this needed?

>diff --git a/toolkit/components/search/tests/xpcshell/test_nodb.js b/toolkit/components/search/tests/xpcshell/test_nodb.js

>+  do_timeout(500,
>+             function()
>+             {
>+               //Check that search-metadata.json has been created

Comment is wrong :) Using a timeout like this isn't ideal, but there really isn't any better way to test that something didn't happen, and the only downside is a false-pass (as opposed to a false-fail).

>diff --git a/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js b/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js

This is a good test!

>+function dumpn(aText) {

already defined in head.js

>\ No newline at end of file

(fix this in this file and others)

>diff --git a/toolkit/components/search/tests/xpcshell/test_regulardb.js b/toolkit/components/search/tests/xpcshell/test_regulardb.js

>+Services.prefs.setBoolPref("browser.search.log", true);

This is also set in head.js

>+function run_test()

>+      //Check that search-metadata.json has been created

This doesn't match the description of the test above (overall this test doesn't seem useful in its current state)

>diff --git a/toolkit/components/search/tests/xpcshell/test_sortEngines.braindump b/toolkit/components/search/tests/xpcshell/test_sortEngines.braindump

>+  // 1. Charger les engins
>+  // 2. Inverser l'ordre
>+  // 3. Sauvegarder l'ordre dans un fichier

Il me semble que ceci est deja vérifié dans test_nodb_pluschanges.js - pas besoin de commetre ce fichier :)

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 79 • 13 years ago

Comment on attachment 597603 [details] [diff] [review]
Use JSON instead of mozStorage

Thanks for your work on this, and I'm sorry that the review process was far from ideal. Bad combination of me being too picky and not having enough time, and for that I apologize. I implemented my review suggestions in the form of a patch on top of your changes - please consider integrating these into your patch. r+ with that.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 80 • 13 years ago

I realized in writing this that some of my review comments led you in the wrong direction, because I didn't fully understand the problem when making suggestions.

	Marco Bonardo [:mak] Reporter
	Comment 81 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #78)
> We use the public domain license header for tests, generally.

Mostly because MPL was too verbose, MPL2 would also be fine fwiw.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 82 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #78)
> Comment on attachment 597602 [details] [diff] [review]
> Companion testsuite
> 
> >diff --git a/toolkit/components/search/tests/xpcshell/data/engine.xml b/toolkit/components/search/tests/xpcshell/data/engine.xml
> 
> You should probably use a generic search plugin rather than just copying the
> Google plugin.

I wanted some "realistic" plug-in, for the test, with the full complement of arguments and an icon. I am sure that I could change the icon and the arguments, if necessary. Is there a strong reason to do that?


> >+/* ***** BEGIN LICENSE BLOCK *****
> 
> We use the public domain license header for tests, generally.

Actually, I have seen both in tests. But let's go for public domain.
 
> >+function run_test() {
> 
> >+  // The search service needs to be started after the jarURIs pref has been
> >+  // set in order to initiate it correctly
> 
> Don't understand this comment - is it just copied from somewhere else?

Indeed, my bad.

> >diff --git a/toolkit/components/search/tests/xpcshell/test_migratedb.js b/toolkit/components/search/tests/xpcshell/test_migratedb.js
> 
> >+Services.prefs.setBoolPref("browser.search.useDBForOrder", true);
> 
> Why is this needed?

Ah, right, this is probably not needed anymore.

> 
> >diff --git a/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js b/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js
> 
> This is a good test!

Thanks :)

> 
> >diff --git a/toolkit/components/search/tests/xpcshell/test_regulardb.js b/toolkit/components/search/tests/xpcshell/test_regulardb.js
> 
> >+Services.prefs.setBoolPref("browser.search.log", true);
> 
> This is also set in head.js
> 
> >+function run_test()
> 
> >+      //Check that search-metadata.json has been created
> 
> This doesn't match the description of the test above (overall this test
> doesn't seem useful in its current state)

Indeed, it is probably not very useful so far. I suppose that I could add the same kind of tests as in test_nodb_pluschanges.js, but I am not sure that this is very interesting. What do you think? Otherwise, I can just remove that test.

> >diff --git a/toolkit/components/search/tests/xpcshell/test_sortEngines.braindump b/toolkit/components/search/tests/xpcshell/test_sortEngines.braindump
> 
> >+  // 1. Charger les engins
> >+  // 2. Inverser l'ordre
> >+  // 3. Sauvegarder l'ordre dans un fichier
> 
> Il me semble que ceci est deja vérifié dans test_nodb_pluschanges.js - pas
> besoin de commetre ce fichier :)

Oops :)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 84 • 13 years ago

Comment on attachment 599445 [details] [diff] [review]
additional changes

Review of attachment 599445 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ -379,5 @@
> - * Sent each time metadata is fully written to disk. The subject is
> - * a nsIVariant holding the nsresult of the write.
> - */
> -#define SEARCH_SERVICE_METADATA_WRITTEN "write-metadata-to-disk-complete";
> -

You remove these constants from the idl but the literal strings are still used in the JavaScript. Does this mean that they become an hidden API only meant to be used for testing?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 85 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #80)
> Created attachment 599445 [details] [diff] [review]
> additional changes
> 
> I realized in writing this that some of my review comments led you in the
> wrong direction, because I didn't fully understand the problem when making
> suggestions.

No worry. Finally getting a r+ for this bug feels like a victory earned in blood, sweat and code :)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 86 • 13 years ago

Same patch, but applying the suggestions also to the preexisting test.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 87 • 13 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #84)
> You remove these constants from the idl but the literal strings are still
> used in the JavaScript. Does this mean that they become an hidden API only
> meant to be used for testing?

Yes - there's no reason to advertise this particular notification.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 88 • 13 years ago

sorry I missed this comment earlier...

(In reply to David Rajchenbach Teller [:Yoric] from comment #82)
> I wanted some "realistic" plug-in, for the test, with the full complement of
> arguments and an icon. I am sure that I could change the icon and the
> arguments, if necessary. Is there a strong reason to do that?

It just seems valuable to have the plugin be very obviously a generic test plugin, rather than a copy of the Google plugin, just to reduce potential confusion down the road (do we need to update this plugin too when we're updating the google one? etc.). Not a big deal either way.

> Actually, I have seen both in tests. But let's go for public domain.

Some tests predate the "use PD for tests" policy. But as Marco notes, the main goal was a simplified header for non-product code, and MPL2 solves that problem anyways.

> Indeed, it is probably not very useful so far. I suppose that I could add
> the same kind of tests as in test_nodb_pluschanges.js, but I am not sure
> that this is very interesting. What do you think? Otherwise, I can just
> remove that test.

If it's redundant now, just remove it.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 89 • 13 years ago

Comment on attachment 599605 [details] [diff] [review]
Companion testsuite

>diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js

>-/* ***** BEGIN LICENSE BLOCK *****
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */

You can't re-license existing files like this without permission from the copyright holders - just leave this as is.

>diff --git a/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js b/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js

Isn't this test redundant, given test_nodb_pluschanges?

I don't understand why the "getResponse()" functions are present in both of these tests, they don't seem to be used.

>diff --git a/toolkit/components/search/tests/xpcshell/test_regulardb.js b/toolkit/components/search/tests/xpcshell/test_regulardb.js

>+ * Ensure that search-metadata.json is used.

This test doesn't actually do this, so let's not add it until it does (we should have a test that ensures this properly).

Otherwise this looks fine. I think this is ready to land once you remove the incomplete/redundant tests.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 90 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #89)
> Comment on attachment 599605 [details] [diff] [review]
> Companion testsuite
> 
> >diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js
> 
> >-/* ***** BEGIN LICENSE BLOCK *****
> >+/* Any copyright is dedicated to the Public Domain.
> >+   http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> You can't re-license existing files like this without permission from the
> copyright holders - just leave this as is.

Sorry about that.

> >diff --git a/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js b/toolkit/components/search/tests/xpcshell/test_downloadAndAddEngine.js
> 
> Isn't this test redundant, given test_nodb_pluschanges?

You are right, now, it is. Removing.

> I don't understand why the "getResponse()" functions are present in both of
> these tests, they don't seem to be used.

You are absolutely right, this is dead code. Removed.

> Otherwise this looks fine. I think this is ready to land once you remove the
> incomplete/redundant tests.

\o/

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 93 • 13 years ago

Comment on attachment 601957 [details] [diff] [review]
Companion testsuite

>diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js

>-/* ***** BEGIN LICENSE BLOCK *****

Don't remove this license block.

>diff --git a/toolkit/components/search/tests/xpcshell/data/search-metadata.json b/toolkit/components/search/tests/xpcshell/data/search-metadata.json

This file seems to be unused now.

>diff --git a/toolkit/components/search/tests/xpcshell/test_migratedb.js b/toolkit/components/search/tests/xpcshell/test_migratedb.js

We should eventually extend this test to check that the migration of the SQLite data was correct.

	Dão Gottwald [:dao]
	Comment 94 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/fae3603a8d1d

	Phil Ringnalda (:philor)
	Comment 95 • 13 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/66426f4a1897 - test_nodb_pluschanges.js hangs on Windows.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 96 • 13 years ago

Surprising. I will investigate.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 97 • 13 years ago

Ok, I have tracked the issue to files that remained open until arbage-collection, which prevented the test clean-up routines from working under Windows.

	Dão Gottwald [:dao]
	Comment 100 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/7df9f3c93402

	Ed Morley [:emorley]
	Comment 101 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/7df9f3c93402