[webkit-reviews] review denied: [Bug 60779] Bug in rubber banding logic for scroll animators : [Attachment 93558] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jon Lee <jonlee at apple.com>'s request for
review:
Bug 60779: Bug in rubber banding logic for scroll animators
https://bugs.webkit.org/show_bug.cgi?id=60779

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93558&action=review

Looking really good, but lets go another around.

> Source/WebCore/page/Frame.cpp:1130
> +    for (const Frame* frame = this; frame; frame =
frame->tree()->traverseNext())
> +	   if (frame->document())
> +	       count += frame->document()->wheelEventHandlerCount();

This should have braces around for-loop.

> Source/WebCore/page/FrameView.cpp:314
> +    scrollAnimator()->didAddHorizontalScrollbar(scrollbar);

This should call ScrollView::didAddHorizontalScrollbar(scrollbar) instead of
calling the animator directly.

> Source/WebCore/page/FrameView.cpp:319
> +    scrollAnimator()->willRemoveHorizontalScrollbar(scrollbar);

Same here.

> Source/WebKit2/UIProcess/API/C/WKPage.h:291
> +WK_EXPORT bool WKPageShouldWaitForWheelEvents(WKPageRef page);

I would rather this have a name and functionality that would work for all
builds, including those without gesture events.  Perhaps, something along the
lines of horizontal scrolls being noops...