Last Comment Bug 760035 - nsIBrowserSearchService asynchronous clients
: nsIBrowserSearchService asynchronous clients
Status: RESOLVED FIXED
: main-thread-io
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Firefox 16
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on: 722332
Blocks: 753157 765210
  Show dependency treegraph
 
Reported: 2012-05-31 03:14 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-06-28 01:08 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First prototype (11.30 KB, patch)
2012-05-31 06:23 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Adapting main clients to async API (6.00 KB, patch)
2012-06-11 03:35 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: review-
Details | Diff | Splinter Review
Attempting to adapt mobile Firefox (1.54 KB, patch)
2012-06-11 03:38 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: feedback-
Details | Diff | Splinter Review
Adapting main clients to async API (5.85 KB, patch)
2012-06-12 23:00 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: review+
Details | Diff | Splinter Review
Adapting main clients to async API (mobile FF) (5.49 KB, patch)
2012-06-12 23:14 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
gavin.sharp: feedback+
Details | Diff | Splinter Review
Adapting main clients to async API (mobile FF) (6.32 KB, patch)
2012-06-13 12:48 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
mbrubeck: review+
Details | Diff | Splinter Review
Adapting main clients to async API (6.05 KB, patch)
2012-06-13 12:49 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Adapting main clients to async API (mobile FF) (6.39 KB, patch)
2012-06-20 05:51 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Adapting main clients to async API (mobile FF) (6.27 KB, patch)
2012-06-20 06:14 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Adapting main clients to async API (4.01 KB, patch)
2012-06-26 01:01 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-31 03:14:19 PDT
Bug 722332 makes nsIBrowserSearchService API async, with a synchronous fallback. This bug is about adapting the clients of that API to ensure that they take advantage of the async path, rather than of the synchronous one.

Whether we wish to adapt all clients or only those who are effectively executed first remains an open question.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-31 06:23:57 PDT
Created attachment 628696 [details] [diff] [review]
First prototype
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 03:35:07 PDT
Created attachment 631844 [details] [diff] [review]
Adapting main clients to async API

Here we go. For the moment, the patch converts:
- search.xml (as experience showed that it is sufficient in practice for Firefox with default options);
- nsSearchSuggestion.js (because it is quite simple to do without loss of performance).
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 03:38:27 PDT
Created attachment 631847 [details] [diff] [review]
Attempting to adapt mobile Firefox

This is a bit of guesswork, but this patch could be sufficient for mobile Firefox.
Comment 4 Dão Gottwald [:dao] 2012-06-11 04:02:43 PDT
Comment on attachment 631844 [details] [diff] [review]
Adapting main clients to async API

>+              Components.reportError("Cannot initialize search service, bailing out.");

Components.utils.reportError
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-11 04:06:25 PDT
(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 631844 [details] [diff] [review]
> Adapting main clients to async API
> 
> >+              Components.reportError("Cannot initialize search service, bailing out.");
> 
> Components.utils.reportError

Oh, is Components.reportError deprecated?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-11 16:11:29 PDT
Comment on attachment 631844 [details] [diff] [review]
Adapting main clients to async API

Thanks, this looks good overall.

>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

>         setTimeout(function (a) { a.init(); }, 0, this);

I think we'll want to remove this delay in calling init() (or just move its contents to the constructor), now that search service initialization doesn't occur synchronously under it.

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

>   /**
>+   * Lazy getter for the search service.

Rather than introducing this, just import Services.jsm and use Services.search. You can then also file a followup to make use of it further in this file.

>-    var self = this;
>-    function onReadyStateChange() {
>-      self.onReadyStateChange();
>-    }
>-    this._request.onreadystatechange = onReadyStateChange;
>+    this._request.onreadystatechange = this.onReadyStateChange;

I vaguely recall discussing this change before in another bug. I don't see how this can work - onReadyStateChange depends on its "this" being correct, and AFAICT it won't be with this change (are you missing a call to "bind"?). In any case it doesn't seem related to this bug.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-11 16:21:06 PDT
Comment on attachment 631847 [details] [diff] [review]
Attempting to adapt mobile Firefox

Don't you need to protect against SearchEngines:Get being sent before initialization is complete? I don't know how likely that is to occur, but if we're just going to rely on the sync fallback there's no point in adding an init() call. #mobile tells me that SearchEngines:Data message is processed asynchronously anyways, so you can wait for initialization to complete before sending it.
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-12 22:59:34 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> Comment on attachment 631844 [details] [diff] [review]
> Adapting main clients to async API
> 
> Thanks, this looks good overall.
> 
> >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml
> 
> >         setTimeout(function (a) { a.init(); }, 0, this);
> 
> I think we'll want to remove this delay in calling init() (or just move its
> contents to the constructor), now that search service initialization doesn't
> occur synchronously under it.

Done.
There is a second setTimeout at line 524, by the way, which may also deserve some attention. I am not familiar with that part of the code, though, so I will not take the initiative to touch it for the moment.

> 
> >diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js
> 
> >   /**
> >+   * Lazy getter for the search service.
> 
> Rather than introducing this, just import Services.jsm and use
> Services.search. You can then also file a followup to make use of it further
> in this file.

Done: bug 764270.

> >-    var self = this;
> >-    function onReadyStateChange() {
> >-      self.onReadyStateChange();
> >-    }
> >-    this._request.onreadystatechange = onReadyStateChange;
> >+    this._request.onreadystatechange = this.onReadyStateChange;
> 
> I vaguely recall discussing this change before in another bug. I don't see
> how this can work - onReadyStateChange depends on its "this" being correct,
> and AFAICT it won't be with this change (are you missing a call to "bind"?).
> In any case it doesn't seem related to this bug.

Ah, my bad. Nine months of rollbacks and rollfowards are not good for sanity, I guess.
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-12 23:00:31 PDT
Created attachment 632559 [details] [diff] [review]
Adapting main clients to async API
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-12 23:13:58 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Comment on attachment 631847 [details] [diff] [review]
> Attempting to adapt mobile Firefox
> 
> Don't you need to protect against SearchEngines:Get being sent before
> initialization is complete? I don't know how likely that is to occur, but if
> we're just going to rely on the sync fallback there's no point in adding an
> init() call. #mobile tells me that SearchEngines:Data message is processed
> asynchronously anyways, so you can wait for initialization to complete
> before sending it.


From what I understand, "SearchEngines:Get" is sent only when the user (first?) opens the AwesomeBar, which should buy us time in the most common scenario.

However, I have just made the change you suggest, as well as the corresponding initialization for addEngine.
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-12 23:14:45 PDT
Created attachment 632562 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-13 12:18:33 PDT
Comment on attachment 632559 [details] [diff] [review]
Adapting main clients to async API

>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

>       <constructor><![CDATA[

>-        setTimeout(function (a) { a.init(); }, 0, this);
>+        this.init();

Just get rid of init() entirely, and put its code into the <constructor>.

>+          this.searchService.init((function search_init_cb(aStatus) {
>+            if (Components.isSuccessCode(aStatus)) {
>+              // Refresh the display (updating icon, etc)
>+              this.updateDisplay();
>+            } else {
>+              Components.utils.reportError("Cannot initialize search service, bailing out.");

Append "aStatus" to the message? This comment also applies to the nsSearchSuggestions code.

r=me with those changes.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-13 12:21:44 PDT
Comment on attachment 632562 [details] [diff] [review]
Adapting main clients to async API (mobile FF)

Looks good to me. Might want to bind this._handleSearchEnginesGet to "this" when passing it to init(), even though it's not currently necessary, just to avoid potential confusion down the line.
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-13 12:48:19 PDT
Created attachment 632820 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-13 12:49:29 PDT
Created attachment 632821 [details] [diff] [review]
Adapting main clients to async API

Done
Comment 16 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-20 05:51:50 PDT
Created attachment 634865 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Comment 17 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-20 06:14:59 PDT
Created attachment 634868 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-06-25 08:35:39 PDT
Needs un-bitrotting.
Comment 19 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-26 01:01:55 PDT
Created attachment 636623 [details] [diff] [review]
Adapting main clients to async API

Here it is, unbitrotten.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-06-27 17:21:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3af99c8297 (*sigh* the commit message pointed to bug 722332 instead of this one...)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3c97022901
Comment 21 Ed Morley [:emorley] 2012-06-28 01:08:00 PDT
https://hg.mozilla.org/mozilla-central/rev/ca3c97022901

Note You need to log in before you can comment on or make changes to this bug.