[webkit-reviews] review canceled: [Bug 120828] loadend ProgressEvent of XHR can not get correct value of attribute of loaded and total : [Attachment 219647] Fixed init issue

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has canceled youenn fablet
<youennf at gmail.com>'s request for review:
Bug 120828: loadend ProgressEvent of XHR can not get correct value of attribute
of loaded and total
https://bugs.webkit.org/show_bug.cgi?id=120828

Attachment 219647: Fixed init issue
https://bugs.webkit.org/attachment.cgi?id=219647&action=review

------- Additional Comments from youenn fablet <youennf at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219647&action=review


>> Source/WebCore/ChangeLog:30
>> +	   
(WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrot
tle):
> 
> I don't quite understand changes to this file. Could you please update the
ChangeLog with some explanations?
> 
> The reason why prepare-ChangeLog generates these lists is because there is an
expectation of per-function comments added manually.

Yes, I should have made a better description of the changes.
I will improve this in a following patch.
Here is some information.

XMLHttpRequestProgressEventThrottle is currently resetting m_loaded and m_total
to zero after sending a progress event.
m_loaded/m_total are used to store the progress event values but also to know
whether to send a progress event or not.

This patch is changing this behavior as follow:
m_loaded and m_total are set to zero when a loadstart event is sent.
m_loaded and m_total are always updated within a dispatchProgressEvent call.
m_loaded and m_total are not reset after a progress event is sent.
To control whether or not a progress event should be sent, the boolean
m_hasThrottledProgressEvent is introduced.

This allows XMLHttpRequestProgressEventThrottle to correctly initialize the
loaded and total fields of any ProgressEvent event (loadend, load...).
This simplifies things in case of error (abort, timeout...) where
XMLHttpRequest.m_response may be reset before the events are initialized.
The first patch was handling this issue by storing the m_response fields before
m_response was reset.
I think Blink fixed the bug similarly to the first patch.

>> Source/WebCore/xml/XMLHttpRequest.cpp:1259
>> +PassRefPtr<Event> XMLHttpRequest::createEvent(const AtomicString& type) 
> 
> This function should return PassRefPtr<XMLHttpRequestProgressEvent>.

Will change this

>> Source/WebCore/xml/XMLHttpRequest.cpp:1272
>> +	m_progressEventThrottle.dispatchProgressEvent(lengthComputable,
m_receivedLength, total);
> 
> It looks unfortunate that this code is duplicated. Fixing this would be a
bigger task though, as XMLHttpRequestProgressEventThrottle interface is
somewhat ugly (it has both dispatchProgressEvent and dispatchEvent, but you
can't pass ProgressEvents to dispatchEvent!)

We could probably merge the two functions in a single one that would take an
event name as parameter.
That would at least remove the duplicated code.

I do not particular like either the direct use of
XMLHttpRequestProgressEventThrottle::dispatchEvent within
XMLHttpRequest::callReadyStateChangeListener().

To improve things, we could probably introduce a
XMLHttpRequestProgressEventThrottle::dispatchProgressEvent (that takes an event
name as input) to dispatch all ProgressEvent events that are not throttled
(loadstart, loaded...).
The current XMLHttpRequestProgressEventThrottle::dispatchProgressEvent could
then be renamed as "dispatchThrottledProgressEvent" or "updateProgress".
Any thought?

Also in XMLHttpRequest::callReadyStateChangeListener, the readystatechange
event is created as a ProgressEvent.
According the XHR spec, its interface should be Event. I wonder whether this
should be fixed or not.

>> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:33
>> +#include <wtf/text/AtomicString.h>
> 
> Please don't include when a forward declaration (or including wtf/Forward.h)
whould suffice.

Will fix that