Fix Clipboard::ReadImage

Properly scope ScopedPlatformPaint so that the native drawing layer and Skia are in agreement on the state of the pixels. Also fix Windows to not return completely transparent bitmaps when the source image has color depth < 32 bpp.

BUG=86085
TEST=Copy an image from MS Paint and paste it into Gmail using 32bpp for the display color depth.

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

[tony, 2011-08-04 22:06:56]

Can you update the change description on how to manually test (e.g., set display
color depth to X, open mspaint, etc)?

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:459: case 24:
The msdn page about BITMAPINFOHEADER says, "The bmiColors color table is used
for optimizing colors used on palette-based devices, and must contain the number
of entries specified by the biClrUsed member of the BITMAPINFOHEADER."

Does that mean we need to check the value of biClrUsed when we have a 24bit
bitmap?

[dcheng, 2011-08-04 22:13:54]

On 2011/08/04 22:06:56, tony wrote:
> Can you update the change description on how to manually test (e.g., set
display
> color depth to X, open mspaint, etc)?

Done.

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:459: case 24:
On 2011/08/04 22:06:56, tony wrote:
> The msdn page about BITMAPINFOHEADER says, "The bmiColors color table is used
> for optimizing colors used on palette-based devices, and must contain the
number
> of entries specified by the biClrUsed member of the BITMAPINFOHEADER."
> 
> Does that mean we need to check the value of biClrUsed when we have a 24bit
> bitmap?

It also says it's NULL right before that. Honestly, I have no idea how to
interpret that sentence.

[tony, 2011-08-04 22:20:52]

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:459: case 24:
On 2011/08/04 22:13:54, dcheng wrote:

> It also says it's NULL right before that. Honestly, I have no idea how to
> interpret that sentence.

So it does.  That's really confusing.  Ok, LGTM.  pkasting might know more about
the bitmap format and might be able to translate what this means.

[Wez, 2011-08-05 00:12:58]

My 2c, for what it's worth. :)

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:443: // depth of 32bpp.
The reason for using a DIB is really that DIBs are self-contained; if you
requested a DDB and the display were palettized then you'd get a bunch of pixel
data specific to the currently-in-effect palette, which isn't what you want.

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:459: case 24:
On 2011/08/04 22:13:54, dcheng wrote:
> On 2011/08/04 22:06:56, tony wrote:
> > The msdn page about BITMAPINFOHEADER says, "The bmiColors color table is
used
> > for optimizing colors used on palette-based devices, and must contain the
> number
> > of entries specified by the biClrUsed member of the BITMAPINFOHEADER."
> > 
> > Does that mean we need to check the value of biClrUsed when we have a 24bit
> > bitmap?
> 
> It also says it's NULL right before that. Honestly, I have no idea how to
> interpret that sentence.

For bit counts of >=16, I think biClrUsed is supposed to specify the number of
palette entries provided to help optimize conversion should the bitmap be blit
to a palettized target.  Presumably that only makes sense for BI_RGB bitmaps,
but I'm not sure.

[dcheng, 2011-08-05 20:33:08]

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_win.cc (right):

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:443: // depth of 32bpp.
On 2011/08/05 00:12:58, Wez wrote:
> The reason for using a DIB is really that DIBs are self-contained; if you
> requested a DDB and the display were palettized then you'd get a bunch of
pixel
> data specific to the currently-in-effect palette, which isn't what you want.

Not having to deal with the palette makes things easier, but the primary reason
I documented this is because the inability to differentiate between different
color depths with DDBs would break the alpha fixup code below.

http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_win.cc:459: case 24:
On 2011/08/05 00:12:58, Wez wrote:
> On 2011/08/04 22:13:54, dcheng wrote:
> > On 2011/08/04 22:06:56, tony wrote:
> > > The msdn page about BITMAPINFOHEADER says, "The bmiColors color table is
> used
> > > for optimizing colors used on palette-based devices, and must contain the
> > number
> > > of entries specified by the biClrUsed member of the BITMAPINFOHEADER."
> > > 
> > > Does that mean we need to check the value of biClrUsed when we have a
24bit
> > > bitmap?
> > 
> > It also says it's NULL right before that. Honestly, I have no idea how to
> > interpret that sentence.
> 
> For bit counts of >=16, I think biClrUsed is supposed to specify the number of
> palette entries provided to help optimize conversion should the bitmap be blit
> to a palettized target.  Presumably that only makes sense for BI_RGB bitmaps,
> but I'm not sure.

Hmm. All the code I can find treats BI_RGB identically to how I handle it here.
Would it be worth adding a DCHECK()?

[Wez, 2011-08-05 23:25:58]

On 2011/08/05 20:33:08, dcheng wrote:
>
http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
> File ui/base/clipboard/clipboard_win.cc (right):
> 
>
http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
> ui/base/clipboard/clipboard_win.cc:443: // depth of 32bpp.
> On 2011/08/05 00:12:58, Wez wrote:
> > The reason for using a DIB is really that DIBs are self-contained; if you
> > requested a DDB and the display were palettized then you'd get a bunch of
> pixel
> > data specific to the currently-in-effect palette, which isn't what you want.
> 
> Not having to deal with the palette makes things easier, but the primary
reason
> I documented this is because the inability to differentiate between different
> color depths with DDBs would break the alpha fixup code below.
> 
>
http://codereview.chromium.org/7471008/diff/9001/ui/base/clipboard/clipboard_...
> ui/base/clipboard/clipboard_win.cc:459: case 24:
> On 2011/08/05 00:12:58, Wez wrote:
> > On 2011/08/04 22:13:54, dcheng wrote:
> > > On 2011/08/04 22:06:56, tony wrote:
> > > > The msdn page about BITMAPINFOHEADER says, "The bmiColors color table is
> > used
> > > > for optimizing colors used on palette-based devices, and must contain
the
> > > number
> > > > of entries specified by the biClrUsed member of the BITMAPINFOHEADER."
> > > > 
> > > > Does that mean we need to check the value of biClrUsed when we have a
> 24bit
> > > > bitmap?
> > > 
> > > It also says it's NULL right before that. Honestly, I have no idea how to
> > > interpret that sentence.
> > 
> > For bit counts of >=16, I think biClrUsed is supposed to specify the number
of
> > palette entries provided to help optimize conversion should the bitmap be
blit
> > to a palettized target.  Presumably that only makes sense for BI_RGB
bitmaps,
> > but I'm not sure.
> 
> Hmm. All the code I can find treats BI_RGB identically to how I handle it
here.
> Would it be worth adding a DCHECK()?

Sounds like a plan; I very much doubt you'll ever see an application produce an
RGB DIB with a palette in it these days, and as I said, I don't think I've ever
seen one.

LGTM FWIW. :)