780179 - SeaMonkey should use the asynchronous API from nsIBrowserSearchService.

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Philip Chee]

Every time I start trunk up I get this error message:

Fri Aug 03 2012 23:33:57
Error: Search service falling back to synchronous initialization at SRCH_SVC__ensureInitialized@resource:///components/nsSearchService.js:2498
@resource:///components/nsSearchService.js:3476
get_currentEngine@chrome://communicator/content/search/search.xml:130
updateDisplay@chrome://communicator/content/search/search.xml:266
init@chrome://communicator/content/search/search.xml:79
@chrome://communicator/content/search/search.xml:70

Source file: resource://app/components/nsSearchService.js
Line: 2499

q.v.
Bug 722332 - add async initialization API to nsIBrowserSearchService
Bug 760035 - Adapt main clients to asynchronous nsIBrowserSearchService API.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Port changes from Firefox.

Also fixed a typo in a comment.

Comment 2

[[e-mail]]

Comment on attachment 648740 [details] [diff] [review]
Patch v1.0 Port changes from Firefox.

>+        // Make sure we rebuild the popup in onpopupshowing
>+        this._needToBuildPopup = true;
[Wouldn't this be better as a field?]

>+        this.searchService.init((function search_init(aStatus) {
>+          if (Components.isSuccessCode(aStatus)) {
>+            // Refresh the display (updating icon, etc)
>+            this.updateDisplay();
>+          } else {
>+            Components.utils.reportError("Cannot initialize search service, bailing out: " + aStatus);
>+          }
>+        }).bind(this));
I'd appreciate it if you could convert this to use this.searchService.init(this) and put the callback code into this.onInitComplete and add nsIBrowserSearchInitObserver to the interface list. If you don't think you're up to it then I'll do it if you agree to review ;-)

Comment 3

[[e-mail]]

What about the search sidebar and the pref pane?

Comment 4

[Philip Chee]

Attachment: Patch v1.1 more fixes.

>>+        // Make sure we rebuild the popup in onpopupshowing
>>+        this._needToBuildPopup = true;
> [Wouldn't this be better as a field?]
Fixed.

>>+        this.searchService.init((function search_init(aStatus) {
>>+          if (Components.isSuccessCode(aStatus)) {
>>+            // Refresh the display (updating icon, etc)
>>+            this.updateDisplay();
>>+          } else {
>>+            Components.utils.reportError("Cannot initialize search service, bailing out: " + aStatus);
>>+          }
>>+        }).bind(this));
> I'd appreciate it if you could convert this to use this.searchService.init(this)
> and put the callback code into this.onInitComplete and add
> nsIBrowserSearchInitObserver to the interface list. If you don't think you're
> up to it then I'll do it if you agree to review ;-)
Fixed.

> What about the search sidebar and the pref pane?
Fixed the search sidebar, pref pane, engine manager, and nsContextMenu.
Did not fix navigator.js, nsSidebar.js and urlbarBindings.xml because I assume that the search service would already have been started via search.xml. It's probably not necessary to make the engine manager async but since I was fixing stuff anyway.

Comment 5

[[e-mail]]

(In reply to Philip Chee from comment #4)
> (In reply to comment #3)
> > What about the search sidebar and the pref pane?
> Fixed the search sidebar, pref pane, engine manager, and nsContextMenu.
> Did not fix navigator.js, nsSidebar.js and urlbarBindings.xml because I
> assume that the search service would already have been started via
> search.xml.
Whoa, so we use the search service synchronously all over. I can't really see any good way of handling that without forcing it to start in nsSuiteGlue :-(

Comment 6

[[e-mail]]

Comment on attachment 649011 [details] [diff] [review]
Patch v1.1 more fixes.

> function nsContextMenu(aXulMenu, aBrowser, aIsShift) {
>   this.shouldDisplay = true;
>-  this.initMenu(aBrowser, aXulMenu, aIsShift);
>+  Services.search.init(function contextSearchInit() {
>+    this.initMenu(aBrowser, aXulMenu, aIsShift);
>+  }.bind(this));
> }
This isn't going to work. The popupshowing handler expects us to call initMenu synchronously, which calls the search service synchronously...

>+  menulist.value = Services.search.defaultEngine.name;
This sucks too :-(

>+function LoadEngineList() {
Nit: move this higher up so that you're not copying lines?

>-          var searchService =
>-            Components.classes["@mozilla.org/browser/search-service;1"]
>-                      .getService(Components.interfaces.nsIBrowserSearchService);
>           // We only detect OpenSearch files
>           var type = Components.interfaces.nsISearchEngine.DATA_XML;
>-          searchService.addEngine(target.getAttribute("uri"), type,
>-                                  target.getAttribute("image"), false);
>+          this.searchService.addEngine(target.getAttribute("uri"), type,
>+                                       target.getAttribute("image"), false);
Even if you agree with starting the search service in nsSuiteGlue, I would of course still like to keep some of this good cleanup :-)

Comment 7

[Philip Chee]

Attachment: Patch v1.2 use nsSuiteGlue to start the search service.

> Whoa, so we use the search service synchronously all over. I can't really see 
> any good way of handling that without forcing it to start in nsSuiteGlue :-(
I've moved the search service initialization to nsSuiteGlue.

>> function nsContextMenu(aXulMenu, aBrowser, aIsShift) {
>>   this.shouldDisplay = true;
>>-  this.initMenu(aBrowser, aXulMenu, aIsShift);
>>+  Services.search.init(function contextSearchInit() {
>>+    this.initMenu(aBrowser, aXulMenu, aIsShift);
>>+  }.bind(this));
>> }
> This isn't going to work. The popupshowing handler expects us to call initMenu 
> synchronously, which calls the search service synchronously...
Gone.

>>+  menulist.value = Services.search.defaultEngine.name;
> This sucks too :-(
Gone.

>>+function LoadEngineList() {
> Nit: move this higher up so that you're not copying lines?
Done.

>>-          var searchService =
>>-            Components.classes["@mozilla.org/browser/search-service;1"]
>>-                      .getService(Components.interfaces.nsIBrowserSearchService);
>>           // We only detect OpenSearch files
>>           var type = Components.interfaces.nsISearchEngine.DATA_XML;
>>-          searchService.addEngine(target.getAttribute("uri"), type,
>>-                                  target.getAttribute("image"), false);
>>+          this.searchService.addEngine(target.getAttribute("uri"), type,
>>+                                       target.getAttribute("image"), false);
> Even if you agree with starting the search service in nsSuiteGlue, I would of 
> course still like to keep some of this good cleanup :-)
Kept. I've also retained the call to search.init(this) because otherwise I'd have to keep the timeout and the init method, instead I'll let the search service start the updateDisplay() in a more deterministic fashion. Besides having our search.xml more or less in sync with Firefox makes patches easier to port.

Comment 8

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/c7dea0a0a083