Brave search conversion promotion in omnibox #12846

simonhong · 2022-03-31T04:43:39Z

This PR is for showing brave search promotion match in omnibox popup.
To do this, PromotionProvider adds promotion matches and BraveOmniboxResultView
renders promotion UI. There are two types of conversion. One is button type and the other is banner.
Button type match is located as a second match and Bannter type is located as a last match.
There position is re-arragned after upstreams match sorting.

First commit is PromotionProvider(non UI) part and second commit is BraveOmniboxResultView (UI) part.

Button type

Banner type

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Button type

Launch browser with --enable-features=BraveSearchOmniboxButton command line switch
Set any search engine except brave search as a default search provider
Type any keyword and check button type autocomplete is visible as a second item

Banner type

Launch browser with --enable-features=BraveSearchOmniboxBanner command line switch
Set any search engine except brave search as a default search provider
Type any keyword and check button type autocomplete is visible as a last item

Note:

When first item is not keyword match from default provider, promotion match will not be added because first item is highly recommended match
When dismissed button or x button clicked, autocomplete match list are refreshed with same input data and promotion match will not be shown anymore.
When user types an input that was tried by promotion match, promotion match will not be shown again. Instead promotion url could be visible as a history match.
This conversion promotion is only enabled for below 5 countries
- US, CA, DE, FR, GB
Only normal profile window give promotion. Private(or tor) window don't show promotion match.

simonhong · 2022-05-13T01:42:19Z

We want to locate conversion ask entry at second or last in omnibox popup.
In omnibox framework, match is located by some factors such as type or relavance.
It seems difficult to set conversion entry visible always as a last entry because omnibox popup has a maximum number of visible entries at once. So, if more higher priority entries are added more, conversion entry could not be visible. Need to find concrete way to locate it as a last entry always.

One more thing - when user select promotion entry, that promotion url could be visibile again in omnibox popup later like other history entries. In this case, history entry should have more priority?

simonhong

@fallaciousreasoning Thanks for review! I think all of your comments are addressed or commented.

app/brave_generated_resources.grd

components/omnibox/browser/promotion_utils.cc

simonhong · 2022-05-31T06:17:49Z

components/omnibox/browser/promotion_utils.cc

+
+  // If first match is not from search query with default provider,
+  // it means there is more proper match from other providers.
+  // In this case, remove promotion match from |result|.


components/omnibox/browser/promotion_utils.cc

components/omnibox/browser/promotion_utils.h

fallaciousreasoning

LGTM but still worth having someone with a bit more experience in the brave codebase take a look.

simonhong · 2022-06-01T00:28:15Z

Kindly ping :)

petemill

In addition to the minor feedback below, I noticed a couple of UI issues:

There is an extra space character (" ") between the search term and the "- Brave Search" string.
The result row is not highlighting on keyboard arrow navigation, only on mouse hover, but the other chromium result rows are
- with mouse:
- with keyboard:

Similarly, I feel like with the banner option, the UX with the keyboard is a little strange. Using the keyboard I can press the down button and I know the banner is selected because the url changes to search.brave.com/xyz in the address bar, but there is no indication in the UI that it is the selected item. It does not have the left "selected indicator" nor does the background change via keyboard or mouse. I'm not sure if this is a design desicion so maybe we should ask @aguscruiz but I feel this banner row needs to either have a selection state, or not be selectable via keyboard.
Example here is where the banner is "selected" but it's not obvious in the UI:

petemill · 2022-06-03T04:01:27Z

browser/ui/views/omnibox/brave_search_conversion_promotion_view.cc

+
+  ResetChildrenVisibility();
+
+  if (type_ == ConversionType::kButton) {


Would it be simpler and easier to read to have 2 separate views - one for the "button" type and another for the "banner" type? Either as children for this view or as replacement for this view.

Actually, this view has two childrens for each types.
button_type_container_ and banner_type_container_ for button and banner type.
Based on type, only one is visibile in this promotion view. Added comment about this - 68d8c81

petemill · 2022-06-03T04:19:36Z

browser/ui/views/omnibox/brave_search_conversion_promotion_view.cc

+  // Use 14px font size for input text.
+  views::Label::CustomFont emphasized_font = {
+      GetFont(14, gfx::Font::Weight::SEMIBOLD)};
+  button_type_contents_input_ = input_container->AddChildView(


In the screenshot this looks like it's the same or very similar text format as the regular search result row. Perhaps you could explain here, or leave a comment in the code as to why we're not using OmniboxMatchCellView or even at the least OmniboxTextView for the "button" version? I'm not very familiar with the omnibox views, so sorry if it's obvious why we can't use them. But I would have hoped we could do less custom views, and extend the existing ones. I'm sure there's a good reason for you to do it like this and I guess it's maybe because we have the button on the right and not underneath, so maybe leave a code comment, perhaps at the beginning of the ConfigureForButtonType method.

Yeah, it seems very similar but has slightly different for our purpose such as layout (you mentioned - buttons posiiton), bg, text colors or font. So, I decided to implement whole UI. In this case, I could set our view to whole area easily by only controlling upstream's view and our view's visibilty w/o touching upstream views.

simonhong

@petemill On selection, selection indicator is only used and background color is not changed based on design spec. and only banner type spec(no indicator) is design decision.
cc @aguscruiz

simonhong · 2022-06-06T13:14:31Z

browser/ui/views/omnibox/brave_search_conversion_promotion_view.cc

+
+  ResetChildrenVisibility();
+
+  if (type_ == ConversionType::kButton) {


Actually, this view has two childrens for each types.
button_type_container_ and banner_type_container_ for button and banner type.
Based on type, only one is visibile in this promotion view. Added comment about this - 68d8c81

simonhong · 2022-06-06T13:15:44Z

browser/ui/views/omnibox/brave_search_conversion_promotion_view.cc

+  // Use 14px font size for input text.
+  views::Label::CustomFont emphasized_font = {
+      GetFont(14, gfx::Font::Weight::SEMIBOLD)};
+  button_type_contents_input_ = input_container->AddChildView(


Yeah, it seems very similar but has slightly different for our purpose such as layout (you mentioned - buttons posiiton), bg, text colors or font. So, I decided to implement whole UI. In this case, I could set our view to whole area easily by only controlling upstream's view and our view's visibilty w/o touching upstream views.

aguscruiz · 2022-06-06T14:05:17Z

@petemill On selection, selection indicator is only used and background color is not changed based on design spec. and only banner type spec(no indicator) is design decision. cc @aguscruiz

For option 3 (the small one) I think we can use the hover state as a focused state too, what do you think?

As for option 2, I've mocked it up here, sorry, didn't think of that state but absolutely, needs a focused state. It's a thicker stroke (2px instead of 1) and in a slightly darker tone

https://www.figma.com/file/8wncTFNBvKN7CS6LTXAMYs/Search-Conversion?node-id=770%3A38037

https://www.figma.com/file/8wncTFNBvKN7CS6LTXAMYs/Search-Conversion?node-id=770%3A38321

simonhong · 2022-06-08T01:13:39Z

Kindly ping to @brave/deps-reviewers @brave/patch-reviewers @brave/chromium-src-reviewers :)

goodov · 2022-06-13T11:25:40Z

patches/chrome-browser-ui-views-omnibox-omnibox_popup_contents_view.cc.patch

+       AddChildView(std::make_unique<OmniboxRowView>(
+           i, edit_model_,
+-          std::make_unique<OmniboxResultView>(this, edit_model_, i),
+          std::make_unique<BraveOmniboxResultView>(this, edit_model_, i),


maybe we can replace OmniboxResultView with BraveOmniboxResultView in header/cc files?

goodov · 2022-06-13T11:27:43Z

components/omnibox/browser/promotion_utils.cc

+using brave_search_conversion::ConversionType;
+
+void SortBraveSearchPromotionMatch(
+    AutocompleteResult* result,


mutable args should go last.

goodov · 2022-06-13T11:27:49Z

components/omnibox/browser/promotion_utils.cc

+    AutocompleteResult* result,
+    const AutocompleteInput& input,
+    brave_search_conversion::ConversionType type) {
+  if (result->size() == 0)


DCHECK(result);

goodov · 2022-06-13T11:30:10Z

components/omnibox/browser/promotion_unittest.cc

+  void SetUp() override {
+    SetMockLocale("en-US");
+    SetMockClient();
+    provider_ = new PromotionProvider(client_mock_.get());


base::MakeRefCounted

goodov · 2022-06-13T11:31:30Z

components/brave_search_conversion/utils.cc

+namespace brave_search_conversion {
+
+bool IsPromotionEnabledCountry(const std::string& country_code) {
+  static const base::flat_set<std::string> supported_country{"US", "CA", "DE",


base::NoDestructor or just a simple array: const base::StringPiece kSupportedCountries[].

Done with simple array.

goodov · 2022-06-13T11:36:55Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+// Move match at |from| to |to|.
+void AutocompleteResult::MoveMatch(size_t from, size_t to) {
+  DCHECK_LE(from, matches_.size());
+  DCHECK_LE(to, matches_.size());


should be DCHECK_LT, but actually we should do proper runtime if checks. currently this looks like a crash after some upstream change.

goodov · 2022-06-13T11:43:16Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+  RemoveMatch(matches_.begin() + from);
+
+  if (from < to)
+    to--;


if from == 0 and to == 1, will this work as expected?

goodov · 2022-06-13T11:47:20Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+  if (from < to)
+    to--;
+
+  matches_.insert(matches_.begin() + to, promotion);


let's rewrite this function via std::rotate with iterators, should be much safer. some examples:
https://chromium-review.googlesource.com/c/chromium/src/+/1526299
https://stackoverflow.com/a/57399634

Done with rotate. It works great! 👍🏼

goodov · 2022-06-13T11:49:19Z

chromium_src/chrome/browser/ui/views/omnibox/omnibox_result_view.h

+#define SetMatch                                    \
+  virtual SetMatch(const AutocompleteMatch& match); \
+  friend class BraveOmniboxResultView;              \
+  void SetMatchUnUsed


this way you don't need to copy-paste args and keep track of it on rebase:

#define SetMatch \ NotUsed(); \ friend class BraveOmniboxResultView; \ virtual void SetMatch

goodov · 2022-06-13T12:18:50Z

components/brave_search_conversion/utils.cc

+  if (prefs && prefs->GetBoolean(prefs::kDismissed))
+    return ConversionType::kNone;
+
+  if (service) {


We should force prefs and service with DCHECK(prefs); and DCHECK(service);, otherwise this function can be misused.

We can split it into two and have the second one generate ConversionType just by using static info (locale/country, features).

Added GetConversionType().

simonhong

@goodov Thanks for review. I think I address all comments except BraveOmniboxResultView patching. It's still difficult for me.

simonhong · 2022-06-14T00:49:12Z

chromium_src/chrome/browser/ui/views/omnibox/omnibox_result_view.h

+#define SetMatch                                    \
+  virtual SetMatch(const AutocompleteMatch& match); \
+  friend class BraveOmniboxResultView;              \
+  void SetMatchUnUsed


simonhong · 2022-06-14T01:28:11Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+  if (from < to)
+    to--;
+
+  matches_.insert(matches_.begin() + to, promotion);


Done with rotate. It works great! 👍🏼

components/brave_search_conversion/features.cc

simonhong · 2022-06-14T01:40:27Z

components/brave_search_conversion/utils.cc

+namespace brave_search_conversion {
+
+bool IsPromotionEnabledCountry(const std::string& country_code) {
+  static const base::flat_set<std::string> supported_country{"US", "CA", "DE",


Done with simple array.

simonhong · 2022-06-14T01:49:03Z

components/omnibox/browser/promotion_unittest.cc

+  void SetUp() override {
+    SetMockLocale("en-US");
+    SetMockClient();
+    provider_ = new PromotionProvider(client_mock_.get());


simonhong · 2022-06-14T01:49:14Z

components/omnibox/browser/promotion_utils.cc

+using brave_search_conversion::ConversionType;
+
+void SortBraveSearchPromotionMatch(
+    AutocompleteResult* result,


simonhong · 2022-06-14T01:49:21Z

components/omnibox/browser/promotion_utils.cc

+    AutocompleteResult* result,
+    const AutocompleteInput& input,
+    brave_search_conversion::ConversionType type) {
+  if (result->size() == 0)


simonhong · 2022-06-14T02:13:07Z

components/brave_search_conversion/utils.cc

+  if (prefs && prefs->GetBoolean(prefs::kDismissed))
+    return ConversionType::kNone;
+
+  if (service) {


Added GetConversionType().

goodov · 2022-06-14T07:38:04Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+// Move match to |index|.
+void AutocompleteResult::ReorderMatch(const ACMatches::iterator& it,
+                                      size_t index) {
+  const bool move_to_end = (index < 0) || (index >= size());


I believe index < 0 is always false. move to end works because (size_t)-1 creates max value which is always >= size(). maybe make it as a flag or pass an iterator instead? currently index doesn't do what it's declaring, this is confusing.

Oops, I fixed making index as int type.

goodov · 2022-06-14T07:54:48Z

components/brave_search_conversion/features.cc

+namespace features {
+
+// Brave search promotion match located at last low in omnibox popup.
+// This type seems more ads banner.


more like ads banner?

goodov · 2022-06-14T07:56:43Z

components/brave_search_conversion/utils.cc

+bool IsPromotionEnabledCountry(const std::string& country_code) {
+  constexpr base::StringPiece kSupportedCountries[] = {"US", "CA", "DE", "FR",
+                                                       "GB"};
+  return std::find(std::begin(kSupportedCountries),


base::Contains

components/brave_search_conversion/utils.cc

components/omnibox/browser/promotion_utils.cc

simonhong

@goodov With match's additional info, code becomes much simpler.
Thanks and PTAL again!

simonhong · 2022-06-14T10:28:52Z

components/brave_search_conversion/features.cc

+namespace features {
+
+// Brave search promotion match located at last low in omnibox popup.
+// This type seems more ads banner.


simonhong · 2022-06-14T10:34:05Z

components/brave_search_conversion/utils.cc

+bool IsPromotionEnabledCountry(const std::string& country_code) {
+  constexpr base::StringPiece kSupportedCountries[] = {"US", "CA", "DE", "FR",
+                                                       "GB"};
+  return std::find(std::begin(kSupportedCountries),


components/brave_search_conversion/utils.cc

simonhong · 2022-06-15T04:09:21Z

chromium_src/components/omnibox/browser/autocomplete_result.cc

+// Move match to |index|.
+void AutocompleteResult::ReorderMatch(const ACMatches::iterator& it,
+                                      size_t index) {
+  const bool move_to_end = (index < 0) || (index >= size());


Oops, I fixed making index as int type.

components/omnibox/browser/promotion_utils.cc

simonhong · 2022-06-15T04:26:53Z

@petemill @aguscruiz Updated screenshots that shows normal/hover or selected state.

goodov · 2022-06-15T07:53:52Z

components/brave_search_conversion/types.h

+enum class ConversionType {
+  kButton,
+  kBanner,
+  kNone,


nit: usually this goes as a first item (kNone == 0) in enums.

goodov · 2022-06-15T08:00:32Z

chromium_src/components/omnibox/browser/autocomplete_controller.cc

+  providers_.push_back(new TopSitesProvider(provider_client_.get()));       \
+  providers_.push_back(new SuggestedSitesProvider(provider_client_.get())); \
+  if (!provider_client_->IsOffTheRecord())                                  \
+    providers_.push_back(new PromotionProvider(provider_client_.get()));


@simonhong I think one more thing that we can add into this PR is to skip this and other Init-like places when both features are turned off to make things safe in case we missed something.
you can make a bool function like bool IsBraveSearchConversionEnabled() and insert it into some places to protect ourselves.

Ok. Added two places. here and UI side (https://github.com/brave/brave-core/pull/12846/files#diff-18e8763975abf287e0ef660d283f3485f5672dd3262390d753b738b132d58262R43).
I thinks double checking at both places (data provider and consumer) would be sufficient.

PromotionProvider Provides the autocomplete match for conversion promotion. And AutocompleteController adjust that match position based on conversion type.

Implement BraveOmniboxResultView by subclassing OmniboxResultView to provide promotion specfic views. BraveOmniboxResultView controls visibility upstream children and promotion children based on match. When promotion match is set, upstream children are all hidden and promotion children get visible.

Deleted chrome/test dependency from brave/components/search_engines. As //brave/components/search_engines target is included by /components/omnibox/browser due to brave search promotion, this deps caused error.

petemill

Nice work. UI looks really nice and polished.

One very minor issue I'm seeing that might be a general issue with the omnibox: Sometimes when I move the mouse from outside the item (for the Button version) to inside and then to outside again, the hover "sticks"

Another non-related issue, but worth noting is that the "set as default" button on brave search is always showing, yet it won't always work since the relevant browser API is limited to a certain amount of calls per day. The website needs to call brave.getCanSetDefaultSearchProvider first to know whether to show the button or not.

Removed extra space between search term and Brave Search string. Added some comments about promotion view's children. Updated search promotion url Added more UI interaction for hover/selected state Updated test for checking promotion is not added in private profile

simonhong · 2022-06-16T03:58:01Z

Squashed into 4 commits and merged.

simonhong self-assigned this Mar 31, 2022

simonhong force-pushed the brave_search_conversion branch 3 times, most recently from a0da9e3 to 6659cc3 Compare May 12, 2022 07:54

simonhong force-pushed the brave_search_conversion branch 14 times, most recently from 6c92777 to 648b01d Compare May 20, 2022 02:54

simonhong force-pushed the brave_search_conversion branch 10 times, most recently from 7f1bd18 to a54829b Compare May 26, 2022 07:19

simonhong changed the title ~~WIP: Brave search conversion~~ Brave search conversion May 26, 2022

simonhong commented May 31, 2022

View reviewed changes

simonhong force-pushed the brave_search_conversion branch from f22922e to e984d9a Compare May 31, 2022 11:26

fallaciousreasoning approved these changes May 31, 2022

View reviewed changes

petemill requested changes Jun 3, 2022

View reviewed changes

simonhong commented Jun 6, 2022

View reviewed changes

simonhong force-pushed the brave_search_conversion branch from 1fb90bf to c6bdf33 Compare June 7, 2022 04:03

simonhong requested a review from petemill June 7, 2022 04:04

simonhong force-pushed the brave_search_conversion branch from c6bdf33 to 840832b Compare June 12, 2022 23:58

goodov reviewed Jun 13, 2022

View reviewed changes

simonhong commented Jun 14, 2022

View reviewed changes

goodov reviewed Jun 14, 2022

View reviewed changes

simonhong commented Jun 15, 2022

View reviewed changes

simonhong force-pushed the brave_search_conversion branch from 21e34c9 to 30dc2c9 Compare June 15, 2022 05:40

goodov approved these changes Jun 15, 2022

View reviewed changes

goodov reviewed Jun 15, 2022

View reviewed changes

simonhong added 3 commits June 15, 2022 17:01

Brave search conversion promotion - Non UI part

76f7f02

PromotionProvider Provides the autocomplete match for conversion promotion. And AutocompleteController adjust that match position based on conversion type.

Fixed build failure on iOS PR builder

23eb9e5

Deleted chrome/test dependency from brave/components/search_engines. As //brave/components/search_engines target is included by /components/omnibox/browser due to brave search promotion, this deps caused error.

simonhong force-pushed the brave_search_conversion branch from 30dc2c9 to 0eb41e9 Compare June 15, 2022 08:35

petemill approved these changes Jun 16, 2022

View reviewed changes

simonhong force-pushed the brave_search_conversion branch from 0eb41e9 to 983694e Compare June 16, 2022 03:57

simonhong merged commit f279bb5 into master Jun 16, 2022

simonhong deleted the brave_search_conversion branch June 16, 2022 03:58

github-actions bot added this to the 1.41.x - Nightly milestone Jun 16, 2022


		ResetChildrenVisibility();

		if (type_ == ConversionType::kButton) {

Brave search conversion promotion in omnibox #12846

Brave search conversion promotion in omnibox #12846

Conversation

simonhong commented Mar 31, 2022 • edited by petemill Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simonhong commented May 13, 2022 • edited Loading

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

simonhong commented Jun 1, 2022

petemill left a comment • edited by simonhong Loading

Choose a reason for hiding this comment

petemill Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguscruiz commented Jun 6, 2022 • edited Loading

simonhong commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

simonhong commented Jun 16, 2022

simonhong commented Mar 31, 2022 •

edited by petemill

Loading

simonhong commented May 13, 2022 •

edited

Loading

petemill left a comment •

edited by simonhong

Loading

petemill Jun 3, 2022 •

edited

Loading

petemill Jun 3, 2022 •

edited

Loading

aguscruiz commented Jun 6, 2022 •

edited

Loading