Summary: | XMLHttpRequest should be an EventTarget | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||
Component: | XML | Assignee: | 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
Alexey Proskuryakov
2006-09-27 04:25:38 PDT
Created attachment 11633 [details]
proposed patch
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?
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. Created attachment 11775 [details]
proposed patch
Used a typedef for the monstrosity.
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 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.
Created attachment 11815 [details]
proposed patch
More typedef usage.
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) {
Committed revision 18172. |