[webkit-reviews] review granted: [Bug 81506] Asserts in XMLHttpRequestProgressEventThrottle : [Attachment 135545] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 81506: Asserts in XMLHttpRequestProgressEventThrottle
https://bugs.webkit.org/show_bug.cgi?id=81506

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

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


> Source/WebCore/ChangeLog:17
> +

Note that you still don't explain the context here and merely describe what you
are doing. As I can't get myself understood, please add the following to the
ChangeLog (preferably at the beginning) before landing:

The issue is that suspending active DOM objects (which suspends the
XMLHttpRequestProgressEventThrottle) doesn't stop JavaScript from running or
suspend any running loader we may have. The previous code would assume those 2
cases were impossible (thus the ASSERTs triggering) and the code was modified
to handle them.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:92
> +	   if (m_deferredEvents.size() > 1 && event->type() ==
eventNames().readystatechangeEvent && event->type() ==
m_deferredEvents.last()->type())
> +	       // Readystatechange events are state-less so avoid repeating two
identical events in a row on resume.
> +	       return;

There should be curly brace around that statement per our coding style.