New issue
Advanced search Search tips

Issue 836116 link

Starred by 2 users
Status: Fixed
Owner: ----
Closed: Jun 2018
Cc:
patricia...@chromium.org
est...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 838052


Sign in to add a comment

Audit and remove callsites that create VectorIcons with no size and do not specify any CANVAS_DIMENSION.

Project Member Reported by patricia...@chromium.org, Apr 24 2018 
Currently, there are 55 [1] icons in the codebase that do not specify their default size (i.e. they do not have a CANVAS_DIMENSIONS line in their corresponding VectorIconRep). This is fine and allowed for the first (i.e. the largest) icon in an .icon file, but if these icons are created without a dip size specified, they will default to a size of 48dip.

These 55 icons should be audited to ensure they never hit this case, then future occurrences should be prevented by adding the following DCHECK to the CreateVectorIcon() function with no dip size in its signature:

DCHECK_EQ(icon.reps[icon.reps_size - 1].path[0].command, CANVAS_DIMENSIONS);


[1] The following 55 icons don't have a CANVAS_DIMENSIONS line as of r552929:

kIcMicBlackIcon
kIcGoogleColorIcon
kIcSearchEngineNotGoogleIcon
kLockScreenArrowIcon
kLockScreenDropdownIcon
kSystemPowerButtonMenuSignOutIcon
kSystemPowerButtonMenuPowerOffIcon
kBluetoothConnectedIcon
kVideocamIcon
kBusinessIcon
kMidiIcon
kCheckCircleIcon
kUsbIcon
kMicIcon
kProtocolHandlerIcon
kWarningIcon
kNotificationsIcon
kLocationOnIcon
kLockIcon
kOpenInNewIcon
kCastForEducationIcon
kLaptopIcon
kCodeIcon
kCrashedTabIcon
kMyLocationIcon
kWebIcon
kSupervisorAccountIcon
kImageIcon
kTabletIcon
kPaintbrushIcon
kAccountBoxIcon
kProfileSwitcherOutlineIcon
kCookieIcon
kSettingsIcon
kFileDownloadIcon
kSyncProblemIcon
kGlobeIcon
kAccountChildIcon
kTabIcon
kMixedContentIcon
kZoomPlusIcon
kExtensionCrashedIcon
kPhotoCameraIcon
kIncognitoIcon
kSadTabIcon
kExtensionIcon
kZoomMinusIcon
kCreditCard20Icon
kSmartphoneIcon
kAppsIcon
kMyLocationIcon
kDaydreamControllerAppButtonIcon
kDaydreamControllerHomeButtonIcon
kSadTabIcon
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 30 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2d7a5197e6f286ca072eff51a2ab7ce99885cef

commit d2d7a5197e6f286ca072eff51a2ab7ce99885cef
Author: Patti <patricialor@chromium.org>
Date: Mon Apr 30 04:29:34 2018

Vector Icons: DCHECK to prevent icons from being created without a size.

gfx::VectorIcons currently support being created via CreateVectorIcon() without
any size. In this case, a default size is retrieved from the icon's file
(provided via a CANVAS_DIMENSIONS directive). However, icons may not always
specify a CANVAS_DIMENSIONS, so if the code is provided no size *and* there is
no CANVAS_DIMENSIONS, it will fall back to a size of 48.

This isn't ideal as it may result in some icons being drawn for a canvas that is
too small or too large, so add a DCHECK to prevent this.

Bug:  836116 
Change-Id: I176d4b5ed7f07fe679a2b70addcce02a09a6b5a3
Reviewed-on: https://chromium-review.googlesource.com/1029556
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554693}
[modify] https://crrev.com/d2d7a5197e6f286ca072eff51a2ab7ce99885cef/ui/gfx/paint_vector_icon.cc
[modify] https://crrev.com/d2d7a5197e6f286ca072eff51a2ab7ce99885cef/ui/gfx/vector_icon_types.h

Comment 2 by patricia...@chromium.org, May 1 2018

Blockedon: 838052

Comment 3 by patricia...@chromium.org, Jun 12 2018

Status: Fixed (was: Available)
I haven't seen any problems with this since #c1, so I'll go ahead and close it as fixed.
►

Sign in to add a comment