Windows: Fix opacity of drag images....

Windows: Fix opacity of drag images.

BUG=XXX
TEST=Go to google.com, drag logo around. Preview image should now look nice.

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

[tony, 2010-07-13 22:37:05]

I also wonder if we should put a generic unmultiply method in
gfx/skbitmap_operations.*.

http://codereview.chromium.org/2963009/diff/1/2
File views/drag_utils_win.cc (right):

http://codereview.chromium.org/2963009/diff/1/2#newcode73
views/drag_utils_win.cc:73: sk_bitmap.copyTo(&tmp_bitmap, sk_bitmap.config());
Can we avoid this copy and just write the unmultiplied values to opaque_bitmap
directly?

[Nico, 2010-07-13 22:51:49]

> I also wonder if we should put a generic unmultiply method in
gfx/skbitmap_operations.*.

Seems like a good idea, except that it should be needed only rarely. What do you
think about putting it into an inner class called "subtle", so that invocations
would look like "SkBitmapOperatotions::subtle::unpremultiply()"? That way,
people would be more inclined to read the method's comment (which would say that
this probably shouldn't be called).

http://codereview.chromium.org/2963009/diff/1/2
File views/drag_utils_win.cc (right):

http://codereview.chromium.org/2963009/diff/1/2#newcode73
views/drag_utils_win.cc:73: sk_bitmap.copyTo(&tmp_bitmap, sk_bitmap.config());
On 2010/07/13 22:37:05, tony wrote:
> Can we avoid this copy and just write the unmultiplied values to opaque_bitmap
> directly?

Good point, done

[tony, 2010-07-13 23:06:36]

LGTM.

You're right that we probably don't need to make a helper method for
unpremultiply now.  Maybe we should wait until there's a second caller that
needs it.  Or maybe brettw has an opinion about this.

On 2010/07/13 22:51:49, Nico wrote:
> > I also wonder if we should put a generic unmultiply method in
> gfx/skbitmap_operations.*.
> 
> Seems like a good idea, except that it should be needed only rarely. What do
you
> think about putting it into an inner class called "subtle", so that
invocations
> would look like "SkBitmapOperatotions::subtle::unpremultiply()"? That way,
> people would be more inclined to read the method's comment (which would say
that
> this probably shouldn't be called).
> 
> http://codereview.chromium.org/2963009/diff/1/2
> File views/drag_utils_win.cc (right):
> 
> http://codereview.chromium.org/2963009/diff/1/2#newcode73
> views/drag_utils_win.cc:73: sk_bitmap.copyTo(&tmp_bitmap, sk_bitmap.config());
> On 2010/07/13 22:37:05, tony wrote:
> > Can we avoid this copy and just write the unmultiplied values to
opaque_bitmap
> > directly?
> 
> Good point, done

[Nico, 2010-07-13 23:12:12]

It might be required for http://crbug.com/26456

I uploaded a patch set that extracts the function, but I'll commit the patch set
before that for now.

[brettw, 2010-07-14 02:34:28]

This previous code is kind of unnecessary and I think it would be a good time to
do a minor clearnup:

Currently, it takes the un-premultipled SkBitmap, copies it to a canvas, then
copies that to a bitmap. These are unnecessary.

Instead, rename CreateBitmapFromCanvas to CreateHBITMAPFromSkBitmap and have it
take a const SkBitmap& instead of a platform canvas. Then just memcpy the data
from the SkBitmap to the "bits" pointer filled by CreateDIBSection. Now all the
canvas stuff can be removed.

Note: probably want to DCHECK(bitmap.rowBytes() == bitmap.width() * 4); since
you're manually memcpying the whole thing. Also, don't forget to lock the pixels
before reading them out of the bitmap, or the pointer will be NULL:
  SkAutoLockPixels lock(bitmap);
  memcpy(bits, bitmap.getPixels(), bitmap.width() * 4);

http://codereview.chromium.org/2963009/diff/9001/10001
File views/drag_utils_win.cc (right):

http://codereview.chromium.org/2963009/diff/9001/10001#newcode17
views/drag_utils_win.cc:17: #include
"third_party/skia/include/core/SkUnPreMultiply.h"
Looks like this include is not necessary.

[brettw, 2010-07-14 02:35:34]

Also, I would remove the "Subtle" namespace. I don't think this is particularly
subtle or something that we actively want to discourage people from doing
(unlike some of the subtle stuff in base where you will probably blow your head
off using it).

[Nikita (slow), 2010-07-14 10:18:32]

I'm reverting this CL.
It breaks
interactive_ui_tests

failed 4

DND
DNDToDifferentMenu
DNDBackToOriginatingMenu

You could see it's in win trybot results in patch set 2
and on buildbot
http://build.chromium.org/buildbot/waterfall/builders/Interactive%20Tests%20(...

[Nico, 2010-07-14 21:48:18]

Once more, with feeling.

This fixes the ui_tests failure _and_ addresses brett's comments. It's like
easter and christmas on the same day.

http://codereview.chromium.org/2963009/diff/4001/5001
File views/drag_utils_win.cc (right):

http://codereview.chromium.org/2963009/diff/4001/5001#newcode82
views/drag_utils_win.cc:82: uint32* dst_pixel = tmp_bitmap.getAddr32(x, y);
This was the problem: I didn't SkAutoLockPixels sk_bitmap, yet accessed its
pixels.

[brettw, 2010-07-14 21:58:22]

LGTM