Bug 44657 – Support FileReaderSync in workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Need to expose FileReaderSync and BlobBuilder in workers.

[Jian Li]

Created attachment 65515 [details]
Proposed Patch

[Jian Li]

Created attachment 65649 [details]
Proposed Patch

[David Levin]

Comment on attachment 65649 [details]
Proposed Patch

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.

[Jian Li]

Created attachment 65791 [details]
Proposed Patch

[Jian Li]

(In reply to comment #3)
> (From update of attachment 65649 [details])
> 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?

Because it contains some binary character. I have a test that reads the non-ascii characters in the binary file.
> 
 

> 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?

This be-consistent-with-WebKit behavior is what ap suggested last time when he reviewed my patch (https://bugs.webkit.org/show_bug.cgi?id=39131).

> 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.

Changed as suggested.

[David Levin]

Comment on attachment 65791 [details]
Proposed Patch

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.

[Jian Li]

Committed as http://trac.webkit.org/changeset/66461.

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/LayoutTests/fast/files/workers/worker-read-blob-async.html

Looks to be flaky on leopard.

[Jian Li]

(In reply to comment #8)
> http://trac.webkit.org/browser/trunk/LayoutTests/fast/files/workers/worker-read-blob-async.html
> 
> Looks to be flaky on leopard.

That's strange. I found out one failure as the following: 
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r66646%20(19656)/results.html

The difference is:

Received loadstart event (from worker-read-blob-async-expected.txt)
....ived loadstart event (from worker-read-blob-async-actual.txt)

The "." above is hexadecimal 00. In this test, we call postMessage() to post the result back from the worker to the main document for display. It seems that we receive the garbage characters occasionally.

I will talk with relevant people to see how this is possible.

[Jian Li]

*** Bug 41567 has been marked as a duplicate of this bug. ***