Closed Bug 1006103 Opened 7 years ago Closed 6 years ago

add speculativeConnect() method to search engines

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.3

People

(Reporter: mconnor, Assigned: mconnor)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch searchSpeculativeConnectAPI (obsolete) — Splinter Review
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.
Attachment #8417596 - Flags: review?(gavin.sharp)
Attachment #8417596 - Flags: review?(gavin.sharp)
Attachment #8417596 - Flags: review?(adw)
Attachment #8417596 - Flags: review?(MattN+bmo)
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
Attachment #8417596 - Flags: review?(adw)
Attachment #8417596 - Flags: review?(MattN+bmo)
Attachment #8417596 - Flags: review+
Attached patch useSpeculativeConnectAPIFennec (obsolete) — Splinter Review
Attachment #8418128 - Flags: review?(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.
Attachment #8418128 - Flags: review?(margaret.leibovic) → review+
Blocks: 1007985
Attached patch specConnectCleanup (obsolete) — Splinter Review
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).
Attachment #8426539 - Flags: review?(gavin.sharp)
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.
Attachment #8426539 - Flags: review?(gavin.sharp)
converted to jsval, it's a good change.  I combined the patches for a cleaner landing/blame, so this is all three patches.
Attachment #8417596 - Attachment is obsolete: true
Attachment #8418128 - Attachment is obsolete: true
Attachment #8426539 - Attachment is obsolete: true
Attachment #8432829 - Flags: review?(gavin.sharp)
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.
Attachment #8432829 - Flags: review?(gavin.sharp) → review+
Flags: firefox-backlog+
Assignee: mconnor → nobody
Points: --- → 2
I'll just land this tomorrow since it's done.
Assignee: nobody → mconnor
No longer blocks: 1007985
https://hg.mozilla.org/mozilla-central/rev/99b1e61cbb92
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Iteration: --- → 33.3
QA Whiteboard: [qa?]
See Also: → 1037372
QA: ensure search suggestions/results work as expected in both regular and PB browsing modes.
QA Whiteboard: [qa?] → [qa+]
QA Contact: camelia.badau
Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.2 using latest Nightly 33.0a1 (buildID: 	20140716030202).
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Depends on: 1356593
You need to log in before you can comment on or make changes to this bug.