When we use XHR like below, we can not get correct value. client = new XMLHttpRequest(); client.open("GET", "data.php"); ... client.onloadend = function(pe) { console.log("onloadend: total:"+pe.total+",loaded="+pe.loaded); } client.send(); In the sample as the attachment, we expect "onloadend: total:1898,loaded=1898", but we get "onloadend: total:0,loaded=0". Firefox(23.0.1) can get correct value.
Created attachment 210704 [details] sample
Created attachment 217991 [details] Initial fix ”Probably
Probably need to fix upload events in the same way. I will check this and fix it as a separate patch if needed
Comment on attachment 217991 [details] Initial fix Attachment 217991 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/39278004 New failing tests: fast/xmlhttprequest/xmlhttprequest-get.xhtml
Created attachment 217996 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 217991 [details] Initial fix Attachment 217991 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/39018092 New failing tests: fast/xmlhttprequest/xmlhttprequest-get.xhtml
Created attachment 218000 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 218001 [details] Added load event test rebaseline
Created attachment 218071 [details] Variant with handling of upload events Fixed upload ProgressEvent events. Made consistent the handling of XMLHttpRequestProgressEventthrottle and XMLHttpRequestUpload events.
Created attachment 219647 [details] Fixed init issue
Comment on attachment 219647 [details] Fixed init issue 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. > Source/WebCore/xml/XMLHttpRequest.cpp:1259 > +PassRefPtr<Event> XMLHttpRequest::createEvent(const AtomicString& type) This function should return PassRefPtr<XMLHttpRequestProgressEvent>. > Source/WebCore/xml/XMLHttpRequest.cpp:1272 > + long long expectedLength = m_response.expectedContentLength(); > + bool lengthComputable = expectedLength > 0 && m_receivedLength <= expectedLength; > + unsigned long long total = lengthComputable ? expectedLength : 0; > + 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!) > 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.
Comment on attachment 219647 [details] Fixed init issue 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
> 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? It would be helpful to make this code look better, but I think that this needn't block landing the fix. > 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. I don't care very much. What do other browsers do?
Created attachment 220431 [details] Updated according review
(In reply to comment #13) > > 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. > > I don't care very much. What do other browsers do? Blink is similar to WebKit. Mozilla is directly instantiating a DOM event. The change is included in the current patch.
Created attachment 220526 [details] Code refactoring refactored code to ease full alignment with XHR spec (see bugs 126574 and 126575)
Comment on attachment 220526 [details] Code refactoring Clearing flags on attachment: 220526 Committed r161532: <http://trac.webkit.org/changeset/161532>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 126677
This made all XMLHttpRequest tests crash with assertion failures, rolling out: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r161538%20(15178)/results.html>.
Something is wrong with this bug, it refuses to re-open!
I guess we'll need a new bug to re-land this once fixed.
(In reply to comment #22) > I guess we'll need a new bug to re-land this once fixed. I opened bug 126681 for that purpose.