[webkit-reviews] review canceled: [Bug 40952] Onloadend event is not supported in XMLHttpRequest : [Attachment 119636] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 40952: Onloadend event is not supported in XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=40952

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119636&action=review


OK, I think your gut feeling was right, If you just upload something along the
line of the previous patch with the different comments addressed and the
synchronous test cases from this patch, it should be fine by me. Thanks.

> Source/WebCore/ChangeLog:11
> +	   or error event followed by a loadend event.

ChangeLog not up-to-date as mentioned.

> Source/WebCore/loader/ImageLoader.cpp:1
> +

Unrelated (bad) change.

> Source/WebCore/xml/XMLHttpRequest.cpp:345
> +	  
m_progressEventThrottle.dispatchFinalProgressEvent(XMLHttpRequestProgressEvent:
:create(eventNames().loadEvent));

You are subtly changing the semantics of the willLoadXHR / didLoadXHR here as
we now include 2 events instead of one. I would rather see that the
loadendEvent is dispatched after didLoadXHR was called and a bug filed about
giving access to the "loadend" event from the WebInspector.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:51
> +    virtual void dispatchEvent(PassRefPtr<Event>, ProgressEventAction =
DoNotFlushProgressEvent);

It shouldn't be virtual.

> Source/WebCore/xml/XMLHttpRequestUpload.h:68
> +	   void dispatchProgressEvent(bool lengthComputable, unsigned long long
loaded, unsigned long long total);

Those 2 should be removed as they don't seem to add much. Same remarks for the
XHR throttle.

> Source/WebCore/xml/XMLHttpRequestUpload.h:69
> +	   void dispatchFinalProgressEvent(PassRefPtr<Event>);

I think I preferred the old name dispatchEventAndLoadendEvent(). ProgressEvent
hints a the "progress" event and yet you allow other type of events.

>
LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html:
93
> +    xhr.open("GET", "resources/get.txt", false); 

Great that you added the synchronous case! For coherency, it would be better to
have those splitted but it's not a strong requirement.