[TabStrip] Cleanup compositor button tinting logic [chromium/src : main]
Neil Coronado (Gerrit)
Neil Coronado voted and added 1 comment![]( "Open in Gerrit")
Votes added by Neil Coronado
| Commit-Queue | +1 |
1 comment
@ColorRes int iconTintRes = R.color.default_icon_color_tint_list;I was hoping to make these all resIds, so we only had to actually pull the color once at the end, but I forgot that some things pull from attrs, so we need to use the util methods (e.g. `SemanticColorUtils.getColorSurfaceContainerLow`), which return a colorInt.
I can make them all @ColorInts for consistency if that's preferred, but I think the repeated `#getColor` calls hurt visibility. Or I suppose I could also wrap these in color state lists, but feels like a bit much. I don't feel too strongly.
Related details
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wu (Gerrit)
Michael Wu added 4 comments![]( "Open in Gerrit")
@ColorRes int iconTintRes = R.color.default_icon_color_tint_list;I was hoping to make these all resIds, so we only had to actually pull the color once at the end, but I forgot that some things pull from attrs, so we need to use the util methods (e.g. `SemanticColorUtils.getColorSurfaceContainerLow`), which return a colorInt.
I can make them all @ColorInts for consistency if that's preferred, but I think the repeated `#getColor` calls hurt visibility. Or I suppose I could also wrap these in color state lists, but feels like a bit much. I don't feel too strongly.
Acknowledged
int iconTint = incognito ? R.color.modern_white : R.color.default_icon_color_tint_list;Since we are doing `else if (incognito) {`, would this run into issues if we are in night mode (would skip the incognito block which sets `iconTintRes = R.color.modern_white`?
int iconColor =
incognito ? R.color.default_icon_color_light : R.color.default_icon_color_tint_list;
int iconColorInt = context.getColorStateList(iconColor).getDefaultColor();I think the new logic accidentally reverses this
@ColorInt int bgDefaultColor = SemanticColorUtils.getColorSurfaceContainerLow(mContext);nit: Should we rename this and below bgTint, bgHoverTint, bgPressedTint to be consistent with the naming in the other files
Related details
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado added 3 comments![]( "Open in Gerrit")
int iconTint = incognito ? R.color.modern_white : R.color.default_icon_color_tint_list;Since we are doing `else if (incognito) {`, would this run into issues if we are in night mode (would skip the incognito block which sets `iconTintRes = R.color.modern_white`?
ah, wait I'm pretty sure the old (and new) logic is bugged, and we should prioritize Incognito > light/dark, so changed to check Incognito first (and instead skip the dark mode block).
if this are no concerns, I'll note this in the CL description as well.
int iconColor =
incognito ? R.color.default_icon_color_light : R.color.default_icon_color_tint_list;
int iconColorInt = context.getColorStateList(iconColor).getDefaultColor();I think the new logic accidentally reverses this
oops, fixed
@ColorInt int bgDefaultColor = SemanticColorUtils.getColorSurfaceContainerLow(mContext);nit: Should we rename this and below bgTint, bgHoverTint, bgPressedTint to be consistent with the naming in the other files
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wu (Gerrit)
Michael Wu added 1 comment![]( "Open in Gerrit")
int iconTint = incognito ? R.color.modern_white : R.color.default_icon_color_tint_list;Neil CoronadoSince we are doing `else if (incognito) {`, would this run into issues if we are in night mode (would skip the incognito block which sets `iconTintRes = R.color.modern_white`?
ah, wait I'm pretty sure the old (and new) logic is bugged, and we should prioritize Incognito > light/dark, so changed to check Incognito first (and instead skip the dark mode block).
if this are no concerns, I'll note this in the CL description as well.
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado added 1 comment![]( "Open in Gerrit")
int iconTint = incognito ? R.color.modern_white : R.color.default_icon_color_tint_list;Neil CoronadoSince we are doing `else if (incognito) {`, would this run into issues if we are in night mode (would skip the incognito block which sets `iconTintRes = R.color.modern_white`?
Michael Wuah, wait I'm pretty sure the old (and new) logic is bugged, and we should prioritize Incognito > light/dark, so changed to check Incognito first (and instead skip the dark mode block).
if this are no concerns, I'll note this in the CL description as well.
That change SGTM! (I think it's not uploaded yet though)
- Michael Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wu (Gerrit)
Michael Wu voted Code-Review+1![]( "Open in Gerrit")
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[TabStrip] Cleanup compositor button tinting logic
Cleanup compositor button tinting logic.
-Follow a consistent pattern of setting defaults, then potentially
overriding, then setting once on the button.
-Also fix pre-existing bug where the NTB prioritized dark mode colors
over Incognito colors.
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTab.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTrailingButtonsCoordinator.java
Code-Review: +1 by Michael Wu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |