959594 - Retrieving the results from the default search provider

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP

Marco, would you mind giving me some high-level feedback on this approach?  Or if you think someone else would be better that's fine too.

This uses the SearchSuggestionController we made for bug 1007979.  That makes it pretty easy to integrate with UnifiedComplete.  The only complication is that it needs a `privateMode` argument, and there's no way that I can tell for UnifiedComplete to know what that should be.  So I added an optional inPrivateContext param to nsIAutoCompleteSearch.startSearch.  Its value originates in nsAutoCompleteController, which knows what it should be because its `input` member is an nsIAutoCompleteInput, which has an inPrivateContext member.  nsAutoCompleteController passes it to its nsIAutoCompleteSearch, which is UnifiedComplete in the case of the awesomebar.

SearchSuggestionController's design is to use one instance per input.  This patch actually uses one per search, but I think that should be OK.  It shouldn't be shared between inputs is all.

Questions -- Marco, please comment on them if you have thoughts:

How many suggestions should be returned?

How should they be ordered in the full list of results?

What should their frecency be?

What should the search "purpose" be?  In order to generate a URL from a query string, you have to give the search service API a search purpose.

What should be contained in the match objects included in the final results?  { value, comment, icon, style }.  Should value be the full URL or should it only be the suggestion string, and then the front-end would generate the URL (based on an action?)?

Comment 2

[:shell escalante]

Hi Guys, not sure if either Mak or Mark have bandwidth to give feedback on the patch drew put up... see Comment 1 for the questions.

Comment 3

[Marco Bonardo [:mak]]

I planned to do this today, then my time has been stolen by a stupid regression uplift with complications and a local code tree corruption... I will try to feedback later or tomorrow, or at least handle the questions asap.

Comment 4

[Drew Willcoxon :adw]

Thanks, Marco.  To be clear, those questions are as much for me (notes to self) as they are for you, but like I said, if you have thoughts on them that would be great.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #1)
> This uses the SearchSuggestionController we made for bug 1007979.  That
> makes it pretty easy to integrate with UnifiedComplete.  The only
> complication is that it needs a `privateMode` argument, and there's no way
> that I can tell for UnifiedComplete to know what that should be.

If it's just whether the search generated in a private browsing window, you could also just reuse the disable-private-actions searchParam that is set in private windows.
Regardless, the search params can be anything and are pretty much expandable to our needs.
This might largely simplify the patch.

Apart this, most of the code for retrieving search suggestions should live in PlacesSearchAutocompleteProvider.jsm. As I said in the past, I'm trying to keep the contexts a little bit more separated, so anything related to searches we can keep in the separate module should likely live there (also cause that module takes care of ensuring Search Service is asynchronously initialized).

> How many suggestions should be returned?

I think potentially browser.urlbar.maxRichResults, so that we have enough to fill all the available space in case no other results are avilable.

> How should they be ordered in the full list of results?
> 
> What should their frecency be?

Good question, First of all I think it's likely not the scope of this bug, you might concentrate just on retriving results, and maybe just append them at the end (we should do this regardless to fill up available space, imo), if you don't wish to expand too much the reach of this bug.
Bug 959595 is about displaying the results and Bug 1162140 about interaction with them.
I did some brainstorming for Bug 1162140.
There are 2 things we must consider:

1. We should not "disturb" the most frecent entries, cause otherwise we might go in the middle of the user flow. I think we should generally avoid the first 3 entries (I'm really guessing here). But we can't predict frecency values for any search. In the past when merging we tried to use a frecency of 1000, the reasoning is in https://bugzilla.mozilla.org/show_bug.cgi?id=975228#c2
Indeed in UnifiedComplete.js you can find FRECENCY_DEFAULT = 1000. It might be a good guess for the first suggestion. Next ones could be calculated starting from that, or we could decide to provide one search suggestion every 2 local ones, starting from the first inserted one. and then append up to browser.urlbar.maxRichResults.

2. Search suggestions come from the network, while local suggestions come from the disk... In both cases they might arrive at any time, and I think in general we should avoid having the autocomplete wait "too long" for suggestions to come. So I think we should start suggestions fetching soon in the search (maybe at browser.urlbar.delay / 2?) and when we reach the first result with frecency < FRECENCY_DEFAULT, check if we have a search suggestion available, or wait a max_timeout to get one. after the timeout if we still don't have an available suggestion, add the queued result. And so on for next suggestions.
 
> What should the search "purpose" be?  In order to generate a URL from a
> query string, you have to give the search service API a search purpose.

Heh, bug 1063530 is still open, we don't have a good purpose atm. See also bug 1071361. We likely need to implement the " keyword" purpose.
 
> What should be contained in the match objects included in the final results?
> { value, comment, icon, style }.  Should value be the full URL or should it
> only be the suggestion string, and then the front-end would generate the URL
> (based on an action?)?

There is a mock-up in Bug 1162140. As I said I think it's not important for this bug to implement the mock-up, let's start with the common style, then we can fix it later.

Comment 6

[Marco Bonardo [:mak]]

PS: we also need to put these entries under the "show search suggestions" pref so we can keep them disabled until they are ready to ship. Remember we are going to ship unified, with or without all features, so we must be able to keep sub-features disabled. The toggle will be implemented in bug 959567 but I assume we could start adding a browser.urlbar.suggest.searches, that in future can be added to the Privacy list of toggles.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #5)
>  Should value be the full URL or should it
> > only be the suggestion string, and then the front-end would generate the URL
> > (based on an action?)?

Ah I forgot, I think this should likely be a action: url
IIRC we already have code to parse and run a "search" action url

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8614937 [details] [diff] [review]
WIP

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

I think while replying I ended up suggesting changes to the high level approach... let me know if I missed something, we can discuss it further.

Comment 9

[Marco Bonardo [:mak]]

just a heads up, I'm reusing your code to build a proof-of-concept for bug 1162140, I will share the code later and we can see how to proceed from there.

Comment 10

[Marco Bonardo [:mak]]

Attachment: very wip POC

this is a wip POC I made to check how far we are from styling for bug 1162140 and which issues are more problematic. it's just trying to insert a suggestion with frecency 10000, and fill up any free space at the end.

I think the biggest blocker is the fact Sqlite onRow handler is synchronous and we add_match directly on it, this makes hard to insert results in the middle of results of a query. So for example here I cannot wait for search suggestions, since that would require me to yield. If suggestions take a bit more to be fetched we can't insert them. I think it would be wise to allow a small timeout for them to be fetched.

Either we make the onRow handler a generator, or we make add_match add to a queue of matches and have a parallel consumer of that queue. The former is more risky for Sqlite.jsm consumers (they may block themselves indefinitely), so I'd probably try the latter.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch v2

This is basically your patch except it also yields at the end of execute() to wait for suggestions to finish when there's still room for suggestions.  I agree with you that the parallel consumer idea is probably better than trying to make onResultRow a generator.  I can't see how that would even work.

Comment 12

[Drew Willcoxon :adw]

Attachment: patch v3

This is the same as the previous patch but it's based on your patch in bug 1162140.

Comment 13

[Drew Willcoxon :adw]

The visual results look good to me by the way.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8620625 [details] [diff] [review]
patch v3

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

::: toolkit/components/places/UnifiedComplete.js
@@ +1662,5 @@
>    //////////////////////////////////////////////////////////////////////////////
>    //// nsIAutoCompleteSearch
>  
>    startSearch: function (searchString, searchParam, previousResult, listener) {
> +

Didn't mean to add this.

Comment 15

[Drew Willcoxon :adw]

Attachment: patch v4

This hooks up browser.urlbar.suggest.searches.  Seems to work.  I think I updated all the right code for the new preference/mozIPlacesAutoComplete.BEHAVIOR, but it's spread out throughout UnifiedComplete so I might have missed something.

This also adds a new "private-window" autocompletesearchparam.  It doesn't seem right to me to pass privateMode=false to the SearchSuggestionController for windows in permanent PB mode.  Seems like they should use privateMode=true too.  It's a different case from handling switch-to-tab.  But I'm not sure, so I can revert that part.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8620649 [details] [diff] [review]
patch v4

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

I think it might be worth to land this first implementation we have, and then work on improvements in follow-ups. improvements are easier to work in parallel and easier to uplift, plus people can start playing with search suggestions.
I'm sure we'll get complains by landing this without a UI prefs toggle, so I'll immediately start working on the options toggle. We should land the 3 patches together.

In the meanwhile, once this patch is updated, could you please do a Try run of these 2 patches we have so far? I want to be sure we are not breaking tests, I'd not want to backout :)

I'm going to file the following follow-up bugs:

1. Hook up the pref to Options / Privacy / Location Bar

2. Allow to consume matches asynchronously to allow a timeout when fetching suggestions. Without this the same search could return suggestions sometimes but not others in very random ways, we want results to be sort-of "consistent" instead. The current solution would also probably constantly fail on high latency and slow connections, that are common in less developed countries (I also put Italy here)

3. Write tests! We need to test that the search toggle correctly disables searches, and that they are returned. The latter might depend on 2.

4. Figure out a better frecency mixup strategy based on feedback we get on first implementation (mostly a brainstorming bug until we have better ideas and nightly feedback)

5. Add a restrictSearchesToken that allows to filter results only on search suggestions, it's not critical for prime time release, but bonus points if we add it, with it the locationbar would be equivalent to the search bar.

::: toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
@@ +141,5 @@
> +}
> +
> +SearchSuggestionControllerWrapper.prototype = {
> +
> +  get allSuggestionsFetchedPromise() {

I'd usually try to avoid duping long names like "suggestions" in methods when it might be clear by the object itself, in this case you'll have something like _suggestionController.allSuggestionsFetchedPromise... while it would be nicer something like _suggestionsController.fetchCompletePromise

@@ +145,5 @@
> +  get allSuggestionsFetchedPromise() {
> +    return this._promise;
> +  },
> +
> +  consumeSuggestion() {

as well as here just consume

@@ +257,5 @@
>        terms: parseUrlResult.terms,
>      };
>    },
> +
> +  fetchSearchSuggestions: Task.async(function* (searchToken, inPrivateContext,

the method name is lying a little bit, since this starts fetching, but doesn't fetch synchronously.

Indeed this code reads strange:
this._searchSuggestionController = yield PlacesSearchAutocompleteProvider.fetchSearchSuggestions

something like getWrappedSuggestionController or just getSuggestionController, to me sounds better.

@@ +259,5 @@
>    },
> +
> +  fetchSearchSuggestions: Task.async(function* (searchToken, inPrivateContext,
> +                                                maxResults) {
> +    yield this.ensureInitialized();

I'd rather make this throw like parseSubmissionURL, the point where we invoke this should have already ensured this module is initialized (if it didn't it's a coding mistake), so you can avoid making this a Task (very trivial perf gain but still...)

::: toolkit/components/places/UnifiedComplete.js
@@ +807,5 @@
>  
>      // IMPORTANT: No other first result heuristics should run after
>      // _matchHeuristicFallback().
>  
> +    yield this._sleep(Prefs.delay / 2);

I forgot to do in my patch, but for clarity it would be better to round()

@@ +820,5 @@
> +          Prefs.maxRichResults
> +        );
> +    }
> +
> +    yield this._sleep(Prefs.delay / 2);

ditto

@@ +1169,5 @@
>      if (!this.pending)
>        return;
>  
> +    if (match.frecency < FRECENCY_DEFAULT) {
> +      this._maybeAddSearchSuggestionMatch();

So, there is another issue here that I didn't address in my POC as well, once we pass the frecency_default threshold, this will basically append a search suggestion every frecency result.

I think we want to still give frecency results a little bit more weight.

While it's a sucky solution, for now I think for now we should try to add one search suggestion every 2 (please put this into a const) frecency ones.
So something like
(init)
this._suggestionsInsertCounter = 0;
(here)
if (match.frecency < FRECENCY_DEFAULT && this._suggestionsInsertCounter++ % 2 == 0) {
  let added = _maybeAdd...
  // Retry at the next round if failed, otherwise wait for some frecency results.
  this._suggestionsInsertCounter = Number(added);
}

We will look into better heuristics in follow-ups and based on first feedback achieved through this.

@@ +1171,5 @@
>  
> +    if (match.frecency < FRECENCY_DEFAULT) {
> +      this._maybeAddSearchSuggestionMatch();
> +      // Search engine matches get FRECENCY_DEFAULT, so there's no danger of
> +      // infinite indirect recursion.

I agree this sucks, sometimes in future we should refactor a little bit to allow for better mixup :)

Comment 17

[Drew Willcoxon :adw]

Thanks Marco.  Yesterday I did push to try with your patch from the other bug plus the latest patch here, and bc1 fails due to that dumb "Non-local network connections," so I'll have to fix that.  Not sure yet what's causing it.  Speculative connect?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a315e56d759c

Comment 18

[Marco Bonardo [:mak]]

I guess it's just some test trying to load search suggestions from the default engine? That's disallowed cause it touches the outside network.

Comment 19

[Marco Bonardo [:mak]]

> Non-local network connections are disabled and a connection attempt to search.yahoo.com (98.137.250.95) was made.

Comment 20

[Drew Willcoxon :adw]

Yeah it's just tests exercising the location bar.  So now they trigger suggestions.  Nothing more complicated than that.  Maybe it would be easiest for now to set browser.urlbar.suggest.searches to false in the testing profile.

Comment 21

[Marco Bonardo [:mak]]

Drew Willcoxon :adw from comment #20)
> Yeah it's just tests exercising the location bar.  So now they trigger
> suggestions.  Nothing more complicated than that.  Maybe it would be easiest
> for now to set browser.urlbar.suggest.searches to false in the testing
> profile.

Yes, we'll enable it in specific tests for it. Whatever else would make tests dependent on a network lag, that's not exactly nice for intermittents.

Comment 22

[Drew Willcoxon :adw]

Try run with the same patches but with browser.urlbar.suggest.searches=false in the testing profile:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=806f52a2d321

I'll do another try run once I address your comments.

Comment 23

[Drew Willcoxon :adw]

Attachment: patch v5

I think it's worth asking for review again since the insertion logic is a little tricky.  This also adds more comments.

It works.  Most of the time I see one suggestion for every two bookmarked unvisited items.  But every now and then I do see mostly the bookmarked unvisited items with maybe only one suggestion at the end, for the same search string, I presume due to the suggestions sometimes taking slightly longer than usual to arrive.  Which we knew will happen.

Try with your patch from the other bug and this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32c0ef470c12

I'll work on tests next.  Even though we can't guarantee the ordering yet we can still check that they show up at all.

Comment 24

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #23)
> I'll work on tests next.  Even though we can't guarantee the ordering yet we
> can still check that they show up at all.

The tests in unifiedcomplete/ mostly ignore the order of results (apart from the first one that is special and thus verified), so that's ok.

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8621145 [details] [diff] [review]
patch v5

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

::: toolkit/components/places/UnifiedComplete.js
@@ +70,5 @@
> +// become available, but they're only inserted once every N results whose
> +// frecencies are less than FRECENCY_DEFAULT.  In other words, for every N
> +// results that fall below that frecency threshold, one search suggestion is
> +// inserted.  N = this value.
> +const SEARCH_SUGGESTION_INSERT_FREQUENCY = 2;

just to avoid confusion between FRECENCY AND FREQUENCY, I'd probably call this _INTERVAL

Comment 26

[Drew Willcoxon :adw]

Attachment: patch v6

Final patch with FREQUENCY -> INTERVAL.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/471f236c32cb

Comment 28

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/ae1d2c1b1d18

Comment 29

[Marco Bonardo [:mak]]

so, as you supposed, we reach init_failed from the timeout here
http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/testrunner.py#337
Otherwise we'd get more warning messages.

I wonder if the problem is just that we were taking 29 seconds, and the simple addition of one pref crossed the timeout boundary... but doesn't look like 30 seconds elapsed here:
14:40:31 INFO - Starting runner
14:40:31 WARNING - Init failed 1

Comment 30

[Marco Bonardo [:mak]]

my suspect was correct. I just changed timeout from 30 to 60 and everything works.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0a04c205b5f

Comment 31

[Marco Bonardo [:mak]]

I filed Bug 1174108 for the web platform timeout increase.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8dd26972a9d8

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8dd26972a9d8