Bug 98700

Summary:

ScrollingCoordinator is a hot mess of if-defs

Product:

WebKit

Reporter:

Beth Dakin <bdakin>

Component:

Platform

Assignee:

Beth Dakin <bdakin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, bdakin, dglazkov, gyuyoung.kim, jamesr, laszlo.gombos, peter+ews, rakuco, simon.fraser, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	webkit.review.bot: commit-queue-
Patch	peter+ews: commit-queue-
Patch	webkit.review.bot: commit-queue-
Patch	none
Patch	none
Patch	andersca: review+

Beth Dakin

Reported 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.

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 attachment proposed patch, testcase, etc.

Beth Dakin

Comment 1 2012-10-09 22:12:06 PDT

Created attachment 167919 [details] Patch

Beth Dakin

Comment 2 2012-10-09 22:14:13 PDT

Created attachment 167921 [details] Patch

WebKit Review Bot

Comment 3 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

Build Bot

Comment 4 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

Peter Beverloo (cr-android ews)

Comment 5 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

Beth Dakin

Comment 6 2012-10-10 11:33:35 PDT

Created attachment 168047 [details] Patch Not sure why Chromium isn't building…

Peter Beverloo (cr-android ews)

Comment 7 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

WebKit Review Bot

Comment 8 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

Beth Dakin

Comment 9 2012-10-10 22:09:59 PDT

Created attachment 168142 [details] Patch

WebKit Review Bot

Comment 10 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

Peter Beverloo (cr-android ews)

Comment 11 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

James Robinson

Comment 12 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!

Beth Dakin

Comment 13 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.

Beth Dakin

Comment 14 2012-10-11 10:21:38 PDT

Created attachment 168248 [details] Patch

James Robinson

Comment 15 2012-10-11 10:29:37 PDT

Created attachment 168251 [details] Patch

James Robinson

Comment 16 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!

Beth Dakin

Comment 17 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!

Beth Dakin

Comment 18 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.

Anders Carlsson

Comment 19 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.

Beth Dakin

Comment 20 2012-10-11 21:24:02 PDT

Thanks Anders! http://trac.webkit.org/changeset/131137

Note You need to log in before you can comment on or make changes to this bug.