Bug 52779

Summary: Cleanup Scrollbar/ScrollbarClient relationship
Product: WebKit Reporter: Sam Weinig <sam>
Component: PlatformAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bdakin, buildbot, dglazkov, eric, gns, pkasting, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
WIP (Do not review)
none
WIP-2 (Do not review)
none
Attempt 3
none
Attempt 4
none
Patch
none
Patch
hyatt: review+
Part 2 none

Description Sam Weinig 2011-01-19 20:26:40 PST
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).
Comment 1 Sam Weinig 2011-01-19 20:28:27 PST
Created attachment 79546 [details]
WIP (Do not review)

Uploading WIP patch to see how the bots feel about it.
Comment 2 Early Warning System Bot 2011-01-19 20:52:01 PST
Attachment 79546 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7539233
Comment 3 WebKit Review Bot 2011-01-19 20:52:32 PST
Attachment 79546 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7516192
Comment 4 WebKit Review Bot 2011-01-19 20:57:15 PST
Attachment 79546 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7568218
Comment 5 Sam Weinig 2011-01-19 21:37:35 PST
Created attachment 79549 [details]
WIP-2 (Do not review)
Comment 6 WebKit Review Bot 2011-01-19 21:49:54 PST
Attachment 79549 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7610233
Comment 7 Early Warning System Bot 2011-01-19 21:53:18 PST
Attachment 79549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7523238
Comment 8 Eric Seidel (no email) 2011-01-20 02:30:19 PST
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
Comment 9 Sam Weinig 2011-01-20 11:11:10 PST
(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!
Comment 10 Sam Weinig 2011-01-20 11:25:02 PST
Created attachment 79621 [details]
Attempt 3
Comment 11 WebKit Review Bot 2011-01-20 11:36:14 PST
Attachment 79621 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7609231
Comment 12 Sam Weinig 2011-01-20 12:40:51 PST
Created attachment 79636 [details]
Attempt 4
Comment 13 Sam Weinig 2011-01-20 12:56:22 PST
Created attachment 79640 [details]
Patch
Comment 14 Sam Weinig 2011-01-20 12:58:28 PST
Created attachment 79641 [details]
Patch
Comment 15 Dave Hyatt 2011-01-20 13:08:51 PST
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.
Comment 16 WebKit Review Bot 2011-01-20 15:22:45 PST
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
Comment 17 Peter Kasting 2011-01-20 15:32:35 PST
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.
Comment 18 Sam Weinig 2011-01-21 11:32:34 PST
*** Bug 45005 has been marked as a duplicate of this bug. ***
Comment 19 Sam Weinig 2011-01-21 11:40:47 PST
Created attachment 79769 [details]
Part 2
Comment 20 WebKit Review Bot 2011-01-21 11:43:12 PST
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.
Comment 21 WebKit Review Bot 2011-01-21 11:59:19 PST
Attachment 79769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7569263
Comment 22 Sam Weinig 2011-01-21 12:05:55 PST
Forgot to mention, part 1 landed in http://trac.webkit.org/changeset/76291.
Comment 23 Sam Weinig 2011-01-21 12:15:45 PST
Comment on attachment 79769 [details]
Part 2

Part 2 landed in http://trac.webkit.org/changeset/76378.
Comment 24 Build Bot 2011-01-21 12:19:42 PST
Attachment 79769 [details] did not build on win:
Build output: http://queues.webkit.org/results/7570267
Comment 25 WebKit Review Bot 2011-01-21 12:33:03 PST
http://trac.webkit.org/changeset/76378 might have broken Chromium Linux Release
Comment 26 Sam Weinig 2011-01-21 13:39:51 PST
Marking fixed for now. Additional cleanups will come later.