[webkit-reviews] review denied: [Bug 38157] Implement FileReader class : [Attachment 54368] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 38157: Implement FileReader class
https://bugs.webkit.org/show_bug.cgi?id=38157

Attachment 54368: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=54368&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
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.