prevent usage of vector icons at inappropriate sizes |
|||||||
Issue descriptionFor icons that have a .1x.icon definition, the only valid size to display them is that which is specified in the .1x.icon file (CANVAS_DIMENSIONS). We should add a DCHECK, but currently there are at least a couple places that break this rule, so the DCHECK would be tripped and can't be added until we fix those places. So far I've found: - NetworkListView::CreatePowerStatusView - LockScreenActionTray::NewNoteActionView
,
Jun 15 2017
either of those statements, if false, indicate another thing should be fixed.
,
Jun 15 2017
the DCHECK I have in mind is here: https://chromium-review.googlesource.com/c/534963/
,
Jun 20 2017
I believe the usage in this file is also problematic: chrome/browser/android/vr_shell/ui_elements/screen_capture_indicator.cc Terry, I wonder if you can track these and attempt to land the DCHECK in the next couple weeks before more issues crop up? (If not I will reassign to myself when I get back.)
,
Jun 20 2017
Sure, I can look into this.
,
Jun 26 2017
,
Jun 26 2017
,
Jun 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c2db8578228a71611f79badf0d1b1d691528973e commit c2db8578228a71611f79badf0d1b1d691528973e Author: Evan Stade <estade@chromium.org> Date: Mon Jun 26 22:55:52 2017 Make the Chrome OS battery image usable at various sizes. Reuse it for tethers in the network list, which uses 20x20 images as opposed to 16x16 as in the tray. This also reduces the number of repeated constants and assumptions in code concerning the size of the visible portion of the battery. Bug: 672263, 733301 , 736895 Change-Id: Ia112ced4e70c7ea079cc5eea5dce1a9825647399 Reviewed-on: https://chromium-review.googlesource.com/535973 Commit-Queue: Terry Anderson <tdanderson@chromium.org> Reviewed-by: Terry Anderson <tdanderson@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Cr-Commit-Position: refs/heads/master@{#482447} [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/resources/vector_icons/BUILD.gn [delete] https://crrev.com/d6c179267fc346b4a5809ec598e877e1515db419/ash/resources/vector_icons/system_tray_battery.1x.icon [delete] https://crrev.com/d6c179267fc346b4a5809ec598e877e1515db419/ash/resources/vector_icons/system_tray_battery.icon [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/system/network/network_list.cc [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/system/power/power_status.cc [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/system/power/power_status.h [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/system/power/power_status_unittest.cc [modify] https://crrev.com/c2db8578228a71611f79badf0d1b1d691528973e/ash/system/power/tray_power.cc
,
Jun 28 2017
,
Jul 7 2017
Evan, handing this back to you since I will be OOO most of next week. A couple of icon fixes landed, and I believe the "screen share" icon ( issue 736350 ) is the only one remaining before the DCHECK can be landed.
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a55c3f11f48845c20d55e9fb7d84f515f600c42 commit 6a55c3f11f48845c20d55e9fb7d84f515f600c42 Author: Evan Stade <estade@chromium.org> Date: Thu Jul 13 22:36:58 2017 Add a DCHECK to discourage people from re-using size-specific vector icons in a different size. Bug: 733301 Change-Id: Idf325b1849ef8cad471044c26b449fa9cbfbb5e0 Reviewed-on: https://chromium-review.googlesource.com/567206 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#486495} [modify] https://crrev.com/6a55c3f11f48845c20d55e9fb7d84f515f600c42/ui/gfx/paint_vector_icon.cc
,
Jul 14 2017
The DCHECK landed successfully, meaning all tested codepaths are fixed. It's possible some untested callsites are still in the wrong and would now DCHECK; these should be fixed. Also, their owners might consider adding test coverage for them. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by tdander...@chromium.org
, Jun 14 2017Labels: Hotlist-VectorIcons