New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733301 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 2
Type: Bug

Blocked on:
issue 736350
issue 736895
issue 737338



Sign in to add a comment

prevent usage of vector icons at inappropriate sizes

Project Member Reported by est...@chromium.org, Jun 14 2017

Issue description

For 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
 
Cc: sgabr...@chromium.org
Labels: Hotlist-VectorIcons
In the past I have also caught instances such as these. +1 to the DCHECK suggestion assuming both of the following statements are true:

(1) For every vector icon in the codebase with separate 1x and 2x versions, both have been hand-designed by someone (i.e., Sebastien) to look good only at a specific size. In other words, nobody has pulled and landed 1x and 2x versions of an icon from go/icons.

(2) For every vector icon in the codebase with separate 1x and 2x versions, it is necessary to have both versions in order for the icon to look good. In other words, it would be visually unacceptable to just render something from go/icons at the target size.

Comment 2 by est...@chromium.org, Jun 15 2017

either of those statements, if false, indicate another thing should be fixed.

Comment 3 by est...@chromium.org, Jun 15 2017

the DCHECK I have in mind is here: https://chromium-review.googlesource.com/c/534963/

Comment 4 by est...@chromium.org, Jun 20 2017

Owner: tdander...@chromium.org
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.)
Sure, I can look into this.
Blockedon: 736350
Blockedon: 736895
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Blockedon: 737338
Owner: est...@chromium.org
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.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
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