[webkit-reviews] review denied: [Bug 33268] make JPEG image dcoder read segmented SharedBuffer : [Attachment 46066] fixed built error

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Yong Li
<yong.li.webkit at gmail.com>'s request for review:
Bug 33268: make JPEG image dcoder read segmented SharedBuffer
https://bugs.webkit.org/show_bug.cgi?id=33268

Attachment 46066: fixed built error
https://bugs.webkit.org/attachment.cgi?id=46066&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.