[webkit-reviews] review denied: [Bug 36156] XHR 'progress' event code assumes wrongly that expectedLength >= 0 : [Attachment 117192] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 36156: XHR 'progress' event code assumes wrongly that expectedLength >= 0
https://bugs.webkit.org/show_bug.cgi?id=36156

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117192&action=review


r- for the test that takes too long.

> Source/WebCore/ChangeLog:6
> +	   Reviewed by Alexey Proskuryakov.

You shouldn't have put this in ChangeLog yet, since I never said r+ before.

>
LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.htm
l:31
> +	   if (e.loaded == 4 && e.total == 0 && !e.lengthComputable)
> +	   {
> +	       log("PASS");
> +	       if (window.layoutTestController)
> +		   layoutTestController.notifyDone();
> +	   }

Nit: it is helpful to have detailed output for failure case, not just missing
"PASS".

> LayoutTests/http/tests/xmlhttprequest/resources/chunked-transfer.php:7
> +sleep(0.5);
> +printf("4\r\n<a/>\r\n");
> +flush();
> +sleep(0.5);

Ugh.. I just noticed that this test takes a long time. Is that really needed?
We generally don't want tests to run for a whole second.

Can't we check lengthComputable before state reaches 4?