[webkit-reviews] review granted: [Bug 43724] Support all available biBitCount values in BitmapInfo : [Attachment 63902] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 43724: Support all available biBitCount values in BitmapInfo
https://bugs.webkit.org/show_bug.cgi?id=43724

Attachment 63902: Patch
https://bugs.webkit.org/attachment.cgi?id=63902&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
>  BitmapInfo bitmapInfoForSize(int width, int height, WORD bitCount)
>  {
> -    ASSERT_ARG(bitCount, bitCount == 16 || bitCount == 32);
> +    ASSERT_ARG(bitCount, bitCount == 1 || bitCount == 4 || bitCount == 8 ||
bitCount == 16 || bitCount == 24 || bitCount == 32);

Maybe an enum would be better for the bitCount parameter?

> @@ -43,11 +43,6 @@ BitmapInfo bitmapInfoForSize(int width, 
>      bitmapInfo.bmiHeader.biPlanes	    = 1;
>      bitmapInfo.bmiHeader.biBitCount	    = bitCount;
>      bitmapInfo.bmiHeader.biCompression   = BI_RGB;
> -    bitmapInfo.bmiHeader.biSizeImage     = 0;
> -    bitmapInfo.bmiHeader.biXPelsPerMeter = 0;
> -    bitmapInfo.bmiHeader.biYPelsPerMeter = 0;
> -    bitmapInfo.bmiHeader.biClrUsed	    = 0;
> -    bitmapInfo.bmiHeader.biClrImportant  = 0;

Please explain this change in the ChangeLog just as you did in an earlier
comment in this bug.
> @@ -44,10 +44,12 @@ struct BitmapInfo : public BITMAPINFO {
>      unsigned width() const { return abs(bmiHeader.biWidth); }
>      unsigned height() const { return abs(bmiHeader.biHeight); }
>      IntSize size() const { return IntSize(width(), height()); }
> -    unsigned paddedWidth() const { return is16bit() ? (width() + 1) & ~0x1 :
width(); }
> +    unsigned bytesPerLine() const { return (width() * bmiHeader.biBitCount +
7) / 8; }
> +    unsigned paddedBytesPerLine() const { return (bytesPerLine() + 3) &
~0x3; }
> +    unsigned paddedWidth() const { return paddedBytesPerLine() * 8 /
bmiHeader.biBitCount; }
>      unsigned numPixels() const { return paddedWidth() * height(); }
> -    unsigned paddedBytesPerLine() const { return is16bit() ? paddedWidth() *
2 : width() * 4; }
> -    unsigned bytesPerLine() const { return width() * bmiHeader.biBitCount /
8; }};

How will someone know whether they should call width() or paddedWidth()? (Ditto
for the other functions.)

r=me