Bug 38157 - Implement FileReader class
: Implement FileReader class
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 40829
  Show dependency treegraph
 
Reported: 2010-04-26 17:46 PST by
Modified: 2011-07-16 20:43 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-26 17:46:14 PST
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 From 2010-04-26 17:56:23 PST -------
Created an attachment (id=54356) [details]
Proposed Patch
------- Comment #2 From 2010-04-26 18:02:13 PST -------
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 From 2010-04-26 18:28:58 PST -------
Created an attachment (id=54368) [details]
Proposed Patch

Fixed the style errors. The complain for WebCore.vcproj seems to be a false alarm.
------- Comment #4 From 2010-04-26 18:34:53 PST -------
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 From 2010-04-29 17:32:00 PST -------
(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.

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 From 2010-04-30 16:09:15 PST -------
Created an attachment (id=54831) [details]
Proposed Patch
------- Comment #7 From 2010-04-30 16:15:12 PST -------
(In reply to comment #5)
> (From update of attachment 54368 [details] [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 From 2010-05-05 10:25:11 PST -------
(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?

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 From 2010-05-05 10:25:56 PST -------
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 From 2010-05-05 10:29:26 PST -------
(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 From 2010-05-05 11:56:25 PST -------
Changed and landed as http://trac.webkit.org/changeset/58832.

(In reply to comment #8)
> (From update of attachment 54831 [details] [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 From 2010-05-05 12:32:52 PST -------
http://trac.webkit.org/changeset/58832 might have broken Qt Linux ARMv5 Release
------- Comment #13 From 2010-05-05 12:34:54 PST -------
Looks like the bot might be sick.