Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Description • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 16 years ago

Comment on attachment 329564 [details] [diff] [review]
wip

Mochitests aren't possible yet, bug 396226 :(

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 16 years ago

I'd like to get this done before Bug 311425

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 16 years ago

Comment on attachment 329564 [details] [diff] [review]
wip

Oops, I think I misunderstood the spec.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 16 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 16 years ago

(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)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 16 years ago

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 :)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 10 • 16 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 16 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 13 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 14 • 16 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 16 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 16 • 16 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 18 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 19 • 16 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 20 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 21 • 16 years ago

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.243&mark=219#189

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 22 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 23 • 16 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 24 • 16 years ago

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

	Jeff Walden [:Waldo]
	Comment 25 • 16 years ago

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