Bug 33882 – The Chromium API should expose EventListeners

• Component: WebKit API
• Status: RESOLVED FIXED

[Jay Campan]

The Chromium API should provide a way to register an EventListener on nodes.

[Jay Campan]

Created attachment 46979 [details]
Initial draft of a patch

A first shot at providing the event listener API to the Chromium API.

[Jay Campan]

Created attachment 46980 [details]
Initial draft of a patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 46980 [details]
Initial draft of a patch

> Index: WebKit/chromium/public/WebEvent.h

> +    virtual bool isUIEvent() const;
> +    virtual bool isMouseEvent() const;
> +    virtual bool isMutationEvent() const;

On this interface, I think these should be declared as WEBKIT_API bool isFooEvent() const;

Note: no need for virtual here since the implementation of these methods is forwarding
to m_private->isFooEvent(), and the method call on m_private is virtual.


> Index: WebKit/chromium/public/WebEventListener.h
...
> +class WebEventListener {
> +public:
> +    // Called when an event is received.
> +    virtual void handleEvent(const WebEvent&) = 0;
> +
> +    // Called when the listener was detached from the node it was listening
> +    // to and that no further event will be received.
> +    // Gives an opportunity to implementation classes to perform necessary
> +    // clean-up.
> +    virtual void listenerDetached() {};
> +};
> +
> +} // namespace WebKit
> +
> +#endif

> Index: WebKit/chromium/public/WebMutationEvent.h
...
> +class WebMutationEvent : public WebEvent {
> +public:
> +
> +#if WEBKIT_IMPLEMENTATION
> +    WebMutationEvent(const WTF::PassRefPtr<WebCore::Event>&);
> +#endif
> +
> +    virtual bool isMutationEvent() const;

^^^ As discussed above, this method can go away.

Is the plan to add some accessors to this class?



> Index: WebKit/chromium/public/WebNode.h
> ===================================================================
> --- WebKit/chromium/public/WebNode.h	(revision 53524)
> +++ WebKit/chromium/public/WebNode.h	(working copy)
> @@ -34,6 +34,9 @@
>  #include "WebCommon.h"
>  #include "WebString.h"
>  
> +#include <utility>
> +#include <wtf/HashMap.h>
> +
>  namespace WebCore { class Node; }
>  #if WEBKIT_IMPLEMENTATION
>  namespace WTF { template <typename T> class PassRefPtr; }
> @@ -41,6 +44,8 @@ namespace WTF { template <typename T> cl
>  
>  namespace WebKit {
>  class WebDocument;
> +class WebEventListener;
> +class WebEventListenerPrivate;
>  class WebFrame;
>  class WebNodeList;
>  
> @@ -115,6 +120,15 @@ public:
>          return res;
>      }
>  
> +    virtual void addEventListener(
> +        const WebString& eventType,
> +        WebEventListener* listener,
> +        bool useCapture);
> +    virtual void removeEventListener(
> +        const WebString& eventType,
> +        WebEventListener* listener,
> +        bool useCapture);
> +
>  protected:
>      typedef WebCore::Node WebNodePrivate;
>      void assign(WebNodePrivate*);
> @@ -129,6 +143,49 @@ protected:
>      {
>          return static_cast<const T*>(m_private);
>      }
> +
> +private:
> +    struct ListenerInfo {
> +        WebString eventType;
> +		    WebEventListener* listener;
> +		    bool useCapture;
> +
> +        bool operator==(const ListenerInfo& value) const
> +        {
> +            return eventType == value.eventType && listener == value.listener && useCapture == value.useCapture;    
> +        }
> +
> +        ListenerInfo(WebString eventType, WebEventListener* listener, bool useCapture)
> +            : eventType(eventType)
> +            , listener(listener)
> +            , useCapture(useCapture)
> +        {
> +        }
> +    };
> +
> +    struct ListenerInfoHash
> +    {
> +        static const bool safeToCompareToEmptyOrDeleted = false;
> +        static unsigned hash(const ListenerInfo& listenerInfo);
> +        static bool equal(const ListenerInfo& a, const ListenerInfo& b);
> +    };
> +
> +    struct ListenerInfoTraits
> +    {
> +        static const bool emptyValueIsZero = false;
> +        static const bool needsDestruction = false;
> +
> +        typedef ListenerInfo TraitType;
> +        static void constructDeletedValue(ListenerInfo& slot);
> +        static bool isDeletedValue(const ListenerInfo& value);
> +        static ListenerInfo emptyValue();
> +    };
> +
> +    typedef HashMap<ListenerInfo,
> +                    WebEventListenerPrivate*,
> +                    ListenerInfoHash,
> +                    ListenerInfoTraits> WebEventListenerMap;
> +    WebEventListenerMap m_eventListenerMap;

We cannot use HashMap and other WTF classes like this in the public headers.

I think I misunderstood our hallway conversation about this approach.


> Index: WebKit/chromium/public/WebString.h
> ===================================================================
> --- WebKit/chromium/public/WebString.h	(revision 53524)
> +++ WebKit/chromium/public/WebString.h	(working copy)
> @@ -71,6 +71,7 @@ public:
>          assign(s);
>          return *this;
>      }
> +    bool operator==(const WebString& s) const;

nit: let's instead define a "WEBKIT_API bool equals(const WebString&) const" method.
then at global scope, we can define methods like this:

  inline bool operator==(const WebString& a, const WebString& b)
  {
      return a.equals(b);
  }

  inline bool operator!=(const WebString& a, const WebString& b)
  {
      return !(a == b);
  }

the advantage here is that you can then write expressions like this:

  WebString x;
  "foo" == x;

as well as

  std::string y;
  y == x;

this is possible because of the implicit constructors defined on WebString.


> +    unsigned hash() const;

Any public method not implemented inline in the WebKit API needs to be
decorated with WEBKIT_API.


> Index: WebKit/chromium/src/WebNode.cpp
...
> +class WebEventListenerPrivate : public EventListener {
> +public:
> +    WebEventListenerPrivate(WebEventListener* webEventListener)
> +        : EventListener(EventListener::JSEventListenerType)
> +        , m_webEventListener(webEventListener)
> +    {
> +    }
> +    virtual ~WebEventListenerPrivate() {
> +        m_webEventListener->listenerDetached();
> +    }
> +
> +    virtual bool operator==(const EventListener& listener)
> +    {
> +        return this == &listener;
> +    }
> +
> +    virtual void handleEvent(ScriptExecutionContext* context, Event* event)
> +    {
> +      WebEvent webEvent = WebEvent::createWebEvent(event);
> +      m_webEventListener->handleEvent(webEvent);
> +    }
> +
> +private:
> +    WebEventListener* m_webEventListener;

can we perhaps avoid the map by making WebEventListener hold a private pointer
back to the EventListener?

[Jay Campan]

Created attachment 47249 [details]
Cleaned-up patch. All needed basic EventListener functionalities implemented.

[Jay Campan]

(In reply to comment #3)
> (From update of attachment 46980 [details])
> > Index: WebKit/chromium/public/WebEvent.h
> 
> > +    virtual bool isUIEvent() const;
> > +    virtual bool isMouseEvent() const;
> > +    virtual bool isMutationEvent() const;
> 
> On this interface, I think these should be declared as WEBKIT_API bool
> isFooEvent() const;
> 
> Note: no need for virtual here since the implementation of these methods is
> forwarding
> to m_private->isFooEvent(), and the method call on m_private is virtual.
> 
> 
> > Index: WebKit/chromium/public/WebEventListener.h
> ...
> > +class WebEventListener {
> > +public:
> > +    // Called when an event is received.
> > +    virtual void handleEvent(const WebEvent&) = 0;
> > +
> > +    // Called when the listener was detached from the node it was listening
> > +    // to and that no further event will be received.
> > +    // Gives an opportunity to implementation classes to perform necessary
> > +    // clean-up.
> > +    virtual void listenerDetached() {};
> > +};
> > +
> > +} // namespace WebKit
> > +
> > +#endif
> 
> > Index: WebKit/chromium/public/WebMutationEvent.h
> ...
> > +class WebMutationEvent : public WebEvent {
> > +public:
> > +
> > +#if WEBKIT_IMPLEMENTATION
> > +    WebMutationEvent(const WTF::PassRefPtr<WebCore::Event>&);
> > +#endif
> > +
> > +    virtual bool isMutationEvent() const;
> 
> ^^^ As discussed above, this method can go away.
> 
> Is the plan to add some accessors to this class?
Yes, I added the accessors.
 
> 
> 
> > Index: WebKit/chromium/public/WebNode.h
> > ===================================================================
> > --- WebKit/chromium/public/WebNode.h	(revision 53524)
> > +++ WebKit/chromium/public/WebNode.h	(working copy)
> > @@ -34,6 +34,9 @@
> >  #include "WebCommon.h"
> >  #include "WebString.h"
> >  
> > +#include <utility>
> > +#include <wtf/HashMap.h>
> > +
> >  namespace WebCore { class Node; }
> >  #if WEBKIT_IMPLEMENTATION
> >  namespace WTF { template <typename T> class PassRefPtr; }
> > @@ -41,6 +44,8 @@ namespace WTF { template <typename T> cl
> >  
> >  namespace WebKit {
> >  class WebDocument;
> > +class WebEventListener;
> > +class WebEventListenerPrivate;
> >  class WebFrame;
> >  class WebNodeList;
> >  
> > @@ -115,6 +120,15 @@ public:
> >          return res;
> >      }
> >  
> > +    virtual void addEventListener(
> > +        const WebString& eventType,
> > +        WebEventListener* listener,
> > +        bool useCapture);
> > +    virtual void removeEventListener(
> > +        const WebString& eventType,
> > +        WebEventListener* listener,
> > +        bool useCapture);
> > +
> >  protected:
> >      typedef WebCore::Node WebNodePrivate;
> >      void assign(WebNodePrivate*);
> > @@ -129,6 +143,49 @@ protected:
> >      {
> >          return static_cast<const T*>(m_private);
> >      }
> > +
> > +private:
> > +    struct ListenerInfo {
> > +        WebString eventType;
> > +		    WebEventListener* listener;
> > +		    bool useCapture;
> > +
> > +        bool operator==(const ListenerInfo& value) const
> > +        {
> > +            return eventType == value.eventType && listener == value.listener && useCapture == value.useCapture;    
> > +        }
> > +
> > +        ListenerInfo(WebString eventType, WebEventListener* listener, bool useCapture)
> > +            : eventType(eventType)
> > +            , listener(listener)
> > +            , useCapture(useCapture)
> > +        {
> > +        }
> > +    };
> > +
> > +    struct ListenerInfoHash
> > +    {
> > +        static const bool safeToCompareToEmptyOrDeleted = false;
> > +        static unsigned hash(const ListenerInfo& listenerInfo);
> > +        static bool equal(const ListenerInfo& a, const ListenerInfo& b);
> > +    };
> > +
> > +    struct ListenerInfoTraits
> > +    {
> > +        static const bool emptyValueIsZero = false;
> > +        static const bool needsDestruction = false;
> > +
> > +        typedef ListenerInfo TraitType;
> > +        static void constructDeletedValue(ListenerInfo& slot);
> > +        static bool isDeletedValue(const ListenerInfo& value);
> > +        static ListenerInfo emptyValue();
> > +    };
> > +
> > +    typedef HashMap<ListenerInfo,
> > +                    WebEventListenerPrivate*,
> > +                    ListenerInfoHash,
> > +                    ListenerInfoTraits> WebEventListenerMap;
> > +    WebEventListenerMap m_eventListenerMap;
> 
> We cannot use HashMap and other WTF classes like this in the public headers.
> 
> I think I misunderstood our hallway conversation about this approach.
Remove the hash and implemented the WebEventListener keeping a list of the different EventListener as we discussed.

> 
> 
> > Index: WebKit/chromium/public/WebString.h
> > ===================================================================
> > --- WebKit/chromium/public/WebString.h	(revision 53524)
> > +++ WebKit/chromium/public/WebString.h	(working copy)
> > @@ -71,6 +71,7 @@ public:
> >          assign(s);
> >          return *this;
> >      }
> > +    bool operator==(const WebString& s) const;
> 
> nit: let's instead define a "WEBKIT_API bool equals(const WebString&) const"
> method.
> then at global scope, we can define methods like this:
> 
>   inline bool operator==(const WebString& a, const WebString& b)
>   {
>       return a.equals(b);
>   }
> 
>   inline bool operator!=(const WebString& a, const WebString& b)
>   {
>       return !(a == b);
>   }
> 
> the advantage here is that you can then write expressions like this:
> 
>   WebString x;
>   "foo" == x;
> 
> as well as
> 
>   std::string y;
>   y == x;
> 
> this is possible because of the implicit constructors defined on WebString.
> 
> 
> > +    unsigned hash() const;
> 
> Any public method not implemented inline in the WebKit API needs to be
> decorated with WEBKIT_API.
> 
> 
> > Index: WebKit/chromium/src/WebNode.cpp
> ...
> > +class WebEventListenerPrivate : public EventListener {
> > +public:
> > +    WebEventListenerPrivate(WebEventListener* webEventListener)
> > +        : EventListener(EventListener::JSEventListenerType)
> > +        , m_webEventListener(webEventListener)
> > +    {
> > +    }
> > +    virtual ~WebEventListenerPrivate() {
> > +        m_webEventListener->listenerDetached();
> > +    }
> > +
> > +    virtual bool operator==(const EventListener& listener)
> > +    {
> > +        return this == &listener;
> > +    }
> > +
> > +    virtual void handleEvent(ScriptExecutionContext* context, Event* event)
> > +    {
> > +      WebEvent webEvent = WebEvent::createWebEvent(event);
> > +      m_webEventListener->handleEvent(webEvent);
> > +    }
> > +
> > +private:
> > +    WebEventListener* m_webEventListener;
> 
> can we perhaps avoid the map by making WebEventListener hold a private pointer
> back to the EventListener?

[Darin Fisher (:fishd, Google)]

Comment on attachment 47249 [details]
Cleaned-up patch. All needed basic EventListener functionalities implemented.

> Index: WebKit/chromium/ChangeLog
...
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Adding EventListeners to the chromium API.

^^^ Add a bug link here.


> +        * WebKit.gyp:
> +        * public/WebEvent.h: Added.
> +        (WebKit::WebEvent::):
> +        (WebKit::WebEvent::WebEvent):
> +        (WebKit::WebEvent::operator=):
> +        (WebKit::WebEvent::isNull):

^^^ when adding a new file, please remove the lines here that
reference the methods contained in that new file.


> Index: WebKit/chromium/public/WebEvent.h
...
> +#ifndef WebEvent_h
> +#define WebEvent_h
> +
> +#include "WebCommon.h"
> +#include "WebNode.h"
> +#include "WebString.h"
> +
> +namespace WebCore { class Event; }
> +#if WEBKIT_IMPLEMENTATION
> +namespace WTF { template <typename T> class PassRefPtr; }
> +#endif
> +
> +namespace WebKit {
> +
> +class WebEvent {
> +public:
> +    enum PhaseType {
> +        CapturingPhase     = 1,
> +        AtTarget           = 2,
> +        BubblingPhase      = 3
> +    };
> +
> +    WebEvent() : m_private(0) { }
> +    WebEvent(const WebEvent& n) : m_private(0) { assign(n); }
> +    WebEvent& operator=(const WebEvent& n)

nit: "n" -> "e"

(In WebNode.h, we use "n" as shorthand for node.)


> +    template<typename T> T toMutationEvent()

the point of this being a template is so that it can be used with
multiple subclasses.  so, calling it toMutationEvent seems wrong.
how about just calling it toEvent?

  WebMutationEvent me = event.toEvent<WebMutationEvent>();

I would even be happy with the method name "to", but then "toConst"
looks a little funny.


> Index: WebKit/chromium/public/WebEventListener.h
...
> +class WebEventListener {
> +public:
> +    WebEventListener();
> +    virtual ~WebEventListener();
> +
> +    // Called when an event is received.
> +    virtual void handleEvent(const WebEvent&) = 0;
> +
> +private:
> +    friend class EventListenerWrapper;
> +    friend class WebNode;

instead of making friends, how about defining public methods
wrapped with #if WEBKIT_IMPLEMENTATION?


> Index: WebKit/chromium/public/WebMutationEvent.h
...
> +namespace WebCore { class Event; }
> +#if WEBKIT_IMPLEMENTATION
> +namespace WTF { template <typename T> class PassRefPtr; }
> +#endif

^^^ this stuff is already declared in WebEvent.h


> +namespace WebKit {
> +
> +class WebMutationEvent : public WebEvent {
> +public:
> +    enum AttrChangeType {
> +        Modification    = 1,
> +        Addition        = 2,
> +        Removal         = 3
> +    };
> +
> +#if WEBKIT_IMPLEMENTATION
> +    WebMutationEvent(const WTF::PassRefPtr<WebCore::Event>&);
> +#endif

nit: convention is to move WEBKIT_IMPLEMENTATION stuff down to
the bottom of the class definition.


> Index: WebKit/chromium/public/WebNode.h
...
> +    virtual void addEventListener(
> +        const WebString& eventType,
> +        WebEventListener* listener,
> +        bool useCapture);
> +    virtual void removeEventListener(
> +        const WebString& eventType,
> +        WebEventListener* listener,
> +        bool useCapture);

minor nit: please move these above the toElement and toConstElement methods.

they also should not be virtual.  please add WEBKIT_API to them.


> Index: WebKit/chromium/public/WebString.h
...
>          assign(s);
>          return *this;
>      }
> +    WEBKIT_API bool equals(const WebString& s) const;

nit: butting this up right next to operator= seems to suggest that it is
related.  instead, how about putting it below assign with a new line separating
it from other methods?


> Index: WebKit/chromium/src/WebEvent.cpp

> +WebString WebEvent::type() const
> +{
> +    return m_private->type();

in all of the methods that dereference m_private, please add an ASSERT(m_private);


> Index: WebKit/chromium/src/WebEventListenerImpl.cpp
...
> +#include "WebEventListenerImpl.h"
> +
> +#include "WebEvent.h"
> +#include "WebEventListener.h"
> +
> +#include "Event.h"
> +#include "EventListener.h"

webkit api convention is to list the WebCore headers
above the WebKit headers.



> +WebEventListener::~WebEventListener()
> +{
> +    // Notifies all WebEventListenerWrappers that we are going away so they can
> +    // invalidate their pointer to us.
> +    WTF::Vector<WebEventListenerPrivate::ListenerInfo>::const_iterator iter;
> +    for (iter = m_private->m_listenerWrappers.begin(); iter != m_private->m_listenerWrappers.end(); ++iter) {
> +        iter->eventListenerWrapper->webEventListenerDeleted();
> +    }

nit: no {}'s around single line statements.


> +EventListenerWrapper* WebEventListenerPrivate::createEventListenerWrapper(const WebString& eventType,
> +        bool useCapture,
> +        Node* node)

nit: don't wrap these parameters.


> +EventListenerWrapper* WebEventListenerPrivate::getEventListenerWrapper(const WebString& eventType,
> +        bool useCapture,
> +        Node* node)

ditto


> +void EventListenerWrapper::handleEvent(ScriptExecutionContext* context, Event* event)
> +{
> +  if (!m_webEventListener)
> +      return;
> +  WebEvent webEvent = WebEvent::createWebEvent(event);
> +  m_webEventListener->handleEvent(webEvent);
> +}

nit: indent by 4 spaces


> +void EventListenerWrapper::webEventListenerDeleted()
> +{
> +    m_webEventListener = NULL;
> +}

nit: use 0 instead of NULL


> Index: WebKit/chromium/src/WebEventListenerImpl.h
...
> +#include "EventListener.h"
> +
> +#include "WebString.h"
> +
> +#include <wtf/Vector.h>

nit: for consistency with other headers, this should just be:

  #include "WebString.h"
  
  #include "EventListener.h"
  #include <wtf/Vector.h>


> +     EventListenerWrapper* createEventListenerWrapper(
> +         const WebString& eventType, bool useCapture, Node* node);

nit: indentation should be 4 spaces


> +
> +     // Gets the ListenerEventWrapper for a specific node.
> +     // Used by WebNode::removeEventListener().
> +     EventListenerWrapper* getEventListenerWrapper(
> +         const WebString& eventType, bool useCapture, Node* node);

ditto


> +private:
> +    friend class WebEventListener;

can you just add a public method for WebEventListener to use?  not
too many people will be using EventListenerWrapper, so it should be
OK to expose a public helper method to all, right?


> +    WTF::Vector<ListenerInfo> m_listenerWrappers;

nit: no need for the WTF:: prefix.  there is a global "using namespace WTF"


> +};
> +
> +class EventListenerWrapper : public EventListener

there should be separate header files for each class.  one for EventListenerWrapper
and one for WebEventListenerPrivate.


> +    virtual bool operator==(const EventListener& listener);
> +    virtual void handleEvent(ScriptExecutionContext* context, Event* event);

nit: in webkit style, you should only mention the parameter name if it adds
value.  in this case, listener, context, and event are all redundant, so the
parameter names should be dropped.


> Index: WebKit/chromium/src/WebMutationEvent.cpp
...
> +}
> +
> +

nit: remove extra new lines at the bottom of the file


> Index: WebKit/chromium/src/WebNode.cpp
...
> +void WebNode::addEventListener(const WebString& eventType,
> +    WebEventListener* listener,
> +    bool useCapture)

nit: either do not wrap the parameters or list them all vertically in a column
like so:

  void Foo(A a,
           B b,
           C c)


> +void WebNode::removeEventListener(const WebString& eventType,
> +    WebEventListener* listener,
> +    bool useCapture)
> +{
> +  EventListenerWrapper* listenerWrapper =
> +      listener->m_private->getEventListenerWrapper(eventType, useCapture, m_private);
> +  m_private->removeEventListener(eventType, listenerWrapper, useCapture);
> +  // listenerWrapper is now deleted.
> +}

nit: indent by 4 spaces

[Jay Campan]

Created attachment 47377 [details]
Adding event handler to the Chromium API

[WebKit Review Bot]

Attachment 47377 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebEventListenerPrivate.h:50:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/src/EventListenerWrapper.h:47:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/src/WebEventListener.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebEvent.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebEvent.cpp:228:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WebKit/chromium/src/EventListenerWrapper.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebEventListenerPrivate.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebEventListenerPrivate.cpp:63:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/chromium/src/WebEventListenerPrivate.cpp:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 9


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 47377 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/210872

[Jay Campan]

Created attachment 47415 [details]
Chromium Event Listener API

Fixed style issues

[Jay Campan]

(In reply to comment #6)

I addressed all the issues you reported.

> (From update of attachment 47249 [details])
> > Index: WebKit/chromium/ChangeLog
> ...
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Adding EventListeners to the chromium API.
> 
> ^^^ Add a bug link here.

Done.

> 
> > +        * WebKit.gyp:
> > +        * public/WebEvent.h: Added.
> > +        (WebKit::WebEvent::):
> > +        (WebKit::WebEvent::WebEvent):
> > +        (WebKit::WebEvent::operator=):
> > +        (WebKit::WebEvent::isNull):
> 
> ^^^ when adding a new file, please remove the lines here that
> reference the methods contained in that new file.

Done.

> 
> > Index: WebKit/chromium/public/WebEvent.h
> ...
> > +#ifndef WebEvent_h
> > +#define WebEvent_h
> > +
> > +#include "WebCommon.h"
> > +#include "WebNode.h"
> > +#include "WebString.h"
> > +
> > +namespace WebCore { class Event; }
> > +#if WEBKIT_IMPLEMENTATION
> > +namespace WTF { template <typename T> class PassRefPtr; }
> > +#endif
> > +
> > +namespace WebKit {
> > +
> > +class WebEvent {
> > +public:
> > +    enum PhaseType {
> > +        CapturingPhase     = 1,
> > +        AtTarget           = 2,
> > +        BubblingPhase      = 3
> > +    };
> > +
> > +    WebEvent() : m_private(0) { }
> > +    WebEvent(const WebEvent& n) : m_private(0) { assign(n); }
> > +    WebEvent& operator=(const WebEvent& n)
> 
> nit: "n" -> "e"
Done.
 
> > +    template<typename T> T toMutationEvent()
> 
> the point of this being a template is so that it can be used with
> multiple subclasses.  so, calling it toMutationEvent seems wrong.
> how about just calling it toEvent?
> 
>   WebMutationEvent me = event.toEvent<WebMutationEvent>();
> 
> I would even be happy with the method name "to", but then "toConst"
> looks a little funny.
Changed to toEvent/toConstEvent.
 
> 
> > Index: WebKit/chromium/public/WebEventListener.h
> ...
> > +class WebEventListener {
> > +public:
> > +    WebEventListener();
> > +    virtual ~WebEventListener();
> > +
> > +    // Called when an event is received.
> > +    virtual void handleEvent(const WebEvent&) = 0;
> > +
> > +private:
> > +    friend class EventListenerWrapper;
> > +    friend class WebNode;
> 
> instead of making friends, how about defining public methods
> wrapped with?
Done.

> 
> > Index: WebKit/chromium/public/WebMutationEvent.h
> ...
> > +namespace WebCore { class Event; }
> > +#if WEBKIT_IMPLEMENTATION
> > +namespace WTF { template <typename T> class PassRefPtr; }
> > +#endif
> 
> ^^^ this stuff is already declared in WebEvent.h
Removed.

> > +namespace WebKit {
> > +
> > +class WebMutationEvent : public WebEvent {
> > +public:
> > +    enum AttrChangeType {
> > +        Modification    = 1,
> > +        Addition        = 2,
> > +        Removal         = 3
> > +    };
> > +
> > +#if WEBKIT_IMPLEMENTATION
> > +    WebMutationEvent(const WTF::PassRefPtr<WebCore::Event>&);
> > +#endif
> 
> nit: convention is to move WEBKIT_IMPLEMENTATION stuff down to
> the bottom of the class definition.
Done.

> > Index: WebKit/chromium/public/WebNode.h
> ...
> > +    virtual void addEventListener(
> > +        const WebString& eventType,
> > +        WebEventListener* listener,
> > +        bool useCapture);
> > +    virtual void removeEventListener(
> > +        const WebString& eventType,
> > +        WebEventListener* listener,
> > +        bool useCapture);
> 
> minor nit: please move these above the toElement and toConstElement methods.
> 
> they also should not be virtual.  please add WEBKIT_API to them.
Done.

> > Index: WebKit/chromium/public/WebString.h
> ...
> >          assign(s);
> >          return *this;
> >      }
> > +    WEBKIT_API bool equals(const WebString& s) const;
> 
> nit: butting this up right next to operator= seems to suggest that it is
> related.  instead, how about putting it below assign with a new line separating
> it from other methods?
Done.

> > Index: WebKit/chromium/src/WebEvent.cpp
> 
> > +WebString WebEvent::type() const
> > +{
> > +    return m_private->type();
> 
> in all of the methods that dereference m_private, please add an
> ASSERT(m_private);
Done.

> > Index: WebKit/chromium/src/WebEventListenerImpl.cpp
> ...
> > +#include "WebEventListenerImpl.h"
> > +
> > +#include "WebEvent.h"
> > +#include "WebEventListener.h"
> > +
> > +#include "Event.h"
> > +#include "EventListener.h"
> 
> webkit api convention is to list the WebCore headers
> above the WebKit headers.
Done.

> > +WebEventListener::~WebEventListener()
> > +{
> > +    // Notifies all WebEventListenerWrappers that we are going away so they can
> > +    // invalidate their pointer to us.
> > +    WTF::Vector<WebEventListenerPrivate::ListenerInfo>::const_iterator iter;
> > +    for (iter = m_private->m_listenerWrappers.begin(); iter != m_private->m_listenerWrappers.end(); ++iter) {
> > +        iter->eventListenerWrapper->webEventListenerDeleted();
> > +    }
> 
> nit: no {}'s around single line statements.
Done.

> > +EventListenerWrapper* WebEventListenerPrivate::createEventListenerWrapper(const WebString& eventType,
> > +        bool useCapture,
> > +        Node* node)
> 
> nit: don't wrap these parameters.
Done.

> > +EventListenerWrapper* WebEventListenerPrivate::getEventListenerWrapper(const WebString& eventType,
> > +        bool useCapture,
> > +        Node* node)
> 
> ditto
Done
 
> > +void EventListenerWrapper::handleEvent(ScriptExecutionContext* context, Event* event)
> > +{
> > +  if (!m_webEventListener)
> > +      return;
> > +  WebEvent webEvent = WebEvent::createWebEvent(event);
> > +  m_webEventListener->handleEvent(webEvent);
> > +}
> 
> nit: indent by 4 spaces
Done.

> > +void EventListenerWrapper::webEventListenerDeleted()
> > +{
> > +    m_webEventListener = NULL;
> > +}
> 
> nit: use 0 instead of NULL
Done. 

> > Index: WebKit/chromium/src/WebEventListenerImpl.h
> ...
> > +#include "EventListener.h"
> > +
> > +#include "WebString.h"
> > +
> > +#include <wtf/Vector.h>
> 
> nit: for consistency with other headers, this should just be:
> 
>   #include "WebString.h"
> 
>   #include "EventListener.h"
>   #include <wtf/Vector.h>
Done.

> > +     EventListenerWrapper* createEventListenerWrapper(
> > +         const WebString& eventType, bool useCapture, Node* node);
> 
> nit: indentation should be 4 spaces
Done.

> > +
> > +     // Gets the ListenerEventWrapper for a specific node.
> > +     // Used by WebNode::removeEventListener().
> > +     EventListenerWrapper* getEventListenerWrapper(
> > +         const WebString& eventType, bool useCapture, Node* node);
> 
> ditto
Done
 
> > +private:
> > +    friend class WebEventListener;
> 
> can you just add a public method for WebEventListener to use?  not
> too many people will be using EventListenerWrapper, so it should be
> OK to expose a public helper method to all, right?
Done.
 
> > +    WTF::Vector<ListenerInfo> m_listenerWrappers;
> 
> nit: no need for the WTF:: prefix.  there is a global "using namespace WTF"
Done.
 
> > +};
> > +
> > +class EventListenerWrapper : public EventListener
> 
> there should be separate header files for each class.  one for
> EventListenerWrapper
> and one for WebEventListenerPrivate.
OK, split each class in its own h/cpp files.

> > +    virtual bool operator==(const EventListener& listener);
> > +    virtual void handleEvent(ScriptExecutionContext* context, Event* event);
> nit: in webkit style, you should only mention the parameter name if it adds
> value.  in this case, listener, context, and event are all redundant, so the
> parameter names should be dropped.
Done.

> > Index: WebKit/chromium/src/WebMutationEvent.cpp
> ...
> > +}
> > +
> > +
> 
> nit: remove extra new lines at the bottom of the file
Done.
 
> > Index: WebKit/chromium/src/WebNode.cpp
> ...
> > +void WebNode::addEventListener(const WebString& eventType,
> > +    WebEventListener* listener,
> > +    bool useCapture)
> 
> nit: either do not wrap the parameters or list them all vertically in a column
> like so:
> 
>   void Foo(A a,
>            B b,
>            C c)
Done.

> > +void WebNode::removeEventListener(const WebString& eventType,
> > +    WebEventListener* listener,
> > +    bool useCapture)
> > +{
> > +  EventListenerWrapper* listenerWrapper =
> > +      listener->m_private->getEventListenerWrapper(eventType, useCapture, m_private);
> > +  m_private->removeEventListener(eventType, listenerWrapper, useCapture);
> > +  // listenerWrapper is now deleted.
> > +}
> 
> nit: indent by 4 spaces
Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 47415 [details]
Chromium Event Listener API

> Index: WebKit/chromium/public/WebEvent.h
...
> +    WebEvent(const WebEvent& n) : m_private(0) { assign(n); }

same nit as before:  "n" -> "e"


> Index: WebKit/chromium/public/WebEventListener.h
...
> +namespace WebCore { class Node; }

nit: please wrap this Node declaration with #if WEBKIT_IMPLEMENTATION


> Index: WebKit/chromium/public/WebMutationEvent.h
...
> +namespace WebCore { class Event; }

nit: please wrap this Event declaration with #if WEBKIT_IMPLEMENTATION


> Index: WebKit/chromium/src/EventListenerWrapper.h
...
> +    EventListenerWrapper(WebEventListener* webEventListener);

nit: no need to name this parameter.


> Index: WebKit/chromium/src/WebEvent.cpp
...
> +WebEvent WebEvent::createWebEvent(const WTF::PassRefPtr<WebCore::Event>& event)

nit: this method should just be named "create".  also, why do we need
this method?  since WebMutationEvent and all subclasses of WebEvent
should have no member variables, we do not need to allocate a
WebMutationEvent here.

I think a simple WebEvent constructor would work best.


> +{
> +    if (event->isMutationEvent())
> +      return WebMutationEvent(event);

nit: indentation error.


looking good otherwise.

[Jay Campan]

Created attachment 47438 [details]
Chromium Event Listener API

[Jay Campan]

Addressed all issues you reported.

(In reply to comment #12)
> (From update of attachment 47415 [details])
> > Index: WebKit/chromium/public/WebEvent.h
> ...
> > +    WebEvent(const WebEvent& n) : m_private(0) { assign(n); }
> 
> same nit as before:  "n" -> "e"
Oops, fixed.

> > Index: WebKit/chromium/public/WebEventListener.h
> ...
> > +namespace WebCore { class Node; }
> 
> nit: please wrap this Node declaration with #if WEBKIT_IMPLEMENTATION
Done

> > Index: WebKit/chromium/public/WebMutationEvent.h
> ...
> > +namespace WebCore { class Event; }
> 
> nit: please wrap this Event declaration with #if WEBKIT_IMPLEMENTATION
Done

> > Index: WebKit/chromium/src/EventListenerWrapper.h
> ...
> > +    EventListenerWrapper(WebEventListener* webEventListener);
> 
> nit: no need to name this parameter.
Removed

> > Index: WebKit/chromium/src/WebEvent.cpp
> ...
> > +WebEvent WebEvent::createWebEvent(const WTF::PassRefPtr<WebCore::Event>& event)
> 
> nit: this method should just be named "create".  also, why do we need
> this method?  since WebMutationEvent and all subclasses of WebEvent
> should have no member variables, we do not need to allocate a
> WebMutationEvent here.
> 
> I think a simple WebEvent constructor would work best.
Ah, good point.
Removed the method, now only using the constructor.

> > +{
> > +    if (event->isMutationEvent())
> > +      return WebMutationEvent(event);
> 
> nit: indentation error.
This code is now gone.

[WebKit Review Bot]

Attachment 47438 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 94, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 87, in main
    style_checker.check_patch(patch)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 932, in check_patch
    self.check_file(file_path, handle_patch_style_error)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 897, in check_file
    process_file(processor, file_path, handle_style_error)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 858, in _process_file
    'One or more unexpected \\r (^M) found;'
TypeError: handle_patch_style_error() takes exactly 4 arguments (5 given)


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 47438 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/214483

[Eric Seidel (no email)]

Something confused the stylebot.  abarth is your man.

[Darin Fisher (:fishd, Google)]

Comment on attachment 47438 [details]
Chromium Event Listener API

> Index: WebKit/chromium/public/WebEvent.h
...
> +    template<typename T> T* unwrap()
> +    {
> +        return static_cast<T*>(m_private);
> +    }
> +
> +    template<typename T> const T* constUnwrap() const
> +    {
> +        return static_cast<const T*>(m_private);
> +    }

^^^ these unwrap methods are not used.

everything else LGTM.


also, it looks like the it doesn't compile properly under chromium linux.

[Jay Campan]

Created attachment 47453 [details]
Chromium Event Listener API

Removed the unused unwrap methods in WebEvent.h
Fixed some includes to fix the build.

[Jay Campan]

(In reply to comment #18)
> (From update of attachment 47438 [details])
> > Index: WebKit/chromium/public/WebEvent.h
> ...
> > +    template<typename T> T* unwrap()
> > +    {
> > +        return static_cast<T*>(m_private);
> > +    }
> > +
> > +    template<typename T> const T* constUnwrap() const
> > +    {
> > +        return static_cast<const T*>(m_private);
> > +    }
> 
> ^^^ these unwrap methods are not used.
Removed.
 
> everything else LGTM.
> 
> 
> also, it looks like the it doesn't compile properly under chromium linux.
I'll keep an eye on the try bot.

[WebKit Commit Bot]

Comment on attachment 47453 [details]
Chromium Event Listener API

Clearing flags on attachment: 47453

Committed r53934: <http://trac.webkit.org/changeset/53934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

It turns out that WebMutationEvent.cpp was missing from the last patch.  I went ahead and checked that in to fix the build bustage.

See http://trac.webkit.org/changeset/53939

[Eric Seidel (no email)]

Thank you fishd!