[Webkit-unassigned] [Bug 33213] Make png image decoder work with segmented SharedBuffer
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 5 12:17:37 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=33213
--- Comment #5 from Yong Li <yong.li.webkit at gmail.com> 2010-01-05 12:17:37 PST ---
(In reply to comment #4)
> (From update of attachment 45911 [details])
> > + 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.
>
You're right. To make it safe, I'll rewrite this code for the corner case, so
that we won't have a hard-to-debug problem if the assumption becomes false in
the future.
> > + 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.
Not sure if any derived class has implemented different setData(). If not, I'll
remove "virtual"
>
> > + , 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?
will use hasFinishedDecoding
>
> > + 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.
Agree, and actually the 2 "if" can be merged.
>
> I'll say r=me even though I normally don't review code in this file. Seems
> fine.
thanks a lot. but I'll post a new patch regarding to above issues.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list