Improve fidelity of XHR progress events.

Improve fidelity of XHR progress events.

When dispatching XMLHttpRequest progress events, for upload as well as
normal/download, supply the required 'loaded' and 'total' properties
on the events, if available, for better reporting to scripts.

Previously just left as 0. The new behavior mirrors what other
implementations are doing.

R=tyoshino,abarth
BUG=318727
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162689

[sof, 2013-11-21 13:05:41]

At your leisure, please have a look.

(If there are other core/ owners which find XHR extra enjoyable, please let me
know & I'll loop them in on any future reviews in the area.)

[abarth-chromium, 2013-11-21 21:59:16]

@tyoshino: Would you like to review this CL?

[tyoshino (SeeGerritForStatus), 2013-11-22 04:48:21]

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:858: 
comment that we save these variables here since they're going to be cleared (by
internalAbort() and clearResponse()).

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:860: unsigned long long receivedLength =
m_receivedLength;
i think we can change m_receivedLength to unsigned long long, but for now,
please use "long long" for receivedLength which is the same type as
m_receivedLength.

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:964: void
XMLHttpRequest::dispatchEventAndLoadEnd(const AtomicString& type, long long
receivedLength, unsigned long long expectedLength)
long long expectedLength

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:971: unsigned long long loaded =
receivedLength >= 0 ? static_cast<unsigned long long>(receivedLength) : 0;
nice. please add this code also between L979 and 980 please

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:993: 
comment the reason why we save them here

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:995: unsigned long long receivedLength =
m_receivedLength;
long long

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:1005: 
comment the reason why we save them here

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:1007: unsigned long long receivedLength =
m_receivedLength;
long long

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:1013: void
XMLHttpRequest::handleRequestError(ExceptionCode exceptionCode, const
AtomicString& type, long long receivedLength, unsigned long long expectedLength)
long long expectedLength

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpRequest.h
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.h:191: void dispatchThrottledProgressEvent(const
AtomicString&);
comment that
- this method builds a ProgressEvent from m_receivedLength and m_response
- dispatches only to m_progressEventThrottle

[sof, 2013-11-22 07:17:05]

Many thanks; handling of m_receivedLength is now hopefully tidier.

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:858: 
On 2013/11/22 04:48:21, tyoshino wrote:
> comment that we save these variables here since they're going to be cleared
(by
> internalAbort() and clearResponse()).

Done.

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:860: unsigned long long receivedLength =
m_receivedLength;
On 2013/11/22 04:48:21, tyoshino wrote:
> i think we can change m_receivedLength to unsigned long long, but for now,
> please use "long long" for receivedLength which is the same type as
> m_receivedLength.

Done; but I'd vote for moving to unsigned long long for m_receivedLength before
too long.

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:971: unsigned long long loaded =
receivedLength >= 0 ? static_cast<unsigned long long>(receivedLength) : 0;
On 2013/11/22 04:48:21, tyoshino wrote:
> nice. please add this code also between L979 and 980 please

Done.

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpRequest.h
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/79953004/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.h:191: void dispatchThrottledProgressEvent(const
AtomicString&);
On 2013/11/22 04:48:21, tyoshino wrote:
> comment that
> - this method builds a ProgressEvent from m_receivedLength and m_response
> - dispatches only to m_progressEventThrottle

Done.

[tyoshino (SeeGerritForStatus), 2013-11-22 07:39:22]

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:965: void
XMLHttpRequest::dispatchEventAndLoadEnd(const AtomicString& type, long long
receivedLength, long long expectedLength)
oh, this method was not used by anyone. we've made any mistake in recent CL?
anyway, let's remove L967-971. it's unnecessary since the only caller
XMLHttpRequest::handleRequestError() has L1036-1040.

please update the comment in .h file accordingly.

[tyoshino (SeeGerritForStatus), 2013-11-22 07:43:14]

On 2013/11/22 07:39:22, tyoshino wrote:
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> Source/core/xml/XMLHttpRequest.cpp:965: void
> XMLHttpRequest::dispatchEventAndLoadEnd(const AtomicString& type, long long
> receivedLength, long long expectedLength)
> oh, this method was not used by anyone. we've made any mistake in recent CL?
> anyway, let's remove L967-971. it's unnecessary since the only caller
> XMLHttpRequest::handleRequestError() has L1036-1040.
> 
> please update the comment in .h file accordingly.

We've stopped using it accidentally in
https://codereview.chromium.org/66323004/diff/140002/Source/core/xml/XMLHttpR...

Yeah, that's fine. Let's reuse it :)

[sof, 2013-11-22 07:49:13]

On 2013/11/22 07:43:14, tyoshino wrote:
> On 2013/11/22 07:39:22, tyoshino wrote:
> >
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> > File Source/core/xml/XMLHttpRequest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> > Source/core/xml/XMLHttpRequest.cpp:965: void
> > XMLHttpRequest::dispatchEventAndLoadEnd(const AtomicString& type, long long
> > receivedLength, long long expectedLength)
> > oh, this method was not used by anyone. we've made any mistake in recent CL?
> > anyway, let's remove L967-971. it's unnecessary since the only caller
> > XMLHttpRequest::handleRequestError() has L1036-1040.
> > 
> > please update the comment in .h file accordingly.
> 
> We've stopped using it accidentally in
>
https://codereview.chromium.org/66323004/diff/140002/Source/core/xml/XMLHttpR...
> 
> Yeah, that's fine. Let's reuse it :)

Right, handleRequestError() could have called upon it and/but does now. So we're
ok as-is?

[tyoshino (SeeGerritForStatus), 2013-11-22 09:26:54]

On 2013/11/22 07:49:13, sof wrote:
> On 2013/11/22 07:43:14, tyoshino wrote:
> > On 2013/11/22 07:39:22, tyoshino wrote:
> > >
> >
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> > > File Source/core/xml/XMLHttpRequest.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
> > > Source/core/xml/XMLHttpRequest.cpp:965: void
> > > XMLHttpRequest::dispatchEventAndLoadEnd(const AtomicString& type, long
long
> > > receivedLength, long long expectedLength)
> > > oh, this method was not used by anyone. we've made any mistake in recent
CL?
> > > anyway, let's remove L967-971. it's unnecessary since the only caller
> > > XMLHttpRequest::handleRequestError() has L1036-1040.
> > > 
> > > please update the comment in .h file accordingly.
> > 
> > We've stopped using it accidentally in
> >
>
https://codereview.chromium.org/66323004/diff/140002/Source/core/xml/XMLHttpR...
> > 
> > Yeah, that's fine. Let's reuse it :)
> 
> Right, handleRequestError() could have called upon it and/but does now. So
we're
> ok as-is?

We can reuse it but if so, L967-971 are redundant I think.

[tyoshino (SeeGerritForStatus), 2013-11-22 09:48:32]

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1042:
dispatchThrottledProgressEvent(EventTypeNames::progress);
this method should also take saved receivedLength and expectedLength.

[sof, 2013-11-22 11:22:42]

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/80001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1042:
dispatchThrottledProgressEvent(EventTypeNames::progress);
On 2013/11/22 09:48:32, tyoshino wrote:
> this method should also take saved receivedLength and expectedLength.

Done.

[tyoshino (SeeGerritForStatus), 2013-11-25 05:06:31]

i think it's almost done.

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:6: *  Copyright (C) 2012 Intel Corporation
abarth: sigbjornf has made substantial amount of changes to this file. do you
think it makes sense to add Opera to the copyright list?

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:974: void
XMLHttpRequest::dispatchThrottledProgressEvent(const AtomicString& type, long
long receivedLength, long long expectedLength)
how about having two methods one takes two arguments, the other uses the member
variables. say... dispatchThrottledProgressEvent and
dispatchThrottledProgressEventUsingSpecifiedValues... the former calls the
latter with m_receivedLength and m_response.expectedContentLength().

it's possible the caller passes 0 to receivedLength and expectedLength. to make
this work correctly for such a case, we need to ensure that m_receivedLength and
m_response.expectedContentLength are set to 0. they're reset where this method
is invoked so, there should be no problem, but it adds time for reviewers to
verify the code.

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:188: // m_progressEventThrottle.
now, this method doesn't dispatch events to m_upload. please update the comment.

[sof, 2013-11-25 06:48:12]

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:974: void
XMLHttpRequest::dispatchThrottledProgressEvent(const AtomicString& type, long
long receivedLength, long long expectedLength)
On 2013/11/25 05:06:31, tyoshino wrote:
> how about having two methods one takes two arguments, the other uses the
member
> variables. say... dispatchThrottledProgressEvent and
> dispatchThrottledProgressEventUsingSpecifiedValues... the former calls the
> latter with m_receivedLength and m_response.expectedContentLength().
> 
> it's possible the caller passes 0 to receivedLength and expectedLength. to
make
> this work correctly for such a case, we need to ensure that m_receivedLength
and
> m_response.expectedContentLength are set to 0. they're reset where this method
> is invoked so, there should be no problem, but it adds time for reviewers to
> verify the code.

Yes, how about dispatchThrottledProgressEventSnapshot() for the one that
defaults to m_receivedLength + m_response values?

[tyoshino (SeeGerritForStatus), 2013-11-25 07:36:18]

On 2013/11/25 06:48:12, sof wrote:
>
https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
> Source/core/xml/XMLHttpRequest.cpp:974: void
> XMLHttpRequest::dispatchThrottledProgressEvent(const AtomicString& type, long
> long receivedLength, long long expectedLength)
> On 2013/11/25 05:06:31, tyoshino wrote:
> > how about having two methods one takes two arguments, the other uses the
> member
> > variables. say... dispatchThrottledProgressEvent and
> > dispatchThrottledProgressEventUsingSpecifiedValues... the former calls the
> > latter with m_receivedLength and m_response.expectedContentLength().
> > 
> > it's possible the caller passes 0 to receivedLength and expectedLength. to
> make
> > this work correctly for such a case, we need to ensure that m_receivedLength
> and
> > m_response.expectedContentLength are set to 0. they're reset where this
method
> > is invoked so, there should be no problem, but it adds time for reviewers to
> > verify the code.
> 
> Yes, how about dispatchThrottledProgressEventSnapshot() for the one that
> defaults to m_receivedLength + m_response values?

Sounds good.

[sof, 2013-11-25 07:40:03]

Now hopefully addressed. Thanks again :)

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/79953004/diff/170001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.h:188: // m_progressEventThrottle.
On 2013/11/25 05:06:31, tyoshino wrote:
> now, this method doesn't dispatch events to m_upload. please update the
comment.

Done.

[tyoshino (SeeGerritForStatus), 2013-11-25 07:46:53]

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:979: expectedLength =
m_response.expectedContentLength();
so now we can remove this

[tyoshino (SeeGerritForStatus), 2013-11-25 08:06:14]

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:82:
dispatchEvent(XMLHttpRequestProgressEvent::create(EventTypeNames::progress,
true, m_lastBytesSent, m_lastTotalBytesToBeSent));
looks we should have some flag indicating we've received bytesSent and
totalBytesToBeSent and switch lengthComputable accordingly.

l67-74 should be moved to this method after removing if-clauses and changed a
bit to generate values based on the flag, m_last... variables.

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.h (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.h:61: void dispatchEventAndLoadEnd(const
AtomicString&, bool = false, unsigned long long = 0, unsigned long long = 0);
these default arguments can also be removed.

[sof, 2013-11-25 08:10:37]

Sorry, too sloppy this morning.

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:979: expectedLength =
m_response.expectedContentLength();
On 2013/11/25 07:46:53, tyoshino wrote:
> so now we can remove this

Done.

[sof, 2013-11-25 11:38:24]

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:82:
dispatchEvent(XMLHttpRequestProgressEvent::create(EventTypeNames::progress,
true, m_lastBytesSent, m_lastTotalBytesToBeSent));
On 2013/11/25 08:06:15, tyoshino wrote:
> looks we should have some flag indicating we've received bytesSent and
> totalBytesToBeSent and switch lengthComputable accordingly.
> 
> l67-74 should be moved to this method after removing if-clauses and changed a
> bit to generate values based on the flag, m_last... variables.

I think it is quite reasonable to derive lengthComputable from the values
recorded for (loaded, totals), and have done so. Along with doing away with the
defaulting that dispatchEventAndLoadEnd() was engaged in.

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.h (right):

https://codereview.chromium.org/79953004/diff/250001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.h:61: void dispatchEventAndLoadEnd(const
AtomicString&, bool = false, unsigned long long = 0, unsigned long long = 0);
On 2013/11/25 08:06:15, tyoshino wrote:
> these default arguments can also be removed.

Done.

[tyoshino (SeeGerritForStatus), 2013-11-25 12:35:32]

sorry for delay. finally reviewed whole CL. LG once the comments are addressed.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/onload-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/onload-event.html:21:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
+ " ' event.");

->

+ "' event.");

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-load.html
(right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-load.html:23:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
" ' event.");

->

"' event.");

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:14:
assert_true(e.loaded > 0, "Non-zero 'loaded' value for '" + context + " '
event.");
" ' event.");

->

"' event.");

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:14:
assert_true(e.loaded > 0, "Non-zero 'loaded' value for '" + context + " '
event.");
e.loaded == 0 is rare and hard to observe but possible, I think.

just check if it's >= 0?

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:41: xhr.open("GET",
"resources/big-response.php", true);
how about using load-and-stall.php? maybe it'll less flaky.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/resources/post-echo.php (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/post-echo.php:10: ?>
can't we use LayoutTests/http/tests/xmlhttprequest/resources/post-echo.cgi?

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/upload-onload-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onload-event.html:18:
assert_equals(e.loaded, expected, "Expected 'loaded' value of '" + eventName + "
' event.");
" ' event.");

->

"' event.");

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html
(right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:23:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
" ' event.");

->

"' event.");

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:35:
}
no longer necessary

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:45:
verifyProgressEvent("onload", e, expectedLength);
"onload" -> "onloadend" or just "loadend"

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:976: bool lengthComputable = expectedLength >
0 && m_receivedLength <= expectedLength;
m_receivedLength -> receivedLength

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:977: unsigned long long loaded =
m_receivedLength >= 0 ? static_cast<unsigned long long>(m_receivedLength) : 0;
m_receivedLength -> receivedLength

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:72: bool lengthComputable =
m_lastTotalBytesToBeSent > 0 && m_lastBytesSent <= m_lastTotalBytesToBeSent;
yeah. it's fine to determine lengthComputable from the member variables. but we
cannot distinguish 0 set by dispatchProgressEvent() and initial 0.

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:74: dispatchEventAndLoadEnd(type, true,
m_lastBytesSent, m_lastTotalBytesToBeSent);
true -> lengthComputable?

[sof, 2013-11-25 13:30:38]

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:72: bool lengthComputable =
m_lastTotalBytesToBeSent > 0 && m_lastBytesSent <= m_lastTotalBytesToBeSent;
On 2013/11/25 12:35:33, tyoshino wrote:
> yeah. it's fine to determine lengthComputable from the member variables. but
we
> cannot distinguish 0 set by dispatchProgressEvent() and initial 0.

Does that matter, practically? if we haven't gotten any information from the
upload by the time we run into an error, then lengthComputable ought to be
false. And if we have delivered one or more upload progress events where no size
could be determined, if that can happen, then lengthComputable ought to be
false. So, we're ok?

[tyoshino (SeeGerritForStatus), 2013-11-25 13:39:14]

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:72: bool lengthComputable =
m_lastTotalBytesToBeSent > 0 && m_lastBytesSent <= m_lastTotalBytesToBeSent;
On 2013/11/25 13:30:38, sof wrote:
> On 2013/11/25 12:35:33, tyoshino wrote:
> > yeah. it's fine to determine lengthComputable from the member variables. but
> we
> > cannot distinguish 0 set by dispatchProgressEvent() and initial 0.
> 
> Does that matter, practically? if we haven't gotten any information from the
> upload by the time we run into an error, then lengthComputable ought to be
> false. And if we have delivered one or more upload progress events where no
size
> could be determined, if that can happen, then lengthComputable ought to be
> false. So, we're ok?

Ah,... right. And, if dispatchEventAndLoadEnd is called with
lengthComputable=true, total=0, it's done and handleRequestError won't be
invoked after that. So, we don't need to be worried about a case where we want
to dispatch an event with lengthComputable=true but total=0. That never happens.

Sorry. Never mind this comment.

[sof, 2013-11-25 15:28:17]

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/onload-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/onload-event.html:21:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> + " ' event.");
> 
> ->
> 
> + "' event.");

Done.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-load.html
(right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-load.html:23:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> " ' event.");
> 
> ->
> 
> "' event.");

Done.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:14:
assert_true(e.loaded > 0, "Non-zero 'loaded' value for '" + context + " '
event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> " ' event.");
> 
> ->
> 
> "' event.");

Done.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:14:
assert_true(e.loaded > 0, "Non-zero 'loaded' value for '" + context + " '
event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> e.loaded == 0 is rare and hard to observe but possible, I think.
> 
> just check if it's >= 0?

Let's do that.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/ontimeout-event.html:41: xhr.open("GET",
"resources/big-response.php", true);
On 2013/11/25 12:35:33, tyoshino wrote:
> how about using load-and-stall.php? maybe it'll less flaky.

Ah, didn't know of its existence. Switched.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/resources/post-echo.php (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/resources/post-echo.php:10: ?>
On 2013/11/25 12:35:33, tyoshino wrote:
> can't we use LayoutTests/http/tests/xmlhttprequest/resources/post-echo.cgi?

Considered at some length, but it is even more basic (no Content-Length: .) And
in perl.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File LayoutTests/http/tests/xmlhttprequest/upload-onload-event.html (right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onload-event.html:18:
assert_equals(e.loaded, expected, "Expected 'loaded' value of '" + eventName + "
' event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> " ' event.");
> 
> ->
> 
> "' event.");

Done.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
File
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html
(right):

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:23:
assert_equals(e.loaded, expected, "Expected 'loaded' value for '" + context + "
' event.");
On 2013/11/25 12:35:33, tyoshino wrote:
> " ' event.");
> 
> ->
> 
> "' event.");

Done.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:35:
}
On 2013/11/25 12:35:33, tyoshino wrote:
> no longer necessary

Gone.

https://codereview.chromium.org/79953004/diff/340001/LayoutTests/http/tests/x...
LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html:45:
verifyProgressEvent("onload", e, expectedLength);
On 2013/11/25 12:35:33, tyoshino wrote:
> "onload" -> "onloadend" or just "loadend"

Done.

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:976: bool lengthComputable = expectedLength >
0 && m_receivedLength <= expectedLength;
On 2013/11/25 12:35:33, tyoshino wrote:
> m_receivedLength -> receivedLength

Done.

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:977: unsigned long long loaded =
m_receivedLength >= 0 ? static_cast<unsigned long long>(m_receivedLength) : 0;
On 2013/11/25 12:35:33, tyoshino wrote:
> m_receivedLength -> receivedLength

Done.

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequestUpload.cpp (right):

https://codereview.chromium.org/79953004/diff/340001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequestUpload.cpp:74: dispatchEventAndLoadEnd(type, true,
m_lastBytesSent, m_lastTotalBytesToBeSent);
On 2013/11/25 12:35:33, tyoshino wrote:
> true -> lengthComputable?

Done.

[tyoshino (SeeGerritForStatus), 2013-11-25 16:04:43]

lgtm

Thank you!

abarth: OWNER review please

[abarth-chromium, 2013-11-25 18:10:50]

rslgtm

[commit-bot: I haz the power, 2013-11-25 20:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/79953004/440001

[commit-bot: I haz the power, 2013-11-25 23:11:16]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-11-26 07:11:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/79953004/440001

[commit-bot: I haz the power, 2013-11-26 09:11:50]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[sof, 2013-11-26 09:50:34]

fast/xmlhttprequest/xmlhttprequest-get turned out unstable due to line
termination differences across platforms; updated the test to avoid running into
the issue.

@tyoshino: ptal?

[tyoshino (SeeGerritForStatus), 2013-11-26 11:01:10]

On 2013/11/26 09:50:34, sof wrote:
> fast/xmlhttprequest/xmlhttprequest-get turned out unstable due to line
> termination differences across platforms; updated the test to avoid running
into
> the issue.
> 
> @tyoshino: ptal?

OK. Let's use some script to generate some binary data to receive later.

lgtm

[sof, 2013-11-26 12:04:37]

On 2013/11/26 11:01:10, tyoshino wrote:
> On 2013/11/26 09:50:34, sof wrote:
> > fast/xmlhttprequest/xmlhttprequest-get turned out unstable due to line
> > termination differences across platforms; updated the test to avoid running
> into
> > the issue.
> > 
> > @tyoshino: ptal?
> 
> OK. Let's use some script to generate some binary data to receive later.
> 
> lgtm

Yes, thanks - let's do that. Additional test coverage wouldn't hurt.

[commit-bot: I haz the power, 2013-11-26 12:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/79953004/460001

[commit-bot: I haz the power, 2013-11-26 13:09:47]

Change committed as 162689