Closed
Bug 1006103
Opened 10 years ago
Closed 10 years ago
add speculativeConnect() method to search engines
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
People
(Reporter: mconnor, Assigned: mconnor)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | 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)
Assignee | ||
Updated•10 years ago
|
Attachment #8417596 -
Flags: review?(gavin.sharp)
Attachment #8417596 -
Flags: review?(adw)
Attachment #8417596 -
Flags: review?(MattN+bmo)
Comment 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8418128 -
Flags: review?(margaret.leibovic)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Assignee: mconnor → nobody
Updated•10 years ago
|
Points: --- → 2
Assignee | ||
Comment 8•10 years ago
|
||
I'll just land this tomorrow since it's done.
Assignee: nobody → mconnor
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated•10 years ago
|
Iteration: --- → 33.3
QA Whiteboard: [qa?]
Updated•10 years ago
|
Comment 11•10 years ago
|
||
QA: ensure search suggestions/results work as expected in both regular and PB browsing modes.
QA Whiteboard: [qa?] → [qa+]
Updated•10 years ago
|
QA Contact: camelia.badau
Comment 12•10 years ago
|
||
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!]
You need to log in
before you can comment on or make changes to this bug.
Description
•