1006103 - add speculativeConnect() method to search engines

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Mike Connor [:mconnor]]

Attachment: searchSpeculativeConnectAPI

We use nsISpeculativeConnect to speed up search suggestions/results in a couple of places (desktop searchbar and mobile search, the latter of which doesn't do the right thing if search and suggest URLs differ).  I'd like to use it in about:home in bug 612453, and eventually about:newtab (though that should be one shared bit of code).  At this point, it seems saner to just make this a method on the engine object.

Thus, here's a patch that does this.  If this is good, I'll replace the mobile caller with the API call as a part 2.

Comment 1

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

Comment on attachment 8417596 [details] [diff] [review]
searchSpeculativeConnectAPI

>diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl

>   /**
>+   * Opens a speculative connection to the engine's search URI
>+   * (and suggest URI, if different) to reduce request latency
>+   *
>+   * @param callbacks any security callbacks for use with SSL for
>+   *        interfaces such as nsIBadCertListener. May be null.

I know you copied this from the nsISpeculativeConnect interface docs, but this is somewhat confusing. More important than "security callbacks", this is used to make sure that we do the right thing in private browsing windows. So the comment should probably reflect that somehow, perhaps by just adding:

"For connections tied to a window context, you should typically pass in the relevant window's nsILoadContext."

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

>+  speculativeConnect: function SRCH_ENG_speculativeConnect(callbacks) {

>+    let searchURI = this.getSubmission("dummy", null, "searchbar").uri;

Not that it much matters, but you should just omit the purpose/responseType arguments and use their default values.

r=me with those changes

Comment 2

[Mike Connor [:mconnor]]

Attachment: useSpeculativeConnectAPIFennec

Comment 3

[:Margaret Leibovic]

Comment on attachment 8418128 [details] [diff] [review]
useSpeculativeConnectAPIFennec

Review of attachment 8418128 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +6869,1 @@
>      } catch (e) {}

Do we still need this try/catch? The desktop code doesn't have it.

Comment 4

[Mike Connor [:mconnor]]

Attachment: specConnectCleanup

While I was in the process of adding this to about:home, I realized that this is an awkward convenience function, and didn't go far enough.  Since all callers are QIing the window to get the load context, I figure we should instead make the API take the window as the arg, and do that work behind the scenes (instead of relying on the callers to copy what is effectively boilerplate).

Comment 5

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

Comment on attachment 8426539 [details] [diff] [review]
specConnectCleanup

Did you omit the IDL change from the patch?

Can you make the API be:

+  void speculativeConnect(in jsval options);

And specify that the options object requires a "window" property? That way if we want to loosen the need for providing a window in the future (or e.g. allow passing in your own nsILoadContext) we can do it without API breakage.

Comment 6

[Mike Connor [:mconnor]]

Attachment: searchSpeculativeConnectAPI

converted to jsval, it's a good change.  I combined the patches for a cleaner landing/blame, so this is all three patches.

Comment 7

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

Comment on attachment 8432829 [details] [diff] [review]
searchSpeculativeConnectAPI

Do we have test coverage for the PB mode behavior here (i.e. does a test fail if you comment out the "callbacks" passing code in your new speculativeConnect impl)?

If not you should probably test that that works manually at least.

Comment 8

[Mike Connor [:mconnor]]

I'll just land this tomorrow since it's done.

Comment 9

[Mike Connor [:mconnor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b1e61cbb92

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99b1e61cbb92

Comment 11

[Tracy Walker [:tracy]]

QA: ensure search suggestions/results work as expected in both regular and PB browsing modes.

Comment 12

[Camelia Badau [:cbadau], Desktop QA]

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.2 using latest Nightly 33.0a1 (buildID: 	20140716030202).