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).
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.
Created attachment 69088 [details] [PATCH] Refactor XMLHttpRequestProgressEventThrottle event dispatching (with ChangeLog) Added ChangeLog
I still question the need to queue multiple events in XMLHttpRequest.
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.
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.
(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.
Alexey, if you had said review- I would not have done a review+. I didn’t notice your comments until afterward.
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.
Ping? Should this bug be closed or is this still for landing?
Looks like this was never landed. I expect the patch has rotten by now.
Created attachment 133091 [details] Patch
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.
Created attachment 133265 [details] Patch for landing
Comment on attachment 133265 [details] Patch for landing Clearing flags on attachment: 133265 Committed r111717: <http://trac.webkit.org/changeset/111717>
All reviewed patches have been landed. Closing bug.