Closed
Bug 435425
Opened 16 years ago
Closed 16 years ago
Progress events for XHR
Categories
(Core :: DOM: Core & HTML, defect, P2)
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)
81.48 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
84.48 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
920 bytes,
patch
|
Details | Diff | Splinter Review | |
2.32 KB,
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•16 years ago
|
||
This depends on the state of the Progress Events spec and XHR spec.
Assignee | ||
Comment 2•16 years ago
|
||
Also XHR2, http://www.w3.org/TR/XMLHttpRequest2/
Comment 4•16 years ago
|
||
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+
Assignee | ||
Comment 5•16 years ago
|
||
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)
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
Assignee | ||
Comment 7•16 years ago
|
||
As far as I know, no, testing POST isn't yet possible. It isn't possible to read the post data in .sjs files.
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #326540 -
Attachment is obsolete: true
Attachment #326540 -
Flags: review?(jonas)
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #329023 -
Attachment is obsolete: true
Attachment #329460 -
Flags: review?(jonas)
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
Assignee | ||
Comment 12•16 years ago
|
||
(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+
Assignee | ||
Comment 14•16 years ago
|
||
(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.
Assignee | ||
Comment 15•16 years ago
|
||
Attachment #330599 -
Flags: superreview?(jst)
Updated•16 years ago
|
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 :(
Assignee | ||
Comment 17•16 years ago
|
||
(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.
Hmm.. sorry, must have looked at the wrong place. Looks good indeed.
Assignee | ||
Comment 19•16 years ago
|
||
Assignee | ||
Comment 20•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 21•16 years ago
|
||
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
Assignee | ||
Comment 22•16 years ago
|
||
The same testrun had worker thread errors too. Some random problems in network?
Comment 23•16 years ago
|
||
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.
Assignee | ||
Comment 25•16 years ago
|
||
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•16 years ago
|
||
(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.
Assignee | ||
Comment 27•16 years ago
|
||
(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)
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?
Updated•16 years ago
|
Keywords: dev-doc-needed
Updated•15 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•