435425 - Progress events for XHR

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

See http://www.w3.org/TR/progress-events/

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This depends on the state of the Progress Events spec and XHR spec.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Also XHR2, http://www.w3.org/TR/XMLHttpRequest2/

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I think I want to do bug 372964 first.

Comment 4

[Damon Sicore (:damons)]

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][[e-mail]]]

Attachment: 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]

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][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

Attachment: up-to-date

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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][[e-mail]]]

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

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][[e-mail]]]

(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][[e-mail]]]

Attachment: 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]

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][[e-mail]]]

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

Hmm.. sorry, must have looked at the wrong place. Looks good indeed.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Had to disable tests because of Windows tbox

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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 (they/them - no direct email)]

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][[e-mail]]]

The same testrun had worker thread errors too. Some random problems in
network?

Comment 23

[Robert Strong (they/them - no direct email)]

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!)]

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][[e-mail]]]

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 (they/them - no direct email)]

(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][[e-mail]]]

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

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.