813379 - Fennec should hint to necko when search engines become visible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Josh Aas]

CC'ing Patrick McManus, who worked on connection hints.

Comment 3

[Mina Almasry]

Attachment: 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?

Comment 4

[Mina Almasry]

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

Comment 5

[Mina Almasry]

Comment on attachment 809391 [details] [diff] [review]
Patch v1

Needs more work, since the reqs changed it seems.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 7

[Mina Almasry]

Needinfo on Comment #6

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(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?

Comment 9

[Mina Almasry]

> 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?

Comment 10

[Mina Almasry]

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.

Comment 11

[Patrick McManus [:mcmanus]]

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)

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

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).

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 15

[Mina Almasry]

Attachment: 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?

Comment 16

[: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.

Comment 17

[:Margaret Leibovic]

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

Comment 18

[Mina Almasry]

Attachment: 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.

Comment 19

[: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.

Comment 20

[Mina Almasry]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=fb8fd17efb1d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6f3a3d0b6ec6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f3a3d0b6ec6