Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10861034:
Supply default icon to extension icon image. (Closed)
|
Created:
8 years, 4 months ago by tbarzic Modified:
8 years, 3 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupply default icon to extension icon image.
Icon image shuld return default representation until actual image is loaded
instead of the empty representation.
Also, handle the case when image loading tracker returns synchronously.
BUG=138025
TEST=IconImageTests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155330
Patch Set 1 #Patch Set 2 : .. #Patch Set 3 : nits #Patch Set 4 : tests #Patch Set 5 : .. #Patch Set 6 : . #
Total comments: 11
Patch Set 7 : some fixes #Patch Set 8 : unneeded includes #Patch Set 9 : test #
Total comments: 2
Patch Set 10 : . #Patch Set 11 : . #
Total comments: 12
Patch Set 12 : nits #
Total comments: 23
Patch Set 13 : . #
Total comments: 46
Patch Set 14 : .. #
Total comments: 6
Patch Set 15 : .. #Patch Set 16 : .. #Patch Set 17 : .. #
Messages
Total messages: 30 (0 generated)
I wonder do we still need to load failure notification if we have a default image. In launcher and app list, the load failure handle just wants to show a default image. And if most of the users of IconImage does a similar thing, maybe we could eliminate the load failure report?
On 2012/08/21 22:30:36, xiyuan wrote: > I wonder do we still need to load failure notification if we have a default > image. In launcher and app list, the load failure handle just wants to show a > default image. And if most of the users of IconImage does a similar thing, maybe > we could eliminate the load failure report? yeah, I was thinking about the same thing. The only place where I find it useful for now is the unit tests...
I'll take a look at the CL tomorrow
https://chromiumcodereview.appspot.com/10861034/diff/12004/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/12004/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:85: return gfx::ImageSkiaRep(representation.sk_bitmap(), scale_factor); pkotwicz: is it safe to assume that e.g. ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_EXTENSIONS_FAVICON) will have representations for all the scales we support? Do you think we should do resize operation on representation if representation/scale_factor doesn't match scale_factor, or just expect that default_icon has all the representations we need?
I am concerned this will create weird flicker since for a moment the icon won't be loaded and the UI will get the default icon. I don't think we should start returning the default icon until load has failed.
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:85: return gfx::ImageSkiaRep(representation.sk_bitmap(), scale_factor); If the default icon comes from the resource bundle, it should be safe to assume that it has all the scale factors we need. Note that it MAY not right now but that's something which should be fixed. http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:162: // representation. Nit: return representation if we have cached it a different scale factor. http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:167: // asynchronous. Nit: I think it might be a bit more understandable if you set the is_async flag for both cases here. http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:38: // make sure null ImageSkiaReps are handled well. An idea: Can we have a default 'default icon' which is completely transparent but non null? None of the ImageSkiaSources handle null ImageSkiaReps and I don't think that they each should have handling to do so (to make writing a new ImageSkiaSource simple).
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:38: // make sure null ImageSkiaReps are handled well. On 2012/08/22 23:17:55, pkotwicz wrote: > An idea: Can we have a default 'default icon' which is completely transparent > but non null? > None of the ImageSkiaSources handle null ImageSkiaReps and I don't think that > they each should have handling to do so (to make writing a new ImageSkiaSource > simple). Yeah, I had the same idea (just about to upload the new version).
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:162: // representation. On 2012/08/22 23:17:55, pkotwicz wrote: > Nit: return representation if we have cached it a different scale factor. are you sure, we could paint two different images in small time span (which is just what Aaron had against returning default icon reps). http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:167: // asynchronous. On 2012/08/22 23:17:55, pkotwicz wrote: > Nit: I think it might be a bit more understandable if you set the is_async flag > for both cases here. Done.
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:162: // representation. I should have been clearer. My nit was about changing the wording of the comment.
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:162: // representation. Ok, I see what you are doing. Sorry about that. Change the comment to "If LoadImages returned synchronously and the requested image rep is cached in the extension return the cached image rep."
http://codereview.chromium.org/10861034/diff/3007/chrome/browser/extensions/e... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/3007/chrome/browser/extensions/e... chrome/browser/extensions/extension_icon_image.cc:220: // the observers perspective, since we still haven't returned the initial think you mean: ...did not really change from the observers perspective, since the initial image representation is returned synchronously.
http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/12004/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:162: // representation. On 2012/08/23 14:09:45, pkotwicz wrote: > Ok, I see what you are doing. Sorry about that. Change the comment to "If > LoadImages returned synchronously and the requested image rep is cached in the > extension return the cached image rep." > Done. http://codereview.chromium.org/10861034/diff/3007/chrome/browser/extensions/e... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/3007/chrome/browser/extensions/e... chrome/browser/extensions/extension_icon_image.cc:220: // the observers perspective, since we still haven't returned the initial On 2012/08/23 17:25:57, xiyuan wrote: > think you mean: > > ...did not really change from the observers perspective, since the initial image > representation is returned synchronously. Done.
pkotwicz and aa, any updates?
Comment nits only! LGTM
http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:126: source_ = new Source(this, resource_size_in_dip); Nit 1: I find it confusing to have both desired_size_in_dip_ and resource_size_in_dip_. Can you remove desired_size_in_dip_. I think we only use it in two places in this file? Nit 2: Can you use a gfx::Size argument for Source() on line 126? http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:183: // If LaodImages returned synchronously and the requested image rep is cached Nit: LoadImages() http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:210: // we'll be certain that icons from resource bundle have all needed scale Nit: we'll be -> we are http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:37: // icon if icon's own representation for some scale factor cannot be loaded. Nit: if a representation for some scale factor cannot be loaded from the extension. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:39: // for all the scale factors we need. Nit: scale factors we need -> scale factors supported by the current platform (eg MacOS). http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image_unittest.cc:217: // Before it the image representation is loaded, image should contain default Nit: Before the image representation is loaded, image should contain blank image representation.
http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:126: source_ = new Source(this, resource_size_in_dip); On 2012/08/28 18:56:57, pkotwicz wrote: > Nit 1: I find it confusing to have both desired_size_in_dip_ and > resource_size_in_dip_. > Can you remove desired_size_in_dip_. I think we only use it in two places in > this file? > Nit 2: Can you use a gfx::Size argument for Source() on line 126? Done. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:183: // If LaodImages returned synchronously and the requested image rep is cached On 2012/08/28 18:56:57, pkotwicz wrote: > Nit: LoadImages() Done. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:210: // we'll be certain that icons from resource bundle have all needed scale On 2012/08/28 18:56:57, pkotwicz wrote: > Nit: we'll be -> we are Done. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:37: // icon if icon's own representation for some scale factor cannot be loaded. On 2012/08/28 18:56:57, pkotwicz wrote: > Nit: if a representation for some scale factor cannot be loaded from the > extension. Done. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:39: // for all the scale factors we need. On 2012/08/28 18:56:57, pkotwicz wrote: > Nit: scale factors we need -> scale factors supported by the current platform > (eg MacOS). Done. http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): http://codereview.chromium.org/10861034/diff/20003/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image_unittest.cc:217: // Before it the image representation is loaded, image should contain default On 2012/08/28 18:56:57, pkotwicz wrote: > Nit: Before the image representation is loaded, image should contain blank image > representation. Done.
I'm trying to make my way through this, but something seems seriously broken that this should be so complex.
I give up -> redirect("jyasskin")
On 2012/08/30 21:32:05, Aaron Boodman wrote: > I'm trying to make my way through this, but something seems seriously broken > that this should be so complex. the main reasons for complexity is that ImageLoadingTracker can cache images and call OnImageLoaded synchronously. Not handling this case can lead to subtle crashes: we call OnExtensionIconImageChanged before |Source| returns image representation. If the observer starts refreshing views right away, we would be be changing views objects while they are painted (mind that icon view is still waiting for a representation to paint) and end up in really messed up state. I figured that it's much safer to handle this case inside this class than rely on observers not screwing up. I guess the other option would be to force OnExtensionIconImageChanged to be async (i.e. always post task in |OnImageLoaded| on UI thread to notify the observer) Oh yeah, and initially returning transparent image.. that's caused by ImageSkia sources not handling empty ImageSkiaReps
I haven't looked through the test in detail yet. http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.cc (right): http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:45: bitmap.setConfig(SkBitmap::kARGB_8888_Config, Lots of boilerplate here. Would CanvasImageSource save enough of it to be worthwhile? It seems like the whole implementation could be just: virtual void Draw(gfx::Canvas* canvas) { canvas->DrawColor(SkColorSetARGB(0, 0, 0, 0)); } http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:88: blank_image_(new BlankImageSource(size_in_dip), size_in_dip) { Why put blank here but default on the IconImage? http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:101: if (host_) How could host_ be null? Oooh: after the IconImage has died. That's subtle and needs a comment, probably on the definition of 'host_'. http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:135: // |source_| could be NULL if resource does not exist. Could it? I see source_ initialized unconditionally in the constructor and never nulled out. http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.cc:211: // we are certain that icons from resource bundle have all needed scale Aren't these icons coming from extensions, where we'll never be sure they include all needed scale factors? http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image.h (right): http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:35: // the UI code will be provided with blank, transparent image representation. Do we want to return a blank image or the default image for not-yet-loaded icons? http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:36: // UI code can supply default icon whose representations will be added to the By "can supply", you mean |default_icon| can be empty? http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:36: // UI code can supply default icon whose representations will be added to the I think this comment could use a more detailed description of exactly how representations are picked for IconImages given a set of images supplied by an extension, including during the delay while an image is loading. http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:39: // If default icon is supplied, it is assumed that it contains representations By "contains representations", do you mean that, for all platform |scale_factor|s, HasRepresentation(scale_factor) returns true, or GetRepresentation(scale_factor) returns an ImageSkiaRep with that scale factor? http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image.h:73: // Loads bitmap for additional scale factor. Please describe the meaning of the return value and how the asynchronous request is handled. http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): http://codereview.chromium.org/10861034/diff/32001/chrome/browser/extensions/... chrome/browser/extensions/extension_icon_image_unittest.cc:26: bool ImageRepsAreEqual(const gfx::ImageSkiaRep& image_rep1, Heh. We should put these helpers in a common place sooner or later.
https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:45: bitmap.setConfig(SkBitmap::kARGB_8888_Config, On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Lots of boilerplate here. Would CanvasImageSource save enough of it to be > worthwhile? It seems like the whole implementation could be just: > > virtual void Draw(gfx::Canvas* canvas) { > canvas->DrawColor(SkColorSetARGB(0, 0, 0, 0)); > } Done. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:88: blank_image_(new BlankImageSource(size_in_dip), size_in_dip) { On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Why put blank here but default on the IconImage? I'd rather keep the default icon as an implementation detail of ExtensionIconImage. And blank so we can return something besides empty image skia until the image is loaded https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:101: if (host_) On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > How could host_ be null? Oooh: after the IconImage has died. That's subtle and > needs a comment, probably on the definition of 'host_'. Done. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:135: // |source_| could be NULL if resource does not exist. On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Could it? I see source_ initialized unconditionally in the constructor and never > nulled out. yeah, that's a leftover from the initial representation where source wasn't unconditionally initialized.. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:211: // we are certain that icons from resource bundle have all needed scale On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Aren't these icons coming from extensions, where we'll never be sure they > include all needed scale factors? yeah, but image loading tracker is supposed to return image that has the representation for the required scale factor. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:35: // the UI code will be provided with blank, transparent image representation. On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Do we want to return a blank image or the default image for not-yet-loaded > icons? blank image.. Aaron had some concerns that returning default image here and than soon after the loaded image could cause some weird flickering.. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:36: // UI code can supply default icon whose representations will be added to the On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > By "can supply", you mean |default_icon| can be empty? yeah, and in that case, if image rep is not loaded successfully nothing will be painted. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:36: // UI code can supply default icon whose representations will be added to the On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > I think this comment could use a more detailed description of exactly how > representations are picked for IconImages given a set of images supplied by an > extension, including during the delay while an image is loading. ok, I've rewritten larger part of the comment, and also added a more detailed implementation description in .cc https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:39: // If default icon is supplied, it is assumed that it contains representations On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > By "contains representations", do you mean that, for all platform > |scale_factor|s, HasRepresentation(scale_factor) returns true, or > GetRepresentation(scale_factor) returns an ImageSkiaRep with that scale factor? the latter, clarified the comment.. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:73: // Loads bitmap for additional scale factor. On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Please describe the meaning of the return value and how the asynchronous request > is handled. Done. https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:26: bool ImageRepsAreEqual(const gfx::ImageSkiaRep& image_rep1, On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > Heh. We should put these helpers in a common place sooner or later. yeah, the same thought cross my mind while writing this.. I'm planning to do this once I land all pending patches for browser action icons :)
https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/32001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:88: blank_image_(new BlankImageSource(size_in_dip), size_in_dip) { On 2012/08/31 22:29:44, tbarzic wrote: > On 2012/08/31 18:43:14, Jeffrey Yasskin wrote: > > Why put blank here but default on the IconImage? > > I'd rather keep the default icon as an implementation detail of > ExtensionIconImage. The whole file is an implementation detail of IconImage, so that's not a great answer. Better is that one of the images _has_ to be here since the ImageSkia can outlive the IconImage, and they can't _both_ be here since only the IconImage knows whether an image is still loading or failed loading. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:19: * The file contains extensions::IconImage implementation, which provides Shorter comments are better, all else equal, to prevent TL;DR syndrome. I think you can replace the first three sentences with: "The ImageSkia provided by extensions::IconImage contains ImageSkiaReps that are computed and updated using the following algorithm:" Try to keep your other comments as concise as possible too. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:22: * ImageSkiaSource. When a new image representation is requesetd from the image sp: requesetd https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:24: * - If the extension doesn't have a icon resource needed for the image There's some complex behavior around which sizes are allowed to satisfy a given scale request. You should refer to LoadImageForScaleFactor() for the definition of that load. Perhaps: "- LoadImageForScaleFactor() searches the extension for an icon of an appropriate size. "- If the extension doesn't have ..." https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:26: * factor is returned by ImageSkiaSource. If the default icon is empty, You wind up saying the empty-default fallback 3 times in this description. Instead, say something like, "If the default icon is empty, a transparent image is used as the default." in the comment header and just refer to the default image thereafter. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:30: * - |ImageLoaderTracker| may return both syncronously and asyncronously. sp: syncronously 2x https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:33: * - If image representation resurce is successfully loaded, the sp: resurce https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:42: * - The image will be manually updated when the |ImageLoadingTracker| Nothing's manual in code. I think you mean that we use AddRepresentation to set the resulting image into the ImageSkia, and tell the Observer to repaint by calling OnExtensionIconImageChanged. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:49: * is updated by the corresponding representatin from the default icon and sp: representatin https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:73: : CanvasImageSource(size_in_dip, false) { I prefer writing something like /*is_opaque=*/false so that future readers don't have to look up CanvasImageSource to figure out what the parameter means. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:105: // IconImage object that is hosting this image source. It is used to load Shorter: "Used to load images, possibly asynchronously. NULLed out when the IconImage is destroyed." https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:167: IconImage::~IconImage() { This still needs to call ResetHost(), right? If this passed your testing, you're missing a test. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:238: DCHECK(image_in.IsEmpty() || image->HasRepresentation(scale_factor)); Your comment says GetRepresentation should work, not that HasRepresentation should return true, so please write a test for the case where the default image computes its representations. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:28: /** I haven't noticed this style comment elsewhere in chrome. Is it actually common? If not, stick with // style. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:37: * If the requesetd resource doesn't exist or can't be loaded and a default icon sp: requesetd https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:80: // If the representation gets loaded syncronously, it is returned by this sp: syncronously https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:28: return gfx::BitmapsAreEqual(image_rep1.sk_bitmap(), image_rep2.sk_bitmap()); Sorry for missing this. This isn't actually a correct implementation of equality for ImageReps, since that would have to include the scale factors. You should just compare the bitmaps directly in the test. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:63: gfx::ImageSkia LoadImage(const std::string& path, int size, bool cache) { Just use ImageLoadingTracker::CacheParam instead of replacing it with harder-to-read booleans. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:257: // If we can't load icon with the exact size, but a bigger resource is Hasn't this already been tested in the SCALE_FACTOR_200P part of the previous test? https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:311: // The image should not be resized. "should not be" makes me nervous in a test. Either you should trust the underlying implementations and say "is not" or you should EXPECT to validate the assumptions. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:359: test_image_loader.LoadImage("16.png", Please extract all of these image loads into a helper class that you instantiate in each test case, so you don't have to reproduce the loading code over and over. Be sure to give them short descriptive field names like "icon_16" or "icon_48_resized_to_32" https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:385: // If resource set is empty, failure should be reported. "failure should be reported" is no longer true. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:417: // If resource set is empty, failure should be reported. Please update comments when you copy them. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:443: // We should still have default icon representation. What do you mean "still"? This has changed from the original representation, which was blank.
https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:19: * The file contains extensions::IconImage implementation, which provides On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Shorter comments are better, all else equal, to prevent TL;DR syndrome. I think > you can replace the first three sentences with: "The ImageSkia provided by > extensions::IconImage contains ImageSkiaReps that are computed and updated using > the following algorithm:" Try to keep your other comments as concise as possible > too. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:22: * ImageSkiaSource. When a new image representation is requesetd from the image On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: requesetd Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:24: * - If the extension doesn't have a icon resource needed for the image On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > There's some complex behavior around which sizes are allowed to satisfy a given > scale request. You should refer to LoadImageForScaleFactor() for the definition > of that load. Perhaps: > > "- LoadImageForScaleFactor() searches the extension for an icon of an > appropriate size. > "- If the extension doesn't have ..." Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:26: * factor is returned by ImageSkiaSource. If the default icon is empty, On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > You wind up saying the empty-default fallback 3 times in this description. > Instead, say something like, "If the default icon is empty, a transparent image > is used as the default." in the comment header and just refer to the default > image thereafter. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:30: * - |ImageLoaderTracker| may return both syncronously and asyncronously. On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: syncronously 2x Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:33: * - If image representation resurce is successfully loaded, the On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: resurce Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:42: * - The image will be manually updated when the |ImageLoadingTracker| On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Nothing's manual in code. I think you mean that we use AddRepresentation to set > the resulting image into the ImageSkia, and tell the Observer to repaint by > calling OnExtensionIconImageChanged. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:49: * is updated by the corresponding representatin from the default icon and On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: representatin Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:73: : CanvasImageSource(size_in_dip, false) { On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > I prefer writing something like /*is_opaque=*/false so that future readers don't > have to look up CanvasImageSource to figure out what the parameter means. yeah, I've started doing this, but I still occasionally miss some places https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:105: // IconImage object that is hosting this image source. It is used to load On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Shorter: "Used to load images, possibly asynchronously. NULLed out when the > IconImage is destroyed." Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:167: IconImage::~IconImage() { On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > This still needs to call ResetHost(), right? If this passed your testing, you're > missing a test. jeez, I'm sometimes amazed with things I do on Friday afternoons... https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:238: DCHECK(image_in.IsEmpty() || image->HasRepresentation(scale_factor)); On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Your comment says GetRepresentation should work, not that HasRepresentation > should return true, so please write a test for the case where the default image > computes its representations. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:28: /** On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > I haven't noticed this style comment elsewhere in chrome. Is it actually common? > If not, stick with // style. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:37: * If the requesetd resource doesn't exist or can't be loaded and a default icon On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: requesetd Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:80: // If the representation gets loaded syncronously, it is returned by this On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > sp: syncronously Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:28: return gfx::BitmapsAreEqual(image_rep1.sk_bitmap(), image_rep2.sk_bitmap()); On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Sorry for missing this. This isn't actually a correct implementation of equality > for ImageReps, since that would have to include the scale factors. You should > just compare the bitmaps directly in the test. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:63: gfx::ImageSkia LoadImage(const std::string& path, int size, bool cache) { On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Just use ImageLoadingTracker::CacheParam instead of replacing it with > harder-to-read booleans. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:257: // If we can't load icon with the exact size, but a bigger resource is On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Hasn't this already been tested in the SCALE_FACTOR_200P part of the previous > test? Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:311: // The image should not be resized. On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > "should not be" makes me nervous in a test. Either you should trust the > underlying implementations and say "is not" or you should EXPECT to validate the > assumptions. yeah, that comment is a bit misplaced.. removed it https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:359: test_image_loader.LoadImage("16.png", On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Please extract all of these image loads into a helper class that you instantiate > in each test case, so you don't have to reproduce the loading code over and > over. Be sure to give them short descriptive field names like "icon_16" or > "icon_48_resized_to_32" Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:385: // If resource set is empty, failure should be reported. On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > "failure should be reported" is no longer true. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:417: // If resource set is empty, failure should be reported. On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > Please update comments when you copy them. Done. https://chromiumcodereview.appspot.com/10861034/diff/38001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:443: // We should still have default icon representation. On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > What do you mean "still"? This has changed from the original representation, > which was blank. Done.
Only small things left. LGTM when they're done. I've been thinking about these icon issues more generally. I wonder if it would make sense to have a general ChangingImageSkia class which would allow observation and general correctness if the ImageSkiaSource returns different images over time (like AnimatedIconImageSource in your ExtensionAction patch). No need to modify this CL in response, but it would be nice to get a general solution instead of a series of 1-off workarounds. https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:230: DCHECK_EQ(scale_factor, rep.scale_factor()); On 2012/09/04 21:07:09, tbarzic wrote: > On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > > Your comment says GetRepresentation should work, not that HasRepresentation > > should return true, so please write a test for the case where the default > image > > computes its representations. > > Done. Great, thanks. I think you're still missing a test for a dynamically-generated default image though. https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:36: // If the requeseted resource doesn't exist or can't be loaded and a default "requeseted" is still misspelled, along with some other words I didn't explicitly call out last time. Could you run your comments through a spell checker? https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:369: // We should have default icon representation now. s/should have/should have/
https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.cc:230: DCHECK_EQ(scale_factor, rep.scale_factor()); On 2012/09/05 23:14:49, Jeffrey Yasskin wrote: > On 2012/09/04 21:07:09, tbarzic wrote: > > On 2012/09/01 01:10:27, Jeffrey Yasskin wrote: > > > Your comment says GetRepresentation should work, not that HasRepresentation > > > should return true, so please write a test for the case where the default > > image > > > computes its representations. > > > > Done. > > Great, thanks. I think you're still missing a test for a dynamically-generated > default image though. sorry, forgot to upload patch with the test.. https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image.h (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image.h:36: // If the requeseted resource doesn't exist or can't be loaded and a default On 2012/09/05 23:14:49, Jeffrey Yasskin wrote: > "requeseted" is still misspelled, along with some other words I didn't > explicitly call out last time. Could you run your comments through a spell > checker? yep, I usually go through the comments once more before committing.. https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://chromiumcodereview.appspot.com/10861034/diff/42001/chrome/browser/ext... chrome/browser/extensions/extension_icon_image_unittest.cc:369: // We should have default icon representation now. On 2012/09/05 23:14:49, Jeffrey Yasskin wrote: > s/should have/should have/ Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10861034/38004
Change committed as 155330 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
