[webkit-reviews] review granted: [Bug 52779] Cleanup Scrollbar/ScrollbarClient relationship : [Attachment 79641] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 52779: Cleanup Scrollbar/ScrollbarClient relationship
https://bugs.webkit.org/show_bug.cgi?id=52779

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79641&action=review

Looks good.

> Source/WebCore/platform/Scrollbar.cpp:310
> +	       if (m_orientation == HorizontalScrollbar)
> +		   client()->scrollToXOffsetWithoutAnimation(m_dragOrigin);
> +	       else
> +		   client()->scrollToYOffsetWithoutAnimation(m_dragOrigin);

I see this pattern in multiple places.	Maybe there should be a
scrollToOffsetWithoutAnimation(ScrollbarOrientation, int offset) function.

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:91
> +    if (that->orientation() == HorizontalScrollbar)
> +	   that->client()->scrollToXOffsetWithoutAnimation(v);
> +    else
> +	   that->client()->scrollToYOffsetWithoutAnimation(v);
>  }

See comment above about this pattern.

> Source/WebCore/platform/gtk/MainFrameScrollbarGtk.cpp:115
> +    if (that->orientation() == HorizontalScrollbar)
> +	  
that->client()->scrollToXOffsetWithoutAnimation(static_cast<int>(gtk_adjustment
_get_value(that->m_adjustment.get())));
> +    else
> +	  
that->client()->scrollToYOffsetWithoutAnimation(static_cast<int>(gtk_adjustment
_get_value(that->m_adjustment.get())));

And again.  Especially gross given the big gtk_adjustment_get_value expression
is repeated twice.

> Source/WebKit/qt/Api/qwebframe.cpp:1078
> +	   if (orientation == Qt::Horizontal)
> +	       sb->client()->scrollToXOffsetWithoutAnimation(value);
> +	   else
> +	       sb->client()->scrollToYOffsetWithoutAnimation(value);

And again.