-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
omnibox brave search promo banner android #14350
Conversation
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make these in alphabetical order
parent.getContext(), R.layout.omnibox_basic_suggestion), | ||
new PedalSuggestionViewBinder<View>(SuggestionViewViewBinder::bind)); | ||
|
||
adapter.registerType(BraveOmniboxSuggestionUiType.BRAVE_SEARCH_PROMO_BANNER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
8896658
to
68091b8
Compare
68091b8
to
54c44d2
Compare
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this one to testMethodsForInvocationExist
and make sure parameters are still the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will do it
|
||
private boolean isBraveSearchPromoBanner() { | ||
Tab activeTab = mActivityTabSupplier.get(); | ||
if (ChromeFeatureList.isEnabled("BraveSearchOmniboxBanner") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define feature name as constant instead of hardcoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
6bf3746
to
c9b77b0
Compare
c9b77b0
to
ed85fae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
chromium_src/chrome/browser/DEPS
Outdated
@@ -13,6 +13,7 @@ include_rules = [ | |||
"+brave/components/brave_shields", | |||
"+brave/components/brave_sync", | |||
"+brave/components/brave_today", | |||
"+brave/components/brave_search_conversion", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place in alphabetical order, please.
@@ -12,6 +13,7 @@ | |||
#define kForceWebContentsDarkMode kForceWebContentsDarkMode, \ | |||
&brave_rewards::features::kBraveRewards, \ | |||
&brave_today::features::kBraveNewsFeature, \ | |||
&brave_search_conversion::features::kOmniboxBanner, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order here as well, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src
& DEPS
LGTM with a couple of formatting nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new design that was discussed via Slack was verified by QA via #14778 (comment) 👍 |
Verification PASSED on
|
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
orPoland
) - launched brave and added
--enable-features=BraveSearchOmniboxBanner
intoCommand Line String
viaQA 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 theMaybe Later
as per omnibox brave search promo banner android #14350 (comment)
- ensure that the
- 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
orMX
- launched brave and changed the default SE from
Brave
to anything else within the list (Google, DDG, Startpage etc..) - added
--enable-features=BraveSearchOmniboxBanner
intoCommand Line String
viaQA 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
Also found brave/brave-browser#25059 while running through the above cases. |
Resolves brave/brave-browser#23062
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: