Created attachment 87161 [details] Patch

Comment on attachment 87161 [details] Patch 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)->instanceForShadowTreeElement(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.

Comment on attachment 87161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87161&action=review Made changes you suggested. Landing soon. >> Source/WebCore/dom/EventDispatcher.cpp:86 >> +} > > 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. Good idea. Done. >> 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. Yup. >> Source/WebCore/dom/EventDispatcher.cpp:109 >> + 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. Returning true/false has to do with WebKit layer attempting to handle the event. Wrote a better comment, changed the code as suggested. >> Source/WebCore/dom/EventDispatcher.cpp:157 >> +void EventDispatcher::dispatchWheelEvent(Node* node, PlatformWheelEvent& e) > > This should be “event”, not “e”. Done. >> Source/WebCore/dom/EventDispatcher.cpp:168 >> + IntPoint pos = dispatcher.m_view->windowToContents(e.pos()); > > This should be “position”, not “p”. Done. >> Source/WebCore/dom/EventDispatcher.cpp:175 >> + // Adjust our pageX and pageY to account for the page zoom. > > This comment adds nothing. Gone. >> Source/WebCore/dom/EventDispatcher.cpp:190 >> + } > > This conversion should have its own function. Having it written out like this is ugly. Done. >> 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”. Yup. >> Source/WebCore/dom/EventDispatcher.cpp:196 > > IntPoint(pos.x(), pos.y()) is the same as just “pos”! :) That's funny. Changed. >> Source/WebCore/dom/EventDispatcher.cpp:201 >> + we.release(); > > This line of code should be removed. Yeah.

Committed r82160: <http://trac.webkit.org/changeset/82160>