[Webkit-unassigned] [Bug 33213] Make png image decoder work with segmented SharedBuffer

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45911|review?                     |review+
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2010-01-05 12:08:26 PST ---
(From update of attachment 45911)
> +    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.

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