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

[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 116587: Patch
https://bugs.webkit.org/attachment.cgi?id=116587&action=review

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


> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:24
> +/**
> + * Implementation notes:
> + *
> + * Current implementation provides the source image size without doing a
full
> + * decode. It seems that libimg does not support partial decoding.
> + */

Please use C++-style comments.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:35
> +static bool	    s_initialized = false;

Remove the spaces between "bool" and "s_initialized". We don't need to
explicitly initialize s_initialized. s_initialized will be zero-initialized and
hence false because it is a static variable. We should also move this
declaration into libInit() since it is the only function that makes use of this
variable.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:53
> +    ImageReader(const char* data, size_t dataSize) :
> +	   m_data(data), m_size(dataSize), m_width(0), m_height(0) { libInit();
}

This should be written:

ImageReader(const char* data, size_t dataSize)
    : m_data(data)
    , m_size(dataSize)
    , m_width(0)
    , m_height(0)
{
    libInit();
}

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:59
> +    int decode(size_t width, size_t height, ImageFrame* theFrame);

The argument name "theFrame" doesn't add any additional value to this signature
that cannot be inferred from its datatype. Please remove it.

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

Remove this empty line. The "private:" and its different indentation level
sufficiently demarcates this part of the class declaration. I don't feel that
this empty line further improves the readability of this class.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:120
> +int ImageReader::decode(size_t width, size_t height, ImageFrame* theFrame)

I suggest renaming theFrame to aFrame or imageFrame or frame. The "the" in
"theFrame" implies that there is only one such ImageFrame object, which isn't
the case.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:132
> +    _uint8* buf = reinterpret_cast<_uint8*>(malloc(stride * height));

buf => buffer

Can we use OwnArrayPtr here? Then we can remove the free(buf) calls since
OwnArrayPtr will deallocate the array on destruction.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:216
> +    // Check to see if it's already decoded. TODO: Could size change

TODO => FIXME

Also, the FIXME should be written on its own line to make it easier to spot.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:37
> +    // This class decodes the JPEG image format.

This comment doesn't say anything more than what can be inferred from the name
of this class. Either improve this comment or remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:47
> +	   virtual ImageFrame* frameBufferAtIndex(size_t index);

The argument name "index" doesn't add any additional value to this signature
that cannot be inferred from the name of this method. Please remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:49
> +	   virtual void setData(SharedBuffer* data, bool allDataReceived);

The argument name "data" doesn't add any additional value to this signature
that cannot be inferred from the name of this method. Please remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:52
> +	   ImageReader* m_reader;

We should use an OwnPtr for this.