[Webkit-unassigned] [Bug 73118] Upstream BlackBerry porting of platform/image-decoders

[bugzilla-daemon at webkit.org]

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


Daniel Bates <dbates at webkit.org> changed:

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




--- Comment #2 from Daniel Bates <dbates at webkit.org>  2011-11-26 11:03:12 PST ---
(From update of attachment 116587)
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.

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