Closed Bug 372964 Opened 13 years ago Closed 12 years ago

Make XMLHttpRequest to work more like a normal event target

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

x86
All
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

()

Details

Attachments

(3 files, 7 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
The patch is just a idea what could be done.
It is a bit scary, especially because with it event listeners aren't
removed automatically all the time like they are currently.
Though, GMail and GCalendar etc seem to work ok.
I'm not quite sure what we want to do with mScriptContext.
Currently the handling of it is a mess, IMO, but in the patch it is 
updated only when XHR is created.

Uploading the patch so that I don't lose it.
mScriptContext will more or less need to go away anyway once we implement the W3C XMLHttpRequest spec -- we'll want to hold a pointer to a window at that point.
Attached patch WIP2 (obsolete) — Splinter Review
Some ideas to remove mScriptContext.
Attachment #257582 - Attachment is obsolete: true
Attached patch WIP3 (obsolete) — Splinter Review
Attachment #257750 - Attachment is obsolete: true
bz, or anyone, any comments on this one?

(I've been using the patch and the patch for 373998 for few days now.
 So far everything seems to work ok; gmail, gmaps etc.)
I'm not going to be in a position to look at this in the foreseeable future... (weeks, probably).
Attached patch WIP4 - up-to-date-with-trunk (obsolete) — Splinter Review
Attachment #260328 - Attachment is obsolete: true
Assignee: general → Olli.Pettay
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch WIP5 (obsolete) — Splinter Review
Do you have any comments on the nsIJSNativeInitializer.h/nsDOMClassInfo.cpp changes?
According to the current spec there must be the pointer from xhr to 
the window and that simplifies scriptcontext mess.

One thing about the events, the patch changes the name of the uploadprogress events to be really "uploadprogress", not "progress".
Will have to check WebAPI WG what the final event name / API will be.
Attachment #261213 - Attachment is obsolete: true
Attachment #264178 - Flags: review?(jst)
I think this will fix bug 295422, since window.close relies on termination functions being called, and current nsXMLHttpRequest::NotifyEventListeners implementation doesn't do that.
Blocks: 295422
Blocks: 435425
Depends on: 435656
The patch depends on bug 435656.
nsEventTargetSH will be useful with xhr.upload.addEventListener too.
Attachment #264178 - Attachment is obsolete: true
Attachment #322610 - Flags: review?(jonas)
Attachment #264178 - Flags: review?(jst)
I'm not quite sure what to do with ClearEventListeners.
It should be removed, but that might break something.
The patch changes few cases when ClearEventListeners is called.
Comment on attachment 322610 [details] [diff] [review]
Adds support for 4th parameter of addEventListener + tests

I think I found a solution to have even better backward compatibility.
Attachment #322610 - Flags: review?(jonas)
I'll make still a different patch without the whole clear-event-listeners-mess.
Attachment #322610 - Attachment is obsolete: true
Attached patch without ClearEventListeners (obsolete) — Splinter Review
Most of the listener-clearing was there to avoid leaks.  That's less of an issue now.  So we could try only clearing what the xhr spec says to clear.
Yes, I think so too. If there are regressions found (during 1.9.1pre), clearing 
event listeners could be added back. Currently XHR1&2 don't seem to specify any 
clearing.
[14:25]	<smaug>	anne: have you tested IE and event listeners?
[14:25]	<smaug>	afaik it clears XHR event listeners after "completition"
[14:25]	<smaug>	and gecko has had to follow that
[14:26]	<smaug>	the spec doesn't say anything about it
[14:26]	<anne>	we had that but removed it because the IE team acknowledged it was a bug
Attachment #322640 - Flags: review?(jonas)
Note, this may break progress events which will be fixed and modified a bit in the patch for bug 435425.
wanted1.9.1? because this blocks wanted1.9.1+ bug 435425.
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P3
I'm a little scared about making a functional change that could break webpages at the same time as a cleanup patch, but since it aligns us with the spec, hopefully few pages will break...
Comment on attachment 322640 [details] [diff] [review]
without ClearEventListeners

What is the purpose of the nsDOMEventListenerWrapper business? Why not store the direct pointers to the listeners in the XHR members instead? The only effect seems to be that you can add the same listener twice, once through .onX and once through AddEventListener. Is this defined by spec? Otherwise this seems like undesired behavior?

> nsXMLHttpRequest::SetOnreadystatechange(nsIDOMEventListener * aOnreadystatechange)
> {
>-  mOnReadystatechangeListener = aOnreadystatechange;
>-  return NS_OK;
>+  NS_NAMED_LITERAL_STRING(rs, READYSTATE_STR);
>+  return RemoveAddEventListener(rs, mOnReadystatechangeListener,
>+                                aOnreadystatechange);
> }

Why not just use NS_LITERAL_STRING inline? Same in the other setters.

>+nsXMLHttpRequest::GetListenerManager(PRBool aCreateIfNotFound,
>+                                     nsIEventListenerManager** aResult)
>+{
>+  if (!mListenerManager) {
>+    if (!aCreateIfNotFound) {
>+      *aResult = nsnull;
>+      return NS_OK;
>+    }
>+    nsresult rv = NS_NewEventListenerManager(getter_AddRefs(mListenerManager));
>+    NS_ENSURE_SUCCESS(rv, rv);

This can return with out setting *aResult if things fail here, no?

I'd just set *aResult up front to nsnull;


Looks great otherwise!

The only thing that I'm worried about is would this make it harder for us to use the XHR object off the main thread when doing worker threads? All objects would still just be used on a single thread, but that thread wouldn't always be the main thread.

I suspect that we'd still want this cleanup though, and we could make whatever changes were needed to the ELM code to make it able to live off the main thread.

Want to understand the wrapper stuff before marking r+
(In reply to comment #19)
> (From update of attachment 322640 [details] [diff] [review])
> What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> the direct pointers to the listeners in the XHR members instead? The only
> effect seems to be that you can add the same listener twice, once through .onX
> and once through AddEventListener. Is this defined by spec? Otherwise this
> seems like undesired behavior?
The main reasons are backward compatibility and to be able to remove 
.onfoo listeners by setting a new value to .onfoo.
If the wrapper was added to ELM but XHR would keep pointer to the original
listener, then removing the .onfoo listener wouldn't be possible, since the
pointer to the wrapper is lost somewhere in ELM's internal data structures.

Why would this be undesired behavior?

> Why not just use NS_LITERAL_STRING inline? Same in the other setters.
There used to be some reason :) Left-overs from some version of the patch.

> This can return with out setting *aResult if things fail here, no?
True. Especially because NS_NewEventListenerManager doesn't set its
out param to null.

> The only thing that I'm worried about is would this make it harder for us to
> use the XHR object off the main thread when doing worker threads? All objects
> would still just be used on a single thread, but that thread wouldn't always be
> the main thread.
AFAIS the additional objects for cycle collector are the only parts which can 
make things harder for worker threads. But that is an issue even before this 
patch - DOM/CC is currently for main thread.
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 322640 [details] [diff] [review] [details])
> > What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> > the direct pointers to the listeners in the XHR members instead? The only
> > effect seems to be that you can add the same listener twice, once through .onX
> > and once through AddEventListener. Is this defined by spec? Otherwise this
> > seems like undesired behavior?
> The main reasons are backward compatibility and to be able to remove 
> .onfoo listeners by setting a new value to .onfoo.
> If the wrapper was added to ELM but XHR would keep pointer to the original
> listener, then removing the .onfoo listener wouldn't be possible, since the
> pointer to the wrapper is lost somewhere in ELM's internal data structures.
To clarify this some more, using wrappers .removeEventListener doesn't work
if listener is added using onfoo. That is how XHR works currently and that is
how also other DOM objects work
Attached patch with nits fixedSplinter Review
Attachment #322640 - Attachment is obsolete: true
Attachment #329075 - Flags: review?(jonas)
Attachment #322640 - Flags: review?(jonas)
Comment on attachment 329075 [details] [diff] [review]
with nits fixed

I think I addressed Jonas' comments.
Attachment #329075 - Flags: superreview?(jst)
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 322640 [details] [diff] [review] [details])
> > What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> > the direct pointers to the listeners in the XHR members instead? The only
> > effect seems to be that you can add the same listener twice, once through .onX
> > and once through AddEventListener. Is this defined by spec? Otherwise this
> > seems like undesired behavior?
> The main reasons are backward compatibility and to be able to remove 
> .onfoo listeners by setting a new value to .onfoo.

That would still work, no? I.e. if you just put the original listener in the member as well as added it to the ELM, then you could still remove the listener by setting .onfoo = null.

> If the wrapper was added to ELM but XHR would keep pointer to the original
> listener, then removing the .onfoo listener wouldn't be possible, since the
> pointer to the wrapper is lost somewhere in ELM's internal data structures.

Sorry, I was unclear. What I propose is that we keep a pointer to the real listener in XHR, as well as add the real listener to the ELM. I.e. kill the whole wrapper class.

> To clarify this some more, using wrappers .removeEventListener doesn't work
> if listener is added using onfoo. That is how XHR works currently and that is
> how also other DOM objects work

Ah, this is indeed true. I.e. if we didn't have wrappers someone could do:

xhr.onfoo = mylistener;
xhr.removeEventListener(mylistener, ...);

which would result in no listener being registered. I don't really feel strongly about if this is desired or not. I think it would be totally ok if the above removed the listener, as long .onfoo got cleared too.

But I guess we can keep the wrappers for now, if nothing else for consistency with nodes .onfoo behavior.
jst? This is blocking rest of the progress event stuff.
Blocks: 448602
Attachment #329075 - Flags: superreview?(jst) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached patch additional fixSplinter Review
bah, I should have remembered this. The old code has inner window check
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.241&mark=1008#997
Attachment #333934 - Flags: superreview?(jst)
Attachment #333934 - Flags: review?(jst)
Attachment #333934 - Flags: superreview?(jst)
Attachment #333934 - Flags: superreview+
Attachment #333934 - Flags: review?(jst)
Attachment #333934 - Flags: review+
Comment on attachment 333934 [details] [diff] [review]
additional fix

Tue Aug 19 16:02:02 2008 +0300 (at Tue Aug 19 16:02:02 2008 +0300)
changeset 17034	81c9a977f5e3
Why does test_bug372964-2.html expect the first call to document.open() to blow away the old inner window? I'm trying to understand why my patch for bug 543435 broke things here. I suspect it has something to do with the generated about:blank doc getting doc->SetIsInitialDocument(PR_TRUE); called on it.
Because that is what has at least traditionally happened;
document.open() after load event blows away the old inner window.

I thought HTML spec even specified this
(but I didn't verify that now - need to run to a meeting).
Thanks. I guess I've misunderstood the semantics of IsInitialDocument() and the initial about:blank should stop claiming to be initial after load in order to have this stuff work.
IsInitialDocument is the first document ever loaded in the window, and in particular one whose inner window might need to be persisted across document loads.  This used to be about:blank in some cases...  but with your changes we might have more than one initial document, depending on how they are done.  :(
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.