[webkit-reviews] review granted: [Bug 33213] Make png image decoder work with segmented SharedBuffer : [Attachment 45911] the patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 5 12:08:25 PST 2010
Darin Adler <darin at apple.com> has granted Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 33213: Make png image decoder work with segmented SharedBuffer
https://bugs.webkit.org/show_bug.cgi?id=33213
Attachment 45911: the patch
https://bugs.webkit.org/attachment.cgi?id=45911&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> + const char* contents;
> + unsigned length = data.getSomeData(contents, 0);
> +
> // We need at least 4 bytes to figure out what kind of image we're
dealing with.
> - int length = data.size();
> if (length < 4)
> return 0;
This seems a bit subtle and non-obvious. I guess the class guarantees that the
first four bytes will all be in a single segment even if they come in one byte
at a time, but nothing in the SharedBuffer header comments makes that clear.
> + virtual void setData(SharedBuffer* data, bool allDataReceived)
> + {
> + m_data = data;
> + m_isAllDataReceived = allDataReceived;
> + }
It's not optimal style to have a virtual function defined in a header. The old
code had that, but it would be better not to do it that way.
> + , m_jobComplete(false)
For booleans data members and function members it's best to name them so they
work in a sentence "image decoder <xxx>". So hasAlpha is a pretty good name but
jobComplete is not as good. Maybe hasFinishedDecoding or hasReceivedAllData?
> + if (!segmentLength) {
> + if (!m_jobComplete) {
> + if (decoder->isAllDataReceived())
> + decoder->pngComplete();
> + }
> + break;
> + }
I suggest putting the other code outside the loop. It seems we should just
break when done and then finish the ob after the loop.
I'll say r=me even though I normally don't review code in this file. Seems
fine.
More information about the webkit-reviews
mailing list