omnibox brave search promo banner android #14350

tapanmodh · 2022-07-27T07:21:40Z

Resolves brave/brave-browser#23062

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have [requested] https://github.com/brave/security/issues/878 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:

When a user types in the url bar, we will place a Brave Search banner at the bottom of the omnibox.
When a user selects “Try Brave Search”, they will be taken to the Brave Search SERP with their intended query.
Pressing “Maybe later” on the banner will hide the banner for the current session. On the next session, the banner will appear again but “Maybe later” will be replaced with “Dismiss”. If a user selects Dismiss, the banner won’t be displayed again.
We will continue to show this banner until the user has made Brave Search their default search engine or 15 days
On the Brave Search SERP, if a user pressed “Set Brave Search as default” the Brave search engine should be set as default

samartnik · 2022-07-27T14:51:14Z

build/android/bytecode/java/org/brave/bytecode/BraveClassAdapter.java

@@ -20,6 +20,9 @@ public static ClassVisitor createAdapter(ClassVisitor chain) {
        chain = new BraveCommandLineInitUtilClassAdapter(chain);
        chain = new BraveContentSettingsResourcesClassAdapter(chain);
        chain = new BraveCustomizationProviderDelegateImplClassAdapter(chain);
+        chain = new BraveDropdownItemViewInfoListManagerClassAdapter(chain);


Please make these in alphabetical order

samartnik · 2022-07-27T15:08:02Z

...x/java/src/org/chromium/chrome/browser/omnibox/suggestions/BraveAutocompleteCoordinator.java

+                                parent.getContext(), R.layout.omnibox_basic_suggestion),
+                        new PedalSuggestionViewBinder<View>(SuggestionViewViewBinder::bind));
+
+                adapter.registerType(BraveOmniboxSuggestionUiType.BRAVE_SEARCH_PROMO_BANNER,


Apart form this the rest seems to be the copy of the parent. Can we extend SuggestionListViewHolder and get there to the adapter and register type in BraveSuggestionListViewHolder?

...rc/org/chromium/chrome/browser/omnibox/suggestions/BraveDropdownItemViewInfoListBuilder.java

samartnik · 2022-08-04T12:52:18Z

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

@@ -373,6 +390,9 @@ public void testMethodsExist() throws Exception {
        Assert.assertTrue(methodExists(
                "org/chromium/chrome/browser/suggestions/tile/MostVisitedTilesMediator",
                "updateTilePlaceholderVisibility", true, void.class));
+        Assert.assertTrue(methodExists(
+                "org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator",
+                "createViewProvider", false, null));


We should move this one to testMethodsForInvocationExist and make sure parameters are still the same.

sure, will do it

samartnik · 2022-08-04T13:00:20Z

...rc/org/chromium/chrome/browser/omnibox/suggestions/BraveDropdownItemViewInfoListBuilder.java

+
+    private boolean isBraveSearchPromoBanner() {
+        Tab activeTab = mActivityTabSupplier.get();
+        if (ChromeFeatureList.isEnabled("BraveSearchOmniboxBanner")


We should define feature name as constant instead of hardcoding.

samartnik

lgtm

SergeyZhukovsky

++

mkarolin · 2022-08-08T17:48:21Z

chromium_src/chrome/browser/DEPS

@@ -13,6 +13,7 @@ include_rules = [
  "+brave/components/brave_shields",
  "+brave/components/brave_sync",
  "+brave/components/brave_today",
+  "+brave/components/brave_search_conversion",


Place in alphabetical order, please.

mkarolin · 2022-08-08T17:49:23Z

chromium_src/chrome/browser/flags/android/chrome_feature_list.cc

@@ -12,6 +13,7 @@
 #define kForceWebContentsDarkMode kForceWebContentsDarkMode, \
    &brave_rewards::features::kBraveRewards,                 \
    &brave_today::features::kBraveNewsFeature,               \
+    &brave_search_conversion::features::kOmniboxBanner,      \


Alphabetical order here as well, please.

mkarolin

chromium_src & DEPS LGTM with a couple of formatting nits.

tmancey

LGTM

deeppandya

LGTM

kjozwiak · 2022-08-29T20:53:56Z

The new design that was discussed via Slack was verified by QA via #14778 (comment) 👍

kjozwiak · 2022-08-30T02:18:28Z

Verification PASSED on Samsung S10+ running Android 12 using the following build(s):

Brave | 1.45.9 Chromium: 105.0.5195.68 (Official Build) canary (32-bit)
--- | ---
Revision | ad13e82529051bac6a0e65f455e6d7a1e5fd7938-refs/branch-heads/5195@{#903}
OS | Android 12; Build/SP1A.210812.016

`Test Case #1` (Supported Regions - US/CA/DE/FR/UK/AT/ES/MX)

installed 1.45.9 Chromium: 105.0.5195.68
ensured that the system language is set to either US, CA, DE, FR, UK, AT, ES or MX
launched brave and changed the default SE from Brave to anything else within the list (Google, DDG, Startpage etc..)
added --enable-features=BraveSearchOmniboxBanner into Command Line String via QA Preferences
re-launched the browser tapped on the URL bar and ensured that the Brave search promo is displayed via the omnibox
ensured that the Brave search promo appears when tapping/editing a URL and the promo hasn't been closed/dismissed
ensured that the Brave search promo doesn't appear if Private is set to another SE but Standard is still selected as Brave
ensured that tapping on Maybe Later closes the promo within the omnibox and doesn't appear again within the session
- ensured that it doesn't appear via the omnibox via NTP when starting to enter a URL/search term
- ensured that tapping on a URL in an existing tab doesn't display the promo
ensured that tapping on Try Brave Search opens https://search.brave.com/search?q=motorsport&action=makeDefault
- ensured that the search term is also appended as per search?q=motorsport
- ensured that tapping on Try Brave Search adds action=makeDefault into the URL

Canada

`Example`	`Example`	`Example`	`Example`	`Example`	`Example`	`Example`

Mexico

`Example`	`Example`	`Example`	`Example`	`Example`	`Example`	`Example`

`Test Case #2` (Regions that don't have Brave Search as default SE/Still use SE onboarding)

installed 1.45.9 Chromium: 105.0.5195.68
ensured that the system language is set to the correct locale (in this case, either Japan or Poland)
launched brave and added --enable-features=BraveSearchOmniboxBanner into Command Line String via QA Preferences
re-launched the browser tapped on the URL bar twice and ensured that the SE onboarding screen is displayed
ensured that Google was selected as the default via the onboarding screen
ensured that Google is default SE for both Standard & Private windows once the onboarding screen has been dismissed
ensured that the Brave search promo isn't appearing via the omnibox when a text/search term is entered

Japan

`Example`	`Example`	`Example`	`Example`

Poland

`Example`	`Example`	`Example`	`Example`

`Test Case #3` (Maybe Later & Dismiss)

ensured that closing the Brave search promo via Maybe Later doesn't re-appear in the same session
- ensure the promo is not re-appearing when typing into the omnibox via NTP
- ensure the promo is not re-appearing when taping on a URL on an existing opened tab
ensured that the promo re-appears again when the browser is re-launched after tapping on Maybe Later
- ensure that the Dismiss button is visible and not the Maybe Later as per omnibox brave search promo banner android #14350 (comment)
ensured that once a user taps/clicks on Dismiss, the promo never re-appears (tried via several re-launches)

`Example`	`Example`	`Example`	`Example`

`Test Case #4` (Automatically dismiss after 15 days)

installed 1.45.9 Chromium: 105.0.5195.68
ensured that the system language is set to either US, CA, DE, FR, UK, AT, ES or MX
launched brave and changed the default SE from Brave to anything else within the list (Google, DDG, Startpage etc..)
added --enable-features=BraveSearchOmniboxBanner into Command Line String via QA Preferences
re-launched the browser tapped on the URL bar and ensured that the Brave search promo is displayed via the omnibox
moved the time/date on the device ahead ~16 days and re-launched the browser
ensured that the Brave search promo doesn't appear via the omnibox when tapping via NTP
ensured that the Brave search promo doesn't appear when tapping on the omnibox if an already opened tab

kjozwiak · 2022-08-30T02:42:07Z

Also found brave/brave-browser#25059 while running through the above cases.

tapanmodh added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Jul 27, 2022

tapanmodh added this to the 1.44.x - Nightly milestone Jul 27, 2022

tapanmodh requested a review from deeppandya July 27, 2022 07:21

tapanmodh requested review from samartnik and a team as code owners July 27, 2022 07:21

tapanmodh self-assigned this Jul 27, 2022

tapanmodh requested a review from a team as a code owner July 27, 2022 14:43

samartnik requested changes Jul 27, 2022

View reviewed changes

tapanmodh added 2 commits August 3, 2022 01:43

omnibox brave search promo banner android

b5beef9

add brave_search_conversion in deps

1b7e84b

tapanmodh force-pushed the search_engine_default_promo_android branch 5 times, most recently from 8896658 to 68091b8 Compare August 3, 2022 20:52

Address PR Comments

54c44d2

tapanmodh force-pushed the search_engine_default_promo_android branch from 68091b8 to 54c44d2 Compare August 4, 2022 08:05

samartnik requested changes Aug 4, 2022

View reviewed changes

move BraveFeatureList class to base package

f799219

tapanmodh requested a review from a team as a code owner August 4, 2022 20:54

tapanmodh requested a review from SergeyZhukovsky August 4, 2022 20:55

samartnik approved these changes Aug 4, 2022

View reviewed changes

tapanmodh force-pushed the search_engine_default_promo_android branch 4 times, most recently from 6bf3746 to c9b77b0 Compare August 5, 2022 12:56

fix bytecode test error

ed85fae

tapanmodh force-pushed the search_engine_default_promo_android branch from c9b77b0 to ed85fae Compare August 5, 2022 13:36

SergeyZhukovsky approved these changes Aug 5, 2022

View reviewed changes

mkarolin reviewed Aug 8, 2022

View reviewed changes

mkarolin approved these changes Aug 8, 2022

View reviewed changes

tmancey approved these changes Aug 8, 2022

View reviewed changes

Address PR Comments

b99e632

deeppandya approved these changes Aug 8, 2022

View reviewed changes

tapanmodh merged commit b71f6c6 into master Aug 9, 2022

tapanmodh deleted the search_engine_default_promo_android branch August 9, 2022 04:53

brave-builds mentioned this pull request Aug 9, 2022

omnibox brave search promo banner android (uplift to 1.43.x) #14542

Merged

7 tasks

deeppandya pushed a commit that referenced this pull request Aug 24, 2022

Uplift of #14350 (squashed) to beta

21c4787

kjozwiak mentioned this pull request Aug 30, 2022

crash in certain cases when enabling search promo and Show suggested sites in autocomplete brave/brave-browser#25059

Closed

deeppandya pushed a commit that referenced this pull request Aug 30, 2022

Uplift of #14350 (squashed) to beta

67bfe84

This was referenced Aug 31, 2022

fix crash omnibox search banner android #14886

Merged

[Android] Search Engine Default Promo brave/brave-browser#23062

Closed

Remove dynamic margin #14911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omnibox brave search promo banner android #14350

omnibox brave search promo banner android #14350

tapanmodh commented Jul 27, 2022 •

edited by deeppandya

Loading

samartnik Jul 27, 2022

samartnik Jul 27, 2022

samartnik Aug 4, 2022

tapanmodh Aug 4, 2022

samartnik Aug 4, 2022

samartnik left a comment

SergeyZhukovsky left a comment

mkarolin Aug 8, 2022

mkarolin Aug 8, 2022

mkarolin left a comment

tmancey left a comment

deeppandya left a comment

kjozwiak commented Aug 29, 2022

kjozwiak commented Aug 30, 2022 •

edited

Loading

kjozwiak commented Aug 30, 2022

omnibox brave search promo banner android #14350

omnibox brave search promo banner android #14350

Conversation

tapanmodh commented Jul 27, 2022 • edited by deeppandya Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

samartnik Jul 27, 2022

Choose a reason for hiding this comment

samartnik Jul 27, 2022

Choose a reason for hiding this comment

samartnik Aug 4, 2022

Choose a reason for hiding this comment

tapanmodh Aug 4, 2022

Choose a reason for hiding this comment

samartnik Aug 4, 2022

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

mkarolin Aug 8, 2022

Choose a reason for hiding this comment

mkarolin Aug 8, 2022

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

tmancey left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

kjozwiak commented Aug 29, 2022

kjozwiak commented Aug 30, 2022 • edited Loading

Test Case #1 (Supported Regions - US/CA/DE/FR/UK/AT/ES/MX)

Test Case #2 (Regions that don't have Brave Search as default SE/Still use SE onboarding)

Test Case #3 (Maybe Later & Dismiss)

Test Case #4 (Automatically dismiss after 15 days)

kjozwiak commented Aug 30, 2022

tapanmodh commented Jul 27, 2022 •

edited by deeppandya

Loading

kjozwiak commented Aug 30, 2022 •

edited

Loading

`Test Case #1` (Supported Regions - US/CA/DE/FR/UK/AT/ES/MX)

`Test Case #2` (Regions that don't have Brave Search as default SE/Still use SE onboarding)

`Test Case #3` (Maybe Later & Dismiss)

`Test Case #4` (Automatically dismiss after 15 days)