[webkit-reviews] review granted: [Bug 28272] WINCE PORT: graphics files only for WINCE : [Attachment 67058] 2) ImageWinCE.cpp

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

Attachment 67058: 2) ImageWinCE.cpp
https://bugs.webkit.org/attachment.cgi?id=67058&action=review

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

> WebCore/platform/graphics/wince/ImageWinCE.cpp:46
> +    NativeImagePtr ret = SharedBitmap::createInstance(false, width(),
height(), false);
A non-abbreviated variable name (like "image") would be better.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:100
> +	   if (bmp && bmp->height() == (size_t)srcSize.height() && bmp->width()
== (size_t)srcSize.width()) {
I suggest using an early-continue and static_cast/

> WebCore/platform/graphics/wince/ImageWinCE.cpp:125
> +    RefPtr<SharedBitmap> bmp = frameAtIndex(m_currentFrame);
> +    if (mayFillWithSolidColor())
> +	   fillWithSolidColor(ctxt, dstRect, solidColor(), styleColorSpace,
compositeOp);
> +    else {
> +	   IntRect intSrcRect(srcRectIn);
> +
> +	   if (bmp->width() != m_source.size().width()) {
It looks like bmp is only used in the else case. Can it be declared inside the
else?

> WebCore/platform/graphics/wince/ImageWinCE.cpp:126
> +	       double rate = (double)bmp->width() / m_source.size().width();
static_cast would be better.

"scaleFactor" seems like a better name than "rate".

> WebCore/platform/graphics/wince/ImageWinCE.cpp:129
> +	       int temp = stableRound(srcRectIn.right() * rate);
> +	       intSrcRect.setX(stableRound(srcRectIn.x() * rate));
> +	       intSrcRect.setWidth(temp - intSrcRect.x());
Why is temp needed? Can't you do this instead?

intSrcRect.setWidth(stableRound(srcRectIn.width() * rate));

> WebCore/platform/graphics/wince/ImageWinCE.cpp:132
> +	       temp = stableRound(srcRectIn.bottom() * rate);
> +	       intSrcRect.setY(stableRound(srcRectIn.y() * rate));
> +	       intSrcRect.setHeight(temp - intSrcRect.y());
Same question here.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:151
> +    RefPtr<SharedBitmap> bmp = nativeImageForCurrentFrame();
> +    if (bmp)
> +	   bmp->drawPattern(ctxt, tileRectIn, patternTransform, phase,
styleColorSpace, op, destRect, m_source.size());
An early return would be nicer here.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:171
> +    if (bmp->width() == 1 && bmp->validHeight() == 1) {
I think this would be cleaner if you reversed the condition and used an early
return.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:175
> +	       unsigned short c = ((unsigned short *)bmp->bytes())[0];
reinterpret_cast would be better.

> WebCore/platform/graphics/wince/ImageWinCE.cpp:184
> +	       unsigned c = ((unsigned *)bmp->bytes())[0];
reinterpret_cast would be better.

The code looks fine, but you should consider these comments.