Please see the bug for context. For pixel-perfect accuracy, we should just use
the favicon when showing the icon at 16px. We can continue to use the svg when
showing it at the larger best-match size.
Differential Revision: https://phabricator.services.mozilla.com/D185475
This does a few things:
* Unify the view implementations of rich suggestions, Firefox Suggest sponsored
results, and best match. I did this by using the best match implementation
and extending it to rich suggestions and Suggest sponsored.
* Use the unified implementation for Pocket suggestions too.
* Add a bottom-text concept since Pocket suggestions shown as top picks need to
show both a description and some text below it. (The actual bottom text per
result is added in D182632 since I didn't want to make this patch bigger than
necessary)
I have a couple motivations for these changes:
* I'm implementing Pocket suggestions, which need to show some text below the
suggestion title as well as the URL. I was going to just use the Firefox
Suggest sponsored approach, where the action text is wrapped below the title,
but that doesn't work because it can't show both the wrapped action text and
the URL.
* IMO we should use rich suggestions as the basis for all rows going forward,
i.e., unify the different row implementations around rich suggestions.
The reason I chose the best match implementation instead of the rich suggestions
implementation is because the grid-based approach of rich suggestions doesn't
work well when the URL also needs to be shown. The URL should be
baseline-aligned with the row title, which isn't easy to do when the URL is
outside the grid. The rich suggestions implementation also doesn't wrap the URL.
Other details:
* The `rich-suggestion=no-icon` attribute value is only used for styling, so we
can replace it with `@supports -moz-bool-pref()`. That lets us make the
`rich-suggestion` attribute a simple boolean.
* I kept the `isBestMatch` property for results since
`searchEngagementTelemetryGroup()` uses it to return "top_pick", and the view
also uses it to create the "Top pick" row/group label. It still has semantic
meaning so I think that's OK. It's no longer used by the view to create
different DOM or styling.
* Move `isRichSuggestion` from the payload to the result itself, since it's no
longer used for only one type of result. It's like `isBestMatch`, which is
also on the result.
* Add `richSuggestionIconSize` to the result too. The best match icon size
is 52. The Pocket best match icon size is 24 (but will have added padding and
a background color to make it appear 52px). IMO this is better than adding
rules for each type of suggestion to the CSS. It's cleaner and also indicates
what the "standard" icon sizes are.
Depends on D182580
Differential Revision: https://phabricator.services.mozilla.com/D182537
Each suggestion has "low" and "high confidence" keywords. When a high confidence
keyword is matched, the suggestion should be shown as a top pick, and otherwise
it should be shown as a normal Suggest result. High confidence keywords must be
matched in full, but low confidence keywords can be matched with prefixes
starting at the first word.
The low confidence matching behavior is the same as addon suggestions, so I
factored out that function into a new helper defined on `SuggestionsMap`.
I added a `full_keyword` property to the suggestions. It's not used yet but
we'll use it when we implement the final UI, which needs to show the full
keyword.
Differential Revision: https://phabricator.services.mozilla.com/D182580
This adds a `quickSuggestScoreMap` Nimbus variable that lets experiments
override suggestion scores. It maps from telemetry types to score values. For
example:
```
"quickSuggestScoreMap": {
"amo": 0.25,
"adm_sponsored": 0.3
}
```
In this example, addon suggestions will always have a score of 0.25, and
sponsored suggestions will always have a score of 0.3. Of course, different
branches within an experiment and different experiments can set different
scores.
While working on this, I saw we have a bug when we try to look up the
`BaseFeature` for a result. To do the lookup, we look up the result's
`telemetryType` in `FEATURE_NAMES_BY_TELEMETRY_TYPE`. That's a problem for `adm`
suggestions because the `telemetryType` will be either `adm_sponsored` or
`adm_nonsponsored`, but neither of those is present in
`FEATURE_NAMES_BY_TELEMETRY_TYPE` -- only `adm` is.
To fix it, I added back the `provider` property to result payloads that I
previously removed, and I added `BaseFeature.merinoProvider` so each feature can
specify its Merino provider. Then, `QuickSuggest` can build a map from Merino
provider names to features, allowing us to look up features without needing to
hardcode something like `FEATURE_NAMES_BY_TELEMETRY_TYPE` or
`FEATURE_NAMES_BY_MERINO_PROVIDER`.
Since I added back the `provider` property, I had to update a lot of tests. (As
a follow up, it would be nice to centralize the creation of expected result
objects in the test helper.)
I also added `BaseFeature.getSuggestionTelemetryType()` to help implement the
score map and to better formalize the idea that telemetry types are an important
property that all quick suggest results should include.
Differential Revision: https://phabricator.services.mozilla.com/D181709
This implements most parts of Pocket suggestions. They don't need any special UI
or a dynamic result type because they're only shown as the usual best match rows
or non-best match rows.
Still to do:
* Implement the "Show less frequently" behavior once we decide what the keywords
will be and how that will work.
* Implement the bottom "Mozilla Pocket" text inside the suggestion row once it's
finalized. We can use the same technique we use to show the "Sponsored" bottom
text for adM suggestions.
Other changes this makes:
* Replace the `type=bestmatch` attribute with an `bestmatch` attribute. That
lets best-match rows have a `type` too, in this case `"pocket"` (actually
either `"rs_pocket"` or `"merino_pocket"`, since I'm using the telemetry
result type).
* Improve how UrlbarProviderQuickSuggest delegates to individual features when
getting result commands, view updates, handling commands, etc., so that we
don't need to add new `case` statements for each new type of suggestion.
Differential Revision: https://phabricator.services.mozilla.com/D180059
This implements the new required matching behavior, which isn't based on min
keyword length anymore. This is how it works:
* Use the full keywords in remote settings to generate keywords that contain the
first word plus each possible substring after the first word. For example if a
full keyword is "video download", then generate these keywords: "video",
"video ", "video d", "video do", etc. If a full keyword is only one word, then
use it as is. The keywords never change even when the user clicks "Show less
frequently". This is implemented in `onRemoteSettingsSync()`, and I modified
`SuggestionsMap.add()` to make it easy to generate new keywords from the
strings in `suggestion.keywords`.
* Keep track of the number of times the user clicked "Show less frequently" in
`showLessFrequentlyCount`.
* When a suggestion is fetched from the suggestions map, filter it out if the
search string isn't long enough given the `showLessFrequentlyCount`. This is
done in `makeResult()`.
Other changes:
* I made some of the private properties in `AddonSuggestions` public so that the
xpcshell test can easily use them. I think it's OK for them to be public.
* Added `show_less_frequently_cap` to the RS config object so that we can
specify a cap in RS as well as Nimbus.
* mv'ed test_quicksuggest_addResults.js to test_suggestionsMap.js, since I
modified this file. I should have done that back when I replaced `addResults()`
with `SuggestionsMap`.
* Fixed a bug in `SuggestionsMap.add()` where the same suggestion could be added
multiple times to the array stored in the map, if it had duplicate keywords.
Differential Revision: https://phabricator.services.mozilla.com/D179867
This patch was generated as follows:
Run:
`./mach esmify --imports . --prefix=toolkit/mozapps/extensions/AddonManager`
In the output there are linter/prettifier errors due to unused
XPCOMUtils or separate importESModule calls. These have been fixed
manually and verified with `./mach lint --outgoing`.
The `esmify` script also inserts many unwanted newlines around imports
that are broken on two lines due to length. Due to the number of these,
I fixed them programatically.
1. Create patch from the changes so far.
2. From the patch, delete all lines that consist of "+" (i.e. added blank line).
3. Reset the working dir and apply the revised patch.
4. Verify that the diff between step 1 and 3 looks reasonable.
5. Verify that this patch as a whole looks reasonable.
Commands:
```
git diff > rename.diff
:%g/^+$/d
git commit -va -m WIP-rename
git revert HEAD
git apply --recount rename.diff
git diff HEAD^ # and verify that the removed lines are ok.
git commit -va # one last review to verify correctness of whole patch.
git rebase -i HEAD~3 # drop the WIP + reverted commit, pick only the last.
```
`git apply` has the `--recount` option to force it to ignore mismatches
in line counts, which happens because we deleted added lines (^+$)
without fixing up the line counts in the file headers.
Differential Revision: https://phabricator.services.mozilla.com/D179874
Please see bug 1832927 for background. This fixes the problem on 115 by moving
the sponsored/nonsponsored logic back to the provider.
I added a task to test_quicksuggest_topPicks.js, and I created xpcshell tests
for addons and dynamic Wikipedia with similar tasks. It would be nice to unify
this check for all quick suggest types somehow -- maybe a task in
test_quicksuggest.js, but that's not quite as simple as it seems because each
suggestion type has its own suggestion object and expected result payload. We
might also want to add more tasks to these new files. We can think about that
later because there are other opportunities for test consolidation too.
Differential Revision: https://phabricator.services.mozilla.com/D177952
This fixes the bug by not starting fetches until a config is set from either
remote settings or Nimbus. By "config" I mean keywords basically, but we sync
more than keywords -- the min keyword length and min keyword length cap -- so
that's why I use different term. So, before remote settings is synced, no config
will be set, so no fetches will happen, so the suggestion will be null. When the
urlbar provider starts a query, it will see the suggestion is null and not add a
result. Once a config is set from RS or Nimbus, we'll start fetching.
Currently we allow zero prefix when keywords or the min keyword length aren't
set. This patch removes that functionality because on second thought, there's
not a safe and obvious way to support zero prefix using these keyword-related
config properties/variables by themselves, and zero prefix isn't a feature
requirement anymore anyway. If we wanted to keep supporting it with these
properties/variables, there are a few options, and I don't like any of them:
* If `keywords` is undefined or null, use zero prefix. This is dangerous because
we may make a mistake in RS or Nimbus and forget to set it. Also, we use null
as the default value for the Nimbus var, and since we use UrlbarPrefs to
access Nimbus vars, there's no good way to tell whether null was set
intentionally or not.
* If `keywords` is an empty array, use zero prefix. This is awkward because the
user can now increment the min keyword length, which means the keywords array
kept by `Weather` can become empty when the min keyword length is incremented
a lot. In that case, no suggestion should be shown at all.
* If `min_keyword_length` is zero/falsey, use zero prefix. This has the same
problems as the first point above.
If we do need to support zero prefix again in the future, I think we should add
an RS property/Nimbus variable that makes it clear what's happening, e.g., a
`useZeroPrefix` boolean.
I removed the exposure scalar because it's entirely based on zero prefix. We can
use Glean for that now anyway.
I also noticed weather suggestions are case insenstive, so I fixed that and
added a test task.
Differential Revision: https://phabricator.services.mozilla.com/D177448
This fixes the bug by not starting fetches until a config is set from either
remote settings or Nimbus. By "config" I mean keywords basically, but we sync
more than keywords -- the min keyword length and min keyword length cap -- so
that's why I use different term. So, before remote settings is synced, no config
will be set, so no fetches will happen, so the suggestion will be null. When the
urlbar provider starts a query, it will see the suggestion is null and not add a
result. Once a config is set from RS or Nimbus, we'll start fetching.
Currently we allow zero prefix when keywords or the min keyword length aren't
set. This patch removes that functionality because on second thought, there's
not a safe and obvious way to support zero prefix using these keyword-related
config properties/variables by themselves, and zero prefix isn't a feature
requirement anymore anyway. If we wanted to keep supporting it with these
properties/variables, there are a few options, and I don't like any of them:
* If `keywords` is undefined or null, use zero prefix. This is dangerous because
we may make a mistake in RS or Nimbus and forget to set it. Also, we use null
as the default value for the Nimbus var, and since we use UrlbarPrefs to
access Nimbus vars, there's no good way to tell whether null was set
intentionally or not.
* If `keywords` is an empty array, use zero prefix. This is awkward because the
user can now increment the min keyword length, which means the keywords array
kept by `Weather` can become empty when the min keyword length is incremented
a lot. In that case, no suggestion should be shown at all.
* If `min_keyword_length` is zero/falsey, use zero prefix. This has the same
problems as the first point above.
If we do need to support zero prefix again in the future, I think we should add
an RS property/Nimbus variable that makes it clear what's happening, e.g., a
`useZeroPrefix` boolean.
I removed the exposure scalar because it's entirely based on zero prefix. We can
use Glean for that now anyway.
I also noticed weather suggestions are case insenstive, so I fixed that and
added a test task.
Differential Revision: https://phabricator.services.mozilla.com/D177448
