[webkit-reviews] review denied: [Bug 73118] Upstream BlackBerry porting of platform/image-decoders : [Attachment 116870] Patch 3

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Sean Wang
<xuewen.wang at torchmobile.com.cn>'s request for review:
Bug 73118: Upstream BlackBerry porting of platform/image-decoders
https://bugs.webkit.org/show_bug.cgi?id=73118

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116870&action=review


> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:135
> +    int stride = ((width * 3) + 3) & ~3;

This line is a bit complicated. The 3 in the sub-expression "width * 3" refers
to the number of color components in a stride: RGB, where as the other 3s in
this line are part of a bitwise trick to ensure that the stride is a multiple
of 2. If possible, it would be nice to emphasize the meaning for the first '3'
and/or query libimg for the number of color components for the image format
IMG_FMT_RGB888.

Nit: I suggest adding an inline comment here to explain that we ensure that the
stride is a multiple of 2. I take it there are some performance implications to
using a multiple of 2. It would be great if we could document the reason we
make the stride a multiple of 2 in such a comment.

Also, the inner parentheses aren't necessary since multiplication has a higher
precedence than addition.

One way to write this line would be:

const int ColorComponents = 3;
int stride = (ColorComponents * width + 3) & ~3; // Use a multiple of 2 bytes
to improve performance

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:155
> +	       aFrame->setRGBA(i, j, *(curPtr), *(curPtr + 1), *(curPtr + 2),
255);

Nit: I would write this as:

aFrame->setRGBA(i, j, curPtr[0], curPtr[1], curPtr[2], 255);

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:237
> +}

Missing inline comment // namespace WebCore

(this file is longer than JPEGImageDecoder.h and hence having such an inline
comment helps demarcate this curly brace as the end of namespace WebCore)

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:55
> +#endif

Nit: This is a short file. That being said, it's convention to put an inline
comment here with the name of the #ifndef, // JPEGImageDecoder_h