Implement remaining SkBitmapOperations as ImageSkiaOperations

Implement remaining SkBitmapOperations as ImageSkiaOperations

Bug=None
Test=Compiles, code looks nicer
R=oshima,sky
TBR=sadrul

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

[pkotwicz, 2012-07-13 01:34:12]

Implement remaining SkBitmapOperations as ImageSkiaOperations.
I consider the list complete though I did not implement a couple which are not
used in the chromium code base.

CreateTransposedBitmap for instance.

[oshima, 2012-07-13 16:54:55]

http://codereview.chromium.org/10704199/diff/1001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10704199/diff/1001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:173: empty.eraseARGB(0, 0,
0, 0);
can we cache this? I think it'd save more than caching ImageSkia

http://codereview.chromium.org/10704199/diff/1001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:241: gfx::ImageSkia
CreateEmptyImage(const gfx::Size& size) {
GetEmptyImage is probably better. Is this only place we need empty image? Just
wondering if this should be in common place.

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

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:191: }
destructor

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:218: }
ditto

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:241: }
ditto

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:264: }
ditto

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:332: SkColor color) {
can these be alined to at (?

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:339: const Rect& subset_bounds) {
can these be alined to at (?

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:341: return ImageSkia();
i wonder "return image" is better. you may keep it if you think this is better.

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

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:44: static ImageSkia
CreateButtonBackground(SkColor color,
If this is specific to image button, i'd keep it in image_button.cc. Is someone
else using this?

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:68: static ImageSkia CreateColorMask(const
gfx::ImageSkia& image, SkColor c);
Are these used in this CL? I prefer to add when you need it. (when you update
the place that uses SkBitmapOperations)

[pkotwicz, 2012-07-15 03:36:47]

Changes as requested. I removed ImageSkiaOprerations::CreateColorMask as we do
not use SkBitmapOperations::CreateColorMask
Also removed ImageSkiaOperations::CreateHSLShiftedImage. I will add it with the
CL to make themes slightly better.
Modified web_notifications_tray icon. Now it shows up properly in high DPI.

http://codereview.chromium.org/10704199/diff/1001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10704199/diff/1001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_icon.cc:241: gfx::ImageSkia
CreateEmptyImage(const gfx::Size& size) {
I haven't found a place which does something similar yet.

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

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:191: }
On 2012/07/13 16:54:55, oshima wrote:
> destructor

Done.

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:339: const Rect& subset_bounds) {
On 2012/07/13 16:54:55, oshima wrote:
> can these be alined to at (?

Done.

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:341: return ImageSkia();
I prefer it this way. SkBitmap::extractSubset returns a boolean. So it makes
more sense to return a null image.
I changed the condition above to do a bounds check on |subset_bounds| in my
latest patch as well.

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

http://codereview.chromium.org/10704199/diff/1001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.h:44: static ImageSkia
CreateButtonBackground(SkColor color,
I want to keep it here if only because this method is in SkBitmapOperations. I
think that it is currently good on erring on the side of adding to
image_skia_operations. This way we can notice similarities between methods and
try to combine them. For instance SkBitmapOperations::CreateSuperimposedBitmap
is the same as SkBitmapOperations::CreateMaskedBitmap except that they use a
different SkXfermode.

[oshima, 2012-07-16 17:05:33]

lgtm

[pkotwicz, 2012-07-16 18:22:24]

Scott for OWNERS

[sky, 2012-07-16 20:56:10]

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:92: const ImageSkia source_;
All of these classes roughly follow the same pattern. They cache and existing
ImageSkia and do some operation on it. This results in double the images (since
we effectively never release the source_). Is there some way to release the
source when we know its not needed anymore?

[oshima, 2012-07-16 21:08:35]

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:92: const ImageSkia source_;
On 2012/07/16 20:56:10, sky wrote:
> All of these classes roughly follow the same pattern. They cache and existing
> ImageSkia and do some operation on it. This results in double the images
(since
> we effectively never release the source_). Is there some way to release the
> source when we know its not needed anymore?

My plan is to make ScaleFactor an object which contains the active scale
factors,
and clears the cached images for inactive scale factors.
 This should be able to address most of your concern. Downside is that there
will be several
images that won't be cleared, but I believe this is better than adding observer
to or
have complete list of all instance of ImageSkia.

[sky, 2012-07-16 21:11:47]

On Mon, Jul 16, 2012 at 2:08 PM,  <oshima@chromium.org> wrote:
>
>
http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
> File ui/gfx/image/image_skia_operations.cc (right):
>
>
http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
> ui/gfx/image/image_skia_operations.cc:92: const ImageSkia source_;
> On 2012/07/16 20:56:10, sky wrote:
>>
>> All of these classes roughly follow the same pattern. They cache and
>
> existing
>>
>> ImageSkia and do some operation on it. This results in double the
>
> images (since
>>
>> we effectively never release the source_). Is there some way to
>
> release the
>>
>> source when we know its not needed anymore?
>
>
> My plan is to make ScaleFactor an object which contains the active scale
> factors,
> and clears the cached images for inactive scale factors.
>  This should be able to address most of your concern. Downside is that
> there will be several
> images that won't be cleared, but I believe this is better than adding
> observer to or
> have complete list of all instance of ImageSkia.
>
> http://codereview.chromium.org/10704199/

As long as there is a plan, I'm happy. Do have a bug and/or TODO filed on this?

  -Scott

[sky, 2012-07-16 21:23:40]

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:290: ImageSkia
ImageSkiaOperations::CreateButtonBackground(SkColor color,
This is only called in one place, should it be moved there? Especially since its
so specialized.

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:39: // We create a button background image
by compositing the color and image
We create -> Creates

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:49: // |image| cannot have image reps of
|kA1_Config| config.
What does this second line mean?
kAll_Config?

[oshima, 2012-07-16 22:52:54]

There is a meta bug crbug.com/125756. I don't have TODO, but you can star
this to follow the progress.
(I'm planning to work this next week)

- oshima

On Mon, Jul 16, 2012 at 2:11 PM, Scott Violet <sky@chromium.org> wrote:

> On Mon, Jul 16, 2012 at 2:08 PM,  <oshima@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
> > File ui/gfx/image/image_skia_operations.cc (right):
> >
> >
>
http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
> > ui/gfx/image/image_skia_operations.cc:92: const ImageSkia source_;
> > On 2012/07/16 20:56:10, sky wrote:
> >>
> >> All of these classes roughly follow the same pattern. They cache and
> >
> > existing
> >>
> >> ImageSkia and do some operation on it. This results in double the
> >
> > images (since
> >>
> >> we effectively never release the source_). Is there some way to
> >
> > release the
> >>
> >> source when we know its not needed anymore?
> >
> >
> > My plan is to make ScaleFactor an object which contains the active scale
> > factors,
> > and clears the cached images for inactive scale factors.
> >  This should be able to address most of your concern. Downside is that
> > there will be several
> > images that won't be cleared, but I believe this is better than adding
> > observer to or
> > have complete list of all instance of ImageSkia.
> >
> > http://codereview.chromium.org/10704199/
>
> As long as there is a plan, I'm happy. Do have a bug and/or TODO filed on
> this?
>
>   -Scott
>

[pkotwicz, 2012-07-17 01:15:41]

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.cc (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.cc:290: ImageSkia
ImageSkiaOperations::CreateButtonBackground(SkColor color,
I would rather keep it here, especially as it is part of SkBitmapOperations. I
would rather err on the side of adding too many methods to ImageSkiaOperations
such that it is easier to notice similarities for now.

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:49: // |image| cannot have image reps of
|kA1_Config| config.
The config of the bitmaps contained by the ImageSkia on which ExtractSubset
operates on cannot be kA1_Config. 
kA1_Config is a config simillar to kARGB_8888_Config. SkBitmap::extractSubset is
not supported for kA1_Config.

[pkotwicz, 2012-07-18 17:24:01]

Ping!

[sky, 2012-07-20 23:21:24]

LGTM

http://codereview.chromium.org/10704199/diff/13002/chrome/browser/chromeos/st...
File chrome/browser/chromeos/status/network_menu_icon.cc (right):

http://codereview.chromium.org/10704199/diff/13002/chrome/browser/chromeos/st...
chrome/browser/chromeos/status/network_menu_icon.cc:888: int width, height;
nit: initialize these (and images) just in case.

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
File ui/gfx/image/image_skia_operations.h (right):

http://codereview.chromium.org/10704199/diff/13002/ui/gfx/image/image_skia_op...
ui/gfx/image/image_skia_operations.h:49: // |image| cannot have image reps of
|kA1_Config| config.
On 2012/07/17 01:15:41, pkotwicz wrote:
> The config of the bitmaps contained by the ImageSkia on which ExtractSubset
> operates on cannot be kA1_Config. 
> kA1_Config is a config simillar to kARGB_8888_Config. SkBitmap::extractSubset
is
> not supported for kA1_Config.

Make the comment clearer than. Right now it is confusing.

[commit-bot: I haz the power, 2012-07-23 18:13:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10704199/21012

[commit-bot: I haz the power, 2012-07-23 20:35:09]

Change committed as 147925