Closed Bug 1355443 Opened 7 years ago Closed 7 years ago

The location bar should speculatively connect to the current search engine

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- fixed

People

(Reporter: florian, Assigned: jj.evelyn)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-performance] [fxsearch])

Attachments

(1 file)

If suggestions are enabled in the awesomebar, we should preconnect to the current engine as soon as the location bar gains focus.

If suggestions aren't enabled, I think we should still preconnect, but wait until the 'Search with <engine name>' item becomes the first one in the awesomebar panel.

This should be easy, see the similar searchbar code at http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/browser/components/search/content/search.xml#504

See bug 735543 for when this was added to the searchbar, and bug 1006103 for when the implementation was moved to the search service.
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon][fxsearch] → [photon-performance] [fxsearch]
Depends on: 1356593
Priority: P2 → P3
Whiteboard: [photon-performance] [fxsearch] → [reserve-photon-performance] [fxsearch]
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> If suggestions are enabled in the awesomebar, we should preconnect to the
> current engine as soon as the location bar gains focus.
> 
> If suggestions aren't enabled, I think we should still preconnect, but wait
> until the 'Search with <engine name>' item becomes the first one in the
> awesomebar panel.
> 

I wonder if we could simplify both cases to one, which is "always speculative connect to search engine when the location bar gains focus", i.e. ignore suggestion pref setting. Per my understanding of current code, if we want to wait until the 'Search with <engine name>' item becomes the first one, it seems we need to put the speculative connection code in `onResultsAdded()` and filter the first result with "searchengine" on its style string. The function will be called many times for every key stroke on the input (although we have `gotResultForCurrentQuery` flag to detect the first "searchengine" item being appended) I'm worry that we will create too many connections to the search engine, which sounds a waste to networking utilization. 

:mak, what do you think? Is there a better place to setup speculative connection in this case?
Flags: needinfo?(mak77)
(In reply to Evelyn Hung [:evelyn] from comment #1)
> (In reply to Florian Quèze [:florian] [:flo] from comment #0)
> > If suggestions are enabled in the awesomebar, we should preconnect to the
> > current engine as soon as the location bar gains focus.
> > 
> > If suggestions aren't enabled, I think we should still preconnect, but wait
> > until the 'Search with <engine name>' item becomes the first one in the
> > awesomebar panel.
> > 
> 
> I wonder if we could simplify both cases to one, which is "always
> speculative connect to search engine when the location bar gains focus",
> i.e. ignore suggestion pref setting.

I don't think connecting to the search engine whenever a user who explicitly opted out of search suggestions starts typing a url is OK.

I think it would be ok to just avoid doing a speculative connection here when the user has disabled search suggestions. It's a non-default case so I think it's ok if it's slightly slower.
(In reply to Evelyn Hung [:evelyn] from comment #3)
> Created attachment 8885153 [details]
> Bug 1355443 - Speculatively connect to the current search engine when
> focusing on location bar.
> 
> Review commit: https://reviewboard.mozilla.org/r/156026/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/156026/

I think Florian's comment 2 makes sense to me, and I also find this post[1] that the user complains the browser didn't respect his/her setting and connect to the search engine in the background. Therefore, this patch only preconnect to the search engine when search suggestion is enabled. 

The suggestion setting I checked is "browser.search.suggest.enabled" although I'm a bit confused by "search.keyword" and "browser.urlbar.suggest.searches" which seem to be similar. I tried to manipulate them in my about:config and felt that "browser.search.suggest.enabled" looked more correct.

[1] https://support.mozilla.org/en-US/questions/980227
Assignee: nobody → ehung
Flags: needinfo?(mak77)
(In reply to Evelyn Hung [:evelyn] from comment #4)
> (In reply to Evelyn Hung [:evelyn] from comment #3)
> > Created attachment 8885153 [details]
> > Bug 1355443 - Speculatively connect to the current search engine when
> > focusing on location bar.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/156026/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/156026/
> 
> I think Florian's comment 2 makes sense to me, and I also find this post[1]
> that the user complains the browser didn't respect his/her setting and
> connect to the search engine in the background. Therefore, this patch only
> preconnect to the search engine when search suggestion is enabled. 
> 
> The suggestion setting I checked is "browser.search.suggest.enabled"
> although I'm a bit confused by "search.keyword" and

s/search.keyword/keyword.enabled/
The try looks not happy about the patch. I'm figuring out how to turn off speculative connect for reftests.
Status: NEW → ASSIGNED
Priority: P3 → P1
(In reply to Evelyn Hung [:evelyn] from comment #1)
> I wonder if we could simplify both cases to one, which is "always
> speculative connect to search engine when the location bar gains focus",
> i.e. ignore suggestion pref setting.

Note, I didn't look at the current patch yet, so these comments may be moot.

I agreee with Florian, and I'd also go the complete opposite direction than what you suggest here, that is just "connect when the default action is a search", rather than on focus. This reduces the likelyhood of pointless connection and preserves "privacy" better.
Something similar to what we did for autofill should work, it may be just matter of checking includes("autofill") || includes("searchengine").

(In reply to Evelyn Hung [:evelyn] from comment #4)
> The suggestion setting I checked is "browser.search.suggest.enabled"
> although I'm a bit confused by "search.keyword" and
> "browser.urlbar.suggest.searches" which seem to be similar. I tried to
> manipulate them in my about:config and felt that
> "browser.search.suggest.enabled" looked more correct.

browser.search.suggest.enabled is the general pref that controls suggestions in both the location bar AND the search bar.
browser.urlbar.suggest.searches is a sub-pref that controls the behavior only in the location bar.
Additionally, keyword.enabled is a pref that controls whether the default action from the location bar should be a search.

Unfortunately, we can't rely only on one of these, since even if in general suggestions are enabled, the user may have disabled them just for the location bar. The use-case here is clear, some privacy-addicted users prefer to keep a separation of concerns between the search bar and the location bar, and thus they disable search features in the location bar.

Even if reading prefs is generally cheap, here we seem to be doing it too often. We likely need a boolean property/field with a cache that states whether we want to speculative connect to search. It should be based on the above prefs, and we should be listening for prefs changes. urlbarbindings already has a prefs observer for the "browser.urlbar." branch, but it doesn't have one for "browser.search.", so maybe it should be added.
My assumption is that we want to speculative connect only if (browser.search.suggest.enabled && browser.urlbar.suggest.searches).

To take into account keyword.enabled, we may check the first heuristic result added to the location bar, if it's a search, then keyword.enabled is true. We basically get it for "free" without the need to listen or read the pref.

To sum up, my suggestion is: speculative connect if the first entry in the locationbar is a search AND suggestions are enabled both globally and in the location bar. Fwiw, this is the default.
Florian, could you please check I didn't say stupidities?
Flags: needinfo?(florian)
(In reply to Marco Bonardo [::mak] from comment #11)
> Florian, could you please check I didn't say stupidities?

I agree with what you said, maybe with a slightly different opinion about what we should do on focus :-).

The reason for speculatively connecting on focus is to get suggestions faster when the user starts typing. Isn't it a bit late to do this when the first entry in the panel is a search? Aren't we already doing a real connection to fetch suggestions at that point?

But I agree that speculatively connecting whenever the location bar is focused raises some privacy concerns. Should we do it only when the focus was set explicitly by the user (ie. when clicking or pressing cmd+l or cmd+t), and skip the speculative connection when the focus was set to the urlbar as a side effect (ie. switching to a tab in which the location bar was previously focused)?
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> The reason for speculatively connecting on focus is to get suggestions
> faster when the user starts typing. Isn't it a bit late to do this when the
> first entry in the panel is a search? Aren't we already doing a real
> connection to fetch suggestions at that point?

No, not yet. We start fetching suggestions after the heuristic result is added. At least for now.

> But I agree that speculatively connecting whenever the location bar is
> focused raises some privacy concerns. Should we do it only when the focus
> was set explicitly by the user (ie. when clicking or pressing cmd+l or
> cmd+t)

We could, provided the focus manager doesn't lie regarding which device set the focus, and our code doesn't set the focus by simulating one of the 2.
This also complicates things a bit, since it will require to check keyword.enabled in addition to the 2 above prefs.
Comment on attachment 8885153 [details]
Bug 1355443 - Speculatively connect to the current search engine.

https://reviewboard.mozilla.org/r/156026/#review162498
Attachment #8885153 - Flags: review?(mak77)
(In reply to Evelyn Hung [:evelyn] from comment #15)
> Comment on attachment 8885153 [details]
> Bug 1355443 - Speculatively connect to the current search engine.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/156026/diff/3-4/

Follow the suggestion in comment 10, I only connect when the default action is a search. I also cache those prefs and listen to pref changes. I replaced some places in the code with cached values but left one or two places that I feel the replacement makes the code less readable. Let me know if you want all of them being replaced. Besides, I'm not sure if the repeating string "browser.search.suggest.enabled" among the code is inevitable, how do we define a constant in xbl?

I'm a bit hesitated to do speculatively connect when getting focus from the user input. Given that we start fetching suggestions after the heuristic result is added, it sounds to me that the speculative connection in the first-result-added phase is still helpful?
Comment on attachment 8885153 [details]
Bug 1355443 - Speculatively connect to the current search engine.

https://reviewboard.mozilla.org/r/156026/#review164610

It looks good overall, though the PB code looks broken now and likely it needs a test to be added/fixed.
And this may also benefit from a test to check speculative connect on a search result? It would probably be similar to the test we already have for the autofill case, but with a search.

::: browser/base/content/urlbarBindings.xml:2159
(Diff revision 4)
> -            // search, and the result is an "autofill" result, that means it's
> -            // the site that user frequently visits. Then we could speculatively
> +            // search and we are not in the private context, we can speculatively
> +            // connect to the intended site as a performance optimization.
> -            // connect to this site as a performance optimization.
>              if (!this.input.gotResultForCurrentQuery &&
>                  this.input.mController.matchCount > 0 &&
> -                this.input.mController.getStyleAt(0).includes("autofill")) {
> +                this.input.speculativeConnectEnabled && !this.inPrivateContext) {

it should probably be this.input.inPrivateContext, or it will always be undefined (and skip the check)...
That makes me think we likely don't have a test checking we don't speculative connect in a pb window, or if we have a test it's bogus.

::: browser/base/content/urlbarBindings.xml:2159
(Diff revision 4)
> -            // search, and the result is an "autofill" result, that means it's
> -            // the site that user frequently visits. Then we could speculatively
> +            // search and we are not in the private context, we can speculatively
> +            // connect to the intended site as a performance optimization.
> -            // connect to this site as a performance optimization.
>              if (!this.input.gotResultForCurrentQuery &&
>                  this.input.mController.matchCount > 0 &&
> -                this.input.mController.getStyleAt(0).includes("autofill")) {
> +                this.input.speculativeConnectEnabled && !this.inPrivateContext) {

let's move these 2 checks above the this.input.mController.matchCount check, that is a bit more expensive. (in general the most expensive check in an and should be the last one)
Attachment #8885153 - Flags: review?(mak77) → review-
Blocks: 481503
(In reply to Evelyn Hung [:evelyn] from comment #19)
> Comment on attachment 8885153 [details]
> Bug 1355443 - Speculatively connect to the current search engine.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/156026/diff/4-5/

I am still working on the test case for speculative-connect-to-search-engine scenario, but this patch added a test case for private browsing (for autofill).
(In reply to Evelyn Hung [:evelyn] from comment #21)
> Comment on attachment 8885153 [details]
> Bug 1355443 - Speculatively connect to the current search engine.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/156026/diff/5-6/

Test case for speculative-connect-to-search-engine added. I didn't test private browsing mode here because (a)the engine.speculativeConnect is read-only so I can't mock it; (b) the same code path has been tested in the autofill scenario.

:mak, I think this version is good for review. Please take a look. Thank you for giving me many useful comments. :)
Comment on attachment 8885153 [details]
Bug 1355443 - Speculatively connect to the current search engine.

https://reviewboard.mozilla.org/r/156026/#review166756

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect_engine.js:1
(Diff revision 7)
> +"use strict";

nit: please add the pd license header. I know some tests are missing it, but we should in general always have it.

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect_engine.js:44
(Diff revision 7)
> +  let engine = await promiseNewSearchEngine(TEST_ENGINE_BASENAME);
> +  let oldCurrentEngine = Services.search.currentEngine;
> +  Services.search.currentEngine = engine;
> +
> +  registerCleanupFunction(async function() {
> +    await PlacesTestUtils.clearHistory();

nit: you can now just use await PlacesUtils.history.clear(); (one day I'll remove the PlacesTestUtils function since it's no more needed).

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect_engine.js:68
(Diff revision 7)
> +      is(gHttpServer.connectionNumber, 1,
> +         `${gHttpServer.connectionNumber} speculative connection has been setup.`)
> +      return gHttpServer.connectionNumber == 1;
> +    }
> +    return false;
> +  }, "Waiting for connection setup");

now that there are multiple tests doing this, it may be worth evaluating the creation of a promiseSpeculativeConnection helper in the head.js file, and just reuse that.
Attachment #8885153 - Flags: review?(mak77) → review+
All comments addressed. Thank you, :mak!
Pushed by ehung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/282362cd36e3
Speculatively connect to the current search engine. r=mak
https://hg.mozilla.org/mozilla-central/rev/282362cd36e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.4 - Aug 1
You need to log in before you can comment on or make changes to this bug.