Bug 38157 - Implement FileReader class
Summary: Implement FileReader class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks: html5test
  Show dependency treegraph
 
Reported: 2010-04-26 17:46 PDT by Jian Li
Modified: 2011-07-16 20:43 PDT (History)
6 users (show)

See Also:


Attachments
Proposed Patch (27.06 KB, patch)
2010-04-26 17:56 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (27.06 KB, patch)
2010-04-26 18:28 PDT, Jian Li
dimich: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (27.61 KB, patch)
2010-04-30 16:09 PDT, Jian Li
abarth: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2010-04-26 17:46:14 PDT
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.
Comment 1 Jian Li 2010-04-26 17:56:23 PDT
Created attachment 54356 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2010-04-26 18:02:13 PDT
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.
Comment 3 Jian Li 2010-04-26 18:28:58 PDT
Created attachment 54368 [details]
Proposed Patch

Fixed the style errors. The complain for WebCore.vcproj seems to be a false alarm.
Comment 4 WebKit Review Bot 2010-04-26 18:34:53 PDT
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.
Comment 5 Dmitry Titov 2010-04-29 17:32:00 PDT
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.
Comment 6 Jian Li 2010-04-30 16:09:15 PDT
Created attachment 54831 [details]
Proposed Patch
Comment 7 Jian Li 2010-04-30 16:15:12 PDT
(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.
Comment 8 Adam Barth 2010-05-05 10:25:11 PDT
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?
Comment 9 Adam Barth 2010-05-05 10:25:56 PDT
BTW, what's the testing plan here?  I presume you haven't done the bindings yet and will test this code once you do.
Comment 10 Jian Li 2010-05-05 10:29:26 PDT
(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.
Comment 11 Jian Li 2010-05-05 11:56:25 PDT
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.
Comment 12 WebKit Review Bot 2010-05-05 12:32:52 PDT
http://trac.webkit.org/changeset/58832 might have broken Qt Linux ARMv5 Release
Comment 13 Adam Barth 2010-05-05 12:34:54 PDT
Looks like the bot might be sick.