Bug 52779 – Cleanup Scrollbar/ScrollbarClient relationship

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

Instead of routing all scrolls through the Scrollbar, the Scrollbar should just be a dumb "view" onto the scrollable area (being told when the scroll offset has changed).

[Sam Weinig]

Created attachment 79546 [details]
WIP (Do not review)

Uploading WIP patch to see how the bots feel about it.

[Early Warning System Bot]

Attachment 79546 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7539233

[WebKit Review Bot]

Attachment 79546 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7516192

[WebKit Review Bot]

Attachment 79546 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7568218

[Sam Weinig]

Created attachment 79549 [details]
WIP-2 (Do not review)

[WebKit Review Bot]

Attachment 79549 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7610233

[Early Warning System Bot]

Attachment 79549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7523238

[Eric Seidel (no email)]

If you're just looking for ews results you can remove the review flag as soon as numbers appear in the bubbles.

you can also submit patches directly using http://queues.webkit.org/submit-to-ews

[Sam Weinig]

(In reply to comment #8)
> If you're just looking for ews results you can remove the review flag as soon as numbers appear in the bubbles.
> 
> you can also submit patches directly using http://queues.webkit.org/submit-to-ews

Cool!

[Sam Weinig]

Created attachment 79621 [details]
Attempt 3

[WebKit Review Bot]

Attachment 79621 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7609231

[Sam Weinig]

Created attachment 79636 [details]
Attempt 4

[Sam Weinig]

Created attachment 79640 [details]
Patch

[Sam Weinig]

Created attachment 79641 [details]
Patch

[Dave Hyatt]

Comment on attachment 79641 [details]
Patch

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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76291 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug

[Peter Kasting]

Sam, do you want to look at bug 45005, and
 * Dupe that against this if relevant
 * See if there were any useful details on that bug you haven't done here

I haven't looked to see what you've done on this bug but from a high level it sounds very much like what that bug was about.

[Sam Weinig]

*** Bug 45005 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Created attachment 79769 [details]
Part 2

[WebKit Review Bot]

Attachment 79769 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/ScrollView.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/win/WebScrollBar.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/ScrollbarThemeComposite.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/Scrollbar.h:46:  The parameter name "orientation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Scrollbar.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 79769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7569263

[Sam Weinig]

Forgot to mention, part 1 landed in http://trac.webkit.org/changeset/76291.

[Sam Weinig]

Comment on attachment 79769 [details]
Part 2

Part 2 landed in http://trac.webkit.org/changeset/76378.

[Build Bot]

Attachment 79769 [details] did not build on win:
Build output: http://queues.webkit.org/results/7570267

[WebKit Review Bot]

http://trac.webkit.org/changeset/76378 might have broken Chromium Linux Release

[Sam Weinig]

Marking fixed for now. Additional cleanups will come later.