Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12704026:
Added new class gfx::ImageFamily. (Closed)
|
Created:
7 years, 9 months ago by Matt Giuca Modified:
7 years, 7 months ago CC:
chromium-reviews, rsesek+watch_chromium.org, benwells, chrome-apps-syd-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdded new class gfx::ImageFamily.
This represents a collection of differently sized images, and has methods to
pick the appropriately sized one.
Initially, this is intended for use as the type for
ShellIntegration::ShortcutInfo::favicon instead of gfx::Image.
BUG=189137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192244
Patch Set 1 #Patch Set 2 : Use Image::Size instead of Width and Height. #
Total comments: 34
Patch Set 3 : Respond to review feedback. #Patch Set 4 : Respond to review feedback. #Patch Set 5 : Refactor Get() method, splitting into three methods and simplifying the algorithm. #
Total comments: 24
Patch Set 6 : #
Total comments: 7
Patch Set 7 : Respond to reviewer feedback. #
Total comments: 12
Patch Set 8 : Respond to reviewer feedback. #Patch Set 9 : Moved const_iterator constructors into .cc file [chromium-style]. #Patch Set 10 : Fixed Windows build (missing UI_EXPORT). #
Messages
Total messages: 27 (0 generated)
Hi, This is the first part of Issue 189137 (the second part work-in-progress, if you want to see what I'm planning to use this for, is https://codereview.chromium.org/12881003/).
Adding pkotwicz.
https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:20: { Opening braces go on the line of the method definition. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:29: DCHECK(aspect > 0.0f); DCHECK_GT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:40: const gfx::Image* ImageFamily::Get(int width, int height) const This method is rather large. Would it make sense to split it up at all? https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:54: DCHECK(desired_aspect > 0.0f); _GT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:78: DCHECK(thinner_aspect > 0.0f); _GT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:79: DCHECK(thinner_aspect < desired_aspect); _LT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:83: DCHECK(wider_aspect > desired_aspect); _GT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:91: DCHECK(closest_aspect > desired_aspect); _GT https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:32: inline const_iterator begin() const { return const_iterator(map_.begin()); } Remove the |inline| keywords throughout this file. The compiler will be smart enough to figure this otu. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:76: inline const_iterator& operator++() { Do all these definitions have to be in the header? I think you can move them into the .cc file but keep the |inline| attribute. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:29: virtual void SetUp() { OVERRIDE https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:51: gfx::ImageFamily icon_family; Needs a trailing underscore https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:100: EXPECT_EQ(static_cast<gfx::Image*>(NULL), empty_family.Get(0, 32)); I generally use EXPECT_FALSE() for checking for NULL, since you don't have to do that cast.
https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:14: class Size; Remove this? https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:23: int height = size.height(); Nit: I believe gfx::Size::IsEmpty() checks if either width or height are 0. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:37: Add(image); Nit, merge the two lines into one https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:60: // If the image contains high-DPI bitmaps, the width refers to the width at Nit: An <aspect ratio, DIP width> pair. "DIP width of image" means width of the image's 1x representation.
Hi, thanks to both for the detailed review and helpful comments. Regarding the refactor of ImageFamily::Get(): I think it was good to force me to split it up because it's a lot easier to reason about now. Note that I did change the algorithm as well as moving code around, and it is not quite as efficient in the common case. The old algorithm would early-exit if it found a matching image of the desired aspect ratio. I have removed that early-exit case because it was making it very hard to split up and reason about. Now there are separate steps to 1. get the best aspect ratio and 2. get the best image in that aspect ratio. Note that in the special (but common) case where there is an image exactly matching the desired aspect ratio, with a width bigger or equal to the desired width, it used to exit with a single map lookup. Now it has to do two map lookups (one to determine that the aspect ratio is an exact match, and another to get the image). I think this is a good price to pay for much more readable code, so hopefully you agree. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:14: class Size; On 2013/03/27 19:04:53, pkotwicz wrote: > Remove this? Actually converted to an #include since I'm using methods of gfx::Size (so a forward declaration is not enough to be include-what-you-use compliant). https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:20: { On 2013/03/27 17:42:50, rsesek wrote: > Opening braces go on the line of the method definition. Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:23: int height = size.height(); On 2013/03/27 19:04:53, pkotwicz wrote: > Nit: I believe gfx::Size::IsEmpty() checks if either width or height are 0. Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:29: DCHECK(aspect > 0.0f); On 2013/03/27 17:42:50, rsesek wrote: > DCHECK_GT Done (and all of the other DCHECKs too, except the iterator comparisons since iterators do not have operator<<). https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:37: Add(image); On 2013/03/27 19:04:53, pkotwicz wrote: > Nit, merge the two lines into one Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:40: const gfx::Image* ImageFamily::Get(int width, int height) const On 2013/03/27 17:42:50, rsesek wrote: > This method is rather large. Would it make sense to split it up at all? Done. I'll discuss in the free-form comments. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:54: DCHECK(desired_aspect > 0.0f); On 2013/03/27 17:42:50, rsesek wrote: > _GT Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:78: DCHECK(thinner_aspect > 0.0f); On 2013/03/27 17:42:50, rsesek wrote: > _GT Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:79: DCHECK(thinner_aspect < desired_aspect); On 2013/03/27 17:42:50, rsesek wrote: > _LT Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:83: DCHECK(wider_aspect > desired_aspect); On 2013/03/27 17:42:50, rsesek wrote: > _GT Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.cc:91: DCHECK(closest_aspect > desired_aspect); On 2013/03/27 17:42:50, rsesek wrote: > _GT Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:32: inline const_iterator begin() const { return const_iterator(map_.begin()); } On 2013/03/27 17:42:50, rsesek wrote: > Remove the |inline| keywords throughout this file. The compiler will be smart > enough to figure this otu. Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:60: // If the image contains high-DPI bitmaps, the width refers to the width at On 2013/03/27 19:04:53, pkotwicz wrote: > Nit: An <aspect ratio, DIP width> pair. > "DIP width of image" means width of the image's 1x representation. Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family.... ui/gfx/image/image_family.h:76: inline const_iterator& operator++() { On 2013/03/27 17:42:50, rsesek wrote: > Do all these definitions have to be in the header? I think you can move them > into the .cc file but keep the |inline| attribute. But they are used (and expected to be inlined) externally to this module. (Not in the current CL, but ImageFamily is designed to be iterated over by external users.) As far as I'm aware, you can't inline a function defined in a .cc file; see http://www.parashift.com/c++-faq-lite/inline-member-fns.html. I'm happy to move these definitions out of the class declaration and into the bottom of the file, if you think that will make things more readable. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:29: virtual void SetUp() { On 2013/03/27 17:42:50, rsesek wrote: > OVERRIDE Done. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:51: gfx::ImageFamily icon_family; On 2013/03/27 17:42:50, rsesek wrote: > Needs a trailing underscore Done. Also renamed to image_family_. https://codereview.chromium.org/12704026/diff/5001/ui/gfx/image/image_family_... ui/gfx/image/image_family_unittest.cc:100: EXPECT_EQ(static_cast<gfx::Image*>(NULL), empty_family.Get(0, 32)); On 2013/03/27 17:42:50, rsesek wrote: > I generally use EXPECT_FALSE() for checking for NULL, since you don't have to do > that cast. Done.
Looks pretty good. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:21: map_[MapKey(1.0f, 0)] = image; Should we just not store |image| if size.IsEmpty() ? https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:48: // Find the closest aspect ratio in the map to the desired one. Nit: I am unsure if the comment above says anything that the function name below does not. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:53: int desired_width = closest_aspect <= desired_aspect ? width : Nit, fix alignment like so: int desired_width = closest_aspect <= desired_aspect ? width : static_cast<int>(ceilf(height * closest_aspect)); https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:93: } Nit, this MIGHT be simpler: DCHECK(!map_.empty()); float wider_aspect = std::numeric_limits<float>::max(); std::map<MapKey, gfx::Image>::const_iterator greater_or_equal = map_.lower_bound(MapKey(desired_aspect, 0.0f)); if (greater_or_equal != map_.end()) { wider_aspect = greater_or_equal->first.aspect(); DCHECK_GE(wider_aspect, desired_aspect); } if (wider_aspect == desired_aspect) return desired_aspect; float thinner_aspect = std::numeric_limits<float>::max(); ... I am fine if you keep it as is though. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:96: const gfx::Image* ImageFamily::GetWithExactAspect(float aspect, int width) For the sake of wrapping: const gfx::Image* ImageFamily::GetWithExactAspect(float aspect, int width) const { https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:103: // We have found the smallest image of the same size or greater. Nit: I am unsure if this comment says anything than the code above does not. But I will leave it up to you to decide whether it is needed. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:110: // This must be true because there must be at least one image with aspect. Nit: |aspect|. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:69: class const_iterator : Is there a way you can avoid creating a custom iterator? It seems as if you are creating a custom iterator to make the iterator to |map_| look like a vector iterator. Would it make sense to have: std::map<MapKey, size_t> map_; Where the value is an index into |images_|. std::vector<gfx::Image> images_; https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family_unittest.cc:17: // reported when a test fails. Can you use a function instead of a macro?
https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:21: map_[MapKey(1.0f, 0)] = image; On 2013/03/29 00:25:35, pkotwicz wrote: > Should we just not store |image| if size.IsEmpty() ? I think that would be more surprising semantics. That would mean that: image_family.Add(gfx::Image()); image_family.Get(32, 32) returns NULL, whereas I would expect it to return the empty image I put in. I would expect it to prioritize any non-empty image, but if I put in only an empty image, I expect it to come out again. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:48: // Find the closest aspect ratio in the map to the desired one. On 2013/03/29 00:25:35, pkotwicz wrote: > Nit: I am unsure if the comment above says anything that the function name below > does not. Done. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:53: int desired_width = closest_aspect <= desired_aspect ? width : On 2013/03/29 00:25:35, pkotwicz wrote: > Nit, fix alignment like so: > int desired_width = closest_aspect <= desired_aspect ? > width : static_cast<int>(ceilf(height * closest_aspect)); Done. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:93: } On 2013/03/29 00:25:35, pkotwicz wrote: > Nit, this MIGHT be simpler: > > DCHECK(!map_.empty()); > > float wider_aspect = std::numeric_limits<float>::max(); > std::map<MapKey, gfx::Image>::const_iterator greater_or_equal = > map_.lower_bound(MapKey(desired_aspect, 0.0f)); > if (greater_or_equal != map_.end()) { > wider_aspect = greater_or_equal->first.aspect(); > DCHECK_GE(wider_aspect, desired_aspect); > } > > if (wider_aspect == desired_aspect) > return desired_aspect; > > float thinner_aspect = std::numeric_limits<float>::max(); > > ... > > I am fine if you keep it as is though. Using float max to avoid having a special case where there is no wider aspect ratio works, but I don't think there is a good way to do it on the thinner end. You set thinner_aspect to float max, but doesn't it have to be 0? (Because thinner_aspect should be <= desired_aspect.) If thinner_aspect is 0, you have divide-by-zero to worry about (and I thought that would just be +infinity, but I checked, and it's actually undefined behaviour). So although my code has a few special cases, I think it accurately reflects that the logic itself has special cases, so I'll stick with it. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:96: const gfx::Image* ImageFamily::GetWithExactAspect(float aspect, int width) On 2013/03/29 00:25:35, pkotwicz wrote: > For the sake of wrapping: > const gfx::Image* ImageFamily::GetWithExactAspect(float aspect, > int width) const { Done. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:103: // We have found the smallest image of the same size or greater. On 2013/03/29 00:25:35, pkotwicz wrote: > Nit: I am unsure if this comment says anything than the code above does not. But > I will leave it up to you to decide whether it is needed. It's not obvious to me as a reader exactly how lower_bound works without having to look it up, so I think it's a helpful comment. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:110: // This must be true because there must be at least one image with aspect. On 2013/03/29 00:25:35, pkotwicz wrote: > Nit: |aspect|. Done. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:69: class const_iterator : On 2013/03/29 00:25:35, pkotwicz wrote: > Is there a way you can avoid creating a custom iterator? I know it's quite excessive and I feel bad about it, but I couldn't think of a way. Note that I originally wrote it to just return the map iterator, but then that exposed the client to the implementation detail that it is using a map. At one point I changed the type of the map key and realised that I was exposing the client to volatility, so I decided to wrap it in an iterator that would just expose what I needed. > Would it make sense to have: > std::map<MapKey, size_t> map_; Where the value is an index into |images_|. > std::vector<gfx::Image> images_; I'd rather not do that, since then I need to manually make sure that the images in the vector are in the correct order. Unless you think I should just have the images iterate in their insertion order? That might be acceptable. Note that as a side-effect, this would make it so that duplicate-sized images are no longer removed, but they are not accessible in the map. This makes the interface to the class more complicated, but maybe it is a good thing to preserve all of the images. What do you think? (I will wait for further comment by you and possibly rsesek before rewriting this, because it's a big change to both the code and the interface.) https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family_unittest.cc:17: // reported when a test fails. On 2013/03/29 00:25:35, pkotwicz wrote: > Can you use a function instead of a macro? The reason I used a macro is so that when the expectations fail, it reports the line number of the test that failed instead of simply line 22 or 23. I had it as a function before, and it was completely useless for identifying which test had failed.
https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family_unittest.cc:17: // reported when a test fails. On 2013/03/30 00:54:14, Matt Giuca wrote: > On 2013/03/29 00:25:35, pkotwicz wrote: > > Can you use a function instead of a macro? > > The reason I used a macro is so that when the expectations fail, it reports the > line number of the test that failed instead of simply line 22 or 23. I had it as > a function before, and it was completely useless for identifying which test had > failed. When I've done this in the past, I just pass __LINE__ to a function. https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:53: width : static_cast<int>(ceilf(height * closest_aspect)); Do you need this cast or can you just let it truncate (though that may cause a warning)? https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:60: struct MapKey : std::pair<float, int> { Public types need to be declared first in the class order. I think forward-declaring this may be sufficient… Does this need to be a public struct, though? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:118: friend class ImageFamily; friends come first
https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family_unittest.cc:17: // reported when a test fails. Can you show me an example? I don't understand what I'd do with __LINE__ once I had it (the EXPECT is still going to report Line 22). And this would also require that every call to this function include __LINE__ which would be annoying. tapted also suggested using SCOPED_TRACE, but that would require four lines of code for each call to this function. Is there a really good reason not to use a macro here? It seems to solve this problem perfectly, and it's fairly small and contained to this module. https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:53: width : static_cast<int>(ceilf(height * closest_aspect)); I explicitly want ceil because it will satisfy the property of finding an image with height >= the desired height. If I use trunc/floor, it might find an image with a height slightly smaller than the desired height. https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:60: struct MapKey : std::pair<float, int> { Thanks for pointing this out. I have re-ordered and made the struct private. However, because MapKey is private, it needs to go after the public const_iterator class. But since const_iterator refers to MapKey, I had to forward-declare it in a special private section at the top of the class. Is this alright? I couldn't find anything in the style guide about it, but it seems like that's the neatest solution. https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:118: friend class ImageFamily; On 2013/04/01 20:57:45, rsesek wrote: > friends come first Done.
LGTM on my part https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:21: map_[MapKey(1.0f, 0)] = image; Fair enough https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:93: } Fair enough https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:69: class const_iterator : Ok, I am for keeping the iterator. I think that we should keep the current behavior where we remove duplicate sized images. https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:66: map_.lower_bound(MapKey(desired_aspect, 0.0f)); Nit: 0 instead of 0.0f to match std::pair<float, int> https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:74: // ratio >= desired_aspect, and less_than will point to the last image with Nits: ratio > |desired_aspect| |greater_or_equal|, |less_than| https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:101: // Find the two images of given aspect ratio on either side of width. Nit: |width| https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:121: float width() const { return second; } Should this be int? https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:126: // map_ must not be empty. |desired_aspect| must be > 0.0. Nit: |map_|. https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:133: // map_ of aspect ratio |aspect|. Nits: "is not at least one image in map_" -> "no image in |map_|"
LGTM % Peter's comments. https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... File ui/gfx/image/image_family_unittest.cc (right): https://codereview.chromium.org/12704026/diff/20001/ui/gfx/image/image_family... ui/gfx/image/image_family_unittest.cc:17: // reported when a test fails. On 2013/04/02 06:11:28, Matt Giuca wrote: > Can you show me an example? I don't understand what I'd do with __LINE__ once I > had it (the EXPECT is still going to report Line 22). And this would also > require that every call to this function include __LINE__ which would be > annoying. > > tapted also suggested using SCOPED_TRACE, but that would require four lines of > code for each call to this function. > > Is there a really good reason not to use a macro here? It seems to solve this > problem perfectly, and it's fairly small and contained to this module. You'd do it like: ImageFamilyTest::CheckNonNullAndSize(gfx::Image*, int width, int height, int line) { const char kTestLine[] = "From test at line "; EXPECT_TRUE(image) << kTestLine << line; EXPECT_EQ(width, image->Width()) << kTestLine << line; EXPECT_EQ(height, image->Height()) << kTestLine << line; } TEST_F() { CheckNonNullAndSize(image, width, height, __LINE__); } But I guess this is fine. https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/26001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:60: struct MapKey : std::pair<float, int> { On 2013/04/02 06:11:28, Matt Giuca wrote: > Thanks for pointing this out. I have re-ordered and made the struct private. > > However, because MapKey is private, it needs to go after the public > const_iterator class. But since const_iterator refers to MapKey, I had to > forward-declare it in a special private section at the top of the class. Is this > alright? I couldn't find anything in the style guide about it, but it seems like > that's the neatest solution. Yeah, that seems correct.
Thanks, I fixed those last nits. ben@: Please approve (for OWNERS of the ui/ directory; this just needs a quick review of the .gyp file changes).
https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family.cc File ui/gfx/image/image_family.cc (right): https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:66: map_.lower_bound(MapKey(desired_aspect, 0.0f)); On 2013/04/02 16:06:31, pkotwicz wrote: > Nit: 0 instead of 0.0f to match std::pair<float, int> Done. (Good catch.) https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:74: // ratio >= desired_aspect, and less_than will point to the last image with On 2013/04/02 16:06:31, pkotwicz wrote: > Nits: ratio > |desired_aspect| > |greater_or_equal|, |less_than| Done. (Also put || around all variable names in comments in this file.) https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.cc:101: // Find the two images of given aspect ratio on either side of width. On 2013/04/02 16:06:31, pkotwicz wrote: > Nit: |width| Done. https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family.h File ui/gfx/image/image_family.h (right): https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:121: float width() const { return second; } On 2013/04/02 16:06:31, pkotwicz wrote: > Should this be int? Done. (Good catch) https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:126: // map_ must not be empty. |desired_aspect| must be > 0.0. On 2013/04/02 16:06:31, pkotwicz wrote: > Nit: |map_|. Done. https://codereview.chromium.org/12704026/diff/32001/ui/gfx/image/image_family... ui/gfx/image/image_family.h:133: // map_ of aspect ratio |aspect|. On 2013/04/02 16:06:31, pkotwicz wrote: > Nits: "is not at least one image in map_" -> "no image in |map_|" Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12704026/41001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12704026/62001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12704026/76002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12704026/76002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12704026/76002
Message was sent while issue was closed.
Change committed as 192244
Message was sent while issue was closed.
Do we really need this? ImageSkia already supports having multiple represenation bitmaps for a single bitmap.
Message was sent while issue was closed.
On 2013/05/02 22:33:14, Nico wrote: > Do we really need this? ImageSkia already supports having multiple represenation > bitmaps for a single bitmap. The bug (http://crbug.com/189137) should serve as motivation for this, but briefly, ImageSkia only supports a single *image* with bitmaps for representing high-DPI versions of that image at different (fixed) scales (specifically 100%, 133%, 140%, 150%, 180% and 200%). Note that those are supposed to be different scales of the same image. We needed a way to represent a collection of different images (representing the same concept) at arbitrary different sizes. The most obvious use for this is application icons: icons can have any size, and differently-sized icons can contain different content (e.g., a 16x16 icon has perspective or details removed compared to a 128x128 icon). This is orthogonal to the image sizes in ImageSkia. For example, it is possible to have the 16x16 icon have a 200% high-DPI size, which is a 32x32 bitmap, but has the logical size of 16x16 (so it has the details removed compared to the 32x32 image).
On Thu, May 2, 2013 at 5:10 PM, <mgiuca@chromium.org> wrote: > On 2013/05/02 22:33:14, Nico wrote: > >> Do we really need this? ImageSkia already supports having multiple >> > represenation > >> bitmaps for a single bitmap. >> > > The bug (http://crbug.com/189137) should serve as motivation for this, but > briefly, ImageSkia only supports a single *image* with bitmaps for > representing > high-DPI versions of that image at different (fixed) scales (specifically > 100%, > 133%, 140%, 150%, 180% and 200%). Note that those are supposed to be > different > scales of the same image. > > We needed a way to represent a collection of different images > (representing the > same concept) at arbitrary different sizes. The most obvious use for this > is > application icons: icons can have any size, and differently-sized icons can > contain different content (e.g., a 16x16 icon has perspective or details > removed > compared to a 128x128 icon). This is orthogonal to the image sizes in > ImageSkia. > For example, it is possible to have the 16x16 icon have a 200% high-DPI > size, > which is a 32x32 bitmap, but has the logical size of 16x16 (so it has the > details removed compared to the 32x32 image). > I see. Could the class comment for ImageFamily call out that people want ImageSkia if they just want an image at different scale factors? Someone thought that ImageFamily is that class, because the name kinda suggests this. > > https://chromiumcodereview.**appspot.com/12704026/<https://chromiumcodereview... > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
