Bug 38157 – Implement FileReader class

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

This bug only covers the implementation of FileReader class as defined in the File API (http://www.w3.org/TR/file-upload/#dfn-filereader). The exposing of IDL interface and adding JSC bindings plus layout test will be in a separate bug.

[Jian Li]

Created attachment 54356 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 54356 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/FileStream.cpp:152:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/FileStream.cpp:155:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/WebCore.vcproj/WebCore.vcproj:30378:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 10 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jian Li]

Created attachment 54368 [details]
Proposed Patch

Fixed the style errors. The complain for WebCore.vcproj seems to be a false alarm.

[WebKit Review Bot]

Attachment 54368 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/WebCore.vcproj/WebCore.vcproj:30378:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Titov]

Comment on attachment 54368 [details]
Proposed Patch

WebCore/html/FileReader.h:54
 +  class FileReader : public RefCounted<FileReader>, public EventTarget, public FileStreamClient {
It seems the Filereader should be derived from ActiveDOMObject as well, just like XMLHttpRequest. The ActiveDOMObject allows JS wrapper with attached event handlers to avoid GC and fire even though the FileReader object is out of JS scope and so its wrapper could be GC'ed (and the object itself destructed). ActiveDOMObjects prevent themselves from being collected by having 'pending activity'. Also, they can be suspended (if page goes into cache for example), and their stop() method could in help with termination. Currently, you stop the loading when the object is destructed, which can happen very long after the page using it is closed - because the lifetime of it depends when JS will do garbage collection.

WebCore/html/FileReader.h:107
 +      static const unsigned bufferSize;
This constant doesn't benefit .h file, it should be simply a static in the cpp file.

WebCore/html/FileReader.h:108
 +      static const double progressNotificationIntervalMS;
Same here.


WebCore/html/FileReader.h:152
 +      long long m_total;
m_total - probably could be a better name. Total what?

WebCore/html/FileReader.cpp:101
 +      if (m_alreadyStarted)
I am not sure this is what spec means by "already started". I think spec refers to the script block being "already started" which is not the same thing as checking if the FileStreamProxy fired didStart().


WebCore/html/FileReader.cpp:110
 +          m_streamProxy = FileStreamProxy::create(m_context.get(), this);
If a script  calls readAs* multiple times, we will can create multiple proxies that will all call didStart() on this object once JS exits. This doesn't look right.

[Jian Li]

Created attachment 54831 [details]
Proposed Patch

[Jian Li]

(In reply to comment #5)
> (From update of attachment 54368 [details])
> WebCore/html/FileReader.h:54
>  +  class FileReader : public RefCounted<FileReader>, public EventTarget,
> public FileStreamClient {
> It seems the Filereader should be derived from ActiveDOMObject as well, just
> like XMLHttpRequest. The ActiveDOMObject allows JS wrapper with attached event
> handlers to avoid GC and fire even though the FileReader object is out of JS
> scope and so its wrapper could be GC'ed (and the object itself destructed).
> ActiveDOMObjects prevent themselves from being collected by having 'pending
> activity'. Also, they can be suspended (if page goes into cache for example),
> and their stop() method could in help with termination. Currently, you stop the
> loading when the object is destructed, which can happen very long after the
> page using it is closed - because the lifetime of it depends when JS will do
> garbage collection.
Fixed.
> 
> WebCore/html/FileReader.h:107
>  +      static const unsigned bufferSize;
> This constant doesn't benefit .h file, it should be simply a static in the cpp
> file.
Fixed.
> 
> WebCore/html/FileReader.h:108
>  +      static const double progressNotificationIntervalMS;
> Same here.
Fixed.
> 
> 
> WebCore/html/FileReader.h:152
>  +      long long m_total;
> m_total - probably could be a better name. Total what?
Fixed.
> 
> WebCore/html/FileReader.cpp:101
>  +      if (m_alreadyStarted)
> I am not sure this is what spec means by "already started". I think spec refers
> to the script block being "already started" which is not the same thing as
> checking if the FileStreamProxy fired didStart().
This is similar to what spec means by "already started". When multiple read methods are called in a script block, m_alreadyStarted is still false and thus readInternal is called multiple times where blob and type are set to the latest value while proxy is only created once. When the script block execution is done, the file thread will get a chance to call didStart where m_alreadyStarted is set to false to make future calling of read methods invalid.
> 
> 
> WebCore/html/FileReader.cpp:110
>  +          m_streamProxy = FileStreamProxy::create(m_context.get(), this);
> If a script  calls readAs* multiple times, we will can create multiple proxies
> that will all call didStart() on this object once JS exits. This doesn't look
> right.
We will not create multiple proxies because we check if the proxy has already been created or not by the line just before that.

[Adam Barth]

Comment on attachment 54831 [details]
Proposed Patch

This looks great.  Please address the comments below before landing.

WebCore/html/FileReader.cpp:173
 +          m_result += String(data, static_cast<unsigned>(bytesRead));
Is this slow?  Should use use a StringBuilder or a SegmentedString instead?

WebCore/html/FileReader.cpp:256
 +          if (m_rawData.size() >= 2 && m_rawData[0] == '\xFE' && m_rawData[1] == '\xFF') {
Yuck.  Is this in the spec?  We should move this to a more generic location.

WebCore/html/FileReader.cpp:270
 +      // FIXME: consider upporting incremental decoding to improve the perf.
supporting

WebCore/html/FileReader.cpp:284
 +      m_result += "base64,";
Is "data:base64,..." a valid data URL?

[Adam Barth]

BTW, what's the testing plan here?  I presume you haven't done the bindings yet and will test this code once you do.

[Jian Li]

(In reply to comment #9)
> BTW, what's the testing plan here?  I presume you haven't done the bindings yet
> and will test this code once you do.

I already get the layout test written and will include it in the patch that turns on FileReader support.

[Jian Li]

Changed and landed as http://trac.webkit.org/changeset/58832.

(In reply to comment #8)
> (From update of attachment 54831 [details])
> This looks great.  Please address the comments below before landing.
> 
> WebCore/html/FileReader.cpp:173
>  +          m_result += String(data, static_cast<unsigned>(bytesRead));
> Is this slow?  Should use use a StringBuilder or a SegmentedString instead?
No, this is what we intend to do. We mimic the usage of ScriptString in XMLHTTPRequest. Please see my comment in FileReader.h.
> 
> WebCore/html/FileReader.cpp:256
>  +          if (m_rawData.size() >= 2 && m_rawData[0] == '\xFE' && m_rawData[1]
> == '\xFF') {
> Yuck.  Is this in the spec?  We should move this to a more generic location.
I added a FIXME for this. Will definitely fix this in my next patch.
> 
> WebCore/html/FileReader.cpp:270
>  +      // FIXME: consider upporting incremental decoding to improve the perf.
> supporting
Fixed.
> 
> WebCore/html/FileReader.cpp:284
>  +      m_result += "base64,";
> Is "data:base64,..." a valid data URL?
Yes, this is valid and tested.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58832 might have broken Qt Linux ARMv5 Release

[Adam Barth]

Looks like the bot might be sick.