Closed Bug 760035 Opened 8 years ago Closed 8 years ago

nsIBrowserSearchService asynchronous clients

Categories

(Firefox :: Search, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: main-thread-io)

Attachments

(2 files, 8 obsolete files)

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.
Attached patch First prototype (obsolete) — Splinter Review
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)
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.
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.
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+
Attachment #632562 - Attachment is obsolete: true
Attachment #632562 - Flags: review?(mark.finkle)
Attachment #632820 - Flags: review?(mark.finkle)
Done
Attachment #632559 - Attachment is obsolete: true
Blocks: 765210
Attachment #632820 - Flags: review?(mark.finkle) → review+
Attachment #632820 - Attachment is obsolete: true
Attachment #634865 - Flags: review+
Needs un-bitrotting.
Keywords: checkin-needed
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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.