[webkit-reviews] review denied: [Bug 110556] Update FileReaderLoader to allow specifying a range and reading as a blob. : [Attachment 189681] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Zachary Kuznia
<zork at chromium.org>'s request for review:
Bug 110556: Update FileReaderLoader to allow specifying a range and reading as
a blob.
https://bugs.webkit.org/show_bug.cgi?id=110556

Attachment 189681: Patch
https://bugs.webkit.org/attachment.cgi?id=189681&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
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?