Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548753002:
HTTP Bad: set color of http_info/https_invalid icon on Android (Closed)
|
Created:
4 years ago by lshang Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHTTP Bad: set color of http_info/https_invalid icon on Android
Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect
their color as in goo.gl/1CUOLK.
BUG=662297
Committed: https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a
Cr-Commit-Position: refs/heads/master@{#441616}
Patch Set 1 #Patch Set 2 : fix test #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 10
Patch Set 5 : update patch #
Total comments: 4
Patch Set 6 : use id for color #
Messages
Total messages: 43 (31 generated)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== HTTP Bad: set color of http_info/https_invalid icon on Android BUG= ========== to ========== HTTP Bad: set color of http_info/https_invalid icon on Android There are two possible icons for http bad warning message in autofill dropdown: R.drawable.omnibox_info for http sites, and R.drawable.omnibox_https_invalid for broken https sites. Icons' colors should be in line with goo.gl/FLcFw8. This CL sets these two icons' colors. For other autofill entries' icons, like credit card icons, use their original color. BUG=662297 ==========
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lshang@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@, PTAL thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:147: int iconColorResId = 0; Hmm...does this code work anymore (even before this patch)? In particular, this patch: https://codereview.chromium.org/2579623003/ Seems to force drawables to be vectors, which they aren't in this case. This line creates a VectorDrawableCompat and that isn't correct for these: https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... I wonder if we need to fix the underlying code as well. Or revert that patch if it is a ton of work. https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:45: int iconColorResID) { lowercase d in the ID since we're in javaland https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:184: iconView.setColorFilter( I don't think tinting actually works on vectors, which is what sent me down the path of the question in my first file whether autofill was broken by the other change. https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (left): https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:57: @Override why remove this? it still seems to be in the interface
csashi@google.com changed reviewers: + csashi@google.com
https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:147: int iconColorResId = 0; On 2017/01/03 18:06:12, Ted C wrote: > Hmm...does this code work anymore (even before this patch)? > > In particular, this patch: > https://codereview.chromium.org/2579623003/ > > Seems to force drawables to be vectors, which they aren't in this case. > > This line creates a VectorDrawableCompat and that isn't correct for these: > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... > > I wonder if we need to fix the underlying code as well. Or revert that > patch if it is a ton of work. Thanks for the catch! I will check with icon designers and see if they want these colors. The recommendation from them was to use the new material design icons in auto fill - which I believe do not have these colors.
https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:147: int iconColorResId = 0; On 2017/01/03 21:07:17, csashi wrote: > On 2017/01/03 18:06:12, Ted C wrote: > > Hmm...does this code work anymore (even before this patch)? > > > > In particular, this patch: > > https://codereview.chromium.org/2579623003/ > > > > Seems to force drawables to be vectors, which they aren't in this case. > > > > This line creates a VectorDrawableCompat and that isn't correct for these: > > > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... > > > > I wonder if we need to fix the underlying code as well. Or revert that > > patch if it is a ton of work. > > Thanks for the catch! I will check with icon designers and see if they want > these colors. The recommendation from them was to use the new material design > icons in auto fill - which I believe do not have these colors. I did not realize this change was still in review. Could you request the icon designers to give you updated icons which have these color schemes. You can rename the vectors, ic_info_outline_black.xml and ic_warning_black.xml to reflect the color scheme. We use these vectors only in this context, you will only need to update the mapping for IDR_AUTOFILL_HTTP_WARNING and IDR_AUTOFILL_HTTPS_INVALID_WARNING.
Description was changed from ========== HTTP Bad: set color of http_info/https_invalid icon on Android There are two possible icons for http bad warning message in autofill dropdown: R.drawable.omnibox_info for http sites, and R.drawable.omnibox_https_invalid for broken https sites. Icons' colors should be in line with goo.gl/FLcFw8. This CL sets these two icons' colors. For other autofill entries' icons, like credit card icons, use their original color. BUG=662297 ========== to ========== HTTP Bad: set color of http_info/https_invalid icon on Android Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. BUG=662297 ==========
Thanks for the discussion on this issue! I've regenerated vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. This actually makes this CL much simpler. I tested them on my device and they work fine! PTAL thanks! https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:147: int iconColorResId = 0; On 2017/01/03 22:39:18, csashi wrote: > On 2017/01/03 21:07:17, csashi wrote: > > On 2017/01/03 18:06:12, Ted C wrote: > > > Hmm...does this code work anymore (even before this patch)? > > > > > > In particular, this patch: > > > https://codereview.chromium.org/2579623003/ > > > > > > Seems to force drawables to be vectors, which they aren't in this case. > > > > > > This line creates a VectorDrawableCompat and that isn't correct for these: > > > > > > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... > > > > > > I wonder if we need to fix the underlying code as well. Or revert that > > > patch if it is a ton of work. > > > > Thanks for the catch! I will check with icon designers and see if they want > > these colors. The recommendation from them was to use the new material design > > icons in auto fill - which I believe do not have these colors. > > I did not realize this change was still in review. Could you request the icon > designers to > give you updated icons which have these color schemes. You can rename the > vectors, ic_info_outline_black.xml > and ic_warning_black.xml to reflect the color scheme. We use these vectors only > in this context, you will > only need to update the mapping for IDR_AUTOFILL_HTTP_WARNING and > IDR_AUTOFILL_HTTPS_INVALID_WARNING. Thanks! I've regenerated the xml files from svg icons and reflect the colors. https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:45: int iconColorResID) { On 2017/01/03 18:06:12, Ted C wrote: > lowercase d in the ID since we're in javaland NaN after update but thanks for the java style reminding! https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:184: iconView.setColorFilter( On 2017/01/03 18:06:12, Ted C wrote: > I don't think tinting actually works on vectors, which is what sent me down the > path of the question in my first file whether autofill was broken by the other > change. I directly reflect the color in vector xml files. https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (left): https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:57: @Override On 2017/01/03 18:06:12, Ted C wrote: > why remove this? it still seems to be in the interface It's a sync merge mistake. Done.
On 2017/01/04 11:56:49, lshang wrote: > Thanks for the discussion on this issue! I've regenerated vector icon's xml > files from svg icon in go/ob-sec-assets and reflect their color as in > goo.gl/1CUOLK. This actually makes this CL much simpler. I tested them on my > device and they work fine! PTAL thanks! > > https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java > (right): > > https://codereview.chromium.org/2548753002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:147: > int iconColorResId = 0; > On 2017/01/03 22:39:18, csashi wrote: > > On 2017/01/03 21:07:17, csashi wrote: > > > On 2017/01/03 18:06:12, Ted C wrote: > > > > Hmm...does this code work anymore (even before this patch)? > > > > > > > > In particular, this patch: > > > > https://codereview.chromium.org/2579623003/ > > > > > > > > Seems to force drawables to be vectors, which they aren't in this case. > > > > > > > > This line creates a VectorDrawableCompat and that isn't correct for these: > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... > > > > > > > > I wonder if we need to fix the underlying code as well. Or revert that > > > > patch if it is a ton of work. > > > > > > Thanks for the catch! I will check with icon designers and see if they want > > > these colors. The recommendation from them was to use the new material > design > > > icons in auto fill - which I believe do not have these colors. > > > > I did not realize this change was still in review. Could you request the icon > > designers to > > give you updated icons which have these color schemes. You can rename the > > vectors, ic_info_outline_black.xml > > and ic_warning_black.xml to reflect the color scheme. We use these vectors > only > > in this context, you will > > only need to update the mapping for IDR_AUTOFILL_HTTP_WARNING and > > IDR_AUTOFILL_HTTPS_INVALID_WARNING. > > Thanks! I've regenerated the xml files from svg icons and reflect the colors. > > https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... > File > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java > (right): > > https://codereview.chromium.org/2548753002/diff/100001/components/autofill/an... > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:45: > int iconColorResID) { > On 2017/01/03 18:06:12, Ted C wrote: > > lowercase d in the ID since we're in javaland > > NaN after update but thanks for the java style reminding! > > https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... > File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): > > https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... > ui/android/java/src/org/chromium/ui/DropdownAdapter.java:184: > iconView.setColorFilter( > On 2017/01/03 18:06:12, Ted C wrote: > > I don't think tinting actually works on vectors, which is what sent me down > the > > path of the question in my first file whether autofill was broken by the other > > change. > > I directly reflect the color in vector xml files. > > https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... > File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (left): > > https://codereview.chromium.org/2548753002/diff/100001/ui/android/java/src/or... > ui/android/java/src/org/chromium/ui/DropdownItemBase.java:57: @Override > On 2017/01/03 18:06:12, Ted C wrote: > > why remove this? it still seems to be in the interface > > It's a sync merge mistake. Done. LG.
lgtm
lgtm w/ small comments https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_info_outline_grey.xml (right): https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... chrome/android/java/res/drawable/ic_info_outline_grey.xml:20: android:fillColor="#5A5A5A" should we use @color/light_normal_color here instead? https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_warning_red.xml (right): https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... chrome/android/java/res/drawable/ic_warning_red.xml:23: android:fillColor="#C53929" and @color/http_bad_warning_message_text here?
Thanks! https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_info_outline_grey.xml (right): https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... chrome/android/java/res/drawable/ic_info_outline_grey.xml:20: android:fillColor="#5A5A5A" On 2017/01/04 17:33:17, Ted C wrote: > should we use @color/light_normal_color here instead? Done. https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... File chrome/android/java/res/drawable/ic_warning_red.xml (right): https://codereview.chromium.org/2548753002/diff/120001/chrome/android/java/re... chrome/android/java/res/drawable/ic_warning_red.xml:23: android:fillColor="#C53929" On 2017/01/04 17:33:17, Ted C wrote: > and @color/http_bad_warning_message_text here? That is in components/, I use google_red_700 here, which is the same color value. Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, csashi@google.com Link to the patchset: https://codereview.chromium.org/2548753002/#ps140001 (title: "use id for color")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483608991516190,
"parent_rev": "9a14df1e4ced27e71e47222e2926e298dbff3af1", "commit_rev":
"26cafddf8768fd25d20e6104e641c478eadd5759"}
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: set color of http_info/https_invalid icon on Android Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. BUG=662297 ========== to ========== HTTP Bad: set color of http_info/https_invalid icon on Android Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. BUG=662297 Review-Url: https://codereview.chromium.org/2548753002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: set color of http_info/https_invalid icon on Android Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. BUG=662297 Review-Url: https://codereview.chromium.org/2548753002 ========== to ========== HTTP Bad: set color of http_info/https_invalid icon on Android Regenerate vector icon's xml files from svg icon in go/ob-sec-assets and reflect their color as in goo.gl/1CUOLK. BUG=662297 Committed: https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a Cr-Commit-Position: refs/heads/master@{#441616} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a Cr-Commit-Position: refs/heads/master@{#441616} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
