[Webkit-unassigned] [Bug 120828] loadend ProgressEvent of XHR can not get correct value of attribute of loaded and total

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120828


youenn fablet <youennf at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #219647|review?                     |
               Flag|                            |




--- Comment #12 from youenn fablet <youennf at gmail.com>  2013-12-24 02:03:55 PST ---
(From update of attachment 219647)
View in context: https://bugs.webkit.org/attachment.cgi?id=219647&action=review

>> Source/WebCore/ChangeLog:30
>> +        (WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle):
> 
> 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.