Closed Bug 435425 Opened 12 years ago Closed 11 years ago

Progress events for XHR

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 2 obsolete files)

This depends on the state of the Progress Events spec and XHR spec.
I think I want to do bug 372964 first.
Depends on: 372964
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Flags: wanted1.9.1?
Priority: -- → P2
Flags: wanted1.9.1? → wanted1.9.1+
Attached patch v1 (obsolete) — Splinter Review
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...
Attachment #326540 - Flags: review?(jonas)
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 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.
Blocks: 444546
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #326540 - Attachment is obsolete: true
Attachment #326540 - Flags: review?(jonas)
Attached patch up-to-dateSplinter Review
Attachment #329023 - Attachment is obsolete: true
Attachment #329460 - Flags: review?(jonas)
Blocks: 445225
Depends on: js-post
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
(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 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!
Attachment #329460 - Flags: review?(jonas) → review+
(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.
Attachment #330599 - Flags: superreview?(jst) → superreview+
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 :(
(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.

Blocks: 311425
Hmm.. sorry, must have looked at the wrong place. Looks good indeed.
Attached patch fix for testsSplinter Review
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 450920
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
The same testrun had worker thread errors too. Some random problems in
network?
The worker thread errors are still showing up so I doubt that they are related... I believe they were just landed as well.
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.
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.
(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.
(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)

Depends on: 451664
Depends on: 451991
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.
Flags: in-testsuite-
Flags: in-testsuite- → in-testsuite?
Depends on: 486247
Depends on: 486531
Depends on: 618250
Depends on: 623203
Depends on: 818281
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.