[webkit-reviews] review denied: [Bug 28272] WINCE PORT: graphics files only for WINCE : [Attachment 72603] 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 72603: 1) SharedBitmap
https://bugs.webkit.org/attachment.cgi?id=72603&action=review

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

I think this is worth one more pass.


> WebCore/platform/graphics/wince/SharedBitmap.cpp:47
> +    return resultantBitmap;

This should be: return resultantBitmap.release();

> WebCore/platform/graphics/wince/SharedBitmap.cpp:57
> +    return result;

Ditto.

By using RefPtr::release you save an extra ref/deref.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:92
> +    if (m_hbitmap)
> +	   DeleteObject(m_hbitmap);

I still think it would be good to use an OwnPtr for m_hbitmap.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:109
> +	   wmemset(reinterpret_cast<wchar_t*>(m_pixels), 0xFFFF, bufferSize);

static_cast should work fine here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:114
> +    unsigned* pixel = reinterpret_cast<unsigned*>(m_pixels);

And here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:115
> +    const unsigned* const bufferEnd = pixel + bufferSize;

I think you can omit the second const here, to match standard WebKit style.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:164
> +	   m_pixelData.swap(newPixelData);

Another way to write this: m_pixelData = newPixelData.release();

I don't think one way is clearly better than the other.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:194
> +HBITMAP SharedBitmap::createHandle(void** pixels, BitmapInfo* bmpInfo, int
height, bool use16bit) const

This should return a PassOwnPtr<HBITMAP>. You can do this even if you don't
make m_hbitmap an OwnPtr.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:203
> +    HBITMAP hbmp = CreateDIBSection(0, bmpInfo, DIB_RGB_COLORS, pixels, 0,
0);

This should be an OwnPtr. Then you can return hbmp.release() at the end.

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

You should use braces around the body of the else because it is more than one
line (even though it's only a single statement).

> WebCore/platform/graphics/wince/SharedBitmap.cpp:351
> +    HBRUSH hBrush = CreatePatternBrush(hbmp);

You could use an OwnPtr for this.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:383
> +static void normalizePhase(LONG& phase, int limit)

Since this uses an out-parameter, it should have a name like
"getNormalizedPhase". But why does it use an out-parameter? A return value
seems better. That would let you shorten this function quite a bit.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:489
> +    RECT dstRectWin =
> +    {

The brace should be on the previous line.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:549
> +    } 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 {

Need braces here.

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

key1 could just be key now.

Maybe this function should return an OwnPtr<HDC>...

> WebCore/platform/graphics/wince/SharedBitmap.cpp:582
> +void SharedBitmap::DCProvider::releaseDC(SharedBitmap*, HDC hdc, unsigned
key1)

...and this function should take a PassOwnPtr<HDC>?

> WebCore/platform/graphics/wince/SharedBitmap.cpp:600
> +	   unsigned short* dst = reinterpret_cast<unsigned short*>(m_pixels);

static_cast should be fine here.

> WebCore/platform/graphics/wince/SharedBitmap.cpp:612
> +    unsigned* dst = reinterpret_cast<unsigned*>(m_pixels);

static_cast should be fine here.

> WebCore/platform/graphics/wince/SharedBitmap.h:35
> +enum CompositeOperator;

Foroward-declaring enums doesn't normally work. Is this declaration really
helping?