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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted 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 71975: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=71975&action=review

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

First impression is that this is awesome.  Thanks for doing this!

Ok, I only had a few comments. Several are simple enough to just handle quickly
and commit. If any require significant changes (and you want another review on
them), then please submit without the change and create another bug for the
extra change.

> WebCore/WebCore.xcodeproj/project.pbxproj:-21238
> -			developmentRegion = English;

Why is this line deleted?

> WebCore/bindings/js/JSFileReaderCustom.cpp:50
> +    else

No else here due to return in if condition.

> WebCore/bindings/v8/custom/V8FileReaderCustom.cpp:69
> +    switch (imp->readType()) {

You didn't need to convert the switch to an if in the jsc code. You can go
either way, but it would be nice to do the same thing in the jsc bindings and
v8 bindings, so pick one and do the same in both.

> WebCore/fileapi/FileReaderLoader.cpp:111
> +    m_loader = 0;

please double check that it is ok to do to this when you get called from
didFail. (That somehow the loader doesn't try to do something after that call
and it already destroyed.)

> WebCore/fileapi/FileReaderLoader.cpp:145
> +    // Bail out if we encounetr an error.

typo: encounetr

> WebCore/fileapi/FileReaderLoader.cpp:204
> +    return m_rawData;

What does it mean to return this when m_rawData keeps changing as we get more
data (in the async case)?

Is the caller able to modify it?

Do we need to make a copy here?

> WebCore/fileapi/FileReaderLoader.cpp:219
> +    if (m_readType == ReadAsBinaryString)

switch on m_readType?

> WebCore/fileapi/FileReaderLoader.cpp:293
> +

Extra returns.

> WebCore/fileapi/FileReaderLoaderClient.h:38
> +class FileReaderLoaderClient {

Since FileReader is the only thing to implement FileReaderLoaderClient, why not
get rid of this interface and just pass in FileReader?