Create dark tint duplicates of default icons
Default icons (Globe / Chrome) were static drawables shared between all
TabListFaviconProvider. While two provider objects are doing different
tint over those static drawables, race condition might occur and wrong
tint might be applied.
This CL create dark tint duplicates for those favicon to avoid race
condition. Also add night mode render tests in StartSurfaceLayoutTest.
Bug: 1222326
Change-Id: I43199051a20eae23c2f24a52b2ee5db3eed2a736
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2977323
Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Yue Zhang <yuezhanggg@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#895228}
diff --git a/chrome/android/features/start_surface/internal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java b/chrome/android/features/start_surface/internal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java
index ce7b705..331b122 100644
--- a/chrome/android/features/start_surface/internal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java
+++ b/chrome/android/features/start_surface/internal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java
@@ -73,7 +73,9 @@
import org.hamcrest.Matcher;
import org.hamcrest.Matchers;
import org.junit.After;
+import org.junit.AfterClass;
import org.junit.Before;
+import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
@@ -82,6 +84,8 @@
import org.chromium.base.Callback;
import org.chromium.base.GarbageCollectionTestUtils;
import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.test.params.ParameterAnnotations;
+import org.chromium.base.test.params.ParameterizedRunner;
import org.chromium.base.test.util.ApplicationTestUtils;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Criteria;
@@ -122,7 +126,7 @@
import org.chromium.chrome.browser.undo_tab_close_snackbar.UndoBarController;
import org.chromium.chrome.browser.util.ChromeAccessibilityUtil;
import org.chromium.chrome.tab_ui.R;
-import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeJUnit4RunnerDelegate;
import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
import org.chromium.chrome.test.util.ActivityTestUtils;
import org.chromium.chrome.test.util.ChromeRenderTestRule;
@@ -139,6 +143,7 @@
import org.chromium.net.test.EmbeddedTestServer;
import org.chromium.ui.base.PageTransition;
import org.chromium.ui.test.util.DisableAnimationsTestRule;
+import org.chromium.ui.test.util.NightModeTestUtils;
import org.chromium.ui.test.util.UiRestriction;
import org.chromium.ui.util.ColorUtils;
import org.chromium.ui.widget.ChipView;
@@ -159,7 +164,8 @@
// clang-format off
/** Tests for the {@link StartSurfaceLayout} */
@SuppressWarnings("ConstantConditions")
-@RunWith(ChromeJUnit4ClassRunner.class)
+@RunWith(ParameterizedRunner.class)
+@ParameterAnnotations.UseRunnerDelegate(ChromeJUnit4RunnerDelegate.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
"force-fieldtrials=Study/Group"})
@EnableFeatures({ChromeFeatureList.TAB_GRID_LAYOUT_ANDROID + "<Study"})
@@ -195,6 +201,22 @@
(bitmap) -> mAllBitmaps.add(new WeakReference<>(bitmap));
private TabSwitcher.TabListDelegate mTabListDelegate;
+ @BeforeClass
+ public static void setUpBeforeActivityLaunched() {
+ ChromeNightModeTestUtils.setUpNightModeBeforeChromeActivityLaunched();
+ }
+
+ @ParameterAnnotations.UseMethodParameterBefore(NightModeTestUtils.NightModeParams.class)
+ public void setupNightMode(boolean isNightMode) {
+ ChromeNightModeTestUtils.setUpNightModeForChromeActivity(isNightMode);
+ mRenderTestRule.setNightModeEnabled(isNightMode);
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() {
+ ChromeNightModeTestUtils.tearDownNightModeAfterChromeActivityDestroyed();
+ }
+
@Before
public void setUp() {
AccessibilityChecks.enable();
@@ -235,8 +257,9 @@
@Feature({"RenderTest"})
// clang-format off
@EnableFeatures({ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study"})
+ @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
@CommandLineFlags.Add({BASE_PARAMS})
- public void testRenderGrid_3WebTabs() throws IOException {
+ public void testRenderGrid_3WebTabs(boolean isNightMode) throws IOException {
// clang-format on
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
prepareTabs(3, 0, "about:blank");
@@ -255,8 +278,9 @@
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS})
@EnableFeatures({ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study"})
+ @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
@DisableIf.Build(sdk_is_greater_than = O_MR1, message = "crbug.com/1077552")
- public void testRenderGrid_10WebTabs() throws IOException {
+ public void testRenderGrid_10WebTabs(boolean isNightMode) throws IOException {
// clang-format on
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
prepareTabs(10, 0, "about:blank");
@@ -275,9 +299,10 @@
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS})
@EnableFeatures({ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study"})
+ @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
@DisabledTest(message = "https://crbug.com/1139807")
@DisableIf.Build(sdk_is_greater_than = O_MR1, message = "crbug.com/1077552")
- public void testRenderGrid_10WebTabs_InitialScroll() throws IOException {
+ public void testRenderGrid_10WebTabs_InitialScroll(boolean isNightMode) throws IOException {
// clang-format on
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
prepareTabs(10, 0, "about:blank");
@@ -315,8 +340,9 @@
@Feature({"RenderTest"})
// clang-format off
@EnableFeatures({ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study"})
+ @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
@CommandLineFlags.Add({BASE_PARAMS})
- public void testRenderGrid_Incognito() throws IOException {
+ public void testRenderGrid_Incognito(boolean isNightMode) throws IOException {
// clang-format on
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
// Prepare some incognito tabs and enter tab switcher.
@@ -331,6 +357,8 @@
mRenderTestRule.render(cta.findViewById(R.id.tab_list_view), "3_incognito_web_tabs");
}
+ // TODO(https://crbug.com/1222917): This test case is not working in dark mode. Use method
+ // parameter after it is fixed.
@Test
@MediumTest
@Feature({"RenderTest"})
@@ -1387,9 +1415,10 @@
// clang-format off
@EnableFeatures({ChromeFeatureList.TAB_GROUPS_ANDROID,
ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study"})
+ @ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
@CommandLineFlags.Add({BASE_PARAMS + "/thumbnail_aspect_ratio/1.0"})
@DisabledTest(message = "https://crbug.com/1205952")
- public void testRenderGrid_withAspectRatioOfOne() throws IOException {
+ public void testRenderGrid_withAspectRatioOfOne(boolean isNightMode) throws IOException {
// clang-format on
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
prepareTabs(3, 0, "about:blank");
diff --git a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListFaviconProvider.java b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListFaviconProvider.java
index 2c8274a..1f657d3 100644
--- a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListFaviconProvider.java
+++ b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListFaviconProvider.java
@@ -9,6 +9,7 @@
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
import android.graphics.PorterDuff;
+import android.graphics.PorterDuffColorFilter;
import android.graphics.drawable.Drawable;
import android.graphics.drawable.LayerDrawable;
@@ -39,6 +40,9 @@
private static Drawable sRoundedChromeDrawable;
private static Drawable sRoundedChromeDrawableForStrip;
private static Drawable sRoundedComposedDefaultDrawable;
+ // Keep dark tint separated if not all providers are forcing dark tint. See crbug.com/1222326.
+ private static Drawable sRoundedGlobeDrawableDarkTint;
+ private static Drawable sRoundedChromeDrawableDarkTint;
private final int mStripFaviconSize;
private final int mDefaultFaviconSize;
private final int mFaviconSize;
@@ -50,8 +54,6 @@
private final int mDefaultIconColor;
@ColorInt
private final int mIncognitoIconColor;
- @ColorInt
- private final int mDarkIconColor;
private boolean mIsInitialized;
private Profile mProfile;
@@ -61,6 +63,7 @@
* Construct the provider that provides favicons for tab list.
* @param context The context to use for accessing {@link android.content.res.Resources}
* @param isTabStrip Indicator for whether this class provides favicons for tab strip or not.
+ * @param forceDarkTint Indicator that the favicon will be forced into dark tint or not.
*
*/
public TabListFaviconProvider(Context context, boolean isTabStrip, boolean forceDarkTint) {
@@ -75,13 +78,22 @@
mIsTabStrip = isTabStrip;
mForceDarkTint = forceDarkTint;
+ final @ColorInt int darkIconColor =
+ mContext.getResources().getColor(R.color.default_icon_color_dark);
+ final PorterDuffColorFilter darkTintFilter =
+ new PorterDuffColorFilter(darkIconColor, PorterDuff.Mode.SRC_IN);
+
if (sRoundedGlobeDrawable == null) {
// TODO(crbug.com/1066709): From Android Developer Documentation, we should avoid
- // resizing vector drawable.
+ // resizing vector drawable.
Drawable globeDrawable =
- AppCompatResources.getDrawable(context, R.drawable.ic_globe_24dp);
+ AppCompatResources.getDrawable(context, R.drawable.ic_globe_24dp).mutate();
sRoundedGlobeDrawable = processBitmap(
getResizedBitmapFromDrawable(globeDrawable, mDefaultFaviconSize), false);
+
+ sRoundedGlobeDrawableDarkTint = processBitmap(
+ getResizedBitmapFromDrawable(globeDrawable, mDefaultFaviconSize), false);
+ sRoundedGlobeDrawableDarkTint.setColorFilter(darkTintFilter);
}
if (sRoundedGlobeDrawableForStrip == null) {
Drawable globeDrawable =
@@ -93,6 +105,9 @@
Bitmap chromeBitmap =
BitmapFactory.decodeResource(mContext.getResources(), R.drawable.chromelogo16);
sRoundedChromeDrawable = processBitmap(chromeBitmap, false);
+
+ sRoundedChromeDrawableDarkTint = processBitmap(chromeBitmap, false);
+ sRoundedChromeDrawableDarkTint.setColorFilter(darkTintFilter);
}
if (sRoundedChromeDrawableForStrip == null) {
Drawable chromeDrawable =
@@ -101,12 +116,16 @@
getResizedBitmapFromDrawable(chromeDrawable, mStripFaviconSize), true);
}
if (sRoundedComposedDefaultDrawable == null) {
+ // sRoundedComposedDefaultDrawable is only used as the top favicon for tab groups, where
+ // a white drawable background is added behind the favicon, so this drawable will always
+ // be in dark tint.
sRoundedComposedDefaultDrawable =
AppCompatResources.getDrawable(context, R.drawable.ic_group_icon_16dp);
+ sRoundedComposedDefaultDrawable.setColorFilter(darkTintFilter);
}
+
mDefaultIconColor = mContext.getResources().getColor(R.color.default_icon_color);
mIncognitoIconColor = mContext.getResources().getColor(R.color.default_icon_color_light);
- mDarkIconColor = mContext.getResources().getColor(R.color.default_icon_color_dark);
}
public void initWithNative(Profile profile) {
@@ -186,29 +205,25 @@
/**
* Asynchronously get the composed, up to 4, favicon Drawable.
* @param urls List of urls, up to 4, whose favicon are requested to be composed.
- * @param isIncognito Whether the processed composed favicon is used for incognito or not.
* @param faviconCallback The callback that requests for the composed favicon.
*/
- public void getComposedFaviconImageAsync(
- List<GURL> urls, boolean isIncognito, Callback<Drawable> faviconCallback) {
+ public void getComposedFaviconImageAsync(List<GURL> urls, Callback<Drawable> faviconCallback) {
assert urls != null && urls.size() > 1 && urls.size() <= 4;
mFaviconHelper.getComposedFaviconImage(mProfile, urls, mFaviconSize, (image, iconUrl) -> {
if (image == null) {
- faviconCallback.onResult(getDefaultComposedImage(isIncognito));
+ faviconCallback.onResult(sRoundedComposedDefaultDrawable);
} else {
faviconCallback.onResult(processBitmap(image, mIsTabStrip));
}
});
}
- private Drawable getDefaultComposedImage(boolean isIncognito) {
- return getTintedDrawable(sRoundedComposedDefaultDrawable, isIncognito);
- }
-
private Drawable getRoundedChromeDrawable(boolean isIncognito) {
if (mIsTabStrip) {
return sRoundedChromeDrawableForStrip;
+ } else if (mForceDarkTint) {
+ return sRoundedChromeDrawableDarkTint;
}
return getTintedDrawable(sRoundedChromeDrawable, isIncognito);
}
@@ -216,22 +231,19 @@
private Drawable getRoundedGlobeDrawable(boolean isIncognito) {
if (mIsTabStrip) {
return sRoundedGlobeDrawableForStrip;
+ } else if (mForceDarkTint) {
+ return sRoundedGlobeDrawableDarkTint;
}
return getTintedDrawable(sRoundedGlobeDrawable, isIncognito);
}
private Drawable getTintedDrawable(Drawable drawable, boolean isIncognito) {
@ColorInt
- int color = mDefaultIconColor;
- if (mForceDarkTint) {
- color = mDarkIconColor;
- } else if (isIncognito) {
- color = mIncognitoIconColor;
- }
+ int color = isIncognito ? mIncognitoIconColor : mDefaultIconColor;
// Since static variable is still loaded when activity is destroyed due to configuration
// changes, e.g. light/dark theme changes, setColorFilter is needed when we retrieve the
// drawable. setColorFilter would be a no-op if color and the mode are the same.
- drawable.setColorFilter(color, PorterDuff.Mode.SRC_IN);
+ drawable.setColorFilter(new PorterDuffColorFilter(color, PorterDuff.Mode.SRC_IN));
return drawable;
}
}
diff --git a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
index c90e4c0..7e12ffa 100644
--- a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
+++ b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
@@ -1624,8 +1624,7 @@
}
// For tab group card in grid tab switcher, the favicon is the composed favicon.
- mTabListFaviconProvider.getComposedFaviconImageAsync(
- urls, pseudoTab.isIncognito(), faviconCallback);
+ mTabListFaviconProvider.getComposedFaviconImageAsync(urls, faviconCallback);
return;
}
diff --git a/chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java b/chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java
index 27ea4533..4900c444 100644
--- a/chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java
+++ b/chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java
@@ -2428,7 +2428,7 @@
mModel.get(0).model.set(TabProperties.FAVICON, null);
doNothing()
.when(mTabListFaviconProvider)
- .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+ .getComposedFaviconImageAsync(any(), mCallbackCaptor.capture());
// Test a group of three.
TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
@@ -2436,7 +2436,7 @@
createTabGroup(tabs, TAB1_ID);
mTabObserverCaptor.getValue().onFaviconUpdated(mTab1, mFaviconBitmap);
List<GURL> urls = new ArrayList<>(Arrays.asList(TAB1_URL, TAB2_URL, TAB3_URL));
- verify(mTabListFaviconProvider).getComposedFaviconImageAsync(eq(urls), anyBoolean(), any());
+ verify(mTabListFaviconProvider).getComposedFaviconImageAsync(eq(urls), any());
mCallbackCaptor.getValue().onResult(mFaviconDrawable);
assertThat(mModel.get(0).model.get(TabProperties.FAVICON), equalTo(mFaviconDrawable));
@@ -2449,7 +2449,7 @@
mTabObserverCaptor.getValue().onFaviconUpdated(mTab2, mFaviconBitmap);
urls = new ArrayList<>(Arrays.asList(TAB2_URL, TAB1_URL, TAB3_URL, TAB2_URL));
- verify(mTabListFaviconProvider).getComposedFaviconImageAsync(eq(urls), anyBoolean(), any());
+ verify(mTabListFaviconProvider).getComposedFaviconImageAsync(eq(urls), any());
mCallbackCaptor.getValue().onResult(mFaviconDrawable);
assertThat(mModel.get(1).model.get(TabProperties.FAVICON), equalTo(mFaviconDrawable));
}
@@ -2463,7 +2463,7 @@
mModel.get(1).model.set(TabProperties.FAVICON, null);
doNothing()
.when(mTabListFaviconProvider)
- .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+ .getComposedFaviconImageAsync(any(), mCallbackCaptor.capture());
TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
@@ -2490,7 +2490,7 @@
mModel.get(1).model.set(TabProperties.FAVICON, null);
doNothing()
.when(mTabListFaviconProvider)
- .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+ .getComposedFaviconImageAsync(any(), mCallbackCaptor.capture());
TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
@@ -2517,7 +2517,7 @@
mModel.get(1).model.set(TabProperties.FAVICON, null);
doNothing()
.when(mTabListFaviconProvider)
- .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+ .getComposedFaviconImageAsync(any(), mCallbackCaptor.capture());
TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
