760035 - nsIBrowserSearchService asynchronous clients

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

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

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 Teller [:Yoric] - still alive but not very active]

Attachment: First prototype

Comment 2

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

Attachment: 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 Teller [:Yoric] - still alive but not very active]

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

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 Teller [:Yoric] - still alive but not very active]

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

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

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 Teller [:Yoric] - still alive but not very active]

(In reply to :Gavin Sharp (use [e-mail] 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 Teller [:Yoric] - still alive but not very active]

Attachment: Adapting main clients to async API

Comment 10

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

(In reply to :Gavin Sharp (use [e-mail] 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 Teller [:Yoric] - still alive but not very active]

Attachment: Adapting main clients to async API (mobile FF)

Comment 12

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

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

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 Teller [:Yoric] - still alive but not very active]

Attachment: Adapting main clients to async API (mobile FF)

Comment 15

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

Attachment: Adapting main clients to async API

Done

Comment 16

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

Attachment: Adapting main clients to async API (mobile FF)

Comment 17

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

Attachment: Adapting main clients to async API (mobile FF)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs un-bitrotting.

Comment 19

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

Attachment: Adapting main clients to async API

Here it is, unbitrotten.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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]]

https://hg.mozilla.org/mozilla-central/rev/ca3c97022901