[webkit-reviews] review granted: [Bug 44657] Support FileReaderSync in workers : [Attachment 65791] 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 44657: Support FileReaderSync in workers
https://bugs.webkit.org/show_bug.cgi?id=44657

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

------- Additional Comments from David Levin <levin at chromium.org>
Please consider adding a simple test in a future patch that verifies that
FileReader can be used more than once and not keep filling the same buffer.



> diff --git a/WebCore/html/FileReaderSync.cpp
b/WebCore/html/FileReaderSync.cpp
> +void FileReaderSync::read(ScriptExecutionContext* scriptExecutionContext,
Blob* blob, ReadType readType, ExceptionCode& ec)
> +{
> +    // The blob is read by routing through the request handling layer given
the blob url.
> +    ResourceRequest request(blob->url());
> +    request.setHTTPMethod("GET");
> +
> +    FileReaderSyncLoader loader((readType == ReadAsBinaryString) ? &m_result
: 0);

Passing in m_result doesn't seem needed.  ScriptStrings are fast to copy (since
they are simply a wrapper around a RefCounted object.


> diff --git a/WebCore/html/FileReaderSync.h b/WebCore/html/FileReaderSync.h

> +#include "ThreadableLoaderClient.h"

Not needed here anymore.

> +#include <wtf/Vector.h>

Not needed here anymore.