The default bug view has changed. See this FAQ.

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
x86
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
See http://www.w3.org/TR/progress-events/
(Assignee)

Comment 1

9 years ago
This depends on the state of the Progress Events spec and XHR spec.
(Assignee)

Comment 2

9 years ago
Also XHR2, http://www.w3.org/TR/XMLHttpRequest2/
(Assignee)

Comment 3

9 years ago
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+
(Assignee)

Comment 5

9 years ago
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...
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

9 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

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

Updated

9 years ago
Blocks: 444546
(Assignee)

Comment 9

9 years ago
Created attachment 329023 [details] [diff] [review]
up-to-date
Attachment #326540 - Attachment is obsolete: true
Attachment #326540 - Flags: review?(jonas)
(Assignee)

Comment 10

9 years ago
Created attachment 329460 [details] [diff] [review]
up-to-date
Attachment #329023 - Attachment is obsolete: true
Attachment #329460 - Flags: review?(jonas)
(Assignee)

Updated

9 years ago
Blocks: 445225
(Assignee)

Updated

9 years ago
Depends on: 366371
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

9 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

9 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

9 years ago
Created attachment 330599 [details] [diff] [review]
forward, don't aggregate in nsXMLHttpProgressEvent, add comments, use LSProgress only for old style progress events
Attachment #330599 - Flags: superreview?(jst)

Updated

9 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

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

(Assignee)

Updated

9 years ago
Blocks: 311425
Hmm.. sorry, must have looked at the wrong place. Looks good indeed.
(Assignee)

Comment 19

9 years ago
Created attachment 333763 [details] [diff] [review]
Had to disable tests because of Windows tbox
(Assignee)

Comment 20

9 years ago
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
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 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
(Assignee)

Comment 22

9 years ago
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.
(Assignee)

Comment 25

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

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

Depends on: 451664
(Assignee)

Updated

9 years ago
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?
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 486247

Updated

8 years ago
Depends on: 486531

Updated

6 years ago
Depends on: 618250
Depends on: 623203
Depends on: 818281
You need to log in before you can comment on or make changes to this bug.