[webkit-reviews] review granted: [Bug 37771] XMLHttpRequestUpload events do not fire when sending a raw file or FormData object : [Attachment 54759] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Jian Li <jianli at chromium.org>'s
request for review:
Bug 37771: XMLHttpRequestUpload events do not fire when sending a raw file or
FormData object
https://bugs.webkit.org/show_bug.cgi?id=37771

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

------- Additional Comments from David Levin <levin at chromium.org>
Yep, it is a regression caused by http://trac.webkit.org/changeset/47291. It
looks like there are other cases in which we should be sending progress events
but aren't.

However, this is definitely a step in the right direction. Feel free to file a
bug on the fact that we probably don't send progress events in some cases (and
reference this bug) and assign it to me.

Just a few comments below. If you do any big changes (like making the test
interactive, which would be cool if not too hard) to address them, then I'll be
happy to re-review.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +2010-04-29  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   XMLHttpRequestUpload events do not fire when sending a raw file or
FormData object.
> +	   https://bugs.webkit.org/show_bug.cgi?id=37771
> +
> +	   Add a layout test to test upload events firing.
> +
> +	   * http/tests/local/formdata/resources/send-form-data-common.js:
> +	   (dumpResponse):
> +	   (sendFormData):
> +	   (testSendingFormData):
> +	   * http/tests/local/formdata/script-tests/upload-events.js: Added.
> +	   * http/tests/local/formdata/upload-events-expected.txt: Added.
> +	   * http/tests/local/formdata/upload-events.html: Added.
> +	   * platform/gtk/Skipped:
> +	   * platform/qt/Skipped:
> +	   * platform/win/Skipped:

Per file or function comments would be nice.

For example, you told me why we are skipping the tests on several of these
platforms but it would be nice to note that reason in the ChangeLog.

> +
>  2010-04-29  Anton Muhin  <antonm at chromium.org>
>  
>	   Reviewed by Darin Adler.
> diff --git
a/LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js
b/LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js

> +function sendFormData(formDataList, fileSliced, sendAsAsync,
xhrSetupCallback)

Consider s/xhrSetupCallback/addEventHandlers/


> diff --git
a/LayoutTests/http/tests/local/formdata/script-tests/upload-events.js
b/LayoutTests/http/tests/local/formdata/script-tests/upload-events.js

> +var progressEventFired = false;
> +function progressHandler(evt)
> +{
> +    // Dump progress event only once in order to get consistent result.

Consider    // Dump the progress event only once in order to get a consistent
result.


> +function setupXMLHttpRequest(xhr, fileSliced)

Consider s/setupXMLHttpRequest/addXHREventHandlers/


> +if (window.eventSender) {
> +    window.jsTestIsAsync = true;
> +    runTest();
> +    // Clean up after ourselves
> +    fileInput.parentNode.removeChild(fileInput);

Why is this needed? (I found it confusing because I didn't see it being added,
but then I found that it is added by the "send-form-data-common.js" script.

> +} else {
> +    testFailed("This test is not interactive, please run using
DumpRenderTree");

How hard would it be to make it interactive with instructions like "Drag a file
to here."?



> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-04-29  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   XMLHttpRequestUpload events do not fire when sending a raw file or
FormData object.
> +	   https://bugs.webkit.org/show_bug.cgi?id=37771
> +
> +	   Test: http/tests/local/formdata/upload-events.html
> +
> +	   * xml/XMLHttpRequest.cpp:
> +	   (WebCore::XMLHttpRequest::createRequest):

A function level comment would be nice here.