ui/gfx: remove redundant erase operations and remove unused function

ui/gfx: remove redundant erase operations and remove unused function

It removes erase operations for SkBitmaps
which touch whole inside pixels
since all pixels should be drawn fully.

Additionally, it removes a unused function, CreateSuperimposedBitmap().

BUG=NONE
TEST=gfx_unittests --gtest_filter="SkBitmapOperationsTest*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283814

[hyunki, 2014-07-16 11:44:58]

Please take a look. Thanks.

[oshima, 2014-07-16 15:09:26]

lgtm if the test passes valgrind. Just want to make sure there is no
edge/special case that needs to be taken care of.

[oshima, 2014-07-16 15:09:26]

lgtm if the test passes valgrind. Just want to make sure there is no
edge/special case that needs to be taken care of.

[sky, 2014-07-16 15:17:20]

LGTM

[danakj, 2014-07-16 15:18:25]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
will it be filled with opaque values? you can save the alloc clearing everything
if so by passing that info along here.

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:98: blended.eraseARGB(0, 0, 0, 0);
ditto for the rest

[hyunki, 2014-07-16 15:28:48]

Please check my comments.

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
On 2014/07/16 15:18:25, danakj wrote:
> will it be filled with opaque values? you can save the alloc clearing
everything
> if so by passing that info along here.

Yes. I think so. Please check below codes to fill all dst_row[x] 32 bits.

  for (int y = 0; y < image.height(); ++y) {
    uint32* image_row = image.getAddr32(0, y);
    uint32* dst_row = inverted.getAddr32(0, y);

    for (int x = 0; x < image.width(); ++x) {
      uint32 image_pixel = image_row[x];
      dst_row[x] = (image_pixel & 0xFF000000) |
                   (0x00FFFFFF - (image_pixel & 0x00FFFFFF));
    }
  }

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:98: blended.eraseARGB(0, 0, 0, 0);
On 2014/07/16 15:18:25, danakj wrote:
> ditto for the rest

ditto.

 dst_row[x] = SkColorSetARGB(a, r, g, b);

[danakj, 2014-07-16 15:30:42]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
On 2014/07/16 15:28:48, hyunki wrote:
> On 2014/07/16 15:18:25, danakj wrote:
> > will it be filled with opaque values? you can save the alloc clearing
> everything
> > if so by passing that info along here.
> 
> Yes. I think so. Please check below codes to fill all dst_row[x] 32 bits.
> 
>   for (int y = 0; y < image.height(); ++y) {
>     uint32* image_row = image.getAddr32(0, y);
>     uint32* dst_row = inverted.getAddr32(0, y);
> 
>     for (int x = 0; x < image.width(); ++x) {
>       uint32 image_pixel = image_row[x];
>       dst_row[x] = (image_pixel & 0xFF000000) |
>                    (0x00FFFFFF - (image_pixel & 0x00FFFFFF));
>     }
>   }

It depends on the input image right?

[hyunki, 2014-07-16 15:35:16]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
Yes. It depends on the input image. All alphas for dst_row will be computed and
filled.

dst_row[x] = (image_pixel & 0xFF000000) |
                   (0x00FFFFFF - (image_pixel & 0x00FFFFFF));

[danakj, 2014-07-16 15:38:12]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
On 2014/07/16 15:35:16, hyunki wrote:
> Yes. It depends on the input image. All alphas for dst_row will be computed
and
> filled.
> 
> dst_row[x] = (image_pixel & 0xFF000000) |
>                    (0x00FFFFFF - (image_pixel & 0x00FFFFFF));

So, do you want to tell allocN32 if it will be opaque and avoid the implicit
erase inside there?

[hyunki, 2014-07-16 15:42:17]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
No. I'm telling below operation fills whole alloced pixels because it covers
whole 32bit and double for-loops covers "image.width() * image.height()" area.

dst_row[x] = (image_pixel & 0xFF000000) |
                   (0x00FFFFFF - (image_pixel & 0x00FFFFFF));

[danakj, 2014-07-16 15:52:29]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
On 2014/07/16 15:42:17, hyunki wrote:
> No. I'm telling below operation fills whole alloced pixels because it covers
> whole 32bit and double for-loops covers "image.width() * image.height()" area.
> 
> dst_row[x] = (image_pixel & 0xFF000000) |
>                    (0x00FFFFFF - (image_pixel & 0x00FFFFFF));

So then how about passing true for opaque to allocN32Pixels?

[hyunki, 2014-07-16 16:03:04]

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.cc
File ui/gfx/skbitmap_operations.cc (left):

https://codereview.chromium.org/394183002/diff/1/ui/gfx/skbitmap_operations.c...
ui/gfx/skbitmap_operations.cc:29: inverted.allocN32Pixels(image.width(),
image.height());
Sorry. For clarification,

(your question) will it be filled with opaque values? 
(my anwser) No, as I mentioned above, it depends on the input image.

Since allocN32Pixels will set |isOpaque| to false by default,
I think just current code changes would be fine.

[hyunki, 2014-07-16 23:28:02]

danakj@,

can you please have a look? thanks.

[danakj, 2014-07-17 15:22:30]

Sure, I think you have all the reviews you need. When isOpaque is false, though,
there's another erase inside allocN32, which I think you can avoid at least
sometimes, that's all I am trying to say.

[hyunki, 2014-07-17 15:28:44]

On 2014/07/17 15:22:30, danakj wrote:
> Sure, I think you have all the reviews you need. When isOpaque is false,
though,
> there's another erase inside allocN32, which I think you can avoid at least
> sometimes, that's all I am trying to say.

Oh, I see. I'll check it. And for now, I'll try to commit.
Thanks for this comment.

[hyunki, 2014-07-17 15:28:51]

The CQ bit was checked by hyunki.baik@samsung.com

[commit-bot: I haz the power, 2014-07-17 15:29:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/394183002/1

[commit-bot: I haz the power, 2014-07-17 17:59:45]

Change committed as 283814