Bug 46743 – Event dispatching in XMLHttpRequestProgressEventThrottle should go through XMLHttpRequestProgressEventThrottle::dispatchEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Anton D'Auria]

In XMLHttpRequestProgressEventThrottle, we should dispatch events through XMLHttpRequestProgressEventThrottle::dispatchEvents, instead of calling m_target->dispatchEvent in various places. This makes it easy to add multiple event queuing (currently, one progress event can be queued on suspend(), if progress events have been throttled).

[Anton D'Auria]

Created attachment 69081 [details]
[PATCH] Refactor XMLHttpRequestProgressEventThrottle event dispatching

In preparation for platform-specific queuing of XMLHttpRequest events, this patch changes all calls to m_target->dispatchEvent to XMLHttpRequestProgressEventThrottle::dispatchEvent. Currently, we queue up only a progress event on suspend() if we have throttled progress events. We should be able to queue all XHR events that  can be generated after suspend(), if the platform network layer continues to receive data. XMLHttpRequest uses XMLHttpRequestProgressEventThrottle to dispatch only download events, so this doesn't change behavior of upload events, which aren't throttled or queued.

[Anton D'Auria]

Created attachment 69088 [details]
[PATCH] Refactor XMLHttpRequestProgressEventThrottle event dispatching (with ChangeLog)

Added ChangeLog

[Alexey Proskuryakov]

I still question the need to queue multiple events in XMLHttpRequest.

[Alexey Proskuryakov]

This makes control flow a little confusing. If XMLHttpRequestProgressEventThrottle::dispatchEvent() is called with FlushProgressEvent action, it will call flushProgressEvent(), which will now call XMLHttpRequestProgressEventThrottle::dispatchEvent() back recursively.

[Darin Adler]

Comment on attachment 69088 [details]
[PATCH] Refactor XMLHttpRequestProgressEventThrottle event dispatching (with ChangeLog)

I do not like the ProgressEventAction design here. It seems if we want to do two things we should call two functions, or we can have a function that does two things, but having function with a passed parameter that tells it to either do two things or one thing seems unnecessarily confusing.

[Darin Adler]

(In reply to comment #4)
> This makes control flow a little confusing. If XMLHttpRequestProgressEventThrottle::dispatchEvent() is called with FlushProgressEvent action, it will call flushProgressEvent(), which will now call XMLHttpRequestProgressEventThrottle::dispatchEvent() back recursively.

I agree that it makes the control flow confusing. Despite that, I said review+. My ProgressEventAction comment might help, though.

[Darin Adler]

Alexey, if you had said review- I would not have done a review+. I didn’t notice your comments until afterward.

[Alexey Proskuryakov]

Comment on attachment 69088 [details]
[PATCH] Refactor XMLHttpRequestProgressEventThrottle event dispatching (with ChangeLog)

My understanding is that Anton wanted to look into whether the change was needed for his project again, so I'll set cq- for now. Please feel free to flip it back if necessary.

[Eric Seidel (no email)]

Ping?  Should this bug be closed or is this still for landing?

[Eric Seidel (no email)]

Looks like this was never landed.  I expect the patch has rotten by now.

[Allan Sandfeld Jensen]

Created attachment 133091 [details]
Patch

[Julien Chaffraix]

Comment on attachment 133091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133091&action=review

> Source/WebCore/ChangeLog:8
> +

Please add some details about your change and most importantly *why* you are doing it (as a stepping stone to implement event queuing in XMLHttpRequestProgressEventThrottle). The original ChangeLog had a very nice description:

In preparation for platform-specific queuing of XMLHttpRequest events,
this patch changes all calls to m_target->dispatchEvent to
XMLHttpRequestProgressEventThrottle::dispatchEvent. Currently,
we queue up only a progress event on suspend() if we have throttled
progress events. We should be able to queue all XHR events that
can be generated after suspend(), if the platform network layer continues
to receive data. XMLHttpRequest uses XMLHttpRequestProgressEventThrottle
to dispatch only download events, so this doesn't change behavior of upload
events, which aren't throttled or queued.

[Allan Sandfeld Jensen]

Created attachment 133265 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133265 [details]
Patch for landing

Clearing flags on attachment: 133265

Committed r111717: <http://trac.webkit.org/changeset/111717>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.