Fennec should hint to necko when search engines become visible

RESOLVED FIXED in Firefox 27

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: blassey, Assigned: almasry.mina)

Tracking

Trunk
Firefox 27
Points:
---

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Necko exposes an API where it can start warming itself up (up to the point of having an https connection already open and ready) in anticipation of the user needing to go somewhere. We should use this for our search engines as soon as the become visible in the awesome screen.

We might want to also do this for other stuff in the awesome screen, but there might be privacy concerns around that, so I'll file it as a separate bug.
Blocks: 813380
tracking-fennec: ? → +
I think we need to be very focused here. Desktop uses nsISpeculativeConnect to hint about the single search engine, only when the user sets focus in the search-only box.

In mobile, we mix search engines with awesomebar results, so we show all search engines. We also go into "search" mode in the awesomebar as soon as you start typing. My point is that we are not very "focused" on search-only.

We do use search suggestions, as long as they are enabled. Maybe we should look at hinting about the search suggestion provider. Of course, we use Java networking to fetch those suggestion, not Gecko.

Comment 2

6 years ago
CC'ing Patrick McManus, who worked on connection hints.
Assignee: nobody → malmasry
(Assignee)

Comment 3

5 years ago
Created attachment 809391 [details] [diff] [review]
Patch v1

So here is a first try at this.

This hints the search term to the top 3 engines, and the top 3 suggestions (all which could be from the same engine).

Right now it sends the hints whenever the search term is changed... I don't know if that's optimal, because it means we send hints while the user is typing. We might want to put a timer in there so that we only send hints when the person is done typing?

Let me know what else is needed. Do you need a test with this? How do I go about writing one?
Attachment #809391 - Flags: feedback?(margaret.leibovic)
(Assignee)

Comment 4

5 years ago
also, the patch for this bug is applied on top of the one for this: https://bugzilla.mozilla.org/show_bug.cgi?id=813380
(Assignee)

Comment 5

5 years ago
Comment on attachment 809391 [details] [diff] [review]
Patch v1

Needs more work, since the reqs changed it seems.
Attachment #809391 - Flags: feedback?(margaret.leibovic)
Let's adjust the scope a bit here. Given that desktop will make a speculative connection anytime the user sets focus into the search box (in the toolbar), we could do the same for the default search engine in mobile.

Desktop will even use a "dummy" URL, so the actual search term is not even needed:
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#541

We could do the same thing when Java fetches the search engines to use in the awesomescreen:
entry point here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6557
work done here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6487

Just to be safe, we could add a param to the JSON Java sends to let JS know we want to set a speculative connection, since we might send the JSON in other, non-awesomescreen, cases.

What do we think of that for a start?
(Assignee)

Comment 7

5 years ago
Needinfo on Comment #6
Flags: needinfo?(blassey.bugs)
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Let's adjust the scope a bit here. Given that desktop will make a
> speculative connection anytime the user sets focus into the search box (in
> the toolbar), we could do the same for the default search engine in mobile.
that sounds fine to me
> 
> Desktop will even use a "dummy" URL, so the actual search term is not even
> needed:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/search/
> content/search.xml#541
I'd rather we be sending the full url to necko and let necko handle obfuscating it. That might need an additional param to tell necko to obfuscate.

> We could do the same thing when Java fetches the search engines to use in
> the awesomescreen:
> entry point here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6557
> work done here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6487
 Are you suggesting getting an obfuscated url? Again, I'd hope necko could be smart about lopping off query params

> Just to be safe, we could add a param to the JSON Java sends to let JS know
> we want to set a speculative connection, since we might send the JSON in
> other, non-awesomescreen, cases.
I think this is only for speculative connections, no?
> 
> What do we think of that for a start?
Flags: needinfo?(blassey.bugs)
(Assignee)

Comment 9

5 years ago
> I'd rather we be sending the full url to necko and let necko handle
> obfuscating it. That might need an additional param to tell necko to
> obfuscate.

We can't do this if we are sending the speculative connection when the user focuses on the search box, because the user hasn't typed in the search term yet, so we don't have the full URL. Firefox desktop sends a URL with a dummy search term, and the proposal is that we do the same here.

What we could do is wait for the user to type in the search term, and when the user is done typing (we can tell the user is done typing via a timer), then we send the speculative connection with the full URL. How about that?

And just to be clear. Are we sending 1 speculative connection for the default search engine only, or 1 per search engine available, or something in between?
Flags: needinfo?(blassey.bugs)
(Assignee)

Comment 10

5 years ago
Clarification up there ^^ : I mean the proposal is to send the speculative connection when the list of engines is retrieved, not when the user focuses the search box. But both have the same effect, I believe.

Also, if we send the speculative connection where mfinkle suggests - when the list of engines is retrieved - the cost of the operation will be much lower I believe. That's because there is no extra JSON to be sent from java to the JS. All the information we need is the engines to send a speculative connection too, and that's already available to the JS. The cost is only doing the actual speculative connections.
fyi the path portion of the speculative connect uri is not used by necko.. just the scheme, host, and port.. (i.e. it isn't doing the search, it is just getting the very slow TLS handshake out of the way if possible)
(In reply to Patrick McManus [:mcmanus] from comment #11)
> fyi the path portion of the speculative connect uri is not used by necko..
> just the scheme, host, and port.. (i.e. it isn't doing the search, it is
> just getting the very slow TLS handshake out of the way if possible)

Brad - Based on Patrick's comment, we never need the full URL. We only need the base URL, which we can get once, in browser.js, and never need to send a bunch of JSON messages from Java to JS.

Mina- Let's see a patch that does what you describe in comment 10, but only make 1 speculative connection to start: the one for the default search engine.

The default search engine is the one used if the user types a non-URL and it's the one listed first in the set of search engines in the awesomebar list.
My point is that the front end shouldn't be scrubbing the urls or providing dummy urls. Let Necko scrub that (as it already is).
Flags: needinfo?(blassey.bugs)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #13)
> My point is that the front end shouldn't be scrubbing the urls or providing
> dummy urls. Let Necko scrub that (as it already is).

Yep. And when I say "dummy" I mean pass "dummy" to the search engine system so we get back a URL, like desktop does:
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#541
(Assignee)

Comment 15

5 years ago
Created attachment 811177 [details] [diff] [review]
Patch v2

Okay here is a patch to do that. Not sure how to check if it worked. I ran it and didn't get any errors.

Do you need a test with this?
Attachment #809391 - Attachment is obsolete: true
Attachment #811177 - Flags: review?(margaret.leibovic)
Comment on attachment 811177 [details] [diff] [review]
Patch v2

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

Looks good, but we need to move where we do this to avoid synchronously initializing the search service.

::: mobile/android/chrome/content/browser.js
@@ +6555,5 @@
>        case "SearchEngines:Get":
>          // Return a list of all engines, including "Hidden" ones.
>          Services.search.init(this._handleSearchEnginesGetAll.bind(this));
> +        // Send a speculative connection to the default engine.
> +        let connector = Services.io.QueryInterface(Components.interfaces.nsISpeculativeConnect);

You can use Ci instead of Components.interfaces (that's defined as a convenience at the top of the file).

@@ +6560,5 @@
> +        let searchURI = Services.search.defaultEngine.getSubmission("dummy").uri;
> +        let callbacks = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                              .getInterface(Components.interfaces.nsIWebNavigation)
> +                              .QueryInterface(Components.interfaces.nsILoadContext);
> +        connector.speculativeConnect(searchURI, callbacks);

We should be doing this in _handleSearchEngeinesGet, since the init call above asynchronously initializes the search serivce, but calling Services.search.defaultEngine here will force synchronous initialization.
Attachment #811177 - Flags: review?(margaret.leibovic) → feedback+
Maybe material for a follow-up, but should we also make a speculative connection for the suggest URI?

http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#540
(Assignee)

Comment 18

5 years ago
Created attachment 811374 [details] [diff] [review]
Patch v3

Like this?

And based on comment #12, I'd say no speculative connection for the suggest URI, but that's a small change to make anyway.
Attachment #811177 - Attachment is obsolete: true
Attachment #811374 - Flags: review?(margaret.leibovic)
Comment on attachment 811374 [details] [diff] [review]
Patch v3

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

Great!

Let's leave out the suggest URI for now, we can always add that in a follow-up bug if we think it would be worthwhile. It won't speed up page loads, but it may speed up how quickly the first search suggestions show up. Perhaps something to test.
Attachment #811374 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6f3a3d0b6ec6
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6f3a3d0b6ec6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.