Bug 15102 - XMLHttpRequests.onreadystatechange and XMLHttpRequests.addEventListener(readystatechange, ...) should have the same behavior
Summary: XMLHttpRequests.onreadystatechange and XMLHttpRequests.addEventListener(ready...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-28 12:02 PDT by Sam Weinig
Modified: 2015-01-03 04:02 PST (History)
3 users (show)

See Also:


Attachments
shortcut testcase (326 bytes, text/html)
2007-08-28 12:03 PDT, Sam Weinig
no flags Details
addEventListener testcase (350 bytes, text/html)
2007-08-28 12:04 PDT, Sam Weinig
no flags Details
proposed fix (5.32 KB, patch)
2007-08-29 13:15 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed fix (5.80 KB, patch)
2007-08-29 21:54 PDT, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-08-28 12:02:05 PDT
For XMLHttpRequests, setting an EventListener using the the shortcut notation (XHR.onreadystatechange = function()) or the addEventListener method should have the same behavior.
Comment 1 Sam Weinig 2007-08-28 12:03:41 PDT
Created attachment 16144 [details]
shortcut testcase
Comment 2 Sam Weinig 2007-08-28 12:04:16 PDT
Created attachment 16145 [details]
addEventListener testcase
Comment 3 Alexey Proskuryakov 2007-08-29 13:15:42 PDT
Created attachment 16158 [details]
proposed fix

I couldn't recall why I didn't implement readystatechange originally, then Sam found out that it was because Gecko 1.8 didn't support it yet, and even raised an exception when trying to add a listener for it.
Comment 4 Darin Adler 2007-08-29 17:12:03 PDT
Comment on attachment 16158 [details]
proposed fix

It's be better to say evt.release() in the first call to dispatchEvent to avoid reference count churn since we don't have to use the event again after the dispatch.

If we're going to keep dispatching the same event to multiple objects, should stopPropagation on the event make us stop?
Comment 5 Alexey Proskuryakov 2007-08-29 21:54:02 PDT
Created attachment 16162 [details]
proposed fix

(In reply to comment #4)
> It's be better to say evt.release() in the first call to dispatchEvent

  Done.

> If we're going to keep dispatching the same event to multiple objects, should
> stopPropagation on the event make us stop?

My understanding is that stopPropagation() only prevents traversing the DOM hierarchy, but listeners registered on the same node are invoked anyway. I've extended the test to check for this - it passes in yesterday's Firefox nightly.
Comment 6 Maciej Stachowiak 2007-10-02 03:34:38 PDT
Comment on attachment 16162 [details]
proposed fix

r=me for feature-branch or when trunk reopens.

Your interpretation of the DOM is correct, stopPropagation only prevents the event from continuing to the next DOM Node.
Comment 7 Alexey Proskuryakov 2007-10-02 22:08:22 PDT
Committed revision 25959 (feature branch).