Created attachment 45903 [details] the patch See bug 33178
Comment on attachment 45903 [details] the patch oops. forgot to fix a bug
Created attachment 45911 [details] the patch
style-queue ran check-webkit-style on attachment 45911 [details] without any errors.
Comment on attachment 45911 [details] the patch > + 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.
(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.
Created attachment 45927 [details] fixed some issues
style-queue ran check-webkit-style on attachment 45927 [details] without any errors.
Comment on attachment 45927 [details] fixed some issues > + // XBMs require 8 bytes of info. > + static const unsigned maxMarkerLength = 8; > + > + char contents[maxMarkerLength]; > + unsigned length = 0; > + for (;;) { > + const char* moreContents; > + unsigned moreContentsLength = data.getSomeData(moreContents, 0); > + if (!moreContentsLength) > + break; > + unsigned bytesToCopy = min(maxMarkerLength - length, moreContentsLength); > + memcpy(contents + length, moreContents, bytesToCopy); > + length += bytesToCopy; > + if (length == maxMarkerLength) > + break; > + } This cries out for a helper function. SharedBuffer could provide a function to that has this logic in it rather than requiring callers to repeatedly call getSomeData. Or at least put this in a separate function in this file. Seems OK. r=me
(In reply to comment #8) > (From update of attachment 45927 [details]) > > + // XBMs require 8 bytes of info. > > + static const unsigned maxMarkerLength = 8; > > + > > + char contents[maxMarkerLength]; > > + unsigned length = 0; > > + for (;;) { > > + const char* moreContents; > > + unsigned moreContentsLength = data.getSomeData(moreContents, 0); > > + if (!moreContentsLength) > > + break; > > + unsigned bytesToCopy = min(maxMarkerLength - length, moreContentsLength); > > + memcpy(contents + length, moreContents, bytesToCopy); > > + length += bytesToCopy; > > + if (length == maxMarkerLength) > > + break; > > + } > > This cries out for a helper function. SharedBuffer could provide a function to > that has this logic in it rather than requiring callers to repeatedly call > getSomeData. Or at least put this in a separate function in this file. > > Seems OK. r=me I'm putting it into a new function with a bug fixed too: getSomeData() should be called with increasing offset but not 0. Would you like to review the new patch? Or I can commit it now?
Comment on attachment 45927 [details] fixed some issues landed @ 52831
bug closed
Darin, there's a bug in yesterday's commit for png decoder: I move "if (!m_hasFinishedDecoding && decoder->isAllDataReceived())" out of the loop, but it hurts another "break". while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) { m_readOffset += segmentLength; m_currentBufferSize = m_readOffset; png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength); if ((sizeOnly && decoder->isSizeAvailable()) || m_hasFinishedDecoding) break; } if (!m_hasFinishedDecoding && decoder->isAllDataReceived()) decoder->pngComplete(); When "sizeonly" is true, decoder->pngComplete() shouldn't be called. Should I post a patch in this bug, or raise a new bug?
Patch in this bug is fine. r=me in advance on adding sizeOnly to the if condition, so you don't need to post a patch at all if that's what you do.
Darin, there's a bug in yesterday's commit for png decoder: I move "if (!m_hasFinishedDecoding && decoder->isAllDataReceived())" out of the loop, but it hurts another "break". while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) { m_readOffset += segmentLength; m_currentBufferSize = m_readOffset; png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength); if ((sizeOnly && decoder->isSizeAvailable()) || m_hasFinishedDecoding) break; } if (!m_hasFinishedDecoding && decoder->isAllDataReceived()) decoder->pngComplete(); When "sizeonly" is true, decoder->pngComplete() shouldn't be called. Should I post a patch in this bug, or raise a new bug?(In reply to comment #13) > Patch in this bug is fine. r=me in advance on adding sizeOnly to the if > condition, so you don't need to post a patch at all if that's what you do. Peter Kasting already raised a bug for it. bug 33258