Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214693005:
Introduce some util code for drawing vector assets. (Closed)
|
Created:
5 years, 5 months ago by Evan Stade Modified:
5 years, 5 months ago CC:
chromium-reviews, tfarina, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce some util code for drawing vector assets.
Replace a couple icons with vector versions (check_circle and photo_camera).
The end result for the camera icon is pixel identical, although the vertical alignment is slightly different. I believe the new vertical alignment is correct (it aligns the center of the camera lens with the center of the user profile photo, instead of ~4px below), so I'm leaving that change in.
The end result for the checkmark icon is pixel identical.
BUG=505953
TBR=thestig@chromium.org
Committed: https://crrev.com/33b823af3e63287dbe850c253ae97593411d4f43
Cr-Commit-Position: refs/heads/master@{#337893}
Patch Set 1 #Patch Set 2 : . #
Total comments: 15
Patch Set 3 : sky review #
Total comments: 31
Patch Set 4 : pksating review #Patch Set 5 : remove constexpr (msvc doesn't support it yet) #Patch Set 6 : fix compile and rebase #
Total comments: 9
Patch Set 7 : sadrul review #
Total comments: 2
Patch Set 8 : compile? #
Messages
Total messages: 37 (8 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
What do you guys think of this approach? The reason for the kooky code in vector_icons.cc is to minimize the size each asset takes up.
This seems fine to me. https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:43: static PathElement path[] = { Style guide says: " and function static variables, must be Plain Old Data (POD)" https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h File ui/gfx/vector_icons.h (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no (c) here and in the .cc. https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:16: enum VectorIconId { enum class, that way you can forward declare it in headers. https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:23: GFX_EXPORT void PaintVectorIcon(Canvas* canvas, VectorIconId id, size_t dp_size, Document what dp_size and color are. https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... ui/views/controls/image_view.cc:256: gfx::PaintVectorIcon(canvas, vector_id_, image_size_.width(), vector_color_); Seems like you should DCHECK on image_size_.
https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... ui/views/controls/image_view.h:59: void SetVectorIcon(gfx::VectorIconId id, SkColor color); Maybe this should take the image_size to reinforce it must be specified?
https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:43: static PathElement path[] = { On 2015/07/01 16:19:52, sky wrote: > Style guide says: " and function static variables, must be Plain Old Data (POD)" "[...] or arrays/structs of POD." https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h File ui/gfx/vector_icons.h (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/07/01 16:19:52, sky wrote: > no (c) here and in the .cc. Done. https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:16: enum VectorIconId { On 2015/07/01 16:19:52, sky wrote: > enum class, that way you can forward declare it in headers. TIL that exists. I've switched to enum class, but it doesn't let me forward declare in image_view.h (or any header file in this CL) because there's a member var of that type: error: field has incomplete type 'gfx::VectorIconId' https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.h#n... ui/gfx/vector_icons.h:23: GFX_EXPORT void PaintVectorIcon(Canvas* canvas, VectorIconId id, size_t dp_size, On 2015/07/01 16:19:52, sky wrote: > Document what dp_size and color are. Done. https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... ui/views/controls/image_view.cc:256: gfx::PaintVectorIcon(canvas, vector_id_, image_size_.width(), vector_color_); On 2015/07/01 16:19:52, sky wrote: > Seems like you should DCHECK on image_size_. Done. https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/20001/ui/views/controls/image... ui/views/controls/image_view.h:59: void SetVectorIcon(gfx::VectorIconId id, SkColor color); On 2015/07/01 16:42:35, sky wrote: > Maybe this should take the image_size to reinforce it must be specified? sure
https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:43: static PathElement path[] = { On 2015/07/01 21:52:58, Evan Stade wrote: > On 2015/07/01 16:19:52, sky wrote: > > Style guide says: " and function static variables, must be Plain Old Data > (POD)" > > "[...] or arrays/structs of POD." I'm pretty sure once you add a constructor that rules doesn't apply.
https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:32: PathElement(SkScalar arg) { this->arg = arg; } Nit: explicit? Or does that break the code below? https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, I don't love inlining these instructions here. It seems like ideally, we'd have text files for each icon containing these sorts of instructions, and during the build process, a script would find them all, automatically declare the icon enum in the header, and then automatically include the actual paths into the compiled source. I suppose that could wait until after this change, but I'd find a way to do it before expanding this functionality much more. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:80: case VectorIconId::VECTOR_ICON_NONE: Nit: Put in same order that the enum values were declared (i.e. put this first) https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:89: SkColor color) { Nit: I think it's easier to read function definitions with all args on independent lines, if they don't all fit on one line. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:91: NOTREACHED(); Don't handle DCHECK failure. This block should just be: DCHECK_NE(VectorIconId::VECTOR_ICON_NONE, id); https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:98: SkScalar scale = dp_size / reference_size; You compute this and then multiply every coordinate by it below. It seems like it would be clearer/simpler/faster/less error-prone to instead simply set a scaling transform? https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:102: case CIRCLE: { Nit: Put in same order that the enum values were declared https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.cc:250: gfx::Vector2d origin = ComputeImageOrigin(image_size_) - gfx::Point(); Nit: Use ComputeImageOrigin(image_size_).OffsetFromOrigin() instead. I might just inline this into the next statement. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:58: // |color|. Since the size of the view and the image will be set to Nit: I think you want to remove "Since" (sentence fragment otherwise) https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:126: gfx::VectorIconId vector_id_; Does it make sense to make bitmap and vector image views sibling classes that mostly inherit a common base, instead of putting both pieces of functionality into one class? https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/throb... File ui/views/controls/throbber.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/throb... ui/views/controls/throbber.cc:66: const int kDipSize = 18; Hardcoding this makes me sad... is there any way to compute it from e.g. the throbber's preferred size, or share this constant with other places, or something?
https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:13: const SkScalar reference_size = 48.0; kReferenceSize?
https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:43: static PathElement path[] = { On 2015/07/01 22:00:55, sky wrote: > On 2015/07/01 21:52:58, Evan Stade wrote: > > On 2015/07/01 16:19:52, sky wrote: > > > Style guide says: " and function static variables, must be Plain Old Data > > (POD)" > > > > "[...] or arrays/structs of POD." > > I'm pretty sure once you add a constructor that rules doesn't apply. It's hard to interpret from the style guide, but it seems that this code adheres to the spirit of the guide, because there is no destructor and the constructor just passes through POD. I can't see how construction/destruction order could matter in this case. I added constexpr to the constructors and arrays and I think that makes this code follow the letter of the law ("However, such variables are allowed if they are constexpr: they have no dynamic initialization or destruction.") https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:13: const SkScalar reference_size = 48.0; On 2015/07/01 22:57:52, tfarina wrote: > kReferenceSize? Done. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:32: PathElement(SkScalar arg) { this->arg = arg; } On 2015/07/01 22:26:57, Peter Kasting wrote: > Nit: explicit? Or does that break the code below? breaks it https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, On 2015/07/01 22:26:57, Peter Kasting wrote: > I don't love inlining these instructions here. > > It seems like ideally, we'd have text files for each icon containing these sorts > of instructions, and during the build process, a script would find them all, > automatically declare the icon enum in the header, and then automatically > include the actual paths into the compiled source. Can you elaborate on why that is better? Do you just not like code and data in the same file? If that's the case all we need to do is move this function to a different file. > I suppose that could wait until after this change, but I'd find a way to do it > before expanding this functionality much more. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:80: case VectorIconId::VECTOR_ICON_NONE: On 2015/07/01 22:26:58, Peter Kasting wrote: > Nit: Put in same order that the enum values were declared (i.e. put this first) Done. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:89: SkColor color) { On 2015/07/01 22:26:58, Peter Kasting wrote: > Nit: I think it's easier to read function definitions with all args on > independent lines, if they don't all fit on one line. Done. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:91: NOTREACHED(); On 2015/07/01 22:26:58, Peter Kasting wrote: > Don't handle DCHECK failure. This block should just be: > > DCHECK_NE(VectorIconId::VECTOR_ICON_NONE, id); Done. https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:98: SkScalar scale = dp_size / reference_size; On 2015/07/01 22:26:58, Peter Kasting wrote: > You compute this and then multiply every coordinate by it below. It seems like > it would be clearer/simpler/faster/less error-prone to instead simply set a > scaling transform? good idea, done https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:102: case CIRCLE: { On 2015/07/01 22:26:57, Peter Kasting wrote: > Nit: Put in same order that the enum values were declared Done. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.cc:250: gfx::Vector2d origin = ComputeImageOrigin(image_size_) - gfx::Point(); On 2015/07/01 22:26:58, Peter Kasting wrote: > Nit: Use ComputeImageOrigin(image_size_).OffsetFromOrigin() instead. > > I might just inline this into the next statement. Done. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:58: // |color|. Since the size of the view and the image will be set to On 2015/07/01 22:26:58, Peter Kasting wrote: > Nit: I think you want to remove "Since" (sentence fragment otherwise) Done. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:126: gfx::VectorIconId vector_id_; On 2015/07/01 22:26:58, Peter Kasting wrote: > Does it make sense to make bitmap and vector image views sibling classes that > mostly inherit a common base, instead of putting both pieces of functionality > into one class? I'm not a huge fan of classes that hold implementation but can't themselves be instantiated; also, that would require updating a lot of places that use ImageView, which I think would be fairly disruptive for little benefit. I did consider making a subclass of ImageView called VectorImageView, but there's so little actual functionality being added here that I didn't think it worth it. side note: I was actually thinking that at some point in the future we might want both things to be drawn. For example, there are content settings icons for every content type, and then exact copies with an [X] drawn on top. The [X] could potentially be vectorized. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/throb... File ui/views/controls/throbber.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/throb... ui/views/controls/throbber.cc:66: const int kDipSize = 18; On 2015/07/01 22:26:58, Peter Kasting wrote: > Hardcoding this makes me sad... is there any way to compute it from e.g. the > throbber's preferred size, or share this constant with other places, or > something? Yea, it's tricky. 18 is the magic dp for which the actual circle (less padding) is 16dp. That is implicit in the drawing instructions, so not sure how to share. I can move these constants to the same place and add a note to reduce the probability of this breaking.
https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, On 2015/07/02 00:08:05, Evan Stade wrote: > On 2015/07/01 22:26:57, Peter Kasting wrote: > > I don't love inlining these instructions here. > > > > It seems like ideally, we'd have text files for each icon containing these > sorts > > of instructions, and during the build process, a script would find them all, > > automatically declare the icon enum in the header, and then automatically > > include the actual paths into the compiled source. > > Can you elaborate on why that is better? Do you just not like code and data in > the same file? If that's the case all we need to do is move this function to a > different file. I want updating icons to not have to touch code, since that will make it easier for designers to do this directly. (I'd like adding and removing icons to be as easy as possible as well.) Also, as this list grows to dozens of items, I think having these in separate, named files will be easier to find and read/change what's happening for an icon than will a hundreds-of-lines file with enormous switch statements. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:126: gfx::VectorIconId vector_id_; On 2015/07/02 00:08:06, Evan Stade wrote: > On 2015/07/01 22:26:58, Peter Kasting wrote: > > Does it make sense to make bitmap and vector image views sibling classes that > > mostly inherit a common base, instead of putting both pieces of functionality > > into one class? > > I'm not a huge fan of classes that hold implementation but can't themselves be > instantiated; also, that would require updating a lot of places that use > ImageView, which I think would be fairly disruptive for little benefit. That latter wouldn't be a concern if the bitmap class was ImageView, the base ImageViewBase, and the vector class VectorImageView. It's hard to know how to weigh the different tradeoffs here, but I tend to dislike classes that conglomerate multiple distinct kinds of functionality/APIs. In such cases, it's often easy to remove all the uses of a certain thing later on, and not realize the functionality is dead, because there are still plenty of references to the class name. If you have a VectorImageView and we remove all instantiations, that's easier to see/codesearch. Glomming the functionality together also makes it hard for the reader to know what bits are only needed by bitmaps, what by vectors, and what by both. The best we can do is add comments to all the relevant members and functions. Splitting into distinct classes makes it easy to see what's shared and what's not. > side note: I was actually thinking that at some point in the future we might > want both things to be drawn. For example, there are content settings icons for > every content type, and then exact copies with an [X] drawn on top. The [X] > could potentially be vectorized. That's reasonable but I don't see why we couldn't do that with two images at the callsite. It'd likely just be a couple lines of code. Whereas combining these into one class that can draw both raises other questions: why is the vector always on top of the bitmap -- what if we want it below? Why is vector + bitmap one image, but vector + vector and bitmap + bitmap both two?
https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/20001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:43: static PathElement path[] = { On 2015/07/02 00:08:05, Evan Stade wrote: > On 2015/07/01 22:00:55, sky wrote: > > On 2015/07/01 21:52:58, Evan Stade wrote: > > > On 2015/07/01 16:19:52, sky wrote: > > > > Style guide says: " and function static variables, must be Plain Old Data > > > (POD)" > > > > > > "[...] or arrays/structs of POD." > > > > I'm pretty sure once you add a constructor that rules doesn't apply. > > It's hard to interpret from the style guide, but it seems that this code adheres > to the spirit of the guide, because there is no destructor and the constructor > just passes through POD. I can't see how construction/destruction order could > matter in this case. > > I added constexpr to the constructors and arrays and I think that makes this > code follow the letter of the law ("However, such variables are allowed if they > are constexpr: they have no dynamic initialization or destruction.") I take it back, MSVC doesn't support constexpr yet. Are you ok with deleting the constexpr tags here but otherwise leaving the code the same? https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, On 2015/07/02 00:23:53, Peter Kasting wrote: > On 2015/07/02 00:08:05, Evan Stade wrote: > > On 2015/07/01 22:26:57, Peter Kasting wrote: > > > I don't love inlining these instructions here. > > > > > > It seems like ideally, we'd have text files for each icon containing these > > sorts > > > of instructions, and during the build process, a script would find them all, > > > automatically declare the icon enum in the header, and then automatically > > > include the actual paths into the compiled source. > > > > Can you elaborate on why that is better? Do you just not like code and data in > > the same file? If that's the case all we need to do is move this function to a > > different file. > > I want updating icons to not have to touch code, since that will make it easier > for designers to do this directly. Currently image assets are separate from code, but I've still never seen a designer check in a change to one. In my recent experience, designers don't even have a working build and lean heavily on canary for testing new functionality. > (I'd like adding and removing icons to be as > easy as possible as well.) Also, as this list grows to dozens of items, I think > having these in separate, named files will be easier to find and read/change > what's happening for an icon than will a hundreds-of-lines file with enormous > switch statements. Even if there were 200 icons in this switch statement, it would be easy to find CHECK_CIRCLE with most text editors. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:126: gfx::VectorIconId vector_id_; On 2015/07/02 00:23:53, Peter Kasting wrote: > On 2015/07/02 00:08:06, Evan Stade wrote: > > On 2015/07/01 22:26:58, Peter Kasting wrote: > > > Does it make sense to make bitmap and vector image views sibling classes > that > > > mostly inherit a common base, instead of putting both pieces of > functionality > > > into one class? > > > > I'm not a huge fan of classes that hold implementation but can't themselves be > > instantiated; also, that would require updating a lot of places that use > > ImageView, which I think would be fairly disruptive for little benefit. > > That latter wouldn't be a concern if the bitmap class was ImageView, the base > ImageViewBase, and the vector class VectorImageView. > > It's hard to know how to weigh the different tradeoffs here, but I tend to > dislike classes that conglomerate multiple distinct kinds of functionality/APIs. > In such cases, it's often easy to remove all the uses of a certain thing later > on, and not realize the functionality is dead, because there are still plenty of > references to the class name. If you have a VectorImageView and we remove all > instantiations, that's easier to see/codesearch. Like GetImageSize, which was a public function but not actually being used anywhere. But I don't think it's actually less likely if there's a separate VectorImageView class. If you remove a single use of VectorImageView, and you're not being particularly thorough, what will force you to realize that it was the last use of VectorImgaeView? > > Glomming the functionality together also makes it hard for the reader to know > what bits are only needed by bitmaps, what by vectors, and what by both. The > best we can do is add comments to all the relevant members and functions. > Splitting into distinct classes makes it easy to see what's shared and what's > not. > > > side note: I was actually thinking that at some point in the future we might > > want both things to be drawn. For example, there are content settings icons > for > > every content type, and then exact copies with an [X] drawn on top. The [X] > > could potentially be vectorized. > > That's reasonable but I don't see why we couldn't do that with two images at the > callsite. It'd likely just be a couple lines of code. I don't think there's much support with the stock layout managers for stacking sibling views on top of each other, so I think it would be more than a couple lines. But yea, this was just a side note and until we have an actual use case probably not worth considering. > Whereas combining these > into one class that can draw both raises other questions: why is the vector > always on top of the bitmap -- what if we want it below? Why is vector + bitmap > one image, but vector + vector and bitmap + bitmap both two? Well, I still dislike this sort of inheritance for sharing code, and I think the amount of code added here is very small (and hence easy to understand, with or without comments). How strongly do you feel about this topic?
https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, On 2015/07/02 00:58:24, Evan Stade wrote: > On 2015/07/02 00:23:53, Peter Kasting wrote: > > On 2015/07/02 00:08:05, Evan Stade wrote: > > > On 2015/07/01 22:26:57, Peter Kasting wrote: > > > > I don't love inlining these instructions here. > > > > > > > > It seems like ideally, we'd have text files for each icon containing these > > > sorts > > > > of instructions, and during the build process, a script would find them > all, > > > > automatically declare the icon enum in the header, and then automatically > > > > include the actual paths into the compiled source. > > > > > > Can you elaborate on why that is better? Do you just not like code and data > in > > > the same file? If that's the case all we need to do is move this function to > a > > > different file. > > > > I want updating icons to not have to touch code, since that will make it > easier > > for designers to do this directly. > > Currently image assets are separate from code, but I've still never seen a > designer check in a change to one. In my recent experience, designers don't even > have a working build and lean heavily on canary for testing new functionality. Designers frequently have me point them to the appropriate image files in the tree and then send me new versions to plug in directly atop them. That's easy to reproduce with separate files here. > > (I'd like adding and removing icons to be as > > easy as possible as well.) Also, as this list grows to dozens of items, I > think > > having these in separate, named files will be easier to find and read/change > > what's happening for an icon than will a hundreds-of-lines file with enormous > > switch statements. > > Even if there were 200 icons in this switch statement, it would be easy to find > CHECK_CIRCLE with most text editors. In theory, yes. In practice, such files are a huge pain. I went through this same process with the search engine prepopulate data. Initially everything was in one giant file. It became annoying to scan it quickly and find what you wanted, know how many engines there were, etc. Updating things required touching several places in the file and making sure not to miss any. We then split things into more files with a bit of scripting for the build system and it improved stuff. The win here should be even larger. https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... File ui/views/controls/image_view.h (right): https://codereview.chromium.org/1214693005/diff/40001/ui/views/controls/image... ui/views/controls/image_view.h:126: gfx::VectorIconId vector_id_; On 2015/07/02 00:58:24, Evan Stade wrote: > On 2015/07/02 00:23:53, Peter Kasting wrote: > > On 2015/07/02 00:08:06, Evan Stade wrote: > > > On 2015/07/01 22:26:58, Peter Kasting wrote: > > > > Does it make sense to make bitmap and vector image views sibling classes > > that > > > > mostly inherit a common base, instead of putting both pieces of > > functionality > > > > into one class? > > > > > > I'm not a huge fan of classes that hold implementation but can't themselves > be > > > instantiated; also, that would require updating a lot of places that use > > > ImageView, which I think would be fairly disruptive for little benefit. > > > > That latter wouldn't be a concern if the bitmap class was ImageView, the base > > ImageViewBase, and the vector class VectorImageView. > > > > It's hard to know how to weigh the different tradeoffs here, but I tend to > > dislike classes that conglomerate multiple distinct kinds of > functionality/APIs. > > In such cases, it's often easy to remove all the uses of a certain thing > later > > on, and not realize the functionality is dead, because there are still plenty > of > > references to the class name. If you have a VectorImageView and we remove all > > instantiations, that's easier to see/codesearch. > > Like GetImageSize, which was a public function but not actually being used > anywhere. But I don't think it's actually less likely if there's a separate > VectorImageView class. If you remove a single use of VectorImageView, and you're > not being particularly thorough, what will force you to realize that it was the > last use of VectorImgaeView? Having added and then ripped out tons of different functionality in the omnibox over time, it's always been easier when it's separated into different classes. It's just much easier to know you can throw a whole class away from one search than to individually try and look at every API and data member to see if they're still used. No, this doesn't prevent forgetting to remove things entirely; it's about making the actual removal process easier. In any case, I think that's a side benefit; the clarity benefit to people reading the code while it's in the product is the main win. > > Whereas combining these > > into one class that can draw both raises other questions: why is the vector > > always on top of the bitmap -- what if we want it below? Why is vector + > bitmap > > one image, but vector + vector and bitmap + bitmap both two? > > Well, I still dislike this sort of inheritance for sharing code, So use composition. I don't have a preference. > and I think the > amount of code added here is very small (and hence easy to understand, with or > without comments). Which means separate classes would be extremely simple and easy to read. > How strongly do you feel about this topic? I think it should be done, but I feel a lot more strongly about the separate files for each icon proposal. That one is pretty much a "this has to happen".
Whatever, I'm tired, land it however you want, LGTM
https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/40001/ui/gfx/vector_icons.cc#... ui/gfx/vector_icons.cc:44: CIRCLE, 24, 24, 20, On 2015/07/02 08:06:11, Peter Kasting wrote: > On 2015/07/02 00:58:24, Evan Stade wrote: > > On 2015/07/02 00:23:53, Peter Kasting wrote: > > > On 2015/07/02 00:08:05, Evan Stade wrote: > > > > On 2015/07/01 22:26:57, Peter Kasting wrote: > > > > > I don't love inlining these instructions here. > > > > > > > > > > It seems like ideally, we'd have text files for each icon containing > these > > > > sorts > > > > > of instructions, and during the build process, a script would find them > > all, > > > > > automatically declare the icon enum in the header, and then > automatically > > > > > include the actual paths into the compiled source. > > > > > > > > Can you elaborate on why that is better? Do you just not like code and > data > > in > > > > the same file? If that's the case all we need to do is move this function > to > > a > > > > different file. > > > > > > I want updating icons to not have to touch code, since that will make it > > easier > > > for designers to do this directly. > > > > Currently image assets are separate from code, but I've still never seen a > > designer check in a change to one. In my recent experience, designers don't > even > > have a working build and lean heavily on canary for testing new functionality. > > Designers frequently have me point them to the appropriate image files in the > tree and then send me new versions to plug in directly atop them. That's easy > to reproduce with separate files here. You bring up a good point, which is how to visualize these resources outside of where they're actually used. However, I don't think drawing commands in a text file is a good way to do it. A designer is not going to pick out an icon by reading {R_CUBIC_TO, 0, 2.21, ...} It would probably be useful to build an icon gallery (in views_examples_exe or a similar small program) or a small program that took these drawing commands as inputs and drew them on the screen. > > > > (I'd like adding and removing icons to be as > > > easy as possible as well.) Also, as this list grows to dozens of items, I > > think > > > having these in separate, named files will be easier to find and read/change > > > what's happening for an icon than will a hundreds-of-lines file with > enormous > > > switch statements. > > > > Even if there were 200 icons in this switch statement, it would be easy to > find > > CHECK_CIRCLE with most text editors. > > In theory, yes. In practice, such files are a huge pain. I went through this > same process with the search engine prepopulate data. Initially everything was > in one giant file. It became annoying to scan it quickly and find what you > wanted, know how many engines there were, etc. Updating things required > touching several places in the file and making sure not to miss any. We then > split things into more files with a bit of scripting for the build system and it > improved stuff. > > The win here should be even larger. Since you said "I suppose that could wait until after this change" is it ok to put this off to another CL?
On 2015/07/06 18:36:00, Evan Stade wrote: > Since you said "I suppose that could wait until after this change" is it ok to > put this off to another CL? Yes.
estade@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul for views/ as sky is ooo
https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc... ui/gfx/vector_icons.cc:38: }; Would it make sense for PathElement to have a bool (e.g. is_command) which gets set in the constructors, and is DCHECK()ed below in PaintVectorIcon to make sure we are specifying the right number of args for each type? Something like: for (i ...) { DCHECK(path_elements[i].is_command); switch (path_elements[i].type) { case X: DCHECK(!path_elements[i + 1].is_command); DCHECK(!path_elements[i + 2].is_command); ... } } https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc... ui/gfx/vector_icons.cc:133: path.getLastPt(&last_point); DCHECK that this returns true? https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc... ui/gfx/vector_icons.cc:147: path.getLastPt(&last_point); ditto https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.h File ui/gfx/vector_icons.h (right): https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.h#... ui/gfx/vector_icons.h:28: size_t dp_size, We generally use 'dip_' prefix for device-independent pixels, and 'pixel_' for physical pixels. I assume 'dp' here is the latter? If so, mind changing this to 'pixel_size'? (also applies to the new variables in throbber.cc) https://codereview.chromium.org/1214693005/diff/100001/ui/views/controls/imag... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/100001/ui/views/controls/imag... ui/views/controls/image_view.cc:34: vector_id_(gfx::VectorIconId::VECTOR_ICON_NONE), Initialize color too?
https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc... ui/gfx/vector_icons.cc:38: }; On 2015/07/07 04:18:11, sadrul wrote: > Would it make sense for PathElement to have a bool (e.g. is_command) which gets > set in the constructors, and is DCHECK()ed below in PaintVectorIcon to make sure > we are specifying the right number of args for each type? Something like: > > for (i ...) { > DCHECK(path_elements[i].is_command); > switch (path_elements[i].type) { > case X: > DCHECK(!path_elements[i + 1].is_command); > DCHECK(!path_elements[i + 2].is_command); > ... > } > } > I think this adds too much boilerplate to the class and code for little gain. It's unlikely someone would get this wrong and not notice.
https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc File ui/gfx/vector_icons.cc (right): https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.cc... ui/gfx/vector_icons.cc:133: path.getLastPt(&last_point); On 2015/07/07 04:18:11, sadrul wrote: > DCHECK that this returns true? It needn't return true. You could start a path with H_LINE_TO and expect it to make a horizontal line from (0, 0) to some point. https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.h File ui/gfx/vector_icons.h (right): https://codereview.chromium.org/1214693005/diff/100001/ui/gfx/vector_icons.h#... ui/gfx/vector_icons.h:28: size_t dp_size, On 2015/07/07 04:18:11, sadrul wrote: > We generally use 'dip_' prefix for device-independent pixels, and 'pixel_' for > physical pixels. I assume 'dp' here is the latter? If so, mind changing this to > 'pixel_size'? (also applies to the new variables in throbber.cc) Done.
https://codereview.chromium.org/1214693005/diff/100001/ui/views/controls/imag... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/100001/ui/views/controls/imag... ui/views/controls/image_view.cc:34: vector_id_(gfx::VectorIconId::VECTOR_ICON_NONE), On 2015/07/07 04:18:11, sadrul wrote: > Initialize color too? Done.
lgtm (The test failures look relevant, btw) https://codereview.chromium.org/1214693005/diff/120001/ui/views/controls/imag... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/120001/ui/views/controls/imag... ui/views/controls/image_view.cc:35: vector_color_(SK_ColorGREEN), Hm, are you sure this is the right color? ;)
https://codereview.chromium.org/1214693005/diff/120001/ui/views/controls/imag... File ui/views/controls/image_view.cc (right): https://codereview.chromium.org/1214693005/diff/120001/ui/views/controls/imag... ui/views/controls/image_view.cc:35: vector_color_(SK_ColorGREEN), On 2015/07/07 21:45:04, sadrul wrote: > Hm, are you sure this is the right color? ;) that's the color I chose for "not set", we should never actually use this value though.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1214693005/#ps140001 (title: "compile?")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214693005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@chromium.org changed reviewers: + thestig@chromium.org
TBR thestig for:
chrome/app/theme/theme_resources.grd
chrome/browser/resources_util_unittest.cc
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214693005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/33b823af3e63287dbe850c253ae97593411d4f43 Cr-Commit-Position: refs/heads/master@{#337893}
Message was sent while issue was closed.
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
Message was sent while issue was closed.
I've landed a speculative revert to see whether this CL caused Mac bot breakage: https://codereview.chromium.org/1228923002/ There's another suspect for the breakage here though: https://codereview.chromium.org/1212443005 Will revert the revert if it doesn't improve things. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
