Skip to content
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.

Sign up for GitHub

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

Update default search for 5 regions #10326

Merged
merged 3 commits into from
Oct 5, 2021
Merged

Update default search for 5 regions #10326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f55f43c
Update default search engines for CA/DE/FR/GB/US
bsclifton Sep 28, 2021
2058682
Update Desktop onboarding to skip Search card for specific regions
bsclifton Sep 29, 2021
5f72f12
Remove onboarding screen for brave default serach engine
deeppandya Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion android/java/org/chromium/chrome/browser/toolbar/top/BraveToolbarLayoutImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
import org.chromium.url.GURL;

import java.net.URL;
import java.util.Arrays;
import java.util.Calendar;
import java.util.Date;
import java.util.EnumSet;
Expand All @@ -132,6 +133,8 @@ public abstract class BraveToolbarLayoutImpl extends ToolbarLayout
BraveRewardsObserver, BraveRewardsNativeWorker.PublisherObserver {
public static final String PREF_HIDE_BRAVE_REWARDS_ICON = "hide_brave_rewards_icon";
private static final String JAPAN_COUNTRY_CODE = "JP";
private static final List<String> mBraveSearchEngineDefaultRegions =
Arrays.asList("CA", "DE", "FR", "GB", "US");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides these, I think we also need remove the US/CA restriction we added in 4808157 (to android/java/org/chromium/chrome/browser/onboarding/SearchEngineOnboardingFragment.java)

That way we can show Brave Search for all other regions (which don't have Brave Search as default)

Copy link
Member Author

@bsclifton bsclifton Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - followed up with @ecnmst - no change needed here for now 😄

Not backing out the region part in 4808157 means that Brave Search will not show in search onboarding for regions outside CA/DE/FR/GB/US and that is OK for now

private static final long MB_10 = 10000000;
private static final long MINUTES_10 = 10 * 60 * 1000;
private static final int URL_FOCUS_TOOLBAR_BUTTONS_TRANSLATION_X_DP = 10;
Expand Down Expand Up @@ -939,12 +942,14 @@ public boolean onLongClick(View v) {
@Override
public void onUrlFocusChange(boolean hasFocus) {
Context context = getContext();
String countryCode = Locale.getDefault().getCountry();
if (hasFocus && PackageUtils.isFirstInstall(context)
&& BraveActivity.getBraveActivity() != null
&& BraveActivity.getBraveActivity().getActivityTab() != null
&& UrlUtilities.isNTPUrl(
BraveActivity.getBraveActivity().getActivityTab().getUrl().getSpec())
&& !OnboardingPrefManager.getInstance().hasSearchEngineOnboardingShown()) {
&& !OnboardingPrefManager.getInstance().hasSearchEngineOnboardingShown()
&& !mBraveSearchEngineDefaultRegions.contains(countryCode)) {
Intent searchActivityIntent = new Intent(context, SearchActivity.class);
context.startActivity(searchActivityIntent);
}
Expand Down
15 changes: 12 additions & 3 deletions chromium_src/components/search_engines/brave_template_url_prepopulate_data_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,24 @@ TEST_F(BraveTemplateURLPrepopulateDataTest, ProvidersFromPrepopulated) {

// Verifies default search provider for locale
TEST_F(BraveTemplateURLPrepopulateDataTest, DefaultSearchProvidersForUSA) {
CheckForCountry('U', 'S', "Google");
CheckForCountry('U', 'S', "Brave");
}

TEST_F(BraveTemplateURLPrepopulateDataTest, DefaultSearchProvidersForGermany) {
CheckForCountry('D', 'E', "Google");
CheckForCountry('D', 'E', "Brave");
}

TEST_F(BraveTemplateURLPrepopulateDataTest, DefaultSearchProvidersForFrance) {
CheckForCountry('F', 'R', "Qwant");
CheckForCountry('F', 'R', "Brave");
}

TEST_F(BraveTemplateURLPrepopulateDataTest,
DefaultSearchProvidersForGreatBritain) {
CheckForCountry('G', 'B', "Brave");
}

TEST_F(BraveTemplateURLPrepopulateDataTest, DefaultSearchProvidersForCanada) {
CheckForCountry('C', 'A', "Brave");
}

TEST_F(BraveTemplateURLPrepopulateDataTest,
Expand Down
69 changes: 52 additions & 17 deletions chromium_src/components/search_engines/template_url_prepopulate_data.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,36 +59,30 @@ const std::vector<BravePrepopulatedEngineID> brave_engines_with_yandex = {
};

const std::vector<BravePrepopulatedEngineID> brave_engines_DE = {
PREPOPULATED_ENGINE_ID_DUCKDUCKGO_DE, PREPOPULATED_ENGINE_ID_BRAVE,
PREPOPULATED_ENGINE_ID_QWANT, PREPOPULATED_ENGINE_ID_GOOGLE,
PREPOPULATED_ENGINE_ID_BING, PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_BRAVE, PREPOPULATED_ENGINE_ID_DUCKDUCKGO_DE,
PREPOPULATED_ENGINE_ID_QWANT, PREPOPULATED_ENGINE_ID_GOOGLE,
PREPOPULATED_ENGINE_ID_BING, PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_ECOSIA,
};

const std::vector<BravePrepopulatedEngineID> brave_engines_FR = {
PREPOPULATED_ENGINE_ID_QWANT, PREPOPULATED_ENGINE_ID_BRAVE,
PREPOPULATED_ENGINE_ID_BRAVE, PREPOPULATED_ENGINE_ID_QWANT,
PREPOPULATED_ENGINE_ID_GOOGLE, PREPOPULATED_ENGINE_ID_DUCKDUCKGO,
PREPOPULATED_ENGINE_ID_BING, PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_ECOSIA,
};

const std::vector<BravePrepopulatedEngineID> brave_engines_AU_IE = {
PREPOPULATED_ENGINE_ID_DUCKDUCKGO_AU_NZ_IE,
PREPOPULATED_ENGINE_ID_BRAVE,
PREPOPULATED_ENGINE_ID_GOOGLE,
PREPOPULATED_ENGINE_ID_QWANT,
PREPOPULATED_ENGINE_ID_BING,
PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_BRAVE, PREPOPULATED_ENGINE_ID_DUCKDUCKGO_AU_NZ_IE,
PREPOPULATED_ENGINE_ID_GOOGLE, PREPOPULATED_ENGINE_ID_QWANT,
PREPOPULATED_ENGINE_ID_BING, PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_ECOSIA,
};

const std::vector<BravePrepopulatedEngineID> brave_engines_NZ = {
PREPOPULATED_ENGINE_ID_DUCKDUCKGO_AU_NZ_IE,
PREPOPULATED_ENGINE_ID_BRAVE,
PREPOPULATED_ENGINE_ID_GOOGLE,
PREPOPULATED_ENGINE_ID_QWANT,
PREPOPULATED_ENGINE_ID_BING,
PREPOPULATED_ENGINE_ID_STARTPAGE,
PREPOPULATED_ENGINE_ID_BRAVE, PREPOPULATED_ENGINE_ID_DUCKDUCKGO_AU_NZ_IE,
PREPOPULATED_ENGINE_ID_GOOGLE, PREPOPULATED_ENGINE_ID_QWANT,
PREPOPULATED_ENGINE_ID_BING, PREPOPULATED_ENGINE_ID_STARTPAGE,
};

// A map to keep track of a full list of default engines for countries
Expand Down Expand Up @@ -234,7 +228,48 @@ BravePrepopulatedEngineID GetDefaultSearchEngine(int country_id, int version) {
{country_codes::CountryCharsToCountryID('U', 'Z'),
PREPOPULATED_ENGINE_ID_YANDEX},
});
if (version > 15) {
static const base::NoDestructor<
base::flat_map<int, BravePrepopulatedEngineID>>
content_v17({
{country_codes::CountryCharsToCountryID('A', 'M'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('A', 'Z'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('B', 'Y'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('C', 'A'),
PREPOPULATED_ENGINE_ID_BRAVE},
{country_codes::CountryCharsToCountryID('D', 'E'),
PREPOPULATED_ENGINE_ID_BRAVE},
{country_codes::CountryCharsToCountryID('F', 'R'),
PREPOPULATED_ENGINE_ID_BRAVE},
{country_codes::CountryCharsToCountryID('G', 'B'),
PREPOPULATED_ENGINE_ID_BRAVE},
{country_codes::CountryCharsToCountryID('K', 'G'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('K', 'Z'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('M', 'D'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('R', 'U'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('T', 'J'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('T', 'M'),
PREPOPULATED_ENGINE_ID_YANDEX},
{country_codes::CountryCharsToCountryID('U', 'S'),
PREPOPULATED_ENGINE_ID_BRAVE},
{country_codes::CountryCharsToCountryID('U', 'Z'),
PREPOPULATED_ENGINE_ID_YANDEX},
});

if (version > 16) {
auto it = content_v17->find(country_id);
if (it == content_v17->end()) {
return default_v6;
}
return it->second;
} else if (version > 15) {
auto it = content_v16->find(country_id);
if (it == content_v16->end()) {
return default_v6;
Expand Down
12 changes: 9 additions & 3 deletions components/brave_welcome_ui/containers/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface State {
shouldUpdateElementOverflow: boolean
}

const totalScreensSize = 5
let totalScreensSize = 5

export class WelcomePage extends React.Component<Props, State> {
constructor (props: Props) {
Expand All @@ -51,6 +51,8 @@ export class WelcomePage extends React.Component<Props, State> {
skipped: false,
shouldUpdateElementOverflow: false
}
const { welcomeData } = this.props
totalScreensSize = welcomeData.hideSearchOnboarding ? 4 : 5
}

componentDidUpdate (prevProps: Props) {
Expand Down Expand Up @@ -129,13 +131,17 @@ export class WelcomePage extends React.Component<Props, State> {
/>
<ShieldsBox index={3} currentScreen={this.currentScreen} />
<SearchBox
index={4}
index={welcomeData.hideSearchOnboarding ? -1 : 4}
currentScreen={this.currentScreen}
onClick={this.onClickNext}
changeDefaultSearchProvider={actions.changeDefaultSearchProvider}
searchProviders={welcomeData.searchProviders}
/>
<RewardsBox index={5} currentScreen={this.currentScreen} onClick={this.onClickRewardsGetStarted} />
<RewardsBox
index={welcomeData.hideSearchOnboarding ? 4 : 5}
currentScreen={this.currentScreen}
onClick={this.onClickRewardsGetStarted}
/>
</SlideContent>
<FooterBox
totalScreensSize={totalScreensSize}
Expand Down
9 changes: 5 additions & 4 deletions components/brave_welcome_ui/reducers/welcome_reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,17 @@ const welcomeReducer: Reducer<Welcome.State | undefined> = (state: Welcome.State
chrome.send('setDefaultSearchEngine', [modelIndex])
break
case types.IMPORT_DEFAULT_SEARCH_PROVIDERS_SUCCESS:
// TODO(bsclifton): remove when ready for other regions
// Regions approved for Brave Search will skip search welcome card
// Regions not approved show the card- but without Brave Search
const showBraveSearch: boolean =
['US', 'CA'].includes(loadTimeData.getString('countryString'))
// Only show Brave Search during onboarding for US/CA
['US', 'CA', 'DE', 'FR', 'GB'].includes(loadTimeData.getString('countryString'))
const filteredSearchList = payload.filter((item: any) => {
return !(item.name && item.name.startsWith('Brave ') && item.canBeRemoved)
})
state = {
...state,
searchProviders: showBraveSearch ? payload : filteredSearchList
searchProviders: showBraveSearch ? payload : filteredSearchList,
hideSearchOnboarding: showBraveSearch
}
break
case types.IMPORT_BROWSER_PROFILES_SUCCESS:
Expand Down
3 changes: 2 additions & 1 deletion components/brave_welcome_ui/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const keyName = 'welcome-data'

export const defaultState = {
searchProviders: [],
browserProfiles: []
browserProfiles: [],
hideSearchOnboarding: false
}

const cleanData = (state: Welcome.State) => {
Expand Down
3 changes: 2 additions & 1 deletion components/definitions/welcome.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ declare namespace Welcome {

export interface State {
searchProviders: Array<SearchEngineEntry>
browserProfiles: Array<BrowserProfile>
browserProfiles: Array<BrowserProfile>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't think you need a comma here...

hideSearchOnboarding: boolean
}
}
2 changes: 1 addition & 1 deletion components/search_engines/brave_prepopulated_engines.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace TemplateURLPrepopulateData {

// IMPORTANT! Make sure to bump this value if you make changes to the
// engines below or add/remove engines.
const int kBraveCurrentDataVersion = 16;
const int kBraveCurrentDataVersion = 17;
// DO NOT CHANGE THIS ONE. Used for backfilling kBraveDefaultSearchVersion.
const int kBraveFirstTrackedDataVersion = 6;

Expand Down
6 changes: 2 additions & 4 deletions components/test/brave_welcome_ui/components/app_test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react'
import { shallow } from 'enzyme'
import { welcomeInitialState } from '../../testData'
import * as storage from '../../../brave_welcome_ui/storage'
import {
WelcomePage,
mapStateToProps
Expand All @@ -10,10 +11,7 @@ describe('welcomePage component', () => {
describe('mapStateToProps', () => {
it('should map the default state', () => {
expect(mapStateToProps(welcomeInitialState)).toEqual({
welcomeData: {
searchProviders: [],
browserProfiles: []
}
welcomeData: storage.defaultState
})
})
})
Expand Down
17 changes: 5 additions & 12 deletions components/test/brave_welcome_ui/reducers/welcome_reducer_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@ describe('welcomeReducer', () => {
})
it('calls storage.load() when initial state is undefined', () => {
const assertion = welcomeReducer(undefined, actions.closeTabRequested())
expect(assertion).toEqual({
searchProviders: [],
browserProfiles: []
})
expect(assertion).toEqual(storage.defaultState)
expect(spy).toBeCalled()
expect(spy.mock.calls[0][1]).toBe(undefined)
})
Expand Down Expand Up @@ -125,11 +122,7 @@ describe('welcomeReducer', () => {
type: types.IMPORT_DEFAULT_SEARCH_PROVIDERS_SUCCESS,
payload: mockSearchProviders
})
const expected = {
...mockState,
searchProviders: mockSearchProviders
}
expect(result).toEqual(expected)
expect(result.searchProviders).toEqual(mockSearchProviders)
})

describe('with the region', () => {
Expand Down Expand Up @@ -167,7 +160,7 @@ describe('welcomeReducer', () => {
expect(spy).toBeCalledWith('countryString')
})

describe('when user is in US/Canada', () => {
describe('when user is in US/Canada/approved regions', () => {
it('should NOT filter out the Brave engine', () => {
const result = welcomeReducer(mockState, {
type: types.IMPORT_DEFAULT_SEARCH_PROVIDERS_SUCCESS,
Expand All @@ -177,9 +170,9 @@ describe('welcomeReducer', () => {
})
})

describe('when user is NOT in US/Canada', () => {
describe('when user is NOT in US/Canada/approved regions', () => {
beforeEach(() => {
countryString = 'GB'
countryString = 'CN'
})
afterEach(() => {
countryString = 'US'
Expand Down