nsIBrowserSearchService asynchronous clients

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Search
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({main-thread-io})

unspecified
Firefox 16
main-thread-io
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

6.27 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
4.01 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
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.
Created attachment 628696 [details] [diff] [review]
First prototype
Blocks: 753157
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).
Assignee: nobody → dteller
Attachment #628696 - Attachment is obsolete: true
Attachment #631844 - Flags: review?(gavin.sharp)
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.
Attachment #631847 - Flags: feedback?(gavin.sharp)
Comment on attachment 631844 [details] [diff] [review]
Adapting main clients to async API

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

Components.utils.reportError
(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?
Attachment #631847 - Flags: review?(mark.finkle)
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.
Attachment #631844 - Flags: review?(gavin.sharp) → review-
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.
Attachment #631847 - Flags: feedback?(gavin.sharp) → feedback-
(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.
Created attachment 632559 [details] [diff] [review]
Adapting main clients to async API
Attachment #631844 - Attachment is obsolete: true
Attachment #632559 - Flags: review?(gavin.sharp)
(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.
Created attachment 632562 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Attachment #631847 - Attachment is obsolete: true
Attachment #631847 - Flags: review?(mark.finkle)
Attachment #632562 - Flags: review?(mark.finkle)
Attachment #632562 - Flags: review?(gavin.sharp)
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.
Attachment #632559 - Flags: review?(gavin.sharp) → review+
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.
Attachment #632562 - Flags: review?(gavin.sharp) → feedback+
Created attachment 632820 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Attachment #632562 - Attachment is obsolete: true
Attachment #632562 - Flags: review?(mark.finkle)
Attachment #632820 - Flags: review?(mark.finkle)
Created attachment 632821 [details] [diff] [review]
Adapting main clients to async API

Done
Attachment #632559 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 765210
Attachment #632821 - Flags: review+
Attachment #632820 - Flags: review?(mark.finkle) → review+
Created attachment 634865 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Attachment #632820 - Attachment is obsolete: true
Attachment #634865 - Flags: review+
Created attachment 634868 [details] [diff] [review]
Adapting main clients to async API (mobile FF)
Attachment #634865 - Attachment is obsolete: true
Attachment #634868 - Flags: review+
Keywords: checkin-needed
Needs un-bitrotting.
Keywords: checkin-needed
Created attachment 636623 [details] [diff] [review]
Adapting main clients to async API

Here it is, unbitrotten.
Attachment #632821 - Attachment is obsolete: true
Attachment #636623 - Flags: review+
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
Flags: in-testsuite-
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/ca3c97022901
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.