Skip to content

Commit

Permalink
[Omnibox] Do not use data provider to get the colors for the Omnibox
Browse files Browse the repository at this point in the history
OmniboxSuggestionsDropdown, LocationBarCoordinator and SearchActivity
should directly get the background colors from resources instead of
getting them from LocationBarDataProvider.

Bug:1348445

Change-Id: I86c83ab340f19767c4b2174e957f5793063875c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3908271
Reviewed-by: Tomasz Wiszkowski <ender@google.com>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Gang Wu <gangwu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1052262}
  • Loading branch information
Gang Wu authored and Chromium LUCI CQ committed Sep 28, 2022
1 parent d6fb7ac commit 7963180
Show file tree
Hide file tree
Showing 10 changed files with 38 additions and 141 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.chromium.chrome.browser.ui.native_page.NativePage;
import org.chromium.chrome.browser.ui.searchactivityutils.SearchActivityConstants;
import org.chromium.components.browser_ui.modaldialog.AppModalPresenter;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.components.browser_ui.util.BrowserControlsVisibilityDelegate;
import org.chromium.components.external_intents.ExternalNavigationHandler;
import org.chromium.components.url_formatter.UrlFormatter;
Expand Down Expand Up @@ -513,8 +514,8 @@ public void onClick(View v) {
getResources().getDimensionPixelOffset(R.dimen.toolbar_edge_padding_modern);
toolbarView.setPaddingRelative(edgePadding, toolbarView.getPaddingTop(), edgePadding,
toolbarView.getPaddingBottom());
toolbarView.setBackground(
new ColorDrawable(mSearchBoxDataProvider.getDropdownStandardBackgroundColor()));
toolbarView.setBackground(new ColorDrawable(ChromeColors.getSurfaceColor(
this, R.dimen.omnibox_suggestion_dropdown_bg_elevation)));
}
return contentView;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler;
import org.chromium.chrome.browser.toolbar.top.ToolbarPhone;
import org.chromium.chrome.browser.ui.searchactivityutils.SearchActivityPreferencesManager;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.ui.base.DeviceFormFactor;
import org.chromium.ui.base.WindowAndroid;
import org.chromium.ui.widget.Toast;
Expand Down Expand Up @@ -62,8 +63,10 @@ public void initialize(@NonNull AutocompleteCoordinator autocompleteCoordinator,
ToolbarPhone.createModernLocationBarBackground(getContext());
if (OmniboxFeatures.shouldShowModernizeVisualUpdate(getContext())) {
backgroundDrawable.setTint(OmniboxFeatures.shouldShowActiveColorOnOmnibox()
? mLocationBarDataProvider.getSuggestionStandardBackgroundColor()
: mLocationBarDataProvider.getDropdownStandardBackgroundColor());
? ChromeColors.getSurfaceColor(
getContext(), R.dimen.omnibox_suggestion_bg_elevation)
: ChromeColors.getSurfaceColor(getContext(),
R.dimen.omnibox_suggestion_dropdown_bg_elevation));
if (OmniboxFeatures.shouldShowActiveColorOnOmnibox()) {
backgroundDrawable.setCornerRadius(getResources().getDimensionPixelSize(
R.dimen.omnibox_suggestion_bg_round_corner_radius));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
import org.chromium.chrome.browser.omnibox.NewTabPageDelegate;
import org.chromium.chrome.browser.omnibox.R;
import org.chromium.chrome.browser.omnibox.UrlBarData;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.ui.searchactivityutils.SearchActivityPreferencesManager;
Expand All @@ -22,10 +21,6 @@

class SearchBoxDataProvider implements LocationBarDataProvider {
private final @ColorInt int mPrimaryColor;
private final @ColorInt int mDropdownStandardBgColor;
private final @ColorInt int mDropdownIncognitoBgColor;
private final @ColorInt int mSuggestionStandardBgColor;
private final @ColorInt int mSuggestionIncognitoBgColor;
private boolean mIsFromQuickActionSearchWidget;
private Tab mTab;

Expand All @@ -36,12 +31,6 @@ class SearchBoxDataProvider implements LocationBarDataProvider {
SearchBoxDataProvider(Context context) {
mIsFromQuickActionSearchWidget = false;
mPrimaryColor = ChromeColors.getPrimaryBackgroundColor(context, isIncognito());
mDropdownStandardBgColor = ChromeColors.getSurfaceColor(
context, R.dimen.omnibox_suggestion_dropdown_bg_elevation);
mDropdownIncognitoBgColor = context.getColor(R.color.omnibox_dropdown_bg_incognito);
mSuggestionStandardBgColor =
ChromeColors.getSurfaceColor(context, R.dimen.omnibox_suggestion_bg_elevation);
mSuggestionIncognitoBgColor = context.getColor(R.color.omnibox_suggestion_bg_incognito);
}

/**
Expand Down Expand Up @@ -148,26 +137,6 @@ public int getSecurityIconContentDescriptionResourceId() {
return 0;
}

@Override
public int getDropdownStandardBackgroundColor() {
return mDropdownStandardBgColor;
}

@Override
public int getDropdownIncognitoBackgroundColor() {
return mDropdownIncognitoBgColor;
}

@Override
public int getSuggestionStandardBackgroundColor() {
return mSuggestionStandardBgColor;
}

@Override
public int getSuggestionIncognitoBackgroundColor() {
return mSuggestionIncognitoBgColor;
}

void setIsFromQuickActionSearchWidget(boolean isFromQuickActionsWidget) {
mIsFromQuickActionSearchWidget = isFromQuickActionsWidget;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,26 +569,6 @@ public int getSecurityIconResource(boolean isTablet) {
public int getSecurityIconContentDescriptionResourceId() {
return 0;
}

@Override
public int getDropdownStandardBackgroundColor() {
return 0;
}

@Override
public int getDropdownIncognitoBackgroundColor() {
return 0;
}

@Override
public int getSuggestionStandardBackgroundColor() {
return 0;
}

@Override
public int getSuggestionIncognitoBackgroundColor() {
return 0;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import android.animation.Animator;
import android.animation.ObjectAnimator;
import android.content.Context;
import android.content.res.Configuration;
import android.view.ActionMode;
import android.view.View;
Expand Down Expand Up @@ -46,6 +47,7 @@
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tabmodel.IncognitoStateProvider;
import org.chromium.chrome.browser.tabmodel.TabWindowManager;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.components.browser_ui.widget.gesture.BackPressHandler;
import org.chromium.components.search_engines.TemplateUrlService;
import org.chromium.ui.KeyboardVisibilityDelegate;
Expand Down Expand Up @@ -172,13 +174,14 @@ public LocationBarCoordinator(View locationBarLayout, View autocompleteAnchorVie
mActivityLifecycleDispatcher = activityLifecycleDispatcher;
mActivityLifecycleDispatcher.register(this);
mAutocompleteAnchorView = autocompleteAnchorView;
Context context = mLocationBarLayout.getContext();

mUrlBar = mLocationBarLayout.findViewById(R.id.url_bar);
// TODO(crbug.com/1151513): Inject LocaleManager instance to LocationBarCoordinator instead
// of using the singleton.
mLocationBarMediator = new LocationBarMediator(mLocationBarLayout.getContext(),
mLocationBarLayout, locationBarDataProvider, profileObservableSupplier,
privacyPreferencesManager, overrideUrlLoadingDelegate, LocaleManager.getInstance(),
mLocationBarMediator = new LocationBarMediator(context, mLocationBarLayout,
locationBarDataProvider, profileObservableSupplier, privacyPreferencesManager,
overrideUrlLoadingDelegate, LocaleManager.getInstance(),
mTemplateUrlServiceSupplier, backKeyBehavior, windowAndroid,
isTablet() && isTabletLayout(), searchEngineLogoUtils, LensController.getInstance(),
launchAssistanceSettingsAction, saveOfflineButtonState, omniboxUma,
Expand Down Expand Up @@ -228,18 +231,17 @@ public LocationBarCoordinator(View locationBarLayout, View autocompleteAnchorVie
mAutocompleteCoordinator.updateSuggestionListLayoutDirection();
}));

mLocationBarLayout.getContext().registerComponentCallbacks(mLocationBarMediator);
context.registerComponentCallbacks(mLocationBarMediator);
mLocationBarLayout.initialize(mAutocompleteCoordinator, mUrlCoordinator, mStatusCoordinator,
locationBarDataProvider, searchEngineLogoUtils);

mDropdownStandardBackgroundColor =
locationBarDataProvider.getDropdownStandardBackgroundColor();
mDropdownIncognitoBackgroundColor =
locationBarDataProvider.getDropdownIncognitoBackgroundColor();
mDropdownStandardBackgroundColor = ChromeColors.getSurfaceColor(
context, R.dimen.omnibox_suggestion_dropdown_bg_elevation);
mDropdownIncognitoBackgroundColor = context.getColor(R.color.omnibox_dropdown_bg_incognito);
mSuggestionStandardBackgroundColor =
locationBarDataProvider.getSuggestionStandardBackgroundColor();
ChromeColors.getSurfaceColor(context, R.dimen.omnibox_suggestion_bg_elevation);
mSuggestionIncognitoBackgroundColor =
locationBarDataProvider.getSuggestionIncognitoBackgroundColor();
context.getColor(R.color.omnibox_suggestion_bg_incognito);

if (isPhoneLayout()) {
mSubCoordinator = new LocationBarCoordinatorPhone(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,4 @@ default boolean isPaintPreview() {
/** Returns the resource ID of the content description for the security icon. */
@StringRes
int getSecurityIconContentDescriptionResourceId();

/** Returns the standard color to use for the suggestrions dropdown background.*/
int getDropdownStandardBackgroundColor();

/** Returns the incognito color to use for the suggestrions dropdown background.*/
int getDropdownIncognitoBackgroundColor();

/** Returns the standard color to use for each individual suggestion background.*/
int getSuggestionStandardBackgroundColor();

/** Returns the incognito color to use for each individual suggestion background.*/
int getSuggestionIncognitoBackgroundColor();
}
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ listModel, new Handler(), modalDialogManagerSupplier, activityTabSupplier,
listModel.set(SuggestionListProperties.OBSERVER, mMediator);

ViewProvider<SuggestionListViewHolder> viewProvider =
createViewProvider(context, listItems, locationBarDataProvider);
createViewProvider(context, listItems);
viewProvider.whenLoaded((holder) -> { mDropdown = holder.dropdown; });
LazyConstructionPropertyMcp.create(listModel, SuggestionListProperties.VISIBLE,
viewProvider, SuggestionListViewBinder::bind);
Expand All @@ -131,8 +131,8 @@ public void destroy() {
}
}

private ViewProvider<SuggestionListViewHolder> createViewProvider(Context context,
MVCListAdapter.ModelList modelList, LocationBarDataProvider locationBarDataProvider) {
private ViewProvider<SuggestionListViewHolder> createViewProvider(
Context context, MVCListAdapter.ModelList modelList) {
return new ViewProvider<SuggestionListViewHolder>() {
private List<Callback<SuggestionListViewHolder>> mCallbacks = new ArrayList<>();
private SuggestionListViewHolder mHolder;
Expand All @@ -141,7 +141,7 @@ private ViewProvider<SuggestionListViewHolder> createViewProvider(Context contex
public void inflate() {
OmniboxSuggestionsDropdown dropdown;
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
dropdown = new OmniboxSuggestionsDropdown(context, locationBarDataProvider);
dropdown = new OmniboxSuggestionsDropdown(context);
}

// Start with visibility GONE to ensure that show() is called.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.chromium.base.TraceEvent;
import org.chromium.base.metrics.TimingMetric;
import org.chromium.base.task.PostTask;
import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
import org.chromium.chrome.browser.omnibox.OmniboxFeatures;
import org.chromium.chrome.browser.omnibox.R;
import org.chromium.chrome.browser.ui.theme.BrandedColorScheme;
Expand Down Expand Up @@ -172,8 +171,7 @@ public ViewHolder getRecycledView(int viewType) {
* Constructs a new list designed for containing omnibox suggestions.
* @param context Context used for contained views.
*/
public OmniboxSuggestionsDropdown(
@NonNull Context context, @NonNull LocationBarDataProvider locationBarDataProvider) {
public OmniboxSuggestionsDropdown(@NonNull Context context) {
super(context, null, android.R.attr.dropDownListViewStyle);
setFocusable(true);
setFocusableInTouchMode(true);
Expand Down Expand Up @@ -207,10 +205,11 @@ public int scrollVerticallyBy(
ViewCompat.setPaddingRelative(this, paddingSide, 0, paddingSide, paddingBottom);

mStandardBgColor = shouldShowModernizeVisualUpdate
? locationBarDataProvider.getDropdownStandardBackgroundColor()
? ChromeColors.getSurfaceColor(
context, R.dimen.omnibox_suggestion_dropdown_bg_elevation)
: ChromeColors.getDefaultThemeColor(context, false);
mIncognitoBgColor = shouldShowModernizeVisualUpdate
? locationBarDataProvider.getDropdownIncognitoBackgroundColor()
? context.getColor(R.color.omnibox_dropdown_bg_incognito)
: ChromeColors.getDefaultThemeColor(context, true);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

import static junit.framework.Assert.assertEquals;

import static org.mockito.Mockito.doReturn;

import android.content.Context;
import android.view.ContextThemeWrapper;

Expand All @@ -19,15 +17,11 @@
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
import org.chromium.chrome.browser.omnibox.test.R;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
Expand All @@ -39,16 +33,8 @@
*/
@RunWith(BaseRobolectricTestRunner.class)
public class OmniboxSuggestionsDropdownUnitTest {
private static final int COLOR_STANDARD = 0;
private static final int COLOR_INCOGNITO = 1;

@Rule
public TestRule mProcessor = new Features.JUnitProcessor();
@Rule
public MockitoRule mMockitoRule = MockitoJUnit.rule();

@Mock
private LocationBarDataProvider mLocationBarDataProvider;

private Context mContext;

Expand All @@ -58,12 +44,6 @@ public class OmniboxSuggestionsDropdownUnitTest {
public void setUp() {
mContext = new ContextThemeWrapper(
ApplicationProvider.getApplicationContext(), R.style.Theme_BrowserUI_DayNight);
doReturn(COLOR_STANDARD)
.when(mLocationBarDataProvider)
.getDropdownStandardBackgroundColor();
doReturn(COLOR_INCOGNITO)
.when(mLocationBarDataProvider)
.getDropdownIncognitoBackgroundColor();
}

@Test
Expand All @@ -76,18 +56,23 @@ public void setUp() {
"force-fieldtrial-params=Study.Group:enable_modernize_visual_update_on_tablet/true"})
public void
testBackgroundColor_withOmniboxModernizeVisualUpdateFlags() {
mDropdown = new OmniboxSuggestionsDropdown(mContext, mLocationBarDataProvider);

assertEquals(mDropdown.getStandardBgColor(), COLOR_STANDARD);
assertEquals(mDropdown.getIncognitoBgColor(), COLOR_INCOGNITO);
mDropdown = new OmniboxSuggestionsDropdown(mContext);

assertEquals(mDropdown.getStandardBgColor(),
ChromeColors.getSurfaceColor(mContext,
org.chromium.chrome.browser.omnibox.R.dimen
.omnibox_suggestion_dropdown_bg_elevation));
assertEquals(mDropdown.getIncognitoBgColor(),
mContext.getColor(
org.chromium.chrome.browser.omnibox.R.color.omnibox_dropdown_bg_incognito));
}

@Test
@SmallTest
@Feature("Omnibox")
@DisableFeatures({ChromeFeatureList.OMNIBOX_MODERNIZE_VISUAL_UPDATE})
public void testBackgroundColor_withoutOmniboxModernizeVisualUpdateFlags() {
mDropdown = new OmniboxSuggestionsDropdown(mContext, mLocationBarDataProvider);
mDropdown = new OmniboxSuggestionsDropdown(mContext);

assertEquals(
mDropdown.getStandardBgColor(), ChromeColors.getDefaultThemeColor(mContext, false));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,6 @@ default boolean isOfflinePage(Tab tab) {

private Tab mTab;
private int mPrimaryColor;
private int mDropdownStandardBgColor;
private int mDropdownIncognitoBgColor;
private int mSuggestionStandardBgColor;
private int mSuggestionIncognitoBgColor;
private LayoutStateProvider mLayoutStateProvider;

private boolean mIsIncognito;
Expand Down Expand Up @@ -201,12 +197,6 @@ public LocationBarModel(Context context, NewTabPageDelegate newTabPageDelegate,
mOfflineStatus = offlineStatus;
mPrimaryColor = ChromeColors.getDefaultThemeColor(context, false);
mSearchEngineLogoUtils = searchEngineLogoUtils;
mDropdownStandardBgColor = ChromeColors.getSurfaceColor(
mContext, R.dimen.omnibox_suggestion_dropdown_bg_elevation);
mDropdownIncognitoBgColor = mContext.getColor(R.color.omnibox_dropdown_bg_incognito);
mSuggestionStandardBgColor =
ChromeColors.getSurfaceColor(context, R.dimen.omnibox_suggestion_bg_elevation);
mSuggestionIncognitoBgColor = context.getColor(R.color.omnibox_suggestion_bg_incognito);
}

/**
Expand Down Expand Up @@ -617,26 +607,6 @@ public int getSecurityIconContentDescriptionResourceId() {
return SecurityStatusIcon.getSecurityIconContentDescriptionResourceId(getSecurityLevel());
}

@Override
public int getDropdownStandardBackgroundColor() {
return mDropdownStandardBgColor;
}

@Override
public int getDropdownIncognitoBackgroundColor() {
return mDropdownIncognitoBgColor;
}

@Override
public int getSuggestionStandardBackgroundColor() {
return mSuggestionStandardBgColor;
}

@Override
public int getSuggestionIncognitoBackgroundColor() {
return mSuggestionIncognitoBgColor;
}

@VisibleForTesting
@ConnectionSecurityLevel
int getSecurityLevel(Tab tab, boolean isOfflinePage, @Nullable String publisherUrl) {
Expand Down

0 comments on commit 7963180

Please sign in to comment.