[webkit-reviews] review canceled: [Bug 81506] Asserts in XMLHttpRequestProgressEventThrottle : [Attachment 135083] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 135083: Patch
https://bugs.webkit.org/attachment.cgi?id=135083&action=review

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


I like the new change. I would like to see the next round but the direction is
very cool.

> Source/WebCore/ChangeLog:8
> +	   Defers events attempted dispatched while the object is suspend.
Coalescing progress and

Can we please have real English here? Usually sentences have a conjugated verb.


> Source/WebCore/ChangeLog:11
> +	   to avoid hitting a recursion assert in ScriptExecutionContext.

I would like the 'why' / context to be explained as requested 2 times now. The
condition of this bug got 3 people in the original bug and those *need* to
appear somewhere.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:92
> +	   if (m_deferredEvents.size() > 1
> +	       && event->type() == eventNames().readystatechangeEvent
> +	       && m_deferredEvents.last()->type() ==
eventNames().readystatechangeEvent)

Those should be on the same line.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:93
> +	       return; // Readystatechange events are state-less so don't
repeat two identical events in a row on resume.

Nit: Normally we place comment on a line by themselves. This isn't documented
or particularly enforced but it arguably looks nicer. If you do change that,
you need some curly braces around the statement.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:131
> +    ASSERT(m_deferEvents);

We usually check that our timer is the right timer:

ASSERT_UNUSED(timer, timer == &m_dispatchDeferredEventsTimer);

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:136
> +    Vector<RefPtr<Event> >::const_iterator it = m_deferredEvents.begin();
> +    const Vector<RefPtr<Event> >::const_iterator end =
m_deferredEvents.end();
> +    for (; it != end; ++it)

We usually prefer to put the iterator |it| in the |for| loop.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:137
> +	   dispatchEvent(*it);

Note that if JavaScript can interact with our associated XHR, maybe even
triggering a suspend(). I really wonder if we shouldn't just swap our
m_deferredEvents Vector before the loop to avoid any possible changes while
iterating.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:173
> +	   m_dispatchDeferredEventsTimer.stop();

We should add, in case something goes wrong:

ASSERT(m_deferEvents);

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:177
> +    ASSERT(m_deferredEvents.isEmpty());

And the matching ASSERT(!m_deferEvents) here too.