[webkit-reviews] review granted: [Bug 18654] [XHR] onProgress event needs to be dispatched according to what the specification states : [Attachment 50790] Take two: Renamed new file to XMLHttpRequestProgressEventThrottle, added suspend/resume support, refactored the code a bit to avoid duplication, made the test more reliable

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 18654: [XHR] onProgress event needs to be dispatched according to what the
specification states
https://bugs.webkit.org/show_bug.cgi?id=18654

Attachment 50790: Take two: Renamed new file to
XMLHttpRequestProgressEventThrottle, added suspend/resume support, refactored
the code a bit to avoid duplication, made the test more reliable
https://bugs.webkit.org/attachment.cgi?id=50790&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +header("Cache-Control: no-cache, must-revalidate");
> +header("Pragma: no-cache");
> +header("Pragma: no-store");

I think no-store should go into Cache-Control. Did such a pragma even exist in
HTTP 1.0? 

> +void XMLHttpRequest::suspend()
> +{
> +    ActiveDOMObject::suspend();
> +    m_progressEventThrottle.suspend();
> +}
> +
> +void XMLHttpRequest::resume()
> +{
> +    ActiveDOMObject::resume();
> +    m_progressEventThrottle.resume();
> +}

We don't call the base class methods in other overrides of suspend/resume, I
think it's better to be consistent.

+     EventTarget* m_target;

This is correct, but it looks suspicious - what guarantees that m_target
doesn't get destroyed? Perhaps having type be XMLHttpRequest would make it less
surprising to people looking at this.

+     Vector<RefPtr<Event> > m_pausedEvents;

Does this need to be vector? Isn't it that we can have no more than one pending
event?

r=me