[webkit-reviews] review denied: [Bug 47691] Support readAsArrayBuffer in FileReader and FileReaderSync : [Attachment 71496] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 47691: Support readAsArrayBuffer in FileReader and FileReaderSync
https://bugs.webkit.org/show_bug.cgi?id=47691

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71496&action=review

I may have more comments later, but there are enough issues here to sort out
first.

> WebCore/ChangeLog:34
> +	   functionalities to FileReaderSync.

Why?

> WebCore/fileapi/FileReader.cpp:99
> +

What happens if FileReader::start is called a second time (while the first call
is still proceeding)?

> WebCore/fileapi/FileReader.cpp:215
> +    if (static_cast<unsigned long long>(m_totalBytes) > 
static_cast<unsigned long long>(std::numeric_limits<unsigned>::max())) {

Why is this cast needed "static_cast<unsigned long
long>(std::numeric_limits<unsigned>::max())"?

> WebCore/fileapi/FileReader.cpp:222
> +	   m_rawData = ArrayBuffer::create(static_cast<unsigned>(m_totalBytes),
1);

static_cast<unsigned>(m_totalBytes)

(Given the number of casts of m_totalBytes) Why not make m_totalBytes unsigned?


You'll need to store the initial assignment of
response.expectedContentLength(); in another variable but after the if check,
you can store the result in m_totalBytes.

> WebCore/fileapi/FileReader.cpp:@
>  void FileReader::didReceiveData(const char* data, int lengthReceived)

It looks like a fair amount of code is duplicated between FileReader and
FilereaderSync can this be reduced?

> WebCore/fileapi/FileReader.cpp:322
> +PassRefPtr<ArrayBuffer> FileReader::arrayBufferResult() const

This should be a ArrayBuffer* since it isn't return ownership.

> WebCore/fileapi/FileReader.cpp:342
> +    // Do the coversion if not yet.

typo: coversion

> WebCore/fileapi/FileReader.h:157
> +    // We have to keep track of all the raw data for all readings other than
binary string.

s/readings/encodings/

> WebCore/fileapi/FileReaderSync.cpp:50
> +#include <wtf/RefPtr.h>

I don't think you have any test to verify that using a FileReaderSync twice in
a row (using the binary string works) because it doesn't appear to at the
moment.

It looks like the second call will get its result appended to the results from
the first call.

Please add this test.

> WebCore/fileapi/FileReaderSync.cpp:61
> +PassRefPtr<ArrayBuffer>
FileReaderSync::readAsArrayBuffer(ScriptExecutionContext*
scriptExecutionContext, Blob* blob, ExceptionCode& ec)

This shouldn't be a PassRefPtr because it isn't returning ownership.

Actually, why not return ownership?
Just do
 return m_rawData.release();

> WebCore/fileapi/FileReaderSync.cpp:100
>  {

ASSERT(!m_rawData.get());  (or set it to 0).

Actually there is a lot of state to be reset here that doesn't appear to be.

This would happen nearly automatically if FileReaderSyncLoader existed. Why did
that go away?

> WebCore/fileapi/FileReaderSync.cpp:153
> +	   m_rawData = ArrayBuffer::create(static_cast<unsigned>(totalBytes),
1);

Why not use m_rawData for all cases?

> WebCore/fileapi/FileReaderSync.cpp:188
> +	   convertToText(static_cast<const char*>(m_rawData->data()),
m_rawData->byteLength(), m_builder);

Why isn't m_rawData cleared here?

> WebCore/fileapi/FileReaderSync.cpp:191
> +	   FileReader::convertToDataURL(static_cast<const
char*>(m_rawData->data()), m_rawData->byteLength(), m_blobType, m_builder);

Why isn't m_rawData cleared here?

> WebCore/fileapi/FileReaderSync.h:89
> +    // We have to keep track of all the raw data for all readings other than
binary string.

This comment doesn't explain to me the need to keep m_rawData. After all, the
code previously supported other encodings without the need to keep around
m_rawData.