[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