Bug 11053

Summary: XMLHttpRequest should be an EventTarget
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: XMLAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://dev.w3.org/cvsweb/~checkout~/2006/webapi/XMLHttpRequest/Overview.html?content-type=text/html;%20charset=utf-8#acknowledgements
Attachments:
Description Flags
proposed patch
ggaren: review-
proposed patch
darin: review-
proposed patch darin: review+

Description Alexey Proskuryakov 2006-09-27 04:25:38 PDT
Objects implementing the XMLHttpRequest interface MUST also implement the EventTarget interface: <http://www.w3.org/TR/DOM-Level-3-Events/events.html#Events-EventTarget>.
Comment 1 Alexey Proskuryakov 2006-11-26 02:07:37 PST
Created attachment 11633 [details]
proposed patch
Comment 2 Geoffrey Garen 2006-12-07 15:45:10 PST
Comment on attachment 11633 [details]
proposed patch

+ HashMap<AtomicStringImpl*, Vector<RefPtr<EventListener> > >

Pleae use a typedef for this monstrosity.

There are subtle differences between the EventTarget implementation here and the one in EventTargetNode. It would be nice to have a common base class that both EventTargetNode and XMLHttpRequest could inherit from ("EventTarget?"). EventTargetNode would have to supplement its behavior -- to support listeners on disconnected nodes, for example -- but the base functionality of tracking event handlers would be identical. This would prevent bugs due to subtle differences in implementation from creeping in, and improve readability. What do you think?
Comment 3 Alexey Proskuryakov 2006-12-07 21:30:25 PST
I absolutely agree, and plan to do that when fixing bug 11610. I don't want to do it in this patch, because that would change existing behavior for onreadystatechange/onload listeners.
Comment 4 Alexey Proskuryakov 2006-12-08 09:57:07 PST
Created attachment 11775 [details]
proposed patch

Used a typedef for the monstrosity.
Comment 5 Darin Adler 2006-12-11 13:39:14 PST
Comment on attachment 11775 [details]
proposed patch

+    HashMap<AtomicStringImpl*, Vector<RefPtr<EventListener> > >& eventListeners = m_impl->eventListeners();

Should use the new typedef (and maybe make a local typedef to get it out of the class).
Comment 6 Darin Adler 2006-12-11 21:29:33 PST
Comment on attachment 11775 [details]
proposed patch

Lets get some more typedef action in JSXMLHttpRequest::mark and perhaps all the occurences of Vector<RefPtr<EventListener> > before we land this.
Comment 7 Alexey Proskuryakov 2006-12-11 23:55:27 PST
Created attachment 11815 [details]
proposed patch

More typedef usage.
Comment 8 Darin Adler 2006-12-11 23:59:03 PST
Comment on attachment 11815 [details]
proposed patch

r=me

+    typedef HashMap<AtomicStringImpl*,  ListenerVector> EventListenersMap;

Two spaces here.

For this code:

+    XMLHttpRequest::EventListenersMap& eventListeners = m_impl->eventListeners();
+    for (XMLHttpRequest::EventListenersMap::iterator mapIter = eventListeners.begin(); mapIter != eventListeners.end(); ++mapIter) {
+        for (XMLHttpRequest::ListenerVector::iterator vecIter = mapIter->second.begin(); vecIter != mapIter->second.end(); ++vecIter) {

I'd do it like this:

    typedef XMLHttpRequest::EventListenersMap ListenerMap;
    typedef XMLHttpRequest:: ListenerVector ListenerVector;
    ListenerMap& eventListeners = m_impl->eventListeners();
    for (ListenerMap::iterator mapIter = eventListeners.begin(); mapIter != eventListeners.end(); ++mapIter) {
        for (ListenerVector::iterator vecIter = mapIter->second.begin(); vecIter != mapIter->second.end(); ++vecIter) {
Comment 9 Alexey Proskuryakov 2006-12-12 00:16:56 PST
Committed revision 18172.