Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274973004:
Fix security icon colors on light themes (Closed)
DescriptionFix security icon colors on light themes
Security icon colors on Chrome for Android and Chrome Custom Tabs should
be as follows:
1. Default toolbar color => green lock/red triangle
2. Dark toolbar color => white icons
3. Light toolbar color with opaque omnibox => green/red
4. Other toolbar color with transparent omnibox => dark icons
#1 and #2 were already working properly; this CL implements #3 and #4.
BUG=633736
TEST=On Chrome for Android, visit the following URLs and observe the
given security icon colors:
1. https://google.com => green lock
2. https://yahoo.com => white lock
3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox
with green lock
4. https://garron.net/test/theme-color/?color=%2300ffff => transparent
omnibox with dark lock
In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com:
1. Default => green lock
2. #ef6c00 => white lock
3. #ffffcc => dark lock
4. #66ffff => dark lock
Committed: https://crrev.com/cf4299907423c27d40182d17099d54a2e97e154c
Cr-Commit-Position: refs/heads/master@{#414897}
Patch Set 1 #
Total comments: 7
Patch Set 2 : indentation fix #
Messages
Total messages: 21 (11 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Description was changed from ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock ========== to ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #1 and #2 were already working properly; this CL implements #3 and #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estark@chromium.org changed reviewers: + yusufo@chromium.org
yusufo, PTAL
estark@chromium.org changed reviewers: + tedchoc@chromium.org
Also +tedchoc for LocationBarLayout.java
https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:511: LocationBarLayout.getColorStateList(securityLevel, getToolbarDataProvider(), I am confused about this working right. Since the toolbar is not editable, it has to always be opaque, right?
https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:511: LocationBarLayout.getColorStateList(securityLevel, getToolbarDataProvider(), On 2016/08/25 20:04:18, Yusuf wrote: > I am confused about this working right. Since the toolbar is not editable, it > has to always be opaque, right? On Clank, when the theme color is light enough, the omnibox turns white and opaque (see the light purple screenshot on https://bugs.chromium.org/p/chromium/issues/detail?id=633736#c14). As far as I can tell, CCT never turns the omnibox white-opaque like that, so this is always false.
https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:511: LocationBarLayout.getColorStateList(securityLevel, getToolbarDataProvider(), On 2016/08/25 20:14:39, estark wrote: > On 2016/08/25 20:04:18, Yusuf wrote: > > I am confused about this working right. Since the toolbar is not editable, it > > has to always be opaque, right? > > On Clank, when the theme color is light enough, the omnibox turns white and > opaque (see the light purple screenshot on > https://bugs.chromium.org/p/chromium/issues/detail?id=633736#c14). As far as I > can tell, CCT never turns the omnibox white-opaque like that, so this is always > false. I see. My expectation would have been for it to always be opaque (see the way the text box shows is making it transparent). But in reality we actually don't have a LocationBarLayout inside CustomTabsToolbar and the way the logic maps might have been wrong from the beginning. lgtm since you have already tried on all scenarios in cct too, I can see that it works. Thanks a lot Emily! Sorry for our not-in-great-shape theme color checking logic here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ one nit and one suggestion (but I'll defer to you to what you think is better) https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1233: Resources resources, boolean isOmniboxOpaque) { Hmm...I wonder if this would be a less error prone API to just pass in whether an omnibox is drawn at all. Then the ColorUtils.shouldUseOpaqueTextboxBackground could be in this method to make it less error prone (at least in my mind). https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1274: getToolbarDataProvider().getPrimaryColor()))); git cl format strikes again I suspect. In java, it should be increments of 8 from the start of the previous line and not the start of that parmam's logical parent.
Thanks! https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1233: Resources resources, boolean isOmniboxOpaque) { On 2016/08/26 22:54:31, Ted C wrote: > Hmm...I wonder if this would be a less error prone API to just pass in whether > an omnibox is drawn at all. Then the > ColorUtils.shouldUseOpaqueTextboxBackground could be in this method to make it > less error prone (at least in my mind). Makes sense, but I decided to leave this as-is because we need to get this landed and merged ASAP. (I'm away from my desk/testing devices until Monday and would want to re-test if I made this change.) I'd be happy to come back and clean this up later as a follow-up though. https://codereview.chromium.org/2274973004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1274: getToolbarDataProvider().getPrimaryColor()))); On 2016/08/26 22:54:31, Ted C wrote: > git cl format strikes again I suspect. In java, it should be increments of 8 > from the start of the previous line and not the start of that parmam's logical > parent. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2274973004/#ps20001 (title: "indentation fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #1 and #2 were already working properly; this CL implements #3 and #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock ========== to ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #1 and #2 were already working properly; this CL implements #3 and #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #1 and #2 were already working properly; this CL implements #3 and #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock ========== to ========== Fix security icon colors on light themes Security icon colors on Chrome for Android and Chrome Custom Tabs should be as follows: 1. Default toolbar color => green lock/red triangle 2. Dark toolbar color => white icons 3. Light toolbar color with opaque omnibox => green/red 4. Other toolbar color with transparent omnibox => dark icons #1 and #2 were already working properly; this CL implements #3 and #4. BUG=633736 TEST=On Chrome for Android, visit the following URLs and observe the given security icon colors: 1. https://google.com => green lock 2. https://yahoo.com => white lock 3. https://just-skyline-137219.appspot.com/lightblue => opaque omnibox with green lock 4. https://garron.net/test/theme-color/?color=%2300ffff => transparent omnibox with dark lock In a CCT client app, set the toolbar color to the following colors and then visit https://www.google.com: 1. Default => green lock 2. #ef6c00 => white lock 3. #ffffcc => dark lock 4. #66ffff => dark lock Committed: https://crrev.com/cf4299907423c27d40182d17099d54a2e97e154c Cr-Commit-Position: refs/heads/master@{#414897} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cf4299907423c27d40182d17099d54a2e97e154c Cr-Commit-Position: refs/heads/master@{#414897} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
