Update FileReaderLoader to allow specifying a range and reading as a blob.
Created attachment 189681 [details] Patch
This is to allow support of the Stream API. The complete version is at: https://bugs.webkit.org/show_bug.cgi?id=110194
Comment on attachment 189681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189681&action=review > Source/WebCore/ChangeLog:3 > + Update FileReaderLoader to allow specifying a range and reading as a blob. If this is a part of big change, it is worth mentioning here. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). Say something about this or just remove this line. The repo reject this patch otherwise. > Source/WebCore/fileapi/FileReaderLoader.cpp:57 > +const int kDefaultBufferLength = 32768; WebKit doesn't prefix "k" for constants. Usually it names each global variable like a local variable, but with more verbose name. > Source/WebCore/fileapi/FileReaderLoader.cpp:95 > + request.setHTTPHeaderField("Range", String("bytes=") + String::number(m_rangeStart) + ("-") + String::number(m_rangeEnd)); There is printf-like method called String::format() which you could use. See Source/WTF/wtf/text/WTFString.h. > Source/WebCore/fileapi/FileReaderLoader.cpp:147 > if (length > numeric_limits<unsigned>::max()) { Does this means that we're going to allocate numeric_limits<unsigned>::max() bytes of the array before switching to variable-length approach? > Source/WebCore/fileapi/FileReaderLoader.cpp:285 > +} Looks like wre're going to create new Blob instance and copy the data into it for each blobResult() call, which seems wasteful. Is this intentional?
Created attachment 189690 [details] Patch
(In reply to comment #3) > (From update of attachment 189681 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189681&action=review > > > Source/WebCore/ChangeLog:3 > > + Update FileReaderLoader to allow specifying a range and reading as a blob. > > If this is a part of big change, it is worth mentioning here. > > > Source/WebCore/ChangeLog:8 > > + No new tests (OOPS!). > > Say something about this or just remove this line. The repo reject this patch otherwise. Done. > > > Source/WebCore/fileapi/FileReaderLoader.cpp:57 > > +const int kDefaultBufferLength = 32768; > > WebKit doesn't prefix "k" for constants. > Usually it names each global variable like a local variable, but with more verbose name. Changed to defaultBufferLength > > > Source/WebCore/fileapi/FileReaderLoader.cpp:95 > > + request.setHTTPHeaderField("Range", String("bytes=") + String::number(m_rangeStart) + ("-") + String::number(m_rangeEnd)); > > There is printf-like method called String::format() which you could use. See Source/WTF/wtf/text/WTFString.h. Done. > > > Source/WebCore/fileapi/FileReaderLoader.cpp:147 > > if (length > numeric_limits<unsigned>::max()) { > > Does this means that we're going to allocate numeric_limits<unsigned>::max() bytes of the array before switching to variable-length approach? No. The variable length method is used if a content length header isn't received. The internal buffer can't be larger than numeric_limits<unsigned>::max(), and so the read fails if that is exceeded. > > > Source/WebCore/fileapi/FileReaderLoader.cpp:285 > > +} > > Looks like wre're going to create new Blob instance and copy the data into it for each blobResult() call, which seems wasteful. > Is this intentional? Changed it to cache the value.
Comment on attachment 189690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189690&action=review > Source/WebCore/ChangeLog:7 > + https://bugs.webkit.org/show_bug.cgi?id=110194 It's better to explicitly say you'll add tests. > Source/WebCore/fileapi/FileReaderLoader.cpp:-141 > - return; (As we chatted offline) when length > unsigned max we'not returning here but may fail at line 183. This feels weird without the comment why we're triggering 'variableLength' mode. If we have some comments and/or compare the length to the special value when we should trigger 'variable' mode that'd be more understandable.
Created attachment 189695 [details] Patch
(In reply to comment #6) > (From update of attachment 189690 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189690&action=review > > > Source/WebCore/ChangeLog:7 > > + https://bugs.webkit.org/show_bug.cgi?id=110194 > > It's better to explicitly say you'll add tests. Done. > > > Source/WebCore/fileapi/FileReaderLoader.cpp:-141 > > - return; > > (As we chatted offline) when length > unsigned max we'not returning here but may fail at line 183. This feels weird without the comment why we're triggering 'variableLength' mode. If we have some comments and/or compare the length to the special value when we should trigger 'variable' mode that'd be more understandable. Done.
Comment on attachment 189695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189695&action=review > Source/WebCore/fileapi/FileReaderLoader.cpp:189 > + if (m_variableLength >= numeric_limits<unsigned>::max()) { This looks wrong?
Created attachment 189697 [details] Patch
(In reply to comment #9) > (From update of attachment 189695 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189695&action=review > > > Source/WebCore/fileapi/FileReaderLoader.cpp:189 > > + if (m_variableLength >= numeric_limits<unsigned>::max()) { > > This looks wrong? Fixed.
Comment on attachment 189697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189697&action=review > Source/WebCore/fileapi/FileReaderLoader.cpp:224 > + memcpy(static_cast<char*>(newData->data()), static_cast<char*>(m_rawData->data()), m_bytesLoaded); Can we use m_rawData->slice() instead to create newData? > Source/WebCore/fileapi/FileReaderLoader.cpp:293 > + blobData->appendData(rawData, 0, BlobDataItem::toEndOfFile); toEndOfFile -> size? (Or is it intentional?)
Created attachment 189702 [details] Patch
(In reply to comment #12) > (From update of attachment 189697 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189697&action=review > > > Source/WebCore/fileapi/FileReaderLoader.cpp:224 > > + memcpy(static_cast<char*>(newData->data()), static_cast<char*>(m_rawData->data()), m_bytesLoaded); Done. > > Can we use m_rawData->slice() instead to create newData? > > > Source/WebCore/fileapi/FileReaderLoader.cpp:293 > > + blobData->appendData(rawData, 0, BlobDataItem::toEndOfFile); > > toEndOfFile -> size? (Or is it intentional?) Done.
Comment on attachment 189702 [details] Patch This looks good/reasonable to me.
Comment on attachment 189702 [details] Patch Let me stamp this then.
Comment on attachment 189702 [details] Patch Rejecting attachment 189702 [details] from commit-queue. zork@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 189702 [details] Patch Oops sorry for that. I should've set the cq+.
Comment on attachment 189702 [details] Patch Clearing flags on attachment: 189702 Committed r143706: <http://trac.webkit.org/changeset/143706>
All reviewed patches have been landed. Closing bug.
(In reply to comment #20) > All reviewed patches have been landed. Closing bug. I landed a 32 bit build fix in http://trac.webkit.org/changeset/143733.
> This is to allow support of the Stream API. This adds a ton of code. Why is it not behind an ENABLE ifdef?
(In reply to comment #22) > > This is to allow support of the Stream API. > > This adds a ton of code. Why is it not behind an ENABLE ifdef? Good point. Which flag should we use though. Is this specific for the stream API? Or is this applicable for non-Stream use?
(In reply to comment #23) > (In reply to comment #22) > > > This is to allow support of the Stream API. > > > > > This adds a ton of code. Why is it not behind an ENABLE ifdef? > Good point. Which flag should we use though. > Is this specific for the stream API? Or is this applicable for non-Stream use? I'm putting the Stream code behind the ENABLE_STREAM flag. I deferred the changes to this class that are Stream specific, and will have them protected by the STREAM flag. Would you like me to wrap setRange() and blobResult() with the flag as well?
> I'm putting the Stream code behind the ENABLE_STREAM flag. I deferred the changes to this class that are Stream specific, and will have them protected by the STREAM flag. > > Would you like me to wrap setRange() and blobResult() with the flag as well? Let's do so. It will clarify that the API is used from the Stream implementation.