chromeos: Fix pixelated icons in app list and launcher (part 2) (by xiyuan)

taking over xiyuan's patch: http://codereview.chromium.org/10701087/

***************************************************************
chromeos: 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.

***************************************************************
For gypi files (I reckon this part is trivial enough)
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152407

[tbarzic, 2012-07-25 19:37:33]

Since xiyuan is ooo until the end of next week, I'll take over his patch (I'll
need this for my work).

Patch set 1 here is equal to Pach set 11 in
http://codereview.chromium.org/10701087/  (I've added Peter's comments from
there)

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.cc:157: gfx::ImageSkiaRep rep =
image.ToImageSkia()->GetRepresentation(scale_factor);
pkotwicz 2012/07/25 02:00:39
Nit: You can DCHECK on HasRepresentation

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.h:33: // to load it and call on
its delegate interface when the resource is loaded.
pkotwicz 2012/07/25 02:00:39
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/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.h:46: bool IsEmpty() const {
return image_skia_.empty(); }
pkotwicz 2012/07/25 02:00:39
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/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_delegate.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_delegate.h:12: // Invoked when
resource finishes loading and |image| is updated.
pkotwicz 2012/07/25 02:00:39
Nit: I think "Invoked when |image| is updated with image reps for additional
scale factors" is clearer.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:118: EXPECT_EQ(1,
image_loaded_count());
pkotwicz 2012/07/25 02:00:39
Nit: Check the size of image_reps instead

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.cc:282: // See if the extension
has the bitmap/image rep already.
pkotwicz 2012/07/25 02:00:39
Nit: bitmap/image rep->bitmap

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.cc:353: // If all pending
bitmaps/image reps are done then report back.
pkotwicz 2012/07/25 02:00:39
Nit: bitmaps/image reps->bitmaps

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:91: // When |resize_metho| is
ALWAYS_RESIZE or when the loaded image is larger
pkotwicz 2012/07/25 02:00:39
Nit:resize_method

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:92: // than |desired_size| it
will be resized to those dimensions.
pkotwicz 2012/07/25 02:00:39
Nit:those->these

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:154: // it was the last bitmap
in the list. The |original_size| should be the size
pkotwicz 2012/07/25 02:00:39
Nit:it was the last bitmap in the list->if the caller will not request image
reps for additional scale factors from the ImageLoadingTracke

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker_unittest.cc:7: #include
<algorithm>
pkotwicz 2012/07/25 02:00:39
Nit: I don't think you need this anymore.

[tbarzic, 2012-07-25 19:53:53]

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.cc:157: gfx::ImageSkiaRep rep =
image.ToImageSkia()->GetRepresentation(scale_factor);
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: You can DCHECK on HasRepresentation

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.h:33: // to load it and call on
its delegate interface when the resource is loaded.
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Can you comment about the expected delegate lifetime here too?
> It seems as if we are assuming that the delegate will outline
> ExtensionIconImage

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image.h:46: bool IsEmpty() const {
return image_skia_.empty(); }
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> 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.)

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_delegate.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_delegate.h:12: // Invoked when
resource finishes loading and |image| is updated.
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: I think "Invoked when |image| is updated with image reps for additional
> scale factors" is clearer.

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:118: EXPECT_EQ(1,
image_loaded_count());
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: Check the size of image_reps instead

the problem with image_reps is there are actually two representations added for
each new GetRepresentation (first, one null rep, and later, when the image rep
is loaded the real one); I feel like the test should not depend on that
implementation detail.

plus, there is a TODO by oshima in image_skia to make image_reps fail when
source is used (like in this case)

If you insist, I'll check image_reps size, but I'd rather keep it this way :)

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.cc:282: // See if the extension
has the bitmap/image rep already.
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: bitmap/image rep->bitmap

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.cc:353: // If all pending
bitmaps/image reps are done then report back.
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: bitmaps/image reps->bitmaps

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:91: // When |resize_metho| is
ALWAYS_RESIZE or when the loaded image is larger
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit:resize_method

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:92: // than |desired_size| it
will be resized to those dimensions.
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit:those->these

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker.h:154: // it was the last bitmap
in the list. The |original_size| should be the size
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit:it was the last bitmap in the list->if the caller will not request image
> reps for additional scale factors from the ImageLoadingTracke

Done.

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
File chrome/browser/extensions/image_loading_tracker_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/1/chrome/browser/extensions/imag...
chrome/browser/extensions/image_loading_tracker_unittest.cc:7: #include
<algorithm>
On 2012/07/25 19:37:34, tbarzic wrote:
> pkotwicz 2012/07/25 02:00:39
> Nit: I don't think you need this anymore.

Done.

[pkotwicz, 2012-07-25 19:56:19]

Just to confirm: "Wait for the last CL to update laucher/app list code to use
LoadImageInDIP to verify" from the CL description doesn't apply anymore?

LGTM just nits since http://codereview.chromium.org/10701087/

[pkotwicz, 2012-07-25 19:56:52]

*assuming just nits

[tbarzic, 2012-07-25 20:18:30]

On 2012/07/25 19:56:19, pkotwicz wrote:
> Just to confirm: "Wait for the last CL to update laucher/app list code to use
> LoadImageInDIP to verify" from the CL description doesn't apply anymore?
> 

I think it still does.. we'll have to make launcher/app list code use extension
icon image to actually see the changes..

> LGTM just nits since http://codereview.chromium.org/10701087/

[tbarzic, 2012-07-30 18:05:35]

On 2012/07/25 20:18:30, tbarzic wrote:
> On 2012/07/25 19:56:19, pkotwicz wrote:
> > Just to confirm: "Wait for the last CL to update laucher/app list code to
use
> > LoadImageInDIP to verify" from the CL description doesn't apply anymore?
> > 
> 
> I think it still does.. we'll have to make launcher/app list code use
extension
> icon image to actually see the changes..
> 
> > LGTM just nits since http://codereview.chromium.org/10701087/

Aaron: ping?

[pkotwicz, 2012-07-30 21:09:56]

For the sake of moving forward, I am ok with this patch. However, I have
outlined in
https://docs.google.com/a/google.com/document/d/14nOTZn4rJvHiOLN40qEp_kU7wqX_...
some uses of ImageLoadingTracker for which additional work is needed so that
they work right. Please let me know if you can't access the doc

[Aaron Boodman, 2012-08-02 17:50:56]

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.h (right):

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:26: class
ExtensionIconImageDelegate;
Can the delegate be defined as a nested class instead of separately?

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:35: class ExtensionIconImage :
public ImageLoadingTracker::Observer,
Please put this in the extensions namespace and rename to just IconImage.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:47: // True if no images can be
loaded. This does not imply any images have
The second sentence of this comment makes no sense.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:39: {
extension_misc::kWebStoreAppId, IDR_PRODUCT_LOGO_128 },
Why are there two that have the same id?

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:149: if
(image_info.resize_method == ImageRepInfo::ALWAYS_RESIZE ||
If it is the exact same size, you shouldn't resample, right?

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:174: SkBitmap* bitmap =
ExtensionIconSource::LoadImageByResourceId(
It's weird layering to call ExtensionIconSource here. Can you just call
ResourceBundle::GetInstance().GetImageNamed() instead?

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:230: bool
ImageLoadingTracker::CanLoadImage(const extensions::Extension* extension,
This method is weird. First of all, it doesn't even answer the question of
whether the image can be loaded.

Secondly, it doesn't seem like we should even have it. Why does the calling code
need to ask this question? It seems like it should be ignorant of whether the
resource it is asking about is legit, and just handle the error in the callback.
That way all error handling is in the same place.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:341:
image_info.scale_factor));
Incorrect indentation. First character of each line of params should left-align.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:70: // Information about a
single bitmap/image rep to load from an extension
When you're dealing with ImageRepInfo (or ImageRepresentation as I suggest
below), just say 'image representation' in the comments. The fact that we are
translating bitmaps into ImageRepresentations is an implementation detail
elsewhere that we don't need to spread around.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:72: struct ImageRepInfo {
per styleguide [1]
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...,
we are supposed to eschew abbreviation. How about just ImageRepresentation?

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:132: // bitmaps/image reps.
Same thing here with 'bitmap/', and throughout.

http://codereview.chromium.org/10825012/diff/8004/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:151: // When a bitmap has
finished loading and been resized on the file thread, it
This comment contains way too much implementation detail. It can pretty much
just say 'Called on the UI thread when a bitmap has finished loading.'

[tbarzic, 2012-08-02 20:12:52]

PTAL

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
File chrome/browser/extensions/extension_icon_image.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.h:26: class
ExtensionIconImageDelegate;
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> Can the delegate be defined as a nested class instead of separately?

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.h:35: class ExtensionIconImage :
public ImageLoadingTracker::Observer,
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> Please put this in the extensions namespace and rename to just IconImage.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.h:47: // True if no images can be
loaded. This does not imply any images have
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> The second sentence of this comment makes no sense.

Yeah, it made (slightly) more sense when the method was called IsEmpty.
removed..

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:39: {
extension_misc::kWebStoreAppId, IDR_PRODUCT_LOGO_128 },
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> Why are there two that have the same id?

looking at the old code, it looks like the second one should be kChromeAppId

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:149: if
(image_info.resize_method == ImageRepInfo::ALWAYS_RESIZE ||
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> If it is the exact same size, you shouldn't resample, right?

Yep, also it may be safer not to rely on ResizeMethod enum having only two
values.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:174: SkBitmap* bitmap =
ExtensionIconSource::LoadImageByResourceId(
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> It's weird layering to call ExtensionIconSource here. Can you just call
> ResourceBundle::GetInstance().GetImageNamed() instead?

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:230: bool
ImageLoadingTracker::CanLoadImage(const extensions::Extension* extension,
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> This method is weird. First of all, it doesn't even answer the question of
> whether the image can be loaded.
> 
> Secondly, it doesn't seem like we should even have it. Why does the calling
code
> need to ask this question? It seems like it should be ignorant of whether the
> resource it is asking about is legit, and just handle the error in the
callback.
> That way all error handling is in the same place.

makes sense.. the version fallbacks to the SCALE_FACTOR_100P
if we fail to load image, not only if the resource for the other scale is not
legit (given this is not the only thing that can go wrong);

Also, I'll remove ExtensionIconImage::isNull() since it can be true only when we
detect desired resource is invalid (using this method) in ExtensionIconImage
ctor(I didn't really find it useful in the first place; and I'm not too sure it
is correct: if there is no resource for 1x, that does not mean there won't be
one for 2x)

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:341:
image_info.scale_factor));
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> Incorrect indentation. First character of each line of params should
left-align.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:70: // Information about a
single bitmap/image rep to load from an extension
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> When you're dealing with ImageRepInfo (or ImageRepresentation as I suggest
> below), just say 'image representation' in the comments. The fact that we are
> translating bitmaps into ImageRepresentations is an implementation detail
> elsewhere that we don't need to spread around.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:72: struct ImageRepInfo {
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> per styleguide [1]
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...,
> we are supposed to eschew abbreviation. How about just ImageRepresentation?

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:132: // bitmaps/image reps.
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> Same thing here with 'bitmap/', and throughout.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/8004/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:151: // When a bitmap has
finished loading and been resized on the file thread, it
On 2012/08/02 17:50:56, Aaron Boodman wrote:
> This comment contains way too much implementation detail. It can pretty much
> just say 'Called on the UI thread when a bitmap has finished loading.'

agree

[pkotwicz, 2012-08-03 17:57:41]

Can you please try this with a use case of ImageLoaadingTracker before
committing this?
My completely biased interests are with:
chrome/browser/ui/views/ash/app_list/search_builder.cc and
chrome/browser/ui/views/ash/launcher/launcher_app_icon_loader.cc

[pkotwicz, 2012-08-03 17:58:38]

https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/extension_icon_image.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/ext...
chrome/browser/extensions/extension_icon_image.h:57: const gfx::ImageSkia&
image_skia() const { return image_skia_; }
I am a bit concerned of what will happen if this function is called before
ExtensionIconImage::OnImageLoaded.

[tbarzic, 2012-08-03 18:44:28]

https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/extension_icon_image.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/13002/chrome/browser/ext...
chrome/browser/extensions/extension_icon_image.h:57: const gfx::ImageSkia&
image_skia() const { return image_skia_; }
On 2012/08/03 17:58:38, pkotwicz wrote:
> I am a bit concerned of what will happen if this function is called before
> ExtensionIconImage::OnImageLoaded.

afaik, this always gets called before OnImageLoaded.
ImageLoadingTracker will be initialized when the image has to be painted (and
for that to happen ui code has to get the image somehow)

Right now, an empty image is initially painted and we have to refresh ui when
the image is actually loaded (i.e. ui code has to listen to
OnExtensionIconImageChanged and update itself).

[tbarzic, 2012-08-06 18:37:43]

fyi, I tries this with
chrome/browser/ui/views/ash/app_list/search_builder.cc and
chrome/browser/ui/views/ash/launcher/launcher_app_icon_loader.cc

and it seems to work (but we're still missing setting default icons)

https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/ext...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/ext...
chrome/browser/extensions/image_loading_tracker.cc:192:
ResourceBundle::GetSharedInstance().GetBitmapNamed(resource_id);
This is causing a crash, so I'll back off on this change (for now)

[tbarzic, 2012-08-06 21:37:11]

https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/ext...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/11015/chrome/browser/ext...
chrome/browser/extensions/image_loading_tracker.cc:192:
ResourceBundle::GetSharedInstance().GetBitmapNamed(resource_id);
On 2012/08/06 18:37:43, tbarzic wrote:
> This is causing a crash, so I'll back off on this change (for now)

nevermind.. just had to remove delete in ReportOnUIThread

[oshima, 2012-08-07 07:43:05]

looks good. just a several minor questions/suggestions.

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.cc:143: }
you may want to add log when the scale is 100P but the image is empty.

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.h (right):

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:38: class Delegate {
This should probably called Observer instead of Delegate. Delegate is used when
you need to defer certain operations to others to complete your task.
I'd leave it to OWNERS tho.

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.h:41: // factors.
How about
// Invoked when a new image reps for additional scale factors
// is loaded and added to |image|.

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:30: }
virtual dtor

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:38: int
image_loaded_count() {
GetImageLoadedCount() as this is not simple accessor.

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:53:
.AppendASCII(name);
doesn't this fit single line?

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:89:
content::TestBrowserThread ui_thread_;
I'm not familiar with TestBrowserThread and this may be just fine, but I wonder
if/where ui_therad_ should be started (or even necessary)

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:103:
scoped_ptr<IconImage> image(new IconImage(
You may create an object on stack. It's up to you tho.

[xiyuan, 2012-08-07 17:32:40]

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

http://codereview.chromium.org/10825012/diff/9015/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image_unittest.cc:89:
content::TestBrowserThread ui_thread_;
On 2012/08/07 07:43:05, oshima wrote:
> I'm not familiar with TestBrowserThread and this may be just fine, but I
wonder
> if/where ui_therad_ should be started (or even necessary)

We have |ui_thread_| so that ImageLoadingTracker's ReportBack/ReportOnUIThread
works. It does not need to be started since the test code assumes the main
thread is the UI thread (where we run |ui_loop_|). 

We run |ui_loop_| when we expect ILT to load a resource asynchronously as in
WaitForImageLoad().

[tbarzic, 2012-08-07 18:10:26]

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
File chrome/browser/extensions/extension_icon_image.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.cc:143: }
On 2012/08/07 07:43:05, oshima wrote:
> you may want to add log when the scale is 100P but the image is empty.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
File chrome/browser/extensions/extension_icon_image.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.h:38: class Delegate {
On 2012/08/07 07:43:05, oshima wrote:
> This should probably called Observer instead of Delegate. Delegate is used
when
> you need to defer certain operations to others to complete your task.
> I'd leave it to OWNERS tho.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.h:41: // factors.
On 2012/08/07 07:43:05, oshima wrote:
> How about
> // Invoked when a new image reps for additional scale factors
> // is loaded and added to |image|.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
File chrome/browser/extensions/extension_icon_image_unittest.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:30: }
On 2012/08/07 07:43:05, oshima wrote:
> virtual dtor

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:38: int
image_loaded_count() {
On 2012/08/07 07:43:05, oshima wrote:
> GetImageLoadedCount() as this is not simple accessor.

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:53:
.AppendASCII(name);
On 2012/08/07 07:43:05, oshima wrote:
> doesn't this fit single line?

Done.

https://chromiumcodereview.appspot.com/10825012/diff/9015/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image_unittest.cc:103:
scoped_ptr<IconImage> image(new IconImage(
On 2012/08/07 07:43:05, oshima wrote:
> You may create an object on stack. It's up to you tho.

Done.

[oshima, 2012-08-08 03:27:43]

lgtm with nits. you need aa's lgtm though.

http://codereview.chromium.org/10825012/diff/7018/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/10825012/diff/7018/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:175: true /* delete
bitmap*/);
bitmap */

http://codereview.chromium.org/10825012/diff/7018/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.cc:209: void
ReportOnUIThread(const SkBitmap* bitmap,
This may be call on non UI Thread? If so, can you improve the name? If it is
indeed called only on UI Thread, then

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)

is better

[tbarzic, 2012-08-08 04:55:45]

https://chromiumcodereview.appspot.com/10825012/diff/7018/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/7018/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:175: true /* delete
bitmap*/);
On 2012/08/08 03:27:43, oshima wrote:
> bitmap */

Done.

https://chromiumcodereview.appspot.com/10825012/diff/7018/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.cc:209: void
ReportOnUIThread(const SkBitmap* bitmap,
On 2012/08/08 03:27:43, oshima wrote:
> This may be call on non UI Thread? If so, can you improve the name? If it is
> indeed called only on UI Thread, then
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)
> 
> is better

Good catch.. I'm not sure, but judging by ctor, it may be called on any thread
different from FILE thread.. will change both name and DCHECK

[tbarzic, 2012-08-08 17:44:55]

Aaron: ping..

[Aaron Boodman, 2012-08-10 21:42:07]

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/e...
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/10825012/diff/4019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.cc:139: if (image.IsEmpty()) {
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/10825012/diff/4019/chrome/browser/extensions/i...
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:75: enum ResizeMethod {
Nit: Better name might be ResizeCondition?

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/i...
chrome/browser/extensions/image_loading_tracker.h:108: // |LoadImageSkia|
instead.
There's no LoadImageSkia() in this class.

[xiyuan, 2012-08-10 22:03:43]

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/4019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_icon_image.cc:139: if (image.IsEmpty()) {
On 2012/08/10 21:42:08, Aaron Boodman wrote:
> I think that resource_match_type_ can handle this fallback for us naturally.
> There's no need for a separate fallback.

If |resource_match_type_| could handle the fallback, ILT would load something
and image.IsEmpty() would not be true here. However, that is not always the
case. 

e.g. suppose extension has only 16x16 and 24x24 icons and we asks for 16x16
(DIP) with MATCH_BIGGER. For 1x, we get 16x16 correctly. Then for 2x, ILT would
fail because we would ask for 32x32 (pixel) with MATCH_BIGGER and that does not
exist.

If we want to use |resource_match_type_| for the fallback, I imagine it might be
something like trying other match types when current one does not give us
anything. i.e. if MATCH_BIGGER does not give us anything, try MATCH_SMALLER
here. 

Think we need a fallback way here. But not sure which way is better though.

[tbarzic, 2012-08-10 23:01:23]

https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/exte...
File chrome/browser/extensions/extension_icon_image.cc (right):

https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/exte...
chrome/browser/extensions/extension_icon_image.cc:84:
resource_size_in_dip_(resource_size_in_dip),
On 2012/08/10 21:42:08, Aaron Boodman wrote:
> 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?

semantically, those have different meanings:
resource_size_in_dip is the size of the extension resource we are loading,
while desired_size_in_dip is the final size of the loaded image.

If you think we can assume these values should always be equal, I'd be happy to
get rid of one of those two :)
(I'm not to confident about this claim: it could easily happen that we e.g. need
17 px icon and load 16 px resource for it; or that we want non-square icon)

https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/exte...
File chrome/browser/extensions/image_loading_tracker.h (right):

https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:75: enum ResizeMethod {
On 2012/08/10 21:42:08, Aaron Boodman wrote:
> Nit: Better name might be ResizeCondition?

Done.

https://chromiumcodereview.appspot.com/10825012/diff/4019/chrome/browser/exte...
chrome/browser/extensions/image_loading_tracker.h:108: // |LoadImageSkia|
instead.
On 2012/08/10 21:42:08, Aaron Boodman wrote:
> There's no LoadImageSkia() in this class.

Done.

[Aaron Boodman, 2012-08-13 23:29:12]

OK, I thought about this over the weekend. I think we should simplify the
interface presented to clients further. The constructor should just be:

IconImage(const Extension* extension,
          const ExtensionIconSet& icon_set,
          int resource_size_in_dip,
          Observer* observer);

- resource_match_type and desired_size_in_dip can go. We should just include the
correct fallback logic within IconImage. The correct fallback logic is:
  * If requested pixel size ends up being >= 32px (including scale factor), then
it is OK to fall back to a bigger size and resample down.
  * If no bigger size exists, or if the requested pixel size is smaller,   fall
back to smaller sizes. Never resize smaller sizes up.

- cache_param can go. Never cache. cache_param was the worst idea evar.

Additionally, extension really ought to be able to go, but the problem is that
ILT needs it for dumb reasons. So it must stay.

[tbarzic, 2012-08-14 03:09:09]

On 2012/08/13 23:29:12, Aaron Boodman wrote:
> OK, I thought about this over the weekend. I think we should simplify the
> interface presented to clients further. The constructor should just be:
> 
> IconImage(const Extension* extension,
>           const ExtensionIconSet& icon_set,
>           int resource_size_in_dip,
>           Observer* observer);
> 
> - resource_match_type and desired_size_in_dip can go. We should just include
the
> correct fallback logic within IconImage. The correct fallback logic is:
>   * If requested pixel size ends up being >= 32px (including scale factor),
then
> it is OK to fall back to a bigger size and resample down.
>   * If no bigger size exists, or if the requested pixel size is smaller,  
fall
> back to smaller sizes. Never resize smaller sizes up.
> 
> - cache_param can go. Never cache. cache_param was the worst idea evar.
> 
> Additionally, extension really ought to be able to go, but the problem is that
> ILT needs it for dumb reasons. So it must stay.

PTAL

Implemented fallback logic as described in comment #24.

also added observer callback to notify about failure (so observer would know it
needs to e.g. use default icon)

[Aaron Boodman, 2012-08-15 04:30:40]

http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/...
chrome/browser/extensions/extension_icon_image.cc:112: if (!is_retry &&
resource_size_in_pixel >= 32) {
We can assume that if an icon is specified in ExtensionIconSet then it will load
successfully. So this async retry business is not necessary. Just check whether
EIS specifies the big one (if necessary), then fall back to the small one if
not, all in one shot.

[tbarzic, 2012-08-15 15:01:36]

http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/...
File chrome/browser/extensions/extension_icon_image.cc (right):

http://codereview.chromium.org/10825012/diff/18031/chrome/browser/extensions/...
chrome/browser/extensions/extension_icon_image.cc:112: if (!is_retry &&
resource_size_in_pixel >= 32) {
On 2012/08/15 04:30:40, Aaron Boodman wrote:
> We can assume that if an icon is specified in ExtensionIconSet then it will
load
> successfully. So this async retry business is not necessary. Just check
whether
> EIS specifies the big one (if necessary), then fall back to the small one if
> not, all in one shot.

Done.

[Aaron Boodman, 2012-08-16 05:07:01]

lgtm

[tbarzic, 2012-08-17 20:20:04]

On 2012/08/16 05:07:01, Aaron Boodman wrote:
> lgtm

tbring Ben

[commit-bot: I haz the power, 2012-08-20 18:01:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10825012/20036

[Ben Goodger (Google), 2012-08-20 20:07:29]

lgtm

[commit-bot: I haz the power, 2012-08-20 22:22:44]

Change committed as 152407