[webkit-reviews] review denied: [Bug 33882] The Chromium API should expose EventListeners : [Attachment 47415] Chromium Event Listener API

[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 47415: Chromium Event Listener API
https://bugs.webkit.org/attachment.cgi?id=47415&action=review

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