[webkit-reviews] review granted: [Bug 128225] Wheel events don't latch to inner scrollable elements : [Attachment 223983] Revised per Smfr's comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 12 12:10:35 PST 2014


Simon Fraser (smfr) <simon.fraser at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 128225: Wheel events don't latch to inner scrollable elements
https://bugs.webkit.org/show_bug.cgi?id=128225

Attachment 223983: Revised per Smfr's comments
https://bugs.webkit.org/attachment.cgi?id=223983&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223983&action=review


> Source/WebCore/page/EventHandler.h:202
> +    void platformPrepareForWheelEvents(const PlatformWheelEvent&, const
HitTestResult&, Element*&, ContainerNode*&, ScrollableArea*&, bool&);

You should give the parameters names here to say what they mean.

> Source/WebCore/page/EventHandler.h:204
> +    bool platformCompleteWheelEvent(const PlatformWheelEvent&,
ContainerNode*, ScrollableArea*);

Here too.

> Source/WebCore/page/mac/EventHandlerMac.mm:745
> +    // node and traversing it's parents (or shadow hosts).

it's :|

> Source/WebCore/page/mac/EventHandlerMac.mm:755
> +static bool isAtMaxDominantScrollPosition(const ScrollableArea& area,
DominantScrollGestureDirection direction, float deltaX, float deltaY)

scrolledToEdgeInDominantDirection?

> Source/WebCore/page/mac/EventHandlerMac.mm:770
> +void EventHandler::platformPrepareForWheelEvents(const PlatformWheelEvent&
e, const HitTestResult& result, Element*& element, ContainerNode*&
scrollableContainer, ScrollableArea*& scrollableArea, bool& isOverWidget)

Please don't abbreviate event to 'e'. 'element' could use a more descriptive
name.

> Source/WebCore/page/mac/EventHandlerMac.mm:778
> +	   scrollableArea = static_cast<ScrollableArea*>(view);

This cast should not be necessary.

> Source/WebCore/page/mac/EventHandlerMac.mm:787
> +	   scrollableContainer = findScrollableContainer(*element);
> +	   if (scrollableContainer) {
> +	       if (RenderBox* box = scrollableContainer->renderBox()) {
> +		   if (box->isListBox())
> +		       scrollableArea = toRenderListBox(box);
> +		   else
> +		       scrollableArea = box->layer();
> +	       }

Maybe findScrollableContainer() should be
ScrollableArea* findEnclosingScrollableArea(Element&*)


More information about the webkit-reviews mailing list