Convert extension action icons code to use ImageSkia instead of SkBitmap

Convert extension action icons code to use ImageSkia instead of SkBitmap

This is needed to support hidpi for browser/page actions.

BUG=138025, 135271
TEST=manually checked that browser and page action buttons are displayed correctly.


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

[tbarzic, 2012-08-07 21:37:18]

Hey guys,
can you take a look at this. I hope I'm on the right track :)

Is there anything else I have to do for mac part (I just made sure mac_rel
trybot is happy)?

Thanks,
Toni

[Robert Sesek, 2012-08-07 21:44:24]

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:257: [self
setImage:gfx::NSImageFromImageSkia(image)];
Shouldn't this use gfx::Image because it's painting in the native format? You
should be able to stuff the ImageSkia from the extension into the Image and then
let it take care of the conversion.

[tbarzic, 2012-08-07 22:29:52]

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:257: [self
setImage:gfx::NSImageFromImageSkia(image)];
On 2012/08/07 21:44:24, rsesek wrote:
> Shouldn't this use gfx::Image because it's painting in the native format? You
> should be able to stuff the ImageSkia from the extension into the Image and
then
> let it take care of the conversion.

wouldn't that do basically the same thing (convert image from ImageSkia
(encapsulated in gfx::Image) to NSImage; since browser_action()->GetIcon() will
always contains ImageSkia).

(I'm not completely sure I got this right :))

[pkotwicz, 2012-08-07 22:31:22]

At a long glance this looks pretty good

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:63: return
canvas.ExtractImageRep();
A couple comments.
It feels like we could introduce ImageSkiaOperations::CreateSuperimposedImage
and it would have the desired behavior for IconWithBackgroundImageSource.
(It's up to you though as to what you feel is clearer)

You don't need to specify SkXfermode::kSrcOver_Mode as that's the default.
Can you paint the two images in ::Draw() instead of overriding GetImageForScale
here?
The canvas constructor which takes in an ImageSkiaRep draws the ImageSkiaRep so
you are not losing any efficiency.

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:78: 
Nit: destructor

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:109: class ActionBadgeImageSource
: public gfx::CanvasImageSource {
Can you move this to new function ExtensionAction::GetBadgeImage() ?

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:123: 
Nit: destructor

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.cc:406: //
scoped_ptr<gfx::Canvas> canvas(
Nit: remove this

http://codereview.chromium.org/10827191/diff/7003/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:158: const SkBitmap&
ExtensionAction::IconAnimation::Apply(
Can we merge this logic into AnimatedIconSource. (Or add a TODO do so)
My understanding is that we won't want to call this directly anymore.
Also don't be afraid to add to ImageSkiaOperations if you think this is a basic
operation which might be useful elsewhere. We might get to a state where we find
out that there are a whole bunch of functions inside of ImageSkiaOperations
which are really similar and that we need to clean it up. But I think that
realization will be a good thing

[tbarzic, 2012-08-08 02:09:29]

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
File chrome/browser/ui/views/browser_action_view.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.cc:63: return
canvas.ExtractImageRep();
On 2012/08/07 22:31:22, pkotwicz wrote:
> A couple comments.
> It feels like we could introduce ImageSkiaOperations::CreateSuperimposedImage
> and it would have the desired behavior for IconWithBackgroundImageSource.
> (It's up to you though as to what you feel is clearer)
> 
> You don't need to specify SkXfermode::kSrcOver_Mode as that's the default.
> Can you paint the two images in ::Draw() instead of overriding
GetImageForScale
> here?
> The canvas constructor which takes in an ImageSkiaRep draws the ImageSkiaRep
so
> you are not losing any efficiency.

Done.

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.cc:78: 
On 2012/08/07 22:31:22, pkotwicz wrote:
> Nit: destructor

Done.

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.cc:109: class ActionBadgeImageSource
: public gfx::CanvasImageSource {
On 2012/08/07 22:31:22, pkotwicz wrote:
> Can you move this to new function ExtensionAction::GetBadgeImage() ?

Done.

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.cc:123: 
On 2012/08/07 22:31:22, pkotwicz wrote:
> Nit: destructor

Done.

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/browser_actions_container.cc:406: //
scoped_ptr<gfx::Canvas> canvas(
On 2012/08/07 22:31:22, pkotwicz wrote:
> Nit: remove this

yeah, I missed that while cleaning up the cl..

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/common/exten...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/7003/chrome/common/exten...
chrome/common/extensions/extension_action.cc:158: const SkBitmap&
ExtensionAction::IconAnimation::Apply(
On 2012/08/07 22:31:22, pkotwicz wrote:
> Can we merge this logic into AnimatedIconSource. (Or add a TODO do so)
> My understanding is that we won't want to call this directly anymore.
> Also don't be afraid to add to ImageSkiaOperations if you think this is a
basic
> operation which might be useful elsewhere. We might get to a state where we
find
> out that there are a whole bunch of functions inside of ImageSkiaOperations
> which are really similar and that we need to clean it up. But I think that
> realization will be a good thing

added todo..

[pkotwicz, 2012-08-08 15:58:32]

LGTM with Nits

[pkotwicz, 2012-08-08 15:58:50]

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:33: // image skia operation while
creating transparent image.
Nit: How about "ImageSource which creates a partially transparent image mask."

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/ex...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:43:
//    scoped_ptr<gfx::Canvas> canvas(view->GetIconWithBadge());
Nit: Remove this

http://codereview.chromium.org/10827191/diff/10005/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10827191/diff/10005/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:428: SkScalar text_width_sk =
text_paint->measureText(text.c_str(), text.size());
Nit: sk_text_width

[Robert Sesek, 2012-08-08 16:02:06]

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:257: [self
setImage:gfx::NSImageFromImageSkia(image)];
On 2012/08/07 22:29:52, tbarzic wrote:
> On 2012/08/07 21:44:24, rsesek wrote:
> > Shouldn't this use gfx::Image because it's painting in the native format?
You
> > should be able to stuff the ImageSkia from the extension into the Image and
> then
> > let it take care of the conversion.
> 
> wouldn't that do basically the same thing (convert image from ImageSkia
> (encapsulated in gfx::Image) to NSImage; since browser_action()->GetIcon()
will
> always contains ImageSkia).
> 
> (I'm not completely sure I got this right :))

Yes, that's what it would do. We've standardized on gfx::Image as the way to
plumb an image that will be converted in the view layer to the platform type.
ImageSkia is really just a multi-resolution SkBitmap, which I think this should
stay gfx::Image.

[sky, 2012-08-08 16:55:45]

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:57: size_t width_;
gfx::Size uses int, not size_t. Also, make these const.

[tbarzic, 2012-08-08 18:53:51]

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10827191/diff/7003/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:257: [self
setImage:gfx::NSImageFromImageSkia(image)];
On 2012/08/08 16:02:06, rsesek wrote:
> On 2012/08/07 22:29:52, tbarzic wrote:
> > On 2012/08/07 21:44:24, rsesek wrote:
> > > Shouldn't this use gfx::Image because it's painting in the native format?
> You
> > > should be able to stuff the ImageSkia from the extension into the Image
and
> > then
> > > let it take care of the conversion.
> > 
> > wouldn't that do basically the same thing (convert image from ImageSkia
> > (encapsulated in gfx::Image) to NSImage; since browser_action()->GetIcon()
> will
> > always contains ImageSkia).
> > 
> > (I'm not completely sure I got this right :))
> 
> Yes, that's what it would do. We've standardized on gfx::Image as the way to
> plumb an image that will be converted in the view layer to the platform type.
> ImageSkia is really just a multi-resolution SkBitmap, which I think this
should
> stay gfx::Image.

OK then.. Done.

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:33: // image skia operation while
creating transparent image.
On 2012/08/08 15:58:50, pkotwicz wrote:
> Nit: How about "ImageSource which creates a partially transparent image mask."

Done.

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:57: size_t width_;
On 2012/08/08 16:55:45, sky wrote:
> gfx::Size uses int, not size_t. Also, make these const.

Done.

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/ex...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/10827191/diff/10005/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:43:
//    scoped_ptr<gfx::Canvas> canvas(view->GetIconWithBadge());
On 2012/08/08 15:58:50, pkotwicz wrote:
> Nit: Remove this

Done.

http://codereview.chromium.org/10827191/diff/10005/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10827191/diff/10005/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:428: SkScalar text_width_sk =
text_paint->measureText(text.c_str(), text.size());
On 2012/08/08 15:58:50, pkotwicz wrote:
> Nit: sk_text_width

Done.

[tbarzic, 2012-08-08 18:59:28]

+kalman

[Robert Sesek, 2012-08-08 19:01:15]

LGTM. Thanks.

[sky, 2012-08-08 21:10:48]

LGTM

[not at google - send to devlin, 2012-08-09 01:53:59]

+jyasskin

[Jeffrey Yasskin, 2012-08-09 21:39:04]

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/exte...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:215:
extension_action_->SetIcon(tab_id_, gfx::Image(bitmap));
Do you want to use the ImageSkiaRep(bitmap, SCALE_FACTOR_100P) constructor
instead, to reflect that
http://developer.chrome.com/trunk/extensions/pageAction.html#method-setIcon
takes canvas.getImageData(), not canvas.getImageDataHD()? Probably comment this
either way.

Outside of this CL, how would we figure out the scale factor that
getImageDataHD() uses?

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:252: gfx::ImageSkia
GetIcon() {
Out of curiosity, why return ImageSkia instead of Image here?

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/v...
File chrome/browser/ui/views/browser_action_view.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.cc:67: return
gfx::ImageSkiaOperations::CreateMaskedImage(image, alpha);
Why do a 2-step transformation instead of directly manipulating |image| in the
ImageSource?

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/v...
File chrome/browser/ui/views/browser_action_view.h (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/browser/ui/v...
chrome/browser/ui/views/browser_action_view.h:157: // view as well as the badge
(if any). Caller is responsible for deleting the
Please update this comment.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.cc:64: return image.size().width();
Once this function is a single expression, it's probably worth inlining it to
the single call site.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.cc:138: // Original representation's
scale factor may be different from scale
It would be nice if CanvasImageSource supported this use case. Not something to
do in this CL, of course.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.cc:415: SkColor text_color =
(SkColorGetA(text_color_in) == 0x00) ?
Interesting. I would probably have kept the if statements, since I find ternary
operators harder to read, but it's not a big deal.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.h:240: void PaintBadge(gfx::Canvas*
canvas, const gfx::Rect& bounds, int tab_id);
Do the new functions let you make this private or remove it? That would be cool.

Don't worry about this if it would need any changes outside this file.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.h:257: class AnimatedIconImageSource;
Do these need to be members of ExtensionAction, or can they just be local
classes in extension_action.cc?

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action.h:311: std::map<int, gfx::ImageSkia>
icon_;
Why ImageSkia instead of Image? It would be good to have a comment in gfx/image/
saying how to decide between them, although that's separate from this CL.

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
File chrome/common/extensions/extension_action_unittest.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/chrome/common/exten...
chrome/common/extensions/extension_action_unittest.cc:68:
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
4-space indent, right?

https://chromiumcodereview.appspot.com/10827191/diff/9006/ui/gfx/image/image_...
File ui/gfx/image/image_skia_operations.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/ui/gfx/image/image_...
ui/gfx/image/image_skia_operations.cc:107: canvas->DrawImageInt(first_, 0, 0,
SkPaint());
There's a DrawImage overload that you don't have to pass an SkPaint to.

https://chromiumcodereview.appspot.com/10827191/diff/9006/ui/gfx/image/image_...
File ui/gfx/image/image_skia_operations.h (right):

https://chromiumcodereview.appspot.com/10827191/diff/9006/ui/gfx/image/image_...
ui/gfx/image/image_skia_operations.h:28: // The second image is centered in
respect to the first image.
I've seen code that wants to control whether centered images round up or down
when they can't be exactly centered. Do you want to expose that here?

[tbarzic, 2012-08-10 06:24:08]

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:215:
extension_action_->SetIcon(tab_id_, gfx::Image(bitmap));
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Do you want to use the ImageSkiaRep(bitmap, SCALE_FACTOR_100P) constructor
> instead, to reflect that
> http://developer.chrome.com/trunk/extensions/pageAction.html#method-setIcon
> takes canvas.getImageData(), not canvas.getImageDataHD()? Probably comment
this
> either way.
> 
I don't think it's necessary.. I will probably just leave this file alone now :)

but I'll make some changes here in very near future to support hd for extension
actions icons (see the bug :) )

> Outside of this CL, how would we figure out the scale factor that
> getImageDataHD() uses?

I don't if there is a way to find this out before paint time .. (peter may know
a bit more about this)

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:252: gfx::ImageSkia
GetIcon() {
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Out of curiosity, why return ImageSkia instead of Image here?

Code that uses return value asks for ImageSkia, so we have to do the conversion
either here or  after this returns (line 1046).

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:67: return
gfx::ImageSkiaOperations::CreateMaskedImage(image, alpha);
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Why do a 2-step transformation instead of directly manipulating |image| in the
> ImageSource?

Done. moved to image_skia_operations

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.h (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.h:157: // view as well as the badge
(if any). Caller is responsible for deleting the
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Please update this comment.

Done.

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:64: return image.size().width();
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Once this function is a single expression, it's probably worth inlining it to
> the single call site.

Done.

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:138: // Original representation's
scale factor may be different from scale
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> It would be nice if CanvasImageSource supported this use case. Not something
to
> do in this CL, of course.

yep.. TODO already added :)

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:415: SkColor text_color =
(SkColorGetA(text_color_in) == 0x00) ?
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Interesting. I would probably have kept the if statements, since I find
ternary
> operators harder to read, but it's not a big deal.

Done.

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:240: void PaintBadge(gfx::Canvas*
canvas, const gfx::Rect& bounds, int tab_id);
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Do the new functions let you make this private or remove it? That would be
cool.
> 
> Don't worry about this if it would need any changes outside this file.

we still need this..

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:257: class AnimatedIconImageSource;
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Do these need to be members of ExtensionAction, or can they just be local
> classes in extension_action.cc

IconWithBadgeImageSource uses private DoPaintBadge. 

I can remove AnimatedIconImageSource, though (it depends on IconAnimatedWrapper,
but that can (should) be changed)

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:311: std::map<int, gfx::ImageSkia>
icon_;
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> Why ImageSkia instead of Image? It would be good to have a comment in
gfx/image/
> saying how to decide between them, although that's separate from this CL.

we are using ImageSkia internally inside this class, so I figured I would make
that obvious...my understanding is that we use gfx::Image when backing image
type is undetermined.. but I could be terribly wrong here :)

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action_unittest.cc (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action_unittest.cc:68:
ui::ResourceBundle::GetSharedInstance().GetImageNamed(
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> 4-space indent, right?

Done.

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:107: canvas->DrawImageInt(first_, 0, 0,
SkPaint());
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> There's a DrawImage overload that you don't have to pass an SkPaint to.

Done.

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:28: // The second image is centered in
respect to the first image.
On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> I've seen code that wants to control whether centered images round up or down
> when they can't be exactly centered. Do you want to expose that here?

might be a good idea, but I'd leave it outside of scope of this change..

[Jeffrey Yasskin, 2012-08-10 07:40:34]

LGTM

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10827191/diff/9006/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:311: std::map<int, gfx::ImageSkia>
icon_;
On 2012/08/10 06:24:08, tbarzic wrote:
> On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> > Why ImageSkia instead of Image? It would be good to have a comment in
> gfx/image/
> > saying how to decide between them, although that's separate from this CL.
> 
> we are using ImageSkia internally inside this class, so I figured I would make
> that obvious...my understanding is that we use gfx::Image when backing image
> type is undetermined.. but I could be terribly wrong here :) 

I know less about this than you do. :)

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10827191/diff/9006/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:28: // The second image is centered in
respect to the first image.
On 2012/08/10 06:24:08, tbarzic wrote:
> On 2012/08/09 21:39:04, Jeffrey Yasskin wrote:
> > I've seen code that wants to control whether centered images round up or
down
> > when they can't be exactly centered. Do you want to expose that here?
> 
> might be a good idea, but I'd leave it outside of scope of this change..

SGTM.

[commit-bot: I haz the power, 2012-08-13 21:59:22]

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

[commit-bot: I haz the power, 2012-08-14 00:31:52]

Change committed as 151399

[Finnur, 2012-09-06 16:05:47]

https://chromiumcodereview.appspot.com/10827191/diff/1045/chrome/common/exten...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10827191/diff/1045/chrome/common/exten...
chrome/common/extensions/extension_action.cc:446: gfx::Rect rect(rect_x, rect_y,
rect_width, rect_height);
I believe this caused a regression:
http://code.google.com/p/chromium/issues/detail?id=144545