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

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

------- Additional Comments from David Levin <levin at chromium.org>
r- mostly concerned about " we choose to ignore this...", but please consider
the rest as well.


Why is "LayoutTests/fast/files/workers/worker-read-file-sync-expected.txt" a
binary file?



WebCore/ChangeLog:8
 +	    Also add FileException interface and make Blob/File/FileError be
useable in
remove "be"

WebCore/html/FileError.idl:34
 +	    NoStaticTables,
Since ordering doesn't matter, I suggesting sorting.

WebCore/html/FileException.idl:35
 +	    DontCheckEnums
Consider sorting.

WebCore/html/FileReaderSync.cpp:133
 +	default:
Please only handle the enums in the switch.

You can change the break to return and put this assert after the switch
statement.

WebCore/html/FileReaderSync.cpp:159
 +	// The File API spec says that we should use the supplied encoding if
it is valid. However, we choose to ignore this
What does Firefox do?

WebCore/html/FileReaderSync.cpp:160
 +	// requirement in order to be consistent with how WebKit decodes the
web content: always has the BOM override the
s/has/have/

WebCore/html/FileReaderSync.h:59
 +	const ScriptString& readAsText(ScriptExecutionContext*
scriptExecutionContext, Blob* blob, ExceptionCode& ec) {
{ placement.

WebCore/html/FileReaderSync.h:87
 +	// See FileReader for comment about why ScriptString is used.
Referring to a comment in another file seems like it will make this comment get
easily invalidated.

WebCore/html/FileReaderSync.idl:35
 +	    NoStaticTables,
Extra ,

WebCore/html/FileReaderSync.h:37
 +  #include "PlatformString.h"
Why do we need this header?

WebCore/html/FileReaderSync.cpp:97
 +	    FileReader::convertToDataURL(m_rawData, blob->type(), m_result);
m_rawData isn't needed after this point, right?

WebCore/html/FileReaderSync.cpp:127
 +	    m_result += String(data, static_cast<unsigned>(lengthReceived));
If this same class is called again, this will keep appending.

Please consider adding a test for this.


WebCore/html/FileReaderSync.cpp:114
 +	ThreadableLoader::loadResourceSynchronously(scriptExecutionContext,
request, *this, options);
It seems like it would be better to break out the ThreadableLoaderClient into a
separate class that could be returned from this method with all the data and
then thrown away after the data is used.

This would get rid of the issues like m_rawData staying around or things being
appended to if the method is called again.