Bug 13596 – Implement .onprogress handler on XMLHttpRequest objects to support progressive download content length information

• Component: XML
• Status: RESOLVED FIXED

[Antoine Quint]

In current WebKit nightlies, the only mean I found to get information about a progressive download is through a rather inefficient method: querying .responseText.length and getting the Content-Length header on each call of .onreadystatechange handler. Mozilla support a special .onprogress handler which is passed an event object with a .position and .totalSize properties as an argument. This is mentioned
on the MozDev page http://developer.mozilla.org/en/docs/XMLHttpRequest.

See bug http://bugs.webkit.org/show_bug.cgi?id=13595 to see how bad the current technique is.

[Julien Chaffraix]

Created attachment 20144 [details]
First version

It is a first version of onprogress handler. It uses a ProgressEvent as required by the XHR2 standard. It means that we do not match Firefox interface to get the attributes from the event (totalSize is replaced by total and position is replaced loaded).
I have tried defining a custom event to match Firefox but we are basically reimplementing ProgressEvent with just another interface so I decided to stand with the standard.

[Julien Chaffraix]

Created attachment 20152 [details]
Updated first version

I had a variable name + some ChangeLog issues that the new patch should remove.

[Alexey Proskuryakov]

Comment on attachment 20152 [details]
Updated first version

+        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.

[Julien Chaffraix]

(In reply to comment #3)
> (From update of attachment 20152 [details] [edit])
> +        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?
> 

As discussed on IRC, I have asked for a clarification on the public-webapi ML (will post a link to the archive here when I have it).

> --- a/WebCore/WebCore.order
> +++ b/WebCore/WebCore.order
> 
> I do not think order files are to be manually edited.
> 
A dumb mistake, will be removed.

> +    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.
> 

I have not though about multipart support, but I think this issue can be postponed until we support it (will add a FIXME to indicate that multipart support should be careful with that).

> +    // 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?
> 

Yes, it will be ! (my test case was loading a big file (~3/4Mb) and showing the download progress, as we get called every 0.01% or so the perform hit was huge)

As you said the issue is the event handler. FYI, my testing were done on gtk/curl so I think the issue should be solved in WebCore. I thought I would work on it on a separate bug (to do maybe more perf testing). If you insist, I can include the fix with this patch as is quite simple (just adding a byte counter and firing the event when it is above a threshold).

> 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.
> 

I will address those in the next iteration.

[Alexey Proskuryakov]

(In reply to comment #4)
> As you said the issue is the event handler. FYI, my testing were done on
> gtk/curl so I think the issue should be solved in WebCore. I thought I would
> work on it on a separate bug (to do maybe more perf testing). If you insist, I
> can include the fix with this patch as is quite simple (just adding a byte
> counter and firing the event when it is above a threshold).

  Either way is OK with me, but I'm not convinced that the fix will be that simple. What happens if the data is sent slowly (e.g. 1 byte per second)? I would expect to get an event for each byte.

  I think that this is a practical use case, as progress of a lengthy operation can be reported in this way.

[Julien Chaffraix]

Discussion address:

http://www.nabble.com/-XHR2--onprogress-Event-issue-to16419994.html


> 
>   Either way is OK with me, but I'm not convinced that the fix will be that
> simple. What happens if the data is sent slowly (e.g. 1 byte per second)? I
> would expect to get an event for each byte.
> 
>   I think that this is a practical use case, as progress of a lengthy operation
> can be reported in this way.
> 

Indeed. I have not thought about that.

I will feel more comfortable if we move the performance issue on an separate bug (as you showed we have to think carefully about it).

[Julien Chaffraix]

Just an update: 

We will have to create a custom event for onprogress as we need to both support LSProgressEvent (without the input attribute) and ProgressEvent interface (see the discussion on public-webapi about that). 
I am planning on calling it XMLHttpRequestProgressEvent (rather long but better long than ambiguous). To avoid adding LSProgressEvent idl without any real use, XMLHttpRequestProgressEvent will inherit from on ProgressEvent and add the LSProgressEvent bits in its idl.

I hope to finish the new version by tomorrow.

[Julien Chaffraix]

Created attachment 20563 [details]
Updated patch (include custom event + Ap's comments)

Patch as promised.

[Julien Chaffraix]

Created attachment 20708 [details]
Updated after XMLHttpRequest changes and IDL introduction

[Alexey Proskuryakov]

Comment on attachment 20708 [details]
Updated after XMLHttpRequest changes and IDL introduction

I think that we need to support adding handlers via addEventListener, not just .onprogress.

I have noticed that the spec actually describes when the event should be fired: "every 350ms (+-200ms) or for every byte received, whichever is least frequent." I think it's OK to land a first version as is, to improve on this later.

+        // XmlHttpRequest 2 extension

Should be "XMLHttpRequest".

+unsigned XMLHttpRequestProgressEvent::position()
+{
+    // FIXME: If lengthComputable is false, what should we return?
+    return m_loaded;
+}

What is wrong with returning m_loaded?

+    evt = new XMLHttpRequestProgressEvent("progress", expectedLength && m_receivedLength <= expectedLength, m_receivedLength, expectedLength);

Should be progressEvent to avoid constructing an AtomicString from "progress" again and again.

I think that this is almost ready for landing, but not quite yet.

[Julien Chaffraix]

Created attachment 20722 [details]
Updated with Ap's latest comments

> I think that we need to support adding handlers via addEventListener, not just
> .onprogress.

As mentioned on IRC, we can can register those handlers but they were not called because of an error on my side (forgotten to call dispatchEvent). Should be corrected.

> I have noticed that the spec actually describes when the event should be fired:
>"every 350ms (+-200ms) or for every byte received, whichever is least
> frequent." I think it's OK to land a first version as is, to improve on this
> later.

I had not seen that. 

> +        // XmlHttpRequest 2 extension
> Should be "XMLHttpRequest".

Corrected.

> What is wrong with returning m_loaded?

I had to check Firefox behaviour first: there was some mention of giving special value if computableLength was false and I thought it could apply here too. I have check and we match Firefox so removed the 2 comments.

> Should be progressEvent to avoid constructing an AtomicString from "progress"
> again and again.

Corrected too.

I only included a "proof of concept" test case and some of your comments ask for more of them. I will bundle more and have them landed later to have a broader regression test suite (the patch is big enough without them ;)).

[Alexey Proskuryakov]

Comment on attachment 20722 [details]
Updated with Ap's latest comments

r=me, good work!

Please do file bugs for the remaining issues we discussed above.

[Julien Chaffraix]

Committed in r32316.

Filed 2 bugs:
- Bug 18655: [XHR] OnProgress needs more test case
- Bug 18654: [XHR] onProgress event needs to be dispatched according to what the specification states