[webkit-reviews] review granted: [Bug 29701] Bring a little sanity to this crazy EventTarget world of ours : [Attachment 40042] webcore patch - changelog in progress

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 29701: Bring a little sanity to this crazy EventTarget world of ours
https://bugs.webkit.org/show_bug.cgi?id=29701

Attachment 40042: webcore patch - changelog in progress
https://bugs.webkit.org/attachment.cgi?id=40042&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> +    if (!result.second) // pre-existing entry
> +	   if (entry.find(registeredListener) != notFound) // duplicate
listener
> +	       return false;

The outer if statement needs braces.

> +
> +    // Notify firing events planning to invoke the listener at 'index' that
> +    // they have one less listener to invoke.
> +    for (size_t i = 0; i < d->firingEventEndIterators.size(); ++i)
> +	   if (eventType == *d->firingEventEndIterators[i].eventType && index <
*d->firingEventEndIterators[i].value)
> +	       --*d->firingEventEndIterators[i].value;

The for-loop needs braces.

> +EventListener* EventTarget::getAttributeEventListener(const AtomicString&
eventType)
> +{
> +    const EventListenerVector& entry = getEventListeners(eventType);
> +    for (size_t i = 0; i < entry.size(); ++i)
> +	   if (entry[i].listener->isAttribute())
> +	       return entry[i].listener.get();

For-loop needs braces.


> +
> +	   bool fireEventListeners(Event*);
> +	   bool isFiringEventListeners();
> +	   bool dispatchEvent(PassRefPtr<Event>, ExceptionCode&); // DOM API
I think it makes sense to put this method up with addEventListener and
removeEventListener.


> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(change);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(click);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(contextmenu);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dblclick);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dragenter);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dragover);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dragleave);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(drop);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dragstart);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(drag);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(dragend);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(input);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(invalid);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(keydown);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(keypress);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(keyup);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mousedown);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mousemove);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mouseout);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mouseover);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mouseup);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(mousewheel);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(scroll);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(select);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(submit);
...
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(beforecut);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(cut);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(beforecopy);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(copy);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(beforepaste);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(paste);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(reset);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(search);
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(selectstart);

These should all be treated the same way as the 4 virtual ones and only on
Element and Document.


> -    void setOnbeforeunload(PassRefPtr<EventListener>);
> -    EventListener* onmessage() const;
> -    void setOnmessage(PassRefPtr<EventListener>);
> -    EventListener* onhashchange() const;
> -    void setOnhashchange(PassRefPtr<EventListener>);
> -    EventListener* onoffline() const;
> -    void setOnoffline(PassRefPtr<EventListener>);
> -    EventListener* ononline() const;
> -    void setOnonline(PassRefPtr<EventListener>);
> +    // Declared virtual in Node

Should be // Declared virtual in Element

> -
> -    EventListener* onbeforeunload() const;
> -    void setOnbeforeunload(PassRefPtr<EventListener>);
> -    EventListener* onhashchange() const;
> -    void setOnhashchange(PassRefPtr<EventListener>);
> -    EventListener* onmessage() const;
> -    void setOnmessage(PassRefPtr<EventListener>);
> -    EventListener* onoffline() const;
> -    void setOnoffline(PassRefPtr<EventListener>);
> -    EventListener* ononline() const;
> -    void setOnonline(PassRefPtr<EventListener>);
> +    // Declared virtual in Node

Should be // Declared virtual in Element


Since these issues are mainly superficial, r=me.  But please make them :).