XHR2 says "while the download is progressing and async is true, synchronously dispatch a progress event called progress at the object every 350ms (±200ms) or for every byte received, whichever is least frequent. "

Comment on attachment 329564 [details] [diff] [review] wip Mochitests aren't possible yet, bug 396226 :(

I'd like to get this done before Bug 311425

Comment on attachment 329564 [details] [diff] [review] wip Oops, I think I misunderstood the spec.

So I don't get how this is supposed to work. I can't see that you're actually suppressing any events anywhere. Also, why give special treatment to upload events? Shouldn't they fire at the most every 350ms too?

(In reply to comment #6) > So I don't get how this is supposed to work. I can't see that you're actually > suppressing any events anywhere. + if (mTimerIsActive) { + // The progress event will be dispatched when the notifier calls Notify(). + mProgressEventWasDelayed = PR_TRUE; + return NS_OK; + } > Also, why give special treatment to upload events? Shouldn't they fire at the > most every 350ms too? Per spec no. (or at least the version of the spec which that patch is based on. W3C is apparently down currently)

Ah, the spec has changed. Will change the patch.

So apparently I had even asked about the 350ms-is-download-only :) <smaug> anne: perhaps I asked this already, but why the 350ms is only about download ? <anne> you did and I fixed :)

Doh! Don't know how i missed that return even though I was looking for it. I see how it's supposed to work then. You should then restart the timer from within xhr::Notify if you are dispatching an event. Otherwise if there is an OnProgress call 10ms after the timer fires we'll immediately fire another progress event.

I need to figure out how to make automated test for this. I've used http://mozilla.pettay.fi/xhr_upload/xhr_upload_demo_timers.html as a test.

(In reply to comment #10) > You should then restart the timer from within xhr::Notify if you are > dispatching an event. That is what even the previous patch did.

Hard to make a good test with httpd.js, so I just converted my test to a mochitest. At least it makes sure progress events don't fire too fast or too slowly. The problem is that progress may not fire at all, if server runs on the same machine or something (not firing progress is ok per the spec).

The 350ms is just what the current version of the spec defines. It may change, and actually I hope progress events would be fired more often.

Comment on attachment 343250 [details] [diff] [review] timer also for upload >@@ -3120,19 +3151,32 @@ nsXMLHttpRequest::OnProgress(nsIRequest >+ if (mTimerIsActive) { >+ // The progress event will be dispatched when the notifier calls Notify(). >+ mProgressEventWasDelayed = PR_TRUE; >+ return NS_OK; >+ } >+ mProgressEventWasDelayed = PR_FALSE; >+ > if (!mErrorLoad) { >+ StartProgressEventTimer(); Can you set mProgressEventWasDelayed in StartProcessEventTimer? Seems like you're starting the timer because you're firing an event, at which point it should be set to false. >+ mTimerIsActive = PR_FALSE; >+ if (NS_SUCCEEDED(CheckInnerWindowCorrectness()) && !mErrorLoad) { >+ if (mProgressEventWasDelayed) { >+ mProgressEventWasDelayed = PR_FALSE; >+ if (!(XML_HTTP_REQUEST_MPART_HEADERS & mState)) { Why do you need the XML_HTTP_REQUEST_MPART_HEADERS check? Can mProgressEventWasDelayed ever become true if we're inside the mpart headers? >+ StartProgressEventTimer(); >+ // We're uploading if our state is XML_HTTP_REQUEST_OPENED or >+ // XML_HTTP_REQUEST_SENT >+ if ((XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT) & mState) { It's a bit unfortunate that we have both this and mUploadComplete. Can the two ever return different things? Is that intentional? >+ DispatchProgressEvent(this, NS_LITERAL_STRING(UPLOADPROGRESS_STR), >+ PR_TRUE, PR_TRUE, mUploadTransferred, >+ mUploadTotal, mUploadProgress, >+ mUploadProgressMax); >+ if (mUpload) { >+ DispatchProgressEvent(mUpload, NS_LITERAL_STRING(PROGRESS_STR), >+ PR_TRUE, PR_TRUE, mUploadTransferred, >+ mUploadTotal, mUploadProgress, >+ mUploadProgressMax); >+ } >+ } else { >+ DispatchProgressEvent(this, NS_LITERAL_STRING(PROGRESS_STR), >+ mLoadLengthComputable, mResponseBody.Length(), >+ mLoadTotal); >+ } >+ } >+ } >+ } else if (mProgressNotifier) { >+ mProgressNotifier->Cancel(); >+ mProgressNotifier = nsnull; Might be worth keeping the timer object around and reusing it? r/sr with those things fixed.

(In reply to comment #15) > Can you set mProgressEventWasDelayed in StartProcessEventTimer? Seems like > you're starting the timer because you're firing an event, at which point it > should be set to false. The first StartProcessEventTimer isn't started because of delaying progress event. > Why do you need the XML_HTTP_REQUEST_MPART_HEADERS check? Can > mProgressEventWasDelayed ever become true if we're inside the mpart headers? It can, unfortunately. If there is a delayed upload event, and we start to load multipart headers, but not yet start to load the first part. > It's a bit unfortunate that we have both this and mUploadComplete. Can the two > ever return different things? Is that intentional? mUploadComplete is PR_TRUE by default. It is set false only if it is detected in .send() that something will be uploaded. > >+ } else if (mProgressNotifier) { > >+ mProgressNotifier->Cancel(); > >+ mProgressNotifier = nsnull; > > Might be worth keeping the timer object around and reusing it? Yes. Just checked that calling Cancel() breaks the cycle I was afraid of.

(In reply to comment #16) > (In reply to comment #15) > > Can you set mProgressEventWasDelayed in StartProcessEventTimer? Seems like > > you're starting the timer because you're firing an event, at which point it > > should be set to false. > The first StartProcessEventTimer isn't started because of delaying progress > event. I mean set mProgressEventWasDelayed to PR_FALSE in StartProcessEventTimer. > > Why do you need the XML_HTTP_REQUEST_MPART_HEADERS check? Can > > mProgressEventWasDelayed ever become true if we're inside the mpart headers? > It can, unfortunately. If there is a delayed upload event, and we start to > load multipart headers, but not yet start to load the first part. But doesn't that mean that if we end up parsing all mpart headers within those 350ms we'll end up with XML_HTTP_REQUEST_MPART_HEADERS set to false again and we'll still fire when we technically shouldn't. Seems like you want to set mProgressEventWasDelayed when firing the 'load' event on the upload object. > > It's a bit unfortunate that we have both this and mUploadComplete. Can the two > > ever return different things? Is that intentional? > mUploadComplete is PR_TRUE by default. It is set false only if it is detected > in .send() that something will be uploaded. So would it be wrong to test !mUploadComplete there? Just trying to understand the flow. We really should clean up the various states that XHR carries around sometime... I hate mState. Looks good either way.

(In reply to comment #17) > I mean set mProgressEventWasDelayed to PR_FALSE in StartProcessEventTimer. Ah, that way. Could do, I think. > But doesn't that mean that if we end up parsing all mpart headers within those > 350ms we'll end up with XML_HTTP_REQUEST_MPART_HEADERS set to false again and > we'll still fire when we technically shouldn't. MPART_HEADERS is cleared when first part is about to be downloaded and then the timer is canceled (and restarted for the download) >Seems like you want to set > mProgressEventWasDelayed when firing the 'load' event on the upload object. Timer is restarted when starting download (so right after firing load on upload) > So would it be wrong to test !mUploadComplete there? Just trying to understand > the flow. Ah, right. Could do it that way too. > We really should clean up the various states that XHR carries around > sometime... I hate mState. I agree.

This removes those unneeded mProgressNotifier = nsnull. Doesn't change the mState handling, because that way mUploadComplete is clearly used in the same way as in the spec.

> MPART_HEADERS is cleared when first part is about to be downloaded and then > the timer is canceled (and restarted for the download) But the initial OnStartRequest is forwarded to the XHR object as well (see the bottom of nsMultipartProxyListener::OnStartRequest), which calls StartProgressEventTimer which will clear mProgressEventWasDelayed (though i now see that that wasn't part of the initial patch). Actually, looking at the code, it looks like this forward makes the XML_HTTP_REQUEST_MPART_HEADERS flag be cleared right after it is set. Am I missing something or does that flag currently not work?

Disabled the 550msec part of the test. It failed on moz2-linux-slave08.

Windows liked the tests even less: Bug 444546, disable tests author Olli Pettay <Olli.Pettay@helsinki.fi> Mon Oct 20 02:29:13 2008 +0300 (at Mon Oct 20 02:29:13 2008 +0300) changeset 20643 055521129e67

I need to figure out some way to test this all with mochitest.

Smaug, can you file a bug against httpd.js in Core:Testing to fix the problems that prevent tests here from landing?