[Webkit-unassigned] [Bug 33268] make JPEG image dcoder read segmented SharedBuffer

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46066|review?                     |review-
               Flag|                            |




--- Comment #21 from David Levin <levin at chromium.org>  2010-02-05 10:59:53 PST ---
(From update of attachment 46066)
I gave this a quick once over since it was sitting in the queue for a while,
but it was surface level and needs a more in depth review in the future.

r- for lack of tests/no indication of how the code is tested.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Make JPEGImageDecoder read segmented SharedBuffer
> +        Also move function copyFromSharedBuffer() to SharedBuffer
> +        and rename to extract().
> +        https://bugs.webkit.org/show_bug.cgi?id=33268
> +

This makes no mention of tests. How is this tested? Do new tests need to be
added?


> +class SharedBufferDataWalker {
> +public:
> +    SharedBufferDataWalker() : m_buffer(0), m_length(0), m_data(0) {}

This isn't the style for initializing member variables (and then it would be
good to put the braces on separate lines).

> +    // Returns the current pointer

Add a period to the end of the comment.


> diff --git a/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp b/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
> + * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.

WebKit encourages uses comma delimited years instead of ranges.

> +void skip_input_data(j_decompress_ptr jd, long numBytes);

It looks like the parameter name "jd" adds no information so it should be
removed.

> +        long bytesToSkip = std::min(numBytes, (long)src->pub.bytes_in_buffer);

Use C++ casting instead of C style casting.

> +        if (numBytes > bytesToSkip)
> +            m_bytesToSkip = (size_t)(numBytes - bytesToSkip);

Use C++ casting instead of C style casting.

> +
> +            if (readOffset == lastOffset) {
> +                if (m_bufferLength == data.size())
> +                    break;
> +
> +                // Need more contiguous data to proceed

Please add periods to ends of sentences.

> +                m_info.src->bytes_in_buffer += segmentLength;
> +                m_info.src->next_input_byte = (JOCTET*)contiguousBuffer.data();

Use C++ casting instead of C style casting.

> +                unsigned totalSize = readOffset + segmentLength;
> +                m_info.src->bytes_in_buffer += totalSize - m_bufferLength;
> +                m_info.src->next_input_byte = (JOCTET*)dataWalker.data();

Use C++ casting instead of C style casting.

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