Bug 98700 - ScrollingCoordinator is a hot mess of if-defs
Summary: ScrollingCoordinator is a hot mess of if-defs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-08 16:00 PDT by Beth Dakin
Modified: 2012-10-11 21:24 PDT (History)
11 users (show)

See Also:

Attachments
Patch (73.84 KB, patch)
2012-10-09 22:12 PDT, Beth Dakin 		no flags Details | Formatted Diff | Diff
Patch (74.43 KB, patch)
2012-10-09 22:14 PDT, Beth Dakin 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (75.38 KB, patch)
2012-10-10 11:33 PDT, Beth Dakin 		peter+ews: commit-queue-
Details | Formatted Diff | Diff
Patch (76.00 KB, patch)
2012-10-10 22:09 PDT, Beth Dakin 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (76.70 KB, patch)
2012-10-11 10:21 PDT, Beth Dakin 		no flags Details | Formatted Diff | Diff
Patch (77.80 KB, patch)
2012-10-11 10:29 PDT, James Robinson 		no flags Details | Formatted Diff | Diff
Patch (76.73 KB, patch)
2012-10-11 11:35 PDT, Beth Dakin 		andersca: review+
Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-10-08 16:00:42 PDT 
ScrollingCoordinator is a really confusing class because of all of the if-defs and because of ScrollingCoordinatorNone and because of platform implementations. It is very difficult to add or change API to this class as a result.

I propose that we make ScrollingCoordinator just be a typedef that defines ScrollingCoordinatorMac to be ScrollingCoordinator, and same for Chromium and other platforms. Any shared functionality can be in something called ScrollingCoordinatorBase which all of the platform implementations will inherit from.
Comment 1 Beth Dakin 2012-10-09 22:12:06 PDT 
Created attachment 167919 [details]
Patch
Comment 2 Beth Dakin 2012-10-09 22:14:13 PDT 
Created attachment 167921 [details]
Patch
Comment 3 WebKit Review Bot 2012-10-09 22:37:24 PDT 
Comment on attachment 167921 [details]
Patch

Attachment 167921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14229880
Comment 4 Build Bot 2012-10-09 23:24:32 PDT 
Comment on attachment 167921 [details]
Patch

Attachment 167921 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14221994
Comment 5 Peter Beverloo (cr-android ews) 2012-10-09 23:50:48 PDT 
Comment on attachment 167921 [details]
Patch

Attachment 167921 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14254051
Comment 6 Beth Dakin 2012-10-10 11:33:35 PDT 
Created attachment 168047 [details]
Patch

Not sure why Chromium isn't building…
Comment 7 Peter Beverloo (cr-android ews) 2012-10-10 12:02:48 PDT 
Comment on attachment 168047 [details]
Patch

Attachment 168047 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14253324
Comment 8 WebKit Review Bot 2012-10-10 12:41:48 PDT 
Comment on attachment 168047 [details]
Patch

Attachment 168047 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14244700
Comment 9 Beth Dakin 2012-10-10 22:09:59 PDT 
Created attachment 168142 [details]
Patch
Comment 10 WebKit Review Bot 2012-10-10 22:37:22 PDT 
Comment on attachment 168142 [details]
Patch

Attachment 168142 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14252458
Comment 11 Peter Beverloo (cr-android ews) 2012-10-10 22:53:51 PDT 
Comment on attachment 168142 [details]
Patch

Attachment 168142 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14252459
Comment 12 James Robinson 2012-10-11 10:16:34 PDT 
Sorry you're having trouble with the chromium build.  I'll patch this in locally and see if I can get it up and running.  The cleanup looks great!
Comment 13 Beth Dakin 2012-10-11 10:21:07 PDT 
(In reply to comment #12)
> Sorry you're having trouble with the chromium build.  I'll patch this in locally and see if I can get it up and running.  The cleanup looks great!

Thanks, James! I knew I would be doing this dance with the ews bots since I wrote so much code without compiling it locally. I have a possible fix for the latest build errors. WIll post now.
Comment 14 Beth Dakin 2012-10-11 10:21:38 PDT 
Created attachment 168248 [details]
Patch
Comment 15 James Robinson 2012-10-11 10:29:37 PDT 
Created attachment 168251 [details]
Patch
Comment 16 James Robinson 2012-10-11 10:30:12 PDT 
Attached a patch that compiles on chromium, but I had some merge conflicts in the WebCore project.pbxproj so it's not complete.  Hope it helps!
Comment 17 Beth Dakin 2012-10-11 11:21:23 PDT 
(In reply to comment #16)
> Attached a patch that compiles on chromium, but I had some merge conflicts in the WebCore project.pbxproj so it's not complete.  Hope it helps!

Sweet, thanks!
Comment 18 Beth Dakin 2012-10-11 11:35:19 PDT 
Created attachment 168258 [details]
Patch

Okay, I *think* I merged everything properly. Here's another go for the bots.
Comment 19 Anders Carlsson 2012-10-11 21:17:12 PDT 
Comment on attachment 168258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168258&action=review

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:110
> +    // These virtual functions are currently unique to the threaded scrolling architecture, and they are
> +    // their meaningful implementations are in ScrollingCoordinatorMac.

Parse error.
Comment 20 Beth Dakin 2012-10-11 21:24:02 PDT 
Thanks Anders! http://trac.webkit.org/changeset/131137