[webkit-reviews] review denied: [Bug 18654] [XHR] onProgress event needs to be dispatched according to what the specification states : [Attachment 49587] Updated patch: corrected build systems and licenses

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied 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 49587: Updated patch: corrected build systems and licenses
https://bugs.webkit.org/attachment.cgi?id=49587&action=review

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

Maybe you also want no-store here?

+You should see a serie of 5 PASSED.

Typo: should be "series". I'd say "sequence", but I'm not a native speaker, so
take that suggestion with a grain of salt.

I have some doubts whether this test will be stable on buildbots. But besides
supporting this change, it tests a peculiar server-side timing, which would be
nice to run through regression tests, even if we decide that it's impossible to
test timing.

I'm not a fan of designs that involve "dispatcher" objects. Those are not
objects, as they don't describe behavior of an entity a human can think about.
I don't have any suggestions though.

+	 m_progressEventDispatcher.dispatchEvent(expectedLength &&
m_receivedLength <= expectedLength, static_cast<unsigned>(m_receivedLength),
static_cast<unsigned>(expectedLength));

Could you add parentheses here? Some ports' compilers will emit warning about
ambiguous code, I think.

+    // FIXME: This is a temporary measure as we do not have the concept of
task and thus cannot post a task.
+    void dispatchEventNow();

We have ScriptExecutionContext::postTask. Is that a different kind of task than
needed here?

But I think that the spec is just being misleading here, and the event should
be dispatched synchronously. The "task" it talks about is just the fact that
network loading is asynchronous.

+    static const double progressEventDispatchingTime = .05; // 50 ms per
specification.

I'd call this "minimumProgressEventDispatchingIntervalInSeconds".

The timer can fire behind an alert or another modal dialog. It shouldn't.
Perhaps XHR should tell the dispatcher when it's suspended. It currently
doesn't override ActiveDOMObject::suspend, because pages with active XHRs
cannot go into b/f cache, and existing progress events do not fire simply
because all loading is paused behind modal dialogs.

+	 m_progressEventDispatcher.dispatchEvent(expectedLength &&
m_receivedLength <= expectedLength, static_cast<unsigned>(m_receivedLength),
static_cast<unsigned>(expectedLength));

I believe that when expected length is not known, it's set to -1
(NSURLResponseUnknownLength), not to 0. Maybe there are platform differences
here, and maybe even pre-existing bugs.