[webkit-reviews] review denied: [Bug 13596] Implement .onprogress handler on XMLHttpRequest objects to support progressive download content length information : [Attachment 20152] Updated first version

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 13596: Implement .onprogress handler on XMLHttpRequest objects to support
progressive download content length information
http://bugs.webkit.org/show_bug.cgi?id=13596

Attachment 20152: Updated first version
http://bugs.webkit.org/attachment.cgi?id=20152&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 Our event does not match Firefox interface but matches the XHR2
standard.

I think we'd need to have more clarity on why these are different. Is Firefox
going to change to match the draft spec?

--- a/WebCore/WebCore.order
+++ b/WebCore/WebCore.order

I do not think order files are to be manually edited.

+    m_receivedLength = 0;

It might make sense to consider how this will work with multipart responses -
although it's not strictly necessary for this patch, given that we don't
support those anyway yet.

+    // FIXME: We are called too often (3-4 times more that firefox) so we have
a huge performance hit

Is this something that needs to be addressed at CFNetwork level? Can we make a
workaround (I assume the hit is caused by the actual event handlers, so perhaps
we could throttle event firing within WebCore)? We can't take a performance hit
without at least having a clear plan on how to remedy it. But also, the scope
of the problem is not quite obvious - what are the scenarios where this would
be noticeable?

Also, "Firefox" with capital "F", and we usually put a dot at the end of a full
sentence like this.

+    if (!expectedLength || m_receivedLength > expectedLength)
+	 evt = new ProgressEvent("progress", false, m_receivedLength,
expectedLength);
+    else
+	 evt = new ProgressEvent("progress", true, m_receivedLength,
expectedLength);

This would look better with a local variable for "!expectedLength ||
m_receivedLength > expectedLength" - no need for "if".

+    void updateAndDispatchOnProgress(int len);

In didReceiveData(), len is an int because it can be -1, indicating a
null-terminated string. But in updateAndDispatchOnProgress(), the length is
always known, so it would be better to use unsigned.

r- for Firefox compatibility and performance questions for now, but this looks
good to me otherwise.