Closed Bug 1359899 Opened 8 years ago Closed 7 years ago

Add a pref to toggle the order of history vs. search suggestions

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: past, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
We want to experiment with the order of results in the awesomebar to find out what provides the most utility for our users. Currently history results appear before search suggestions (assuming those are enabled in about:preferences). We would like to add a pref in about:config to toggle the order so we can conduct some experiments.

No UI for flipping the pref is necessary as this is not something that we want to support long term.
Marco, could you indicate where this hard-coded logic resides so that someone else can work on this?
Flags: needinfo?(mak77)
Whiteboard: [fxsearch]
At a minimum, we want to be able to put search suggestions first, history results second. 

(adding this under a meta/userstory bug 1344933. Though this may be the only A/B experiment pref we do)
Blocks: 1344933
Most of the logic is in toolkit/components/places/UnifiedComplete.js
in particular see:
MINIMUM_LOCAL_MATCHES
_remoteMatchesStartIndex
_remoteMatchesCount
_localMatchesStartIndex
_localMatchesCount
_getInsertIndexForMatch
Flags: needinfo?(mak77)
I plan to start investigating this next week, I'd like to first land some additional prefs for things we already have, and test how much flexibility we can add in mixing up results.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Blocks: 1378034
Blocks: 1378035
This implements my `buckets` idea.
To separate results better, I'm thinking in the future we could add an attribute to each bucket, and autocomplete could set it as an attribute on the richlistitem. Then we could style each richlistitem based on the attribute, and likely add a label/separator above the first item with a given attribute.
On paper it should do.

Things left to do here:
1. document the approach in the commit message
2. pass existing tests
3. write new tests

I'm tempted to move most of the new tests (3) to a follow-up, so that we can start experimenting with this sooner than later. Let's see how much annoying it will be to write those tests.
Attachment #8883754 - Flags: review?(standard8)
Attachment #8883754 - Flags: review?(paolo.mozmail)
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review160678

The code is easy to follow, but it will still take some time for me to get up to speed on it, so if Mark knows this code already and he wants to go ahead he should feel free to.

I'll add things as I read the patch.

::: toolkit/components/places/UnifiedComplete.js:128
(Diff revision 3)
> +//   0: The match type of the acceptable entries.
> +//   1: available number of slots in this bucket.
> +//   2: a minimum frecency threshold. Any frecency-based match with a frecency
> +//      greater or equal to the threshold may be added to this bucket.
> +//
> +// First buckets. Anything with an Infinity frecency ends up here.

It seems to me this set of buckets is improperly reusing the Frecency field to indicate something different, that is whether the result is special and should be displayed before others.

::: toolkit/components/places/UnifiedComplete.js:502
(Diff revision 3)
> +    // JSON doesn't support Infinity, thus it needs special parsing.
> +    store.matchBuckets = JSON.parse(store.matchBuckets, (k, v) => {
> +      if (v === "Infinity")
> +        return Infinity;
> +      if (v === "-Infinity")
> +        return -Infinity;
> +      return v;
> +    });

I wasn't a big fan of using Infinity in the bucket definitions to begin with, but after seeing this I think we can come up with something better... whether it's -1, or using objects instead of arrays with meaningful, optional property names.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review160682

::: toolkit/components/places/UnifiedComplete.js:1496
(Diff revision 3)
>        comment: match.engineName,
>        icon: match.iconUrl,
>        style: "priority-search",
>        finalCompleteValue: match.url,
> -      frecency: FRECENCY_DEFAULT
> +      frecency: Infinity,
> +      type: MATCHTYPE.PLACES

_addMatch with MATCHTYPE.PLACES is used enough times that it could be the default if not specified?
(In reply to :Paolo Amadini from comment #10)
> It seems to me this set of buckets is improperly reusing the Frecency field
> to indicate something different, that is whether the result is special and
> should be displayed before others.

it actually uses frecency to filter matches with a frecency value lower than the given one for the bucket.
if the match has a frecency lower than the given one, the bucket won't accept it.
An Infinity frecency just means it is a bucket that only accept matches with Infinity frecency.

> I wasn't a big fan of using Infinity in the bucket definitions to begin
> with, but after seeing this I think we can come up with something better...
> whether it's -1, or using objects instead of arrays with meaningful,
> optional property names.

The fact is using Infinity simplifies the logic, since Infinity > Infinity, I can't tell the same for any other value. If I'd use any value here (included -1) then I'd need special code to handle that.
Long term I'd expect us to just drop any frecency filtering.  Filtering on an absolute frecency value is just bogus and not going to do what we expect. So maybe it's pointless to concentrate too much on the format.
I could actually even create a MATCHTYPE.HEURISTIC, have a bucket for those, and get rid of frecency completely, provided we don't need to experiment on frecency thresholds.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review160700

::: toolkit/components/places/UnifiedComplete.js:131
(Diff revision 3)
> +//      greater or equal to the threshold may be added to this bucket.
> +//
> +// First buckets. Anything with an Infinity frecency ends up here.
> +const DEFAULT_BUCKETS_BEFORE = [
> +  [MATCHTYPE.EXTENSION, MAXIMUM_ALLOWED_EXTENSION_MATCHES, Infinity],
> +  [MATCHTYPE.PLACES, Infinity, Infinity],

I think you're already special-casing the Infinity frecency in the code. I'll call these the "top" results.

As far as field collapsing goes, in this second bucket, you wouldn't be able to add a set of "top" results that are also sorted by frecency. If this sorting is never needed, and top results are always explicitly indicated as such when calling _addMatch, then maybe we're effectively dealing with a different MATCHTYPE, just like MATCHTYPE.EXTENSION is different?
Ah, I guess we just came to the same conclusions at the same time...
Ok, I will look into doing that and removing frecency filtering, one less footgun around.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

Clearing review for now.
Attachment #8883754 - Flags: review?(standard8)
Attachment #8883754 - Flags: review?(paolo.mozmail)
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review161936

The code looks good to me, we can land this logic and iterate later if necessary. Before, though, I'd like you to consider some changes to the preference format:

::: toolkit/components/places/UnifiedComplete.js:48
(Diff revision 4)
> +const PREF_MAX_HISTORICAL_SUGGESTIONS =  [ "maxHistoricalSearchSuggestions", 0];
>  
>  const PREF_PRELOADED_SITES_ENABLED =  [ "usepreloadedtopurls.enabled",   true ];
>  const PREF_PRELOADED_SITES_EXPIRE_DAYS = [ "usepreloadedtopurls.expire_days",  14 ];
>  
> +const PREF_MATCH_BUCKETS = ["matchBuckets", `[[1,5],[2,"Infinity"]]` ];

I believe a non-JSON would be much more readable:

PLACES:5,SUGGESTION:Infinity

Or even:

PLACES:5,SUGGESTION

You could easily allow future fields separated by colon. This would also make the regression tests more self-explanatory, I had to look up the meaning of the numbers otherwise. Also, since you're parsing with split, there is less chance for a manually edited value to trigger exceptions later in the code.

Some alternate formats if you really want to use JSON:

[["PLACES",5],["SUGGESTION","Infinity"]]

[{type:"PLACES",max:5],{type:"SUGGESTION"}]

::: toolkit/components/places/UnifiedComplete.js:115
(Diff revision 4)
> +  HEURISTIC: 0,
> +  PLACES: 1,
> +  SUGGESTION: 2,
> +  EXTENSION: 3

These constants should map to strings, and later we should just use an object instead of an array to store the partial counts.
Attachment #8883754 - Flags: review?(paolo.mozmail)
Attachment #8883754 - Flags: review?(standard8) → feedback+
(In reply to :Paolo Amadini from comment #19)
> I believe a non-JSON would be much more readable:
> 
> PLACES:5,SUGGESTION:Infinity

I don't have strong feelings for the format, I used json just because it has some sort of syntax verification, so potentially if the input doesn't parse, we could fallback to a default value. But I'm actually not doing that in the patch!
I can surely move to a simpler format, if we don't care about users possibly breaking their own experience. In general we never cared much, about:config has a warning for a reason :)

> You could easily allow future fields separated by colon. This would also
> make the regression tests more self-explanatory, I had to look up the
> meaning of the numbers otherwise.

If we add further fields in the future, we'll still lose some readability.
But I see there's a benefit with readable types.

I'll shortly look into addressing these comments.
Using strings for the formats may also be positive since we could reuse those strings as attributes in future css styling. so we can have a matchtype="something" attribute for each richlistitem.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review162088

r+ after solving the last issue noted below.

::: commit-message-0893f:3
(Diff revision 5)
> +Allows to set browser.urlbar.maxHistoricalSearchSuggestions to fetch a given number of
> +historical search suggestions.
> +Allots to set browser.urlbar.matchBuckets to change the location bar mixup of results.
> +The system is based on an array of buckets, each bucket is an array containing the type
> +of the match accepted by the bucket (MATCHTYPE) and the number of available slots in the bucket.
> +At every new match insertion, addMatch() loops all the buckets to find the first
> +compatible and available slot.
> +
> +Note that the pref format may change in the future, we plan to add an id to each bucket and
> +frecency may change from an absolute to a relative value.

Is there a better place to document the meaning and format of the preferences? The commit message is better than nothing, but it is actually quite difficult to find for those less familiar with Bugzilla archaeology. The current code comments where the preferences are documented are also difficult to find.

::: toolkit/components/places/UnifiedComplete.js:508
(Diff revision 5)
> +    try {
> +      store.matchBuckets = store.matchBuckets
> +                                .split(",")
> +                                .map(v => {
> +                                  let bucket = v.split(":");
> +                                  return [ bucket[0].trim(), Number(bucket[1]) ];

What about Number(bucket[1] || Infinity) so we can make the field optional and omit writing ":Infinity" in the definition?

::: toolkit/components/places/UnifiedComplete.js:511
(Diff revision 5)
> +                                .map(v => {
> +                                  let bucket = v.split(":");
> +                                  return [ bucket[0].trim(), Number(bucket[1]) ];
> +                                });
> +    } catch (ex) {
> +      store.matchBuckets = PREF_MATCH_BUCKETS[1];

This doesn't look correct...
Attachment #8883754 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review162088

> This doesn't look correct...

Why? it's setting store.matchBuckets to the pref default value. I admit the format is not great, but regardless we will change it in bug 1371611.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review162454

::: commit-message-0893f:3
(Diff revision 5)
> +Allows to set browser.urlbar.maxHistoricalSearchSuggestions to fetch a given number of
> +historical search suggestions.
> +Allots to set browser.urlbar.matchBuckets to change the location bar mixup of results.
> +The system is based on an array of buckets, each bucket is an array containing the type
> +of the match accepted by the bucket (MATCHTYPE) and the number of available slots in the bucket.
> +At every new match insertion, addMatch() loops all the buckets to find the first
> +compatible and available slot.
> +
> +Note that the pref format may change in the future, we plan to add an id to each bucket and
> +frecency may change from an absolute to a relative value.

I'm not sure what to use. Currently all the documentation is inline in code. I'd like to have a readthedocs documentation folder, but it's a larger project than I can handle alone.

If you have ideas, I'm listening.
Dunno what's up with mozreview today, I have difficulties replying to comments...

(In reply to :Paolo Amadini from comment #23)
> What about Number(bucket[1] || Infinity) so we can make the field optional
> and omit writing ":Infinity" in the definition?

I prefer keeping a complete notation, so it will be easier in the future to add further fields. It should not be a big deal, we're the only consumers for now, the pref is hidden and it's not intended to be extremely human readable fwiw.
Comment on attachment 8883754 [details]
Bug 1359899 - Add a way to control the awesomebar contents mixup through prefs.

https://reviewboard.mozilla.org/r/154684/#review162088

> Why? it's setting store.matchBuckets to the pref default value. I admit the format is not great, but regardless we will change it in bug 1371611.

Which is a string, while the property holds the parsed version of that string. Or did I misread the code?
Attachment #8883754 - Flags: review?(standard8)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/0f938aa405ee
Add a way to control the awesomebar contents mixup through prefs. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/0f938aa405ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1181644
Depends on: 1392263
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: