[webkit-reviews] review granted: [Bug 57247] Move more events to EventDispatcher. : [Attachment 87161] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 57247: Move more events to EventDispatcher.
https://bugs.webkit.org/show_bug.cgi?id=57247

Attachment 87161: Patch
https://bugs.webkit.org/attachment.cgi?id=87161&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87161&action=review

I reviewed the code you moved, rather than just reviewing the move.

> Source/WebCore/dom/EventDispatcher.cpp:86
> +inline static EventTarget* eventTargetRespectingSVGTargetRules(Node*
referenceNode)
> +{
> +    ASSERT(referenceNode);
> +
> +#if ENABLE(SVG)
> +    if (!referenceNode->isSVGElement())
> +	   return referenceNode;
> +
> +    // Spec: The event handling for the non-exposed tree works as if the
referenced element had been textually included
> +    // as a deeply cloned child of the 'use' element, except that events are
dispatched to the SVGElementInstance objects
> +    for (Node* n = referenceNode; n; n = n->parentNode()) {
> +	   if (!n->isShadowRoot() || !n->isSVGElement())
> +	       continue;
> +
> +	   Element* shadowTreeParentElement = n->shadowHost();
> +	   ASSERT(shadowTreeParentElement->hasTagName(SVGNames::useTag));
> +
> +	   if (SVGElementInstance* instance =
static_cast<SVGUseElement*>(shadowTreeParentElement)->instanceForShadowTreeElem
ent(referenceNode))
> +	       return instance;
> +    }
> +#endif
> +
> +    return referenceNode;
> +}

This function might be too long to inline in two places. Instead perhaps we
could inline the non-SVG part and make the SVG part be a non-inline function.

> Source/WebCore/dom/EventDispatcher.cpp:96
> +bool EventDispatcher::dispatchKeyEvent(Node* node, const
PlatformKeyboardEvent& key)

An event is not a key, so this argument should be named “event”, not “key”.

The function name should probably be dispatchKeyboardEvent.

> Source/WebCore/dom/EventDispatcher.cpp:109
> +    bool r = dispatcher.dispatchEvent(keyboardEvent);
> +
> +    // We want to return false if default is prevented (already taken care
of)
> +    // or if the element is default-handled by the DOM. Otherwise we let it
just
> +    // let it get handled by AppKit.
> +    if (keyboardEvent->defaultHandled())
> +	   r = false;
> +
> +    return r;

I think that “r” is not a good name for the boolean return value.

The mention of AppKit in this comment is comical and outdated.

I would write this code like this:

    return dispatch.dispatchEvent(keyboardEvent) &&
!keyboardEvent->defaultHandled();

A comment explaining why this function looks at defaultHandled and the standard
dispatchEvent return value does not would also be welcome. Since the existing
comment says nothing so I couldn’t reword it; we’d have to research why this
difference between the dispatchEvent return value and the dispatchKeyEvent
return value is a good idea.

> Source/WebCore/dom/EventDispatcher.cpp:157
> +void EventDispatcher::dispatchWheelEvent(Node* node, PlatformWheelEvent& e)

This should be “event”, not “e”.

> Source/WebCore/dom/EventDispatcher.cpp:168
> +    IntPoint pos = dispatcher.m_view->windowToContents(e.pos());

This should be “position”, not “p”.

> Source/WebCore/dom/EventDispatcher.cpp:175
> +	       // Adjust our pageX and pageY to account for the page zoom.

This comment adds nothing.

> Source/WebCore/dom/EventDispatcher.cpp:190
> +    WheelEvent::Granularity granularity;
> +    switch (e.granularity()) {
> +    case ScrollByPageWheelEvent:
> +	   granularity = WheelEvent::Page;
> +	   break;
> +    case ScrollByPixelWheelEvent:
> +    default:
> +	   granularity = WheelEvent::Pixel;
> +	   break;
> +    }

This conversion should have its own function. Having it written out like this
is ugly.

> Source/WebCore/dom/EventDispatcher.cpp:192
> +    RefPtr<WheelEvent> we = WheelEvent::create(e.wheelTicksX(),
e.wheelTicksY(), e.deltaX(), e.deltaY(), granularity,

This should be named “wheelEvent”, not “we”.

> Source/WebCore/dom/EventDispatcher.cpp:196
> +    we->setAbsoluteLocation(IntPoint(pos.x(), pos.y()));

IntPoint(pos.x(), pos.y()) is the same as just “pos”!

> Source/WebCore/dom/EventDispatcher.cpp:201
> +    we.release();

This line of code should be removed.