[webkit-reviews] review denied: [Bug 42071] [WINCE] Add additonal methodes to BitmapInfo : [Attachment 61211] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 42071: [WINCE] Add additonal methodes to BitmapInfo
https://bugs.webkit.org/show_bug.cgi?id=42071

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   [WINCE] Add additonal methodes to BitmapInfo.

Typo: methodes

> -BitmapInfo bitmapInfoForSize(int width, int height)
> +BitmapInfo bitmapInfoForSize(int width, int height, WORD bitCount)
>  {
> +    ASSERT(bitCount == 16 || bitCount == 32);

ASSERT_ARG would be a bit better here:

ASSERT_ARG(bitCount, bitCount == 16 || bitCount == 32);

>      BitmapInfo bitmapInfo;
> -    bitmapInfo.bmiHeader.biSize	    = sizeof(BITMAPINFOHEADER);

Perhaps the ChangeLog should mention that the BitmapInfo constructor does this
for us. I was confused by this change at first.

> +#if !OS(WINCE) || PLATFORM(QT) || IMAGE_NO_ALPHA_USE_RGB555

Where/in what situations is IMAGE_NO_ALPHA_USE_RGB555 defined?

> +    if (bitCount == 16) {
> +	   ((DWORD*)bitmapInfo.bmiColors)[0] = 0x0000F800;
> +	   ((DWORD*)bitmapInfo.bmiColors)[1] = 0x000007E0;
> +	   ((DWORD*)bitmapInfo.bmiColors)[2] = 0x0000001F;
> +    }
> +#endif

Where do these constants come from?

reinterpret_cast would be better here.

The changes look fine other than the cast, though I am interested in your
answers to these questions.