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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 234303: Patch
https://bugs.webkit.org/attachment.cgi?id=234303&action=review

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


review- because of the strange always-null Element* argument (see comments
below)

> Source/WebCore/page/EventHandler.cpp:2619
> +    return platformCompleteWheelEvent(e, element, scrollableContainer,
scrollableArea);

This always passes null, because we have an early return above if element is
non-null. I think it would be clearer to say nullptr here instead of “element”.
But also, I just don’t see the point of adding this argument!

> Source/WebCore/page/mac/EventHandlerMac.mm:785
> +static NSView* platformWidgetForEventTarget(ContainerNode* eventTarget)

This should take en Element* rather than a ContainerNode*.

The return value should have the space on the other side because it’s an
Objective-C type (although I want us to change that style rule soon).

> Source/WebCore/page/mac/EventHandlerMac.mm:788
> +	   return nullptr;

Should be return nil, since this is an Objective-C object pointer.

> Source/WebCore/page/mac/EventHandlerMac.mm:792
> +	   return nullptr;

Should be return nil, since this is an Objective-C object pointer.

> Source/WebCore/page/mac/EventHandlerMac.mm:794
> +    return toRenderWidget(target)->widget()->platformWidget();

What guarantees that widget() is non-null?

> Source/WebCore/page/mac/EventHandlerMac.mm:803
> +    if (![widget isKindOfClass: [NSScrollView class]])

Should not have a space after the colon here.

> Source/WebCore/page/mac/EventHandlerMac.mm:806
> +    NSScrollView *scrollView = reinterpret_cast<NSScrollView*>(widget);

It’s a little non-idiomatic to use reinterpret_cast here. Doesn’t static_cast
work? Often we just use C-style casts for Objective-C object pointers. Also
missing a space after NSScrollView before the *.

> Source/WebCore/page/mac/EventHandlerMac.mm:820
> +    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX) {
> +	   CGFloat horizontalScroll = scrollView.horizontalScroller.floatValue;

> +	   if (deltaX < 0)
> +	       return horizontalScroll == 1.0;
> +	   
> +	   return !horizontalScroll;
> +    }
> +
> +    CGFloat verticalScroll = scrollView.verticalScroller.floatValue;
> +    if (deltaY < 0)
> +	   return verticalScroll == 1.0;
> +    
> +    return !verticalScroll;

I would write it like this instead:

    if (!deltaIsPredominantlyVertical(deltaX, deltaY) && deltaX)
	return scrollView.horizontalScroller.doubleValue == (deltaX >= 0 ? 0 :
1);
    return scrollView.verticalScroller.doubleValue == (deltaY >= 0 ? 0 : 1);

I think the tighter version is easier to read and floatValue is just a slower
version of doubleValue that calls doubleValue and then converts to a float.