[webkit-reviews] review denied: [Bug 33882] The Chromium API should expose EventListeners : [Attachment 47249] Cleaned-up patch. All needed basic EventListener functionalities implemented.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 33882: The Chromium API should expose EventListeners
https://bugs.webkit.org/show_bug.cgi?id=33882

Attachment 47249: Cleaned-up patch. All needed basic EventListener
functionalities implemented.
https://bugs.webkit.org/attachment.cgi?id=47249&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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