699856 - (jsonSearchSvc) Refactor nsSearchService.js to not use a database engine

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Marco Bonardo [:mak]]

nsSearchService.js should stop using synchronous Storage APIs.

Comment 1

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

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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

Comment 5

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

(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.

Comment 6

[(dormant account)]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 7

[Dietrich Ayala (:dietrich)]

(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?

Comment 8

[(dormant account)]

(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

Comment 9

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

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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Early preview

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.

Comment 12

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

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

This looks like the wrong patch.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Early preview (fixed)

My bad about that. This is the early preview.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Early preview

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

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Async version (contains debugging code)

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.

Comment 17

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

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)

Comment 18

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

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

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

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

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: The Client.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

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

Cancelling review, I have found a bug.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

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

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

Bug fixed.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Review ping?

Comment 26

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

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 <[e-mail]> (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.

Comment 27

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

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.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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 <[e-mail]> (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?

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

Applied feedback.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 32

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

(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.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Attachment: The API.

Fixes applied.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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>

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 37

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous API (shallow changes)

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.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: search.xml

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: nsContextMenu.js

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

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: nsSafeMode.js

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

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: nsBrowserContentHandler.js

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: nsDefaultURIFixup.cpp

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous clients: nsSearchSuggest.js

Comment 46

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

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.

Comment 47

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

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?

Comment 48

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

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.

Comment 49

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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:

Comment 50

[David Teller [:Yoric] - still alive but not very active]

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

Comment 51

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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).

Comment 52

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 53

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 54

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

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.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 57

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

(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!

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 59

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 60

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

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.

Comment 61

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

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?

Comment 62

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

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.

Comment 63

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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 :)

Comment 64

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 65

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 66

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 67

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 68

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 69

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

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

Comment 70

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

(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.

Comment 71

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

(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).

Comment 72

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

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.

Comment 73

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

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.

Comment 74

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 75

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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 [e-mail] 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.

Comment 76

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 77

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 78

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

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 :)

Comment 79

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

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.

Comment 80

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

Attachment: 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.

Comment 81

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 82

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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 :)

Comment 83

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 84

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 85

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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 :)

Comment 86

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

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

Comment 87

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

(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.

Comment 88

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

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.

Comment 89

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

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.

Comment 90

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gavin Sharp (use [e-mail] 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/

Comment 91

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 92

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

Comment 93

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

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.

Comment 94

[Dão Gottwald [:dao]]

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

Comment 95

[Phil Ringnalda (:philor)]

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

Comment 96

[David Teller [:Yoric] - still alive but not very active]

Surprising. I will investigate.

Comment 97

[David Teller [:Yoric] - still alive but not very active]

Attachment: Use JSON instead of mozStorage

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.

Comment 98

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 99

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 100

[Dão Gottwald [:dao]]

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

Comment 101

[Ed Morley [:emorley]]

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