Bug 29701

Summary: Bring a little sanity to this crazy EventTarget world of ours
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: WebCore Misc.Assignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 29713    
Attachments:
Description Flags
swap vectors with inline capacities
sam: review+, ggaren: commit-queue-
layout test changes
sam: review+, ggaren: commit-queue-
webkit support
sam: review+, ggaren: commit-queue-
webcore patch - changelog in progress sam: review+, ggaren: commit-queue-

Description Geoffrey Garen 2009-09-23 20:16:58 PDT 
So crazy!
Comment 1 Geoffrey Garen 2009-09-23 20:17:38 PDT 
Created attachment 40037 [details]
swap vectors with inline capacities
Comment 2 Geoffrey Garen 2009-09-23 20:18:03 PDT 
Created attachment 40038 [details]
layout test changes
Comment 3 Geoffrey Garen 2009-09-23 20:18:43 PDT 
Created attachment 40039 [details]
webkit support
Comment 4 Geoffrey Garen 2009-09-23 20:57:18 PDT 
Created attachment 40042 [details]
webcore patch - changelog in progress
Comment 5 Sam Weinig 2009-09-23 21:39:38 PDT 
Comment on attachment 40042 [details]
webcore patch - changelog in progress

> +    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 :).