[webkit-reviews] review granted: [Bug 39029] REGRESSION: (r47291): Upload progress events are not fired for simple cross-origin XHR : [Attachment 55911] Proposed fix v2.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 12 17:01:48 PDT 2010


Alexey Proskuryakov <ap at webkit.org> has granted David Levin
<levin at chromium.org>'s request for review:
Bug 39029: REGRESSION: (r47291): Upload progress events are not fired for
simple cross-origin XHR
https://bugs.webkit.org/show_bug.cgi?id=39029

Attachment 55911: Proposed fix v2.
https://bugs.webkit.org/attachment.cgi?id=55911&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
The code changes look good. Maybe I'd have renamed forcePreflight local
variable to uploadEvents to match the current spec draft, and to make the code
slightly less confusing.

According to the spec, the upload events flag should be always set to false for
sync requests. So, it's wrong to force preflight because of installed event
listeners, and I'm not sure what we do with actual events in this case.

+	 * http/tests/xmlhttprequest/simple-cross-origin-progress-events.html:
Add a test which sets up
+	   at least one the progress event listener before the XMLHttpRequest
send is done.

This only verifies the fix indirectly (by testing that preflight is forced,
which is a great thing to test). Can you add a test to verify that progress
events for successful upload are fired, too?

r=me as is, but please consider investigating and testing the sync case, and
adding a test for successful preflight.

It may also be interesting to investigate the case where preflight is enabled
for some other reason (such as a different content type), and event listeners
are only added after send(). WebKit would dispatch upload events, and this
seems to be the logical thing to do, but I'm not sure how interpret the spec
text.


More information about the webkit-reviews mailing list