Bug 38157

Created attachment 54356 [details] Proposed Patch

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.

Created attachment 54368 [details] Proposed Patch Fixed the style errors. The complain for WebCore.vcproj seems to be a false alarm.

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

Created attachment 54831 [details] Proposed Patch

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

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

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

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.

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

Looks like the bot might be sick.