[webkit-reviews] review denied: [Bug 134569] [Mac] WebKit1 WebView iframe not responding to scroll gestures : [Attachment 234291] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 134569: [Mac] WebKit1 WebView iframe not responding to scroll gestures
https://bugs.webkit.org/show_bug.cgi?id=134569

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

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


r- for the  NSScrollView stuff and a better name for
platformCompleteWidgetWheelEvent

> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=134569

Is there a radar too?

> Source/WebCore/ChangeLog:7
> +	   Reviewed by NOBODY (OOPS!).
> +

Please describe the problem and how you fixed it briefly here.

> Source/WebCore/page/EventHandler.cpp:2534
> +bool EventHandler::platformCompleteWidgetWheelEvent(const
PlatformWheelEvent&, ContainerNode*, bool)

Not sure what a WidgetWheelEvent is.

> Source/WebCore/page/EventHandler.cpp:2593
> +	       bool widgetIsPlatformWidget = !!widget->platformWidget();

You don't need the !! in C++

you could move this bool to just before the call to
platformCompleteWidgetWheelEvent()

> Source/WebCore/page/mac/EventHandlerMac.mm:813
> +    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX) {
> +	   SEL horizontalScrollerSelector =
NSSelectorFromString(@"horizontalScroller");
> +	   if (![widget respondsToSelector:horizontalScrollerSelector])
> +	       return true;
> +
> +	   CGFloat horizontalScroll = [[widget
performSelector:horizontalScrollerSelector] floatValue];
> +	   if (deltaX < 0)
> +	       return horizontalScroll == 1.0;
> +	   
> +	   return !horizontalScroll;
> +    }

This should do something with NSScrollView, not this selector stuff. if
([widget isKindOfClass:[NSScrollView class]])...

> Source/WebCore/page/mac/EventHandlerMac.mm:932
> +    if (!widgetIsPlatformWidget)
> +	   return true;

Why bother passing widgetIsPlatformWidget if you just early return on it here?