[webkit-reviews] review granted: [Bug 61793] Update the behavior of multiple reads for FileReader : [Attachment 95523] 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 61793: Update the behavior of multiple reads for FileReader
https://bugs.webkit.org/show_bug.cgi?id=61793

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

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

Looks good. Just a few things to consider before checking in.

The one that concerns me the most is the wording for the error message
operationNotAllowedExceptionDescriptions.

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

Please fix this (and consider upgrading xcode).

> Source/WebCore/bindings/js/JSDOMBinding.cpp:36
>  #if ENABLE(BLOB) || ENABLE(FILE_SYSTEM)

fwiw, these ifdef sections should come after all other includes (but I know you
are just adding to an existing section).

> Source/WebCore/dom/ExceptionCode.cpp:218
> +    "It was not allowed to call multiple concurrent read methods on the same
object when the ready state is LOADING."

How about:
   "A read method was called while another read was in progress."
or
   "A read method was called while the object was in the LOADING state due to a
previous read call."

> Source/WebCore/fileapi/FileReader.cpp:140
> +    m_loader = adoptPtr(new FileReaderLoader(m_readType, this));

The constructor for FileReaderLoader should be private and have a static create
method but of course this is just a move of code.

> Source/WebCore/fileapi/FileReader.h:37
> +#include "ExceptionCode.h"

Out of order. (Should be after EventTarget)