|
|
Created:
8 years, 5 months ago by xiyuan Modified:
8 years, 4 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionchromeos: Fix pixelated icons in app list and launcher (part 2)
Add an ExtensionIconImage, which loads image that supports DIP
by having a special ImageSkiaSource that makes
ImageLoadingTracker to load image for additional DIP scale.
BUG=131738, 131739
TEST=None. Wait for the last CL to update laucher/app list code to use LoadImageInDIP to verify.
Patch Set 1 #Patch Set 2 : #Patch Set 3 : address oshima's comments from http://codereview.chromium.org/10699065/ #
Total comments: 7
Patch Set 4 : for comments in #3, add an ImageSource and makes ImageLoadingTracker auto load image for additional… #
Total comments: 34
Patch Set 5 : for pkotwicz and oshima's comments in #4 #Patch Set 6 : Defer actual resource loading until ImageSource gets request #
Total comments: 15
Patch Set 7 : addres comments in #6 #Patch Set 8 : add more tests #
Total comments: 17
Patch Set 9 : for pkotwicz's comments in #8 #Patch Set 10 : rebase and refactor #
Total comments: 6
Patch Set 11 : for comments in #10 #
Total comments: 17
Messages
Total messages: 43 (0 generated)
Splitted from http://codereview.chromium.org/10699065/ so that we have a more focused review on ImageLoadingTracker.
At a high level, I believe that you should follow the same pattern as http://codereview.chromium.org/10500015/ (process all scale factors which are currently in use) and then use an ImageSkiaSource to load more scale factors as needed. The reason that I advocate this method is 1) We do not download icons for 1x and 2x when a user is using CrOS at 1x. 2) When the ImageSkia is returned, it is guaranteed to be non null so we don't need to do any special processing there. ImageSkias with an ImageSkiaSource cannot possible be null by design.
Can you sync with Oshima, Ben, and Aaron as to whether or not we want to fetch the largest image when DIP is enabled and downscale?
http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:100: void LoadDIPImage(const extensions::Extension* extension, Nit: I am not a fan of this method name, but I don't have any better suggestions. You should pass in the match type. http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:109: const std::vector<ImageInfo>& info_list, Can you please write a multires version of this method as well?
http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:222: } instead of having different code path, won't this work? const ui::ScaleFactor scale_factor = gfx::Screen::IsDIPEnabled() ? ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_100P ? http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:100: void LoadDIPImage(const extensions::Extension* extension, On 2012/07/11 21:02:25, pkotwicz wrote: > Nit: I am not a fan of this method name, but I don't have any better > suggestions. > > You should pass in the match type. LoadImageInDIP is better
On 2012/07/11 20:44:03, pkotwicz wrote: > At a high level, I believe that you should follow the same pattern as > http://codereview.chromium.org/10500015/ (process all scale factors which are > currently in use) and then use an ImageSkiaSource to load more scale factors as > needed. > The reason that I advocate this method is 1) We do not download icons for 1x and > 2x when a user is using CrOS at 1x. > 2) When the ImageSkia is returned, it is guaranteed to be non null so we don't > need to do any special processing there. ImageSkias with an ImageSkiaSource > cannot possible be null by design. The pattern looks good. Actually, it now seems to me that we could handle this with just one LoadImage. I'll need to modify the prototype a bit to pass down args to get ExtensionResource instead of passing the resource itself. This would all our ImageSkiaSource to find proper resource later when different DIP scale factor is needed. Let me revise the CL.
I have updated the CL to follow pattern in http://codereview.chromium.org/10500015/. LoadImageInDIP loads images for interesting scale factors and installs an ImageSkiaSource that loads additional images when needed. So if observer of ImageLoadingTracker wants to handle DIP change, all it needs to do is to keep the ImageLoadingTracker around and OnImageLoaded would be called when DIP is changed and new representation is loaded. I still keep LoadImage around because some callers construct the ExtensionResource to load using path instead of size. http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:222: } On 2012/07/11 21:09:33, oshima wrote: > instead of having different code path, won't this work? > > const ui::ScaleFactor scale_factor = > gfx::Screen::IsDIPEnabled() ? ui::SCALE_FACTOR_200P : ui::SCALE_FACTOR_100P ? Done. http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:100: void LoadDIPImage(const extensions::Extension* extension, On 2012/07/11 21:09:33, oshima wrote: > On 2012/07/11 21:02:25, pkotwicz wrote: > > Nit: I am not a fan of this method name, but I don't have any better > > suggestions. > > > > You should pass in the match type. > > LoadImageInDIP is better Changed name to LoadImageInDIP and pass in match_type as well. http://codereview.chromium.org/10701087/diff/6002/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:109: const std::vector<ImageInfo>& info_list, On 2012/07/11 21:02:25, pkotwicz wrote: > Can you please write a multires version of this method as well? I take this function as a low-level function that just loads extension resource and construct an ImageSkia using the loaded bitmap and scale_factor info in ImageInfo. And in this sense, it supports multi-DIP, multi-resolution already.
I will review tonight
+sky
http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:90: BrowserThread::FILE, FROM_HERE, this is not the scope of this change, but at some point, you'll need to change this to use thread pool. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:276: // gfx::Screen::GetScaleFactorsInUse(); I thought we're going to load the correct image for current scale factor? Am I wrong? http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:445: if (image) { DCHECK(!images.HasRepresentation(image_info.scale_factor)) http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:48: // is change and ImageSkia requests a representation for the new scale factor, "and Image" (space)
It looks pretty good. Can you do me a huge favor and clean up the variable and method names. In particular, all SkBitmap variables should not be named image. ImageInfo should be named ImageRepInfo instead because it refers a bitmap at a particular scale. I am fine with keeping the method name ImageLoadingTracker::LoadImages as is http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... chrome/browser/extensions/app_shortcut_manager.cc:117: ui::SCALE_FACTOR_NONE)); ui::SCALE_FACTOR_100P http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... chrome/browser/extensions/app_shortcut_manager.cc:137: ui::SCALE_FACTOR_NONE)); Change this to ui::SCALE_FACTOR_100P http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:190: // An ImageSkiaSource to load image for additional scale factor. Nit: factors http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:261: info_list.push_back(ImageInfo(resource, max_size, ui::SCALE_FACTOR_NONE)); Just use SCALE_FACTOR_100P. (SCALE_FACTOR_NONE is value equivalent. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:307: load_info.max_size_in_dip); The loaded bitmap is not always resized to max_size. In particular, it is only resized if it is greater than max size. For dip images, the bitmaps should always be resized. (We seem to be resizing the results of ImageLoadingTracker to the desired size in the few cases I checked). I suggest in ImageInfo, renaming max_size to desired_size and adding a resize method field which would have values: RESIZE_WHEN_LARGER, ALWAYS_RESIZE http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:311: DoLoadImages(id, info_list); Call this DoLoadImage. You are still loading a single gfx::Image, though it is a multiresolution image. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:365: OnImageLoaded(&image, *it, it->max_size, id, false); Rename image->bitmap. Can you add a TODO to support caching with LoadImageInDIP? http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:391: return; You need to check here if |load_info->pending_count==0| or do something else to avoid duplicate requests. In particular, if you get any requests from ImageSkiaSource before the loading for the initial scale factors has completed. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:455: image ? *image : SkBitmap(), Rename image->bitmap http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:79: // Information about a single image to load from a extension resource. image->bitmap/image rep http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:101: void LoadImage(const extensions::Extension* extension, Can you put a comment to the effect that the method above is deprecated? http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:108: void LoadImageInDIP(const extensions::Extension* extension, Can you call this simply "LoadImageSkia"? http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:152: gfx::ImageSkia images; Can you call this variable image_skia? I think it makes things a bit simplier. Sorry that I asking you to do a whole bunch of mundane renames but it makes reading this file much more understandable. In general, variables of type SkBitmap should not be named image.
http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... File chrome/browser/extensions/app_shortcut_manager.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... chrome/browser/extensions/app_shortcut_manager.cc:117: ui::SCALE_FACTOR_NONE)); On 2012/07/16 17:33:52, pkotwicz wrote: > ui::SCALE_FACTOR_100P Changed but AppShortcutManager loads multi-resolution images to create an icon out of it and does not care DIP. Shoudn't SCALE_FACTOR_NONE sounds more correct in this sense? http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/a... chrome/browser/extensions/app_shortcut_manager.cc:137: ui::SCALE_FACTOR_NONE)); On 2012/07/16 17:33:52, pkotwicz wrote: > Change this to ui::SCALE_FACTOR_100P Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:90: BrowserThread::FILE, FROM_HERE, On 2012/07/16 17:22:33, oshima wrote: > this is not the scope of this change, but at some point, you'll need to change > this to use thread pool. Agree. But I'd prefer to do it in a follow up CL so that we could focus. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:190: // An ImageSkiaSource to load image for additional scale factor. On 2012/07/16 17:33:52, pkotwicz wrote: > Nit: factors Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:261: info_list.push_back(ImageInfo(resource, max_size, ui::SCALE_FACTOR_NONE)); On 2012/07/16 17:33:52, pkotwicz wrote: > Just use SCALE_FACTOR_100P. (SCALE_FACTOR_NONE is value equivalent. Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:276: // gfx::Screen::GetScaleFactorsInUse(); On 2012/07/16 17:22:33, oshima wrote: > I thought we're going to load the correct image for current scale factor? Am I > wrong? You are correct. The code is in comment because http://codereview.chromium.org/10702098/ is not landed. :) http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:307: load_info.max_size_in_dip); On 2012/07/16 17:33:52, pkotwicz wrote: > The loaded bitmap is not always resized to max_size. In particular, it is only > resized if it is greater than max size. > For dip images, the bitmaps should always be resized. > (We seem to be resizing the results of ImageLoadingTracker to the desired size > in the few cases I checked). > I suggest in ImageInfo, renaming max_size to desired_size > and adding a resize method field which would have values: RESIZE_WHEN_LARGER, > ALWAYS_RESIZE Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:311: DoLoadImages(id, info_list); On 2012/07/16 17:33:52, pkotwicz wrote: > Call this DoLoadImage. You are still loading a single gfx::Image, though it is a > multiresolution image. Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:365: OnImageLoaded(&image, *it, it->max_size, id, false); On 2012/07/16 17:33:52, pkotwicz wrote: > Rename image->bitmap. > Can you add a TODO to support caching with LoadImageInDIP? Rename done. Caching is supported on the bitmap/image rep basis. When LoadImageSkia is called, cached bitmap/image rep will be used if it is present. Only the bitmap/image rep that does not exist in cache would go to |loader_| to get loaded. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:391: return; On 2012/07/16 17:33:52, pkotwicz wrote: > You need to check here if |load_info->pending_count==0| or do something else to > avoid duplicate requests. > In particular, if you get any requests from ImageSkiaSource before the loading > for the initial scale factors has completed. Good catch. Think we should allow LoadImageForScaleFactor being called before previous pending loads are finished. Thus, we should update load_info->pending_count in DoLoadImage by adding the new requests count (i.e. info_list.size) instead of replacing it. This should handle the case. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:445: if (image) { On 2012/07/16 17:22:33, oshima wrote: > DCHECK(!images.HasRepresentation(image_info.scale_factor)) This DCHECK actually would fail because ImageLoadingTracker::ImageSource returns an empty ImageSkiaRep in its GetImageForScale and expect ImageLoadingTracker to loads the image and replace the empty ImageSkiaRep here. The ImageSkaRep replacement logic is in pkotwicz's http://codereview.chromium.org/10500015/. And this CL depends on that. :p http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.cc:455: image ? *image : SkBitmap(), On 2012/07/16 17:33:52, pkotwicz wrote: > Rename image->bitmap Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:48: // is change and ImageSkia requests a representation for the new scale factor, On 2012/07/16 17:22:33, oshima wrote: > "and Image" (space) Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:79: // Information about a single image to load from a extension resource. On 2012/07/16 17:33:52, pkotwicz wrote: > image->bitmap/image rep Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:101: void LoadImage(const extensions::Extension* extension, On 2012/07/16 17:33:52, pkotwicz wrote: > Can you put a comment to the effect that the method above is deprecated? Added a comment that indicates this method loads raw pixel sized image and asks the caller to use LoadImageSkia if a DIP-aware image is needed. Not sure about whether this is going to deprecate though because in some cases, loading raw pixel sized image might be needed and this function might be handy than creating an ImageRepInfo vector and calling LoadImages. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:108: void LoadImageInDIP(const extensions::Extension* extension, On 2012/07/16 17:33:52, pkotwicz wrote: > Can you call this simply "LoadImageSkia"? Done. http://codereview.chromium.org/10701087/diff/9001/chrome/browser/extensions/i... chrome/browser/extensions/image_loading_tracker.h:152: gfx::ImageSkia images; On 2012/07/16 17:33:52, pkotwicz wrote: > Can you call this variable image_skia? I think it makes things a bit simplier. > Sorry that I asking you to do a whole bunch of mundane renames but it makes > reading this file much more understandable. In general, variables of type > SkBitmap should not be named image. Done.
CL updated to defer resource loading until ImageSource is asked for a representation per dicussion with oshima.
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), You need to validate that you are capable of generating a non null image. This is not necessarily the case as Extension::GetIconResource can generate an empty IconResource. In particular, you want to return an empty gfx::Image if Extension::GetIconResource fails. (Check out line 246 on the left side of the diff and line 65 in launcher_app_icon_loader) The only suggestion I have is to load immediately at least one scale factor and decide that if you can't load any of those scale factors that you won't able to load any other scale factors.
The validation is super cheap so you can also validate whether you can generate a Bitmap for at least one scale factor here. If you can't you can return an empty image instead.
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), The validation is super cheap so you can also validate whether you can generate a Bitmap for at least one scale factor here. If you can't you can return an empty image instead.
approach lg. a few nits, + please address peter's comment. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:391: load_info->resource_match_type); you can move this to previous line. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { how about add dcheck to make sure image_skia dones't have ImageSkiaRep for image_info.scale_factor? http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:96: // always resize it. I think it's better to move this comment to enum above. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:100: // than |desired_size| it will be resized to those dimensions. remove extra space between those and dimensions. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:104: ui::ScaleFactor scale_factor; can these be const?
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:296: observer_->OnImageLoaded(gfx::Image(load_info.image_skia), On 2012/07/18 14:17:53, pkotwicz wrote: > The validation is super cheap so you can also validate whether you can generate > a Bitmap for at least one scale factor here. If you can't you can return an > empty image instead. Good catch. Added a CanLoadImage to check whether resource could be loaded and returns an empty Image if we could not. Also fixed OnBitmapLoaded where we should return an empty Image if loaded ImageSkia is empty (i.e. has no reps). http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:391: load_info->resource_match_type); On 2012/07/18 15:16:28, oshima wrote: > you can move this to previous line. Done. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { On 2012/07/18 15:16:28, oshima wrote: > how about add dcheck to make sure image_skia dones't have ImageSkiaRep for > image_info.scale_factor? Should not do that because ImageLoadingTracker is also used to load multiple images to construct icons (on windows and mac I think), where scale_factor is always 1x for those bitmaps. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:96: // always resize it. On 2012/07/18 15:16:28, oshima wrote: > I think it's better to move this comment to enum above. Done. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:100: // than |desired_size| it will be resized to those dimensions. On 2012/07/18 15:16:28, oshima wrote: > remove extra space between those and dimensions. Done. http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:104: ui::ScaleFactor scale_factor; On 2012/07/18 15:16:28, oshima wrote: > can these be const? We cannot use default copy constructor if they are const.
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { On 2012/07/18 17:28:32, xiyuan wrote: > On 2012/07/18 15:16:28, oshima wrote: > > how about add dcheck to make sure image_skia dones't have ImageSkiaRep for > > image_info.scale_factor? > > Should not do that because ImageLoadingTracker is also used to load multiple > images to construct icons (on windows and mac I think), where scale_factor is > always 1x for those bitmaps. Can you point me the code? We shouldn't add multiple image reps with the same scale factor.
http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:438: if (bitmap) { On 2012/07/18 20:41:49, oshima wrote: > On 2012/07/18 17:28:32, xiyuan wrote: > > On 2012/07/18 15:16:28, oshima wrote: > > > how about add dcheck to make sure image_skia dones't have ImageSkiaRep for > > > image_info.scale_factor? > > > > Should not do that because ImageLoadingTracker is also used to load multiple > > images to construct icons (on windows and mac I think), where scale_factor is > > always 1x for those bitmaps. > > Can you point me the code? We shouldn't add multiple image reps with the same > scale factor. The one I mentioned is AppShortcutManager::InstallApplicationShortcuts. It seems like only care about raw pixel sized resource. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
On Wed, Jul 18, 2012 at 1:45 PM, <xiyuan@chromium.org> wrote: > > http://codereview.chromium.**org/10701087/diff/17004/** > chrome/browser/extensions/**image_loading_tracker.cc<http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc> > File chrome/browser/extensions/**image_loading_tracker.cc (right): > > http://codereview.chromium.**org/10701087/diff/17004/** > chrome/browser/extensions/**image_loading_tracker.cc#**newcode438<http://codereview.chromium.org/10701087/diff/17004/chrome/browser/extensions/image_loading_tracker.cc#newcode438> > chrome/browser/extensions/**image_loading_tracker.cc:438: if (bitmap) { > On 2012/07/18 20:41:49, oshima wrote: > >> On 2012/07/18 17:28:32, xiyuan wrote: >> > On 2012/07/18 15:16:28, oshima wrote: >> > > how about add dcheck to make sure image_skia dones't have >> > ImageSkiaRep for > >> > > image_info.scale_factor? >> > >> > Should not do that because ImageLoadingTracker is also used to load >> > multiple > >> > images to construct icons (on windows and mac I think), where >> > scale_factor is > >> > always 1x for those bitmaps. >> > > Can you point me the code? We shouldn't add multiple image reps with >> > the same > >> scale factor. >> > > The one I mentioned is AppShortcutManager::**InstallApplicationShortcuts. > It seems like only care about raw pixel sized resource. > Sounds to me that it shouldn't use ImageSkia for this purpose. How this was working before we introduced ImageSkia? - oshima > > http://code.google.com/**searchframe#OAMlx_jo-ck/src/** > chrome/browser/extensions/app_**shortcut_manager.cc&exact_** > package=chromium&q=**AppShortcutManager::**InstallApplicationShortcuts&** > type=cs&l=80<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/app_shortcut_manager.cc&exact_package=chromium&q=AppShortcutManager::InstallApplicationShortcuts&type=cs&l=80> > > http://codereview.chromium.**org/10701087/<http://codereview.chromium.org/107... >
On 2012/07/18 20:49:48, oshima wrote: > Sounds to me that it shouldn't use ImageSkia for this purpose. How this was > working before we introduced ImageSkia? Before we touch it, ImageLoadingTracker was using std::vector<SkBitmap> for all loaded bitmaps and construct a gfx::Image out of it. Peter changed it to return an gfx::Image from ImageSkia in r137520 and my change was based on that, assuming it's okay to use ImageSkia to carry multiple bitmaps of the same scale factor. http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/ima...
On 2012/07/18 20:57:40, xiyuan wrote: > On 2012/07/18 20:49:48, oshima wrote: > > Sounds to me that it shouldn't use ImageSkia for this purpose. How this was > > working before we introduced ImageSkia? > > Before we touch it, ImageLoadingTracker was using std::vector<SkBitmap> for all > loaded bitmaps and construct a gfx::Image out of it. > > Peter changed it to return an gfx::Image from ImageSkia in r137520 and my change > was based on that, assuming it's okay to use ImageSkia to carry multiple bitmaps > of the same scale factor. > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/ima... OK, we'll address this separately. lgtm for my part.
I'm not an OWNER in this directory, so you don't really need my to review this. That said, I had a couple of high level comments below. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:48: // One needs to keep ImageLoadingTracker around as long as the returned This is a weird restriction. Seems like we should allow destroying the tracker once the image is obtained. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:125: void LoadImageSkia(const extensions::Extension* extension, Is there a way to avoid having the two methods? That is, can we make LoadImage always do what LoadImageSkia is doing? If not, can we add another parameter to LoadImage?
This CL changes launcher behavior slightly. Currently, we display a default icon in the mean time that the icon is loading. This CL will attempt to paint the initially empty image which will then kick off the download. This change of behavior might be ok, but you should be aware of it. There will be some work as a result of this CL to attempt making the OnImageLoaded functions efficient and slim. Note that you are changing launcher behavior. This might be ok, but you should be aware that you are. Right now, when
http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:37: } const kSpecialComponentExtensionResources[] = { Nit: New line after end of struct definition http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:312: const bool can_load = CanLoadImage(load_info); Nit: Why don't you call OnImageLoaded(gfx::Image(), load_info.extension_id, id) here immediately if can_load is false and return? http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:347: bool ImageLoadingTracker::CanLoadImage(const PendingLoadInfo& load_info) { Can you rename this to CanLoadImageSkia as this is specific to loading to LoadImageSkia? http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:429: ExtensionResource resource = load_info->extension->GetIconResource( if load_info->image_skia has no reps and resource is not valid, you must fall back to 1x here. This may be the case for HiDPI mac when we never ask for the 1x scale factor and the 2x version is not valid http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:45: // To support DIP, LoadImageSkia should be used. It returns an ImageSkia that support DIP -> support scale factors other than 1x http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:46: // has ImageLoadingTracker::ImageSource. ImageSource would use this class to How about: Calling ImageSkia::GetRepresentation(ui::ScaleFactor) either explicitly or by gfx::Canvas::DrawImageInt will kick off the fetching the resource and call ImageLoadingTracker::OnImageLoaded when finished. ImageLoadingTracker must be kept around ... http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:125: void LoadImageSkia(const extensions::Extension* extension, I think that we will end up always having two methods. Here is the split that I envision: Uses of ImageLoadingTracker where we load an image which is displayed in the UI. Here it is ok to use an ImageSkiaSource Uses of ImageLoadingTracker where we want to write the result to file. app_shortcut_manager.cc for instance. Here ideally, we would pass in the desired scale factors. I really wouldn't want AppShortcutManager::OnImageLoaded to always be called twice, and a second time with the desired data.
Xiyuan, thank you for your patience. I think it might be time to ping Aaron or someone from his team for their opinion. They are much more aware of how ImageLoadingTracker is used and how much work will be involved in using ImageLoadingTracker::ImageSkia in extensions code than I am.
I wanted to ping that I am following this and am really sorry I've not commented yet. I will have comments within the next day.
ImageSkia/ImageSkiaSource is really cool. Can we separate concerns here and break this into two features: 1) Teach ImageLoadingTracker how to load multiple sizes at once. 2) Add a new extensions::IconSource that implements ImageSkiaSource for a given Extension or ExtensionIconSet and wraps an ImageLoadingTracker. I think this would make the code easier to understand and easier to use.
Nevermind, I misunderstood what ImageSkia/ImageSkiaSource are doing. I thought they would support an async impl of ImageSkiaSource. Can we make them do that? It would be super useful.
Aaron please ping me as to how you envision an async ImageSkiaSource working. The challenge in implementing an async ImageSkiaSource is figuring out who to notify once the async loading is finished. It is possible for the "finished" event to occur multiple times because the scale factor at which the ImageSkia is painted CAN (but very likely will not) change. I think I know how to create an async implementation specifically for images which are displayed in the UI. gfx::Canvas would keep track of ImageSkias with pending loads and call SchedulePaint on a delegate once loading is finished
Just realized that my suggestion in #30 will not work
On 2012/07/19 05:22:56, Aaron Boodman wrote: > Nevermind, I misunderstood what ImageSkia/ImageSkiaSource are doing. I thought > they would support an async impl of ImageSkiaSource. > > Can we make them do that? It would be super useful. The current thinking of supporting the async is to reuse the existing mechanism, at least for M22 and M23. ImageLoadingTracker::ImageSource serves this purpose. This adds a restriction that we have to keep ImageLoadingTracker around as long as the loaded ImageSkia is in use for potential scale factor change. It is not ideal. However, it seems would work well because most of the places ImageLoadingTracker is used, it is kept around.
Per yesterday's discussion, I will refactoring the CL to extract ImageSource into its own class and put the feature into an abstraction around ILT instead of in it. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:37: } const kSpecialComponentExtensionResources[] = { On 2012/07/19 01:37:45, pkotwicz wrote: > Nit: New line after end of struct definition Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:312: const bool can_load = CanLoadImage(load_info); On 2012/07/19 01:37:45, pkotwicz wrote: > Nit: Why don't you call OnImageLoaded(gfx::Image(), load_info.extension_id, id) > here immediately if can_load is false and return? Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:347: bool ImageLoadingTracker::CanLoadImage(const PendingLoadInfo& load_info) { On 2012/07/19 01:37:45, pkotwicz wrote: > Can you rename this to CanLoadImageSkia as this is specific to loading to > LoadImageSkia? Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:429: ExtensionResource resource = load_info->extension->GetIconResource( On 2012/07/19 01:37:45, pkotwicz wrote: > if load_info->image_skia has no reps and resource is not valid, you must fall > back to 1x here. > This may be the case for HiDPI mac when we never ask for the 1x scale factor and > the 2x version is not valid Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:45: // To support DIP, LoadImageSkia should be used. It returns an ImageSkia that On 2012/07/19 01:37:45, pkotwicz wrote: > support DIP -> support scale factors other than 1x Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:46: // has ImageLoadingTracker::ImageSource. ImageSource would use this class to On 2012/07/19 01:37:45, pkotwicz wrote: > How about: Calling ImageSkia::GetRepresentation(ui::ScaleFactor) either > explicitly or by gfx::Canvas::DrawImageInt will kick off the fetching the > resource and call ImageLoadingTracker::OnImageLoaded when finished. > ImageLoadingTracker must be kept around ... Done. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:48: // One needs to keep ImageLoadingTracker around as long as the returned On 2012/07/18 23:50:57, sky wrote: > This is a weird restriction. Seems like we should allow destroying the tracker > once the image is obtained. The tracker is kept around to allow additional bitmap resource to be loaded when scale factor changes. This is needed for a couple of reasons: 1. We want to only load minimum resource that is going to be used instead of loading all possible resources; 2. There is currently no mechanism for an ImageSkia to notify its host that it is changed and needs repaint. The current thinking is to reuse the existing async loading mechanism; If we could figure out some way to do #2, then this restriction could be removed. http://codereview.chromium.org/10701087/diff/27003/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:125: void LoadImageSkia(const extensions::Extension* extension, Peter says it.
CL refactored. Wrapped the ImageSkiaSource and load image for scale factor logic into an ExtensionIconImage class. UI code could host this class and be its delegate. The underlying logic is similar to before the refactoring. This class provide an ImageSkia for UI to paint with. When GetRepresentation of this ImageSkia is called (explicitly or implicitly by Canvas::DrawBitmapInt), its ImageSkiaSource would asks ExtensionIconImage to load more image and ExtensionIconImage would in turn fetches the resource via ILT asynchronously. Once ILT finishes loading, ExtensionIconImage gets the notification and calls its delegate.
http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:83: // Check whether extension has at least 1x resource. Nit: just put this code in the constructor? http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:37: ExtensionIconImage(const extensions::Extension* extension, It would be useful to make this slightly more abstract. Instead of accepting an extension and always getting the main icons out of the manifest, it could take an Extension* and ExtensionIconSet. That way this will be naturally reusable for things like crbug.com/138025, where the icons aren't the main manifest ones. http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:47: const gfx::ImageSkia& image_skia() const { return image_skia_; } Can ExtensionIconImage inherit ImageSkia rather than encapsulating it?
http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:83: // Check whether extension has at least 1x resource. On 2012/07/20 22:12:39, Aaron Boodman wrote: > Nit: just put this code in the constructor? Done. http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:37: ExtensionIconImage(const extensions::Extension* extension, On 2012/07/20 22:12:39, Aaron Boodman wrote: > It would be useful to make this slightly more abstract. Instead of accepting an > extension and always getting the main icons out of the manifest, it could take > an Extension* and ExtensionIconSet. > > That way this will be naturally reusable for things like crbug.com/138025, where > the icons aren't the main manifest ones. Done. http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:47: const gfx::ImageSkia& image_skia() const { return image_skia_; } On 2012/07/20 22:12:39, Aaron Boodman wrote: > Can ExtensionIconImage inherit ImageSkia rather than encapsulating it? ImageSkia is copyable and deriving from it seems not like a good idea. Many things could go wrong.
On 2012/07/20 22:43:16, xiyuan wrote: > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... > File chrome/browser/extensions/extension_icon_image.cc (right): > > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... > chrome/browser/extensions/extension_icon_image.cc:83: // Check whether extension > has at least 1x resource. > On 2012/07/20 22:12:39, Aaron Boodman wrote: > > Nit: just put this code in the constructor? > > Done. > > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... > File chrome/browser/extensions/extension_icon_image.h (right): > > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... > chrome/browser/extensions/extension_icon_image.h:37: ExtensionIconImage(const > extensions::Extension* extension, > On 2012/07/20 22:12:39, Aaron Boodman wrote: > > It would be useful to make this slightly more abstract. Instead of accepting > an > > extension and always getting the main icons out of the manifest, it could take > > an Extension* and ExtensionIconSet. > > > > That way this will be naturally reusable for things like crbug.com/138025, > where > > the icons aren't the main manifest ones. > > Done. > > http://codereview.chromium.org/10701087/diff/30012/chrome/browser/extensions/... > chrome/browser/extensions/extension_icon_image.h:47: const gfx::ImageSkia& > image_skia() const { return image_skia_; } > On 2012/07/20 22:12:39, Aaron Boodman wrote: > > Can ExtensionIconImage inherit ImageSkia rather than encapsulating it? > > ImageSkia is copyable and deriving from it seems not like a good idea. Many > things could go wrong. Ah, ok.
LGTM with nits
On 2012/07/24 15:06:59, pkotwicz wrote: > LGTM with nits pkotwicz: what nits (did you forget to publish comments)?
http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:157: gfx::ImageSkiaRep rep = image.ToImageSkia()->GetRepresentation(scale_factor); Nit: You can DCHECK on HasRepresentation http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:33: // to load it and call on its delegate interface when the resource is loaded. Can you comment about the expected delegate lifetime here too? It seems as if we are assuming that the delegate will outline ExtensionIconImage. http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:46: bool IsEmpty() const { return image_skia_.empty(); } Can you change the name here to IsNull() and check |image_skia_|.isNull(). I think it's slightly clearer. The only time IsNull()/IsEmpty() would be true is when no images can be loaded. (This does not imply that the images have actually been loaded) Can you add a comment to this effect.) http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image_delegate.h (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image_delegate.h:12: // Invoked when resource finishes loading and |image| is updated. Nit: I think "Invoked when |image| is updated with image reps for additional scale factors" is clearer. http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image_unittest.cc:118: EXPECT_EQ(1, image_loaded_count()); Nit: Check the size of image_reps instead http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:282: // See if the extension has the bitmap/image rep already. Nit: bitmap/image rep->bitmap http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.cc:353: // If all pending bitmaps/image reps are done then report back. Nit: bitmaps/image reps->bitmaps http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker.h (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:91: // When |resize_metho| is ALWAYS_RESIZE or when the loaded image is larger Nit:resize_method http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:92: // than |desired_size| it will be resized to those dimensions. Nit:those->these http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:92: // than |desired_size| it will be resized to those dimensions. Nit:those->these http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker.h:154: // it was the last bitmap in the list. The |original_size| should be the size Nit:it was the last bitmap in the list->if the caller will not request image reps for additional scale factors from the ImageLoadingTracker http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/image_loading_tracker_unittest.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/image_loading_tracker_unittest.cc:7: #include <algorithm> Nit: I don't think you need this anymore.
http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:76: const ExtensionIconSet& icon_set, Since currently the EIS that is passed is always extension->icons(), how about just passing Extension*. Future CLs that need this param can add it back. http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:84: resource_size_in_dip_(resource_size_in_dip), I don't understand why resource_size_in_dip is necessary. Why not just tell this class the desired size and let it pass this through to ILT? http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:93: const bool can_load = ImageLoadingTracker::CanLoadImage( I don't understand the value of this check. Just let the code work through the normal path and respond asynchronously with an error. Less paths == better. http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:126: // If resource does not exists, fallback to 1x if needed. I think that resource_match_type_ can handle this fallback for us naturally. There's no need for a separate fallback. http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10701087/diff/30014/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:5: #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ICON_IMAGE_H_ This should be in the extensions namespace.
Sorry for the confusion. I'll close this CL to avoid future confusions. Tony has taken over this change. His CL is here: http://codereview.chromium.org/10825012/ |