[webkit-reviews] review denied: [Bug 28272] WINCE PORT: graphics files only for WINCE : [Attachment 71636] 1) SharedBitmap

[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 28272: WINCE PORT: graphics files only for WINCE
https://bugs.webkit.org/show_bug.cgi?id=28272

Attachment 71636: 1) SharedBitmap
https://bugs.webkit.org/attachment.cgi?id=71636&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71636&action=review

> WebCore/platform/graphics/wince/SharedBitmap.cpp:49
> +    SharedBitmap* resultantBitmap = new SharedBitmap(size, bitCount,
initPixels);
> +    if (resultantBitmap && !resultantBitmap->bytes()) {
> +	   delete resultantBitmap;
> +	   return 0;
> +    }
> +    return adoptRef(resultantBitmap);

This should be:

RefPtr<SharedBitmap> resultantBitmap = adoptRef(new ...);
if (...)
    return 0;
return resultantBitmap.release();

> WebCore/platform/graphics/wince/SharedBitmap.cpp:59
> +    PassRefPtr<SharedBitmap> result = create(size, BitmapInfo::BitCount32,
false);
> +    if (result) {
> +	   memcpy(result->bytes(), data.data(), data.size() *
sizeof(unsigned));
> +	   result->setHasAlpha(hasAlpha);
> +    }
> +    return result;

result should be a RefPtr and you should return result.release().

> WebCore/platform/graphics/wince/SharedBitmap.cpp:62
> +SharedBitmap::SharedBitmap(const BitmapInfo& bmpInfo, HBITMAP hbmp, void*
pixels)

This constructor doesn't seem to be used.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:79
> +    , m_reference(1)

This member doesn't seem to be used.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:111
> +    if (m_hbitmap)
> +	   DeleteObject(m_hbitmap);
> +    else
> +	   delete[] m_pixels;

Using OwnPtr/OwnArrayPtr would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:165
> +    unsigned short* p16 = reinterpret_cast<unsigned short*>(newPixels);
> +    const unsigned* p32 = reinterpret_cast<const unsigned*>(m_pixels);

static_cast should work here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:167
> +    const bool skips = paddedWidth != width;

We don't normally mark locals const like this.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:169
> +    const unsigned short* const p16end = p16 + bufferSize;

...or this (second const).

> WebCore/platform/graphics/wince/SharedBitmap.cpp:288
> +    if (!hbitmap) {

An early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:296
> +	   StretchDIBits(hdc, dstRect.x(), dstRect.y(), dstRect.width(),
dstRect.height()
> +	       , srcRect.x(), srcRect.y(), srcRect.width(), srcRect.height(),
m_pixels, &m_bmpInfo, DIB_RGB_COLORS, rop);

This comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:304
> +	       bool success = alphaBlendIfSupported(hdc, dstRect.x(),
dstRect.y(), dstRect.width(), dstRect.height(), hmemdc.get()
> +		   , srcRect.x(), srcRect.y(), srcRect.width(),
srcRect.height(), blend);

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:308
> +	       TransparentBlt(hdc, dstRect.x(), dstRect.y(), dstRect.width(),
dstRect.height(), hmemdc.get()
> +		   , srcRect.x(), srcRect.y(), srcRect.width(),
srcRect.height(), transparentColor());

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:331
> +    if (newBmp) {

An early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:336
> +	   StretchDIBits(dcNew.get(), 0, 0, copyWidth, copyHeight, rect.x(),
rect.y(), copyWidth, copyHeight
> +	       , bytes(), &bitmapInfo(), DIB_RGB_COLORS, SRCCOPY);

and this one

> WebCore/platform/graphics/wince/SharedBitmap.cpp:353
> +    PassRefPtr<SharedBitmap> newBmp = create(IntSize(copyWidth, copyHeight),
useAlpha && is32bit() ? BitmapInfo::BitCount32 : BitmapInfo::BitCount16,
false);

This should be a RefPtr and you should return newBmp.release().

> WebCore/platform/graphics/wince/SharedBitmap.cpp:361
> +    StretchDIBits(dcNew.get(), 0, 0, copyWidth, copyHeight, rect.x(),
rect.y(), copyWidth, copyHeight
> +	   , bytes(), &bitmapInfo(), DIB_RGB_COLORS, SRCCOPY);

Comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:369
> +    if (hBrush) {

Early-return would be better. Do we really need the null-check at all?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:388
> +	       StretchDIBits(hdc, dstX, dstY, sourceW, sourceH, sourceX,
sourceY, sourceW, sourceH
> +		   , bmp->bytes(), &bmp->bitmapInfo(), DIB_RGB_COLORS,
SRCCOPY);

Comma should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:521
> +    {

Why is this extra scope needed?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:536
> +	   if (hbmpTemp) {

Early-return would be better.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:574
> +		   bool success = alphaBlendIfSupported(hdc, dstRectWin.left,
dstRectWin.top, dstRectWin.right - dstRectWin.left, dstRectWin.bottom -
dstRectWin.top
> +		       , hmemdc.get(), 0, 0, srcRectWin.right,
srcRectWin.bottom, blend);
> +		   ASSERT_UNUSED(success, success);
> +	       } else if (useAlpha && !hasAlphaBlendSupport() || op ==
CompositeSourceOver && usesTransparentColor())
> +		   TransparentBlt(hdc, dstRectWin.left, dstRectWin.top,
dstRectWin.right - dstRectWin.left
> +		       , dstRectWin.bottom - dstRectWin.top, hmemdc.get(), 0,
0, srcRectWin.right, srcRectWin.bottom, transparentColor());
> +	       else {
> +		   DWORD bmpOp = op == CompositeCopy ? SRCCOPY
> +			       : op == CompositeSourceOver ? SRCCOPY
> +			       : op == CompositeXOR ? PATINVERT
> +			       : op == CompositeClear ? WHITENESS
> +			       : SRCCOPY; // FIXEME: other types?
> +
> +		   StretchDIBits(hdc, dstRectWin.left, dstRectWin.top,
dstRectWin.right - dstRectWin.left
> +		       , dstRectWin.bottom - dstRectWin.top, 0, 0,
srcRectWin.right, srcRectWin.bottom
> +		       , pixels, &bmpInfo, DIB_RGB_COLORS, bmpOp);

Please fix the leading commas.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:582
> +SharedBitmap::DCProvider s_dcProvider;
> +SharedBitmap::DCProvider* SharedBitmap::m_dcProvider = &s_dcProvider;

Why is the s_dcProvider needed? Why not just do m_dcProvider = new DCProvider?

The current s_dcProvider should be marked static if it needs to stay.
m_dcProvider should be named s_dcProvider.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:584
> +HDC SharedBitmap::DCProvider::getDC(SharedBitmap* bmp, unsigned* key1,
unsigned* key2)

Why do we have key2 if it's never used?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:590
> +    if (hdc) {

Early-return would be better. Is the null-check really needed?

> WebCore/platform/graphics/wince/SharedBitmap.h:44
> +    virtual ~SharedBitmap();

Why is this virtual?

> WebCore/platform/graphics/wince/SharedBitmap.h:146
> +#endif // SharedBitmap_h
>  \ No newline at end of file

Please add a newline.