[Webkit-unassigned] [Bug 28272] WINCE PORT: graphics files only for WINCE

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28272


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67058|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #10 from Adam Roben (aroben) <aroben at apple.com>  2010-09-14 09:39:06 PST ---
(From update of attachment 67058)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.