Last Comment Bug 435425 - Progress events for XHR
: Progress events for XHR
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: P2 normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
http://www.w3.org/TR/progress-events/
Depends on: 623203 js-post 372964 450920 451664 451991 486247 486531 618250 818281
Blocks: 311425 444546 445225
  Show dependency treegraph
 
Reported: 2008-05-23 08:32 PDT by Olli Pettay [:smaug]
Modified: 2012-12-04 16:53 PST (History)
15 users (show)
jonas: wanted1.9.1+
roc: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (89.03 KB, patch)
2008-06-24 12:58 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (81.45 KB, patch)
2008-07-10 23:00 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (81.48 KB, patch)
2008-07-14 08:13 PDT, Olli Pettay [:smaug]
jonas: review+
Details | Diff | Splinter Review
forward, don't aggregate in nsXMLHttpProgressEvent, add comments, use LSProgress only for old style progress events (84.48 KB, patch)
2008-07-21 10:50 PDT, Olli Pettay [:smaug]
jst: superreview+
Details | Diff | Splinter Review
Had to disable tests because of Windows tbox (920 bytes, patch)
2008-08-14 08:17 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
fix for tests (2.32 KB, patch)
2008-08-14 08:22 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2008-05-23 08:32:35 PDT
See http://www.w3.org/TR/progress-events/
Comment 1 Olli Pettay [:smaug] 2008-05-23 09:18:58 PDT
This depends on the state of the Progress Events spec and XHR spec.
Comment 2 Olli Pettay [:smaug] 2008-05-23 09:23:49 PDT
Also XHR2, http://www.w3.org/TR/XMLHttpRequest2/
Comment 3 Olli Pettay [:smaug] 2008-05-25 08:27:58 PDT
I think I want to do bug 372964 first.
Comment 4 Damon Sicore (:damons) 2008-06-04 16:41:26 PDT
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Comment 5 Olli Pettay [:smaug] 2008-06-24 12:58:20 PDT
Created attachment 326540 [details] [diff] [review]
v1

Adds support for .upload and relevant events.
Contains some tests (280), but since mochitest doesn't support
POST, tests are a bit hackish and don't necessarily test things which should be
tested. (I've been using also http://mozilla.pettay.fi/posttest.html)

The patch tries to aggressively keep the current progress event handling, and 
"just" add support for new nsIDOMProgressEvent interface.

Jonas, if you have time to comment this ;)
The patch is pretty large, but it contains tests and nsDOMProgressEvent 
implementation is simple. Though this does depend on bug 372964...
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-06-25 10:53:48 PDT
You should be able to write tests for POST, see

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F
Comment 7 Olli Pettay [:smaug] 2008-06-25 11:42:53 PDT
As far as I know, no, testing POST isn't yet possible. It isn't possible to
read the post data in .sjs files.
Comment 8 Olli Pettay [:smaug] 2008-07-09 10:42:33 PDT
Comment on attachment 326540 [details] [diff] [review]
v1

Note, nsDOMProgressEvent.h|cpp was implemented also in the patch for <video>, so removing ifdef MOZ_MEDIA is enough to make it available for XHR.
Comment 9 Olli Pettay [:smaug] 2008-07-10 23:00:03 PDT
Created attachment 329023 [details] [diff] [review]
up-to-date
Comment 10 Olli Pettay [:smaug] 2008-07-14 08:13:34 PDT
Created attachment 329460 [details] [diff] [review]
up-to-date
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-07-20 21:16:41 PDT
Comment on attachment 329460 [details] [diff] [review]
up-to-date

>diff --git a/content/base/public/nsIXMLHttpRequest.idl b/content/base/public/nsIXMLHttpRequest.idl

>+[scriptable, uuid(09ff3682-7759-4441-a765-f70e1a1fabcf)]
>+interface nsIXMLHttpRequestUpload : nsIXMLHttpRequestEventTarget {
>+  // for future use
>+};

Can we just leave this out for now and add it if/when we need it?

>+[scriptable, uuid(6e127bd2-b4c1-4a82-be0d-012bd24efb37)]
>+interface nsIXMLHttpRequestUploadGetter : nsISupports {
>+  /**
>+   * Upload process can be tracked by adding event listener to |upload|.
>+   */
>+  readonly attribute nsIXMLHttpRequestUpload upload;
>+};

Just add it to the nsIXMLHttpRequest interface, it's not frozen.

>diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp

>+nsXMLHttpRequestEventTarget::RemoveEventListener(const nsAString& aType,
>+                                      nsIDOMEventListener* aListener,
>+                                      PRBool aUseCapture)

Nit: Whitespace is off, same goes for a number of other functions in this class. You could shorten the name to just nsXHREventTarget if it's hard to fit the arguments within 80 columns.

>+NS_IMETHODIMP
>+nsXMLHttpRequestEventTarget::GetOnload(nsIDOMEventListener** aOnLoad)
>+{
>+  return GetInnerEventListener(mOnLoadListener, aOnLoad);
>+}
>+
>+NS_IMETHODIMP
>+nsXMLHttpRequestEventTarget::SetOnload(nsIDOMEventListener* aOnLoad)
>+{
>+  return RemoveAddEventListener(NS_LITERAL_STRING(LOAD_STR),
>+                                mOnLoadListener, aOnLoad);
>+}

I wonder if it'd be worth implementing these using macros? Up to you.

> nsXMLHttpRequest::GetOnreadystatechange(nsIDOMEventListener * *aOnreadystatechange)
> {
>-  return GetInnerEventListener(mOnReadystatechangeListener,
>-                               aOnreadystatechange);
>+  return nsXMLHttpRequestEventTarget::GetInnerEventListener(
>+                                        mOnReadystatechangeListener,
>+                                        aOnreadystatechange);
> }

Why the "nsXMLHttpRequestEventTarget::"? Same in a few other places.

still got some left
Comment 12 Olli Pettay [:smaug] 2008-07-21 00:15:26 PDT
(In reply to comment #11)
> (From update of attachment 329460 [details] [diff] [review])
> >diff --git a/content/base/public/nsIXMLHttpRequest.idl b/content/base/public/nsIXMLHttpRequest.idl
> 
> >+[scriptable, uuid(09ff3682-7759-4441-a765-f70e1a1fabcf)]
> >+interface nsIXMLHttpRequestUpload : nsIXMLHttpRequestEventTarget {
> >+  // for future use
> >+};
> 
> Can we just leave this out for now and add it if/when we need it?
> 
That is from the spec, where the interface is empty too.
XHR.upload should implement nsIXMLHttpRequestUpload anyway.

> >+[scriptable, uuid(6e127bd2-b4c1-4a82-be0d-012bd24efb37)]
> >+interface nsIXMLHttpRequestUploadGetter : nsISupports {
> >+  /**
> >+   * Upload process can be tracked by adding event listener to |upload|.
> >+   */
> >+  readonly attribute nsIXMLHttpRequestUpload upload;
> >+};
> 
> Just add it to the nsIXMLHttpRequest interface, it's not frozen.
This was discussed on mailing list, and I said I won't change XHR in 1.9.1.


> 
> >diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp
> 
> >+nsXMLHttpRequestEventTarget::RemoveEventListener(const nsAString& aType,
> >+                                      nsIDOMEventListener* aListener,
> >+                                      PRBool aUseCapture)
> 
> Nit: Whitespace is off, same goes for a number of other functions in this
> class. You could shorten the name to just nsXHREventTarget if it's hard to fit
> the arguments within 80 columns.
Ok. Always hard to find the right indentation for parameters when class and
method names are long.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-07-21 02:07:16 PDT
Comment on attachment 329460 [details] [diff] [review]
up-to-date

So I think we should only support the nsIDOMLSProgressEvent interface on the events that currently are dispatch (for example none of the events on .upload). That makes it less likely that people are going to use the properties on that interface by mistake which would permanent them on the web.

It'd be great if you could even warn in the console for anyone using those properties that we are deprecating them.


>+  NS_ADDREF(*aUpload = mUpload.get());
>   return NS_OK;
> }

You shouldn't need the .get() there.


>-NS_INTERFACE_MAP_BEGIN(nsXMLHttpProgressEvent)
>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsXMLHttpProgressEvent)
>+
>+// QueryInterface implementation for nsXMLHttpProgressEvent
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsXMLHttpProgressEvent)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMLSProgressEvent)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMLSProgressEvent)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMEvent)
>   NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(XMLHttpProgressEvent)
>-NS_INTERFACE_MAP_END
>+NS_INTERFACE_MAP_END_AGGREGATED(mInner)

This isn't correct since mInner isn't doing proper aggregation. I.e. you are breaking XPCOM rules here. Forward the whole ProgressEvent interface instead.


>+  // This creates a trusted event, which is not cancelable and doesn't
>+  // bubble.
>+  static nsresult CreateEvent(const nsAString& aType, nsIDOMEvent** aDOMEvent);

If this is just used for onreadystatechange now you could remove the |aType| argument.

>+  static void DispatchProgressEvent(nsPIDOMEventTarget* aTarget,
>+                                    const nsAString& aType,
>+                                    PRBool aLengthComputable,
>+                                    PRUint64 aLoaded, PRUint64 aTotal,
>+                                    PRUint64 aPosition, PRUint64 aTotalSize);

It'd be nice to get the arguments here better documented and named so it was more obvious that they were specific to LS events.


Looks great otherwise!
Comment 14 Olli Pettay [:smaug] 2008-07-21 09:42:17 PDT
(In reply to comment #11)
> Why the "nsXMLHttpRequestEventTarget::"? Same in a few other places.

Without that I get linking error.
/home/smaug/mozilla/mozilla_cvs/hg/mozilla/content/base/src/nsXMLHttpRequest.cpp:789: undefined reference to `nsXMLHttpRequest::GetInnerEventListener(nsRefPtr<nsDOMEventListenerWrapper>&, nsIDOMEventListener**)'
/usr/bin/ld: ../../content/base/src/libgkconbase_s.a(nsXMLHttpRequest.o): relocation R_X86_64_PC32 against `nsXMLHttpRequest::GetInnerEventListener(nsRefPtr<nsDOMEventListenerWrapper>&, nsIDOMEventListener**)' can not be used when making a shared object; recompile with -fPIC
And that looks strange because -fPIC is used.
Maybe related to http://benjamin.smedbergs.us/blog/2005-10-27/gcc-40-workaround/ , although that pragma didn't help.
Comment 15 Olli Pettay [:smaug] 2008-07-21 10:50:41 PDT
Created attachment 330599 [details] [diff] [review]
forward, don't aggregate in nsXMLHttpProgressEvent, add comments, use LSProgress only for old style progress events
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-07-25 13:42:30 PDT
So I just noticed that we have this hack in nsXMLHttpRequest::OpenRequest. We check for registered progress listeners and only enable progress events if ones have been added.

So you probably should change that code to look in the ELM directly, as well as the upload ELM :(
Comment 17 Olli Pettay [:smaug] 2008-07-25 13:48:19 PDT
(In reply to comment #16)
> So you probably should change that code to look in the ELM directly, as well as
> the upload ELM :(
What you mean? I changed that code to use HasListenersFor and mUpload->HasListenersFor
HasListenersFor forwards the call directly to ELM.

Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-07-25 13:53:06 PDT
Hmm.. sorry, must have looked at the wrong place. Looks good indeed.
Comment 19 Olli Pettay [:smaug] 2008-08-14 08:17:28 PDT
Created attachment 333763 [details] [diff] [review]
Had to disable tests because of Windows tbox
Comment 20 Olli Pettay [:smaug] 2008-08-14 08:22:53 PDT
Created attachment 333764 [details] [diff] [review]
fix for tests

And this should fix the tests. Must not remove optional events, since
there may or may not be several progress events.
Will push this ASAP
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2008-08-17 01:26:55 PDT
There was a failure of this test tonight on WINNT 5.2 mozilla-central qm-win2k3-unittest-hw dep unit test

ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug435425.html | Wrong event! - got "error", expected "load"

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1218956939.1218960908.32099.gz&fulltext=1#err0

Might just be sporadic
Comment 22 Olli Pettay [:smaug] 2008-08-17 03:21:25 PDT
The same testrun had worker thread errors too. Some random problems in
network?
Comment 23 Robert Strong [:rstrong] (use needinfo to contact me) 2008-08-17 12:28:56 PDT
The worker thread errors are still showing up so I doubt that they are related... I believe they were just landed as well.
Comment 24 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-08-17 13:14:30 PDT
Those worker thread errors are expected - the test is to ensure that errors are thrown. I guess I need to find a way to keep them out of the tinderbox error log though.
Comment 25 Olli Pettay [:smaug] 2008-08-17 13:20:09 PDT
random errors are expected in worker thread tests?

But yeah, perhaps those aren't related. The problem described in #21 looks still
random. Loading XHR should work.
Comment 26 Robert Strong [:rstrong] (use needinfo to contact me) 2008-08-17 13:24:39 PDT
(In reply to comment #25)
> random errors are expected in worker thread tests?

(In reply to comment #24)
> Those worker thread errors are expected - the test is to ensure that errors are
> thrown. I guess I need to find a way to keep them out of the tinderbox error
> log though.
So, the errors displayed in the log are expected and are there because Ben hasn't found a way to keep them out of the logs... they also don't report as a test failure since the test is testing for this expected error.

> But yeah, perhaps those aren't related. The problem described in #21 looks
> still random. Loading XHR should work.
Likely random though it may be that the test could be more robust so random errors don't occur.
Comment 27 Olli Pettay [:smaug] 2008-08-17 13:29:07 PDT
(In reply to comment #26)
> > But yeah, perhaps those aren't related. The problem described in #21 looks
> > still random. Loading XHR should work.
> Likely random though it may be that the test could be more robust so random
> errors don't occur.
Well, there really should be a load event, not error, but if something goes
wrong on network side or so, error will be dispatched.
(Testing XHR with current httpd.js is anyway hard)

Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-09-07 19:24:50 PDT
The tests for this bug started failing almost constantly on Linux, after the checkin for bug 443610. Since there were already sporadic failures, I don't think we can blame 443610. So I disabled the tests.

Note You need to log in before you can comment on or make changes to this bug.