Added search promotion button in omnibox #25409

simonhong · 2024-09-03T07:28:30Z

fix brave/brave-browser#40776

Resolves

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 wiki
- npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Manual test: see the linked issue
BraveLocationBarViewBrowserTest.SearchConversionButtonTest

simonhong · 2024-09-12T11:42:34Z

chromium_src/chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -36,6 +36,15 @@
    add_trailing_decoration(item, /*intra_item_padding=*/0);    \
  }

+#define BRAVE_LAYOUT_HANDLE_SEARCH_PROMOTION_BUTTON_VISIBILITY              \
+  }                                                                         \
+  else if (GetSearchPromotionButton() &&                                    \


Can't avoid this presubmit error. @goodov can you advice how should I handle this?

brave/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view.cc:41: (cpplint) If an else has a brace on one side, it should have it on both [readability/braces] [5]

Would combining this line with the above help? Or does that trigger the formatter?

Right, it's the formatter. Even with that style, still presubmit has same complaints.

// NOLINT ?

wow, NOLINT works! thanks.

github-actions · 2024-09-13T01:47:41Z

browser/ui/views/location_bar/brave_search_conversion/promotion_button_view.cc

+
+  // Unretained here is safe as |close_button| is the child of this class.
+  close_button->SetCallback(base::BindOnce(&PromotionButtonView::OnClosePressed,
+                                           base::Unretained(this)));


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

github-actions · 2024-09-13T01:47:41Z

browser/ui/views/location_bar/brave_search_conversion/promotion_button_view.cc

+
+  // Unretained here is safe as this callback is used by itself.
+  SetCallback(base::BindOnce(&PromotionButtonView::OnButtonPressed,
+                             base::Unretained(this)));


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

fallaciousreasoning

lgtm % nits

browser/ui/color/brave_color_mixer.cc

browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc

fallaciousreasoning · 2024-09-13T13:23:45Z

browser/ui/views/location_bar/brave_search_conversion/promotion_button_view.h

+  base::OnceClosure dismissed_callback_;
+  base::OnceClosure make_default_callback_;


I take it that its fine to have these as OnceClosures because the button hides after being interacted with?

Thanks for catching here.
Right, OnceClosure fits for dismissed_callback_ as this button is not shown again after dismissed.
However, make_default_callback_ should be RepeatingClosure because we show this button again
if user set to non brave search again.
@rebron when user clicks this button, brave search is set as a default and not set dismissed bit.
So, user will see this promotion button again if user change to another search provider again.
Is this what we want?

We shouldn't show the promotion button again if the user already set or dismissed it already.

browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.h

browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc

fallaciousreasoning · 2024-09-13T13:33:03Z

chromium_src/chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -36,6 +36,15 @@
    add_trailing_decoration(item, /*intra_item_padding=*/0);    \
  }

+#define BRAVE_LAYOUT_HANDLE_SEARCH_PROMOTION_BUTTON_VISIBILITY              \
+  }                                                                         \
+  else if (GetSearchPromotionButton() &&                                    \


Would combining this line with the above help? Or does that trigger the formatter?

github-actions · 2024-09-14T05:09:47Z

browser/ui/views/location_bar/brave_search_conversion/promotion_button_view.cc

+
+  // Unretained here is safe as |close_button| is the child of this class.
+  close_button->SetCallback(base::BindOnce(&PromotionButtonView::OnClosePressed,
+                                           base::Unretained(this)));


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

goodov · 2024-09-16T12:18:16Z

patches/chrome-browser-ui-views-location_bar-location_bar_view.cc.patch

+       }
+       selected_keyword_view_->SetCustomImage(image);
+     }
+  BRAVE_LAYOUT_HANDLE_SEARCH_PROMOTION_BUTTON_VISIBILITY


nit: BRAVE_LOCATION_BAR_VIEW_LAYOUT_... (all defines in this file can be updated).

mkarolin

strings++

fix brave/brave-browser#40776

github-actions · 2024-09-17T05:41:14Z

[puLL-Merge] - brave/brave-core@25409

Description

This PR adds a new search conversion promotion button to the location bar in Brave browser. The button aims to encourage users to switch to Brave Search as their default search engine.

Changes

Changes

app/brave_generated_resources.grd:
- Added a new message string for the search conversion promotion button label.
app/theme/brave_theme_resources.grd:
- Added a new image resource for the Brave Search icon in the conversion button.
browser/ui/BUILD.gn:
- Added new source files for the promotion button controller and view.
browser/ui/color/brave_color_id.h and browser/ui/color/brave_color_mixer.cc:
- Added new color definitions for the search conversion button.
browser/ui/views/location_bar/brave_location_bar_view.cc and .h:
- Integrated the new promotion button into the location bar view.
- Added logic to show/hide the button based on certain conditions.
New files:
- browser/ui/views/location_bar/brave_search_conversion/promotion_button_controller.cc and .h:
  - Implements the controller logic for the promotion button.
- browser/ui/views/location_bar/brave_search_conversion/promotion_button_view.cc and .h:
  - Implements the view for the promotion button.
browser/ui/views/location_bar/brave_location_bar_view_browsertest.cc:
- Added browser tests for the new search conversion button functionality.
components/brave_search_conversion/features.cc and .h:
- Added a new feature flag for the omnibox promotion button.
components/vector_icons/BUILD.gn:
- Added new vector icons for the promotion button.
patches/chrome-browser-ui-views-location_bar-location_bar_view.cc.patch:
- Modified the layout of the location bar to include the new promotion button.
test/BUILD.gn:
- Added the new browser tests to the build configuration.

Possible Issues

The animation for the promotion button expansion might cause performance issues on low-end devices.
The button's visibility logic might interfere with other location bar elements in certain scenarios.

Security Hotspots

None identified. The changes primarily involve UI elements and don't directly interact with sensitive data or security-critical components.

simonhong self-assigned this Sep 3, 2024

simonhong added the CI/skip Do not run CI builds (except noplatform) label Sep 3, 2024

simonhong force-pushed the search_conversion_button branch 11 times, most recently from 1ea573f to cad4c85 Compare September 11, 2024 03:07

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Sep 11, 2024

simonhong force-pushed the search_conversion_button branch 5 times, most recently from 4586a2f to d06f456 Compare September 12, 2024 11:39

simonhong marked this pull request as ready for review September 12, 2024 11:40

simonhong requested review from a team as code owners September 12, 2024 11:40

simonhong commented Sep 12, 2024

View reviewed changes

simonhong requested a review from fallaciousreasoning September 12, 2024 11:42

simonhong force-pushed the search_conversion_button branch from d06f456 to 218f4a3 Compare September 13, 2024 01:43

github-actions bot added the puLL-Merge label Sep 13, 2024

github-actions bot reviewed Sep 13, 2024

View reviewed changes

github-actions bot added the needs-security-review label Sep 13, 2024

github-actions bot assigned goodov Sep 13, 2024

github-actions bot assigned iefremov and thypon Sep 13, 2024

fallaciousreasoning reviewed Sep 13, 2024

View reviewed changes

fallaciousreasoning approved these changes Sep 13, 2024

View reviewed changes

simonhong force-pushed the search_conversion_button branch from 218f4a3 to 432e061 Compare September 14, 2024 05:05

github-actions bot reviewed Sep 14, 2024

View reviewed changes

goodov reviewed Sep 16, 2024

View reviewed changes

goodov approved these changes Sep 16, 2024

View reviewed changes

mkarolin approved these changes Sep 16, 2024

View reviewed changes

Added search promotion button in omnibox

901cb2b

fix brave/brave-browser#40776

simonhong force-pushed the search_conversion_button branch from 432e061 to 901cb2b Compare September 17, 2024 05:40

simonhong merged commit 5287f50 into master Sep 17, 2024
17 checks passed

simonhong deleted the search_conversion_button branch September 17, 2024 07:11

github-actions bot added this to the 1.72.x - Nightly milestone Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added search promotion button in omnibox #25409

Added search promotion button in omnibox #25409

simonhong commented Sep 3, 2024 •

edited

Loading

simonhong Sep 12, 2024

fallaciousreasoning Sep 13, 2024

simonhong Sep 14, 2024

goodov Sep 16, 2024

simonhong Sep 17, 2024

github-actions bot Sep 13, 2024

github-actions bot Sep 13, 2024

fallaciousreasoning left a comment

fallaciousreasoning Sep 13, 2024

simonhong Sep 14, 2024

rebron Sep 16, 2024

fallaciousreasoning Sep 13, 2024

github-actions bot Sep 14, 2024

goodov Sep 16, 2024

simonhong Sep 17, 2024

mkarolin left a comment

github-actions bot commented Sep 17, 2024

Changes

		base::OnceClosure dismissed_callback_;
		base::OnceClosure make_default_callback_;

Added search promotion button in omnibox #25409

Added search promotion button in omnibox #25409

Conversation

simonhong commented Sep 3, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

github-actions bot Sep 13, 2024

Choose a reason for hiding this comment

github-actions bot Sep 13, 2024

Choose a reason for hiding this comment

fallaciousreasoning 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

github-actions bot Sep 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024

Description

Changes

Possible Issues

Security Hotspots

simonhong commented Sep 3, 2024 •

edited

Loading