[webkit-reviews] review denied: [Bug 17589] Scroll wheel sensitivity ignored on Vista : [Attachment 20747] patch attempt

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Marcus <gamander at gmail.com>'s
request for review:
Bug 17589: Scroll wheel sensitivity ignored on Vista
http://bugs.webkit.org/show_bug.cgi?id=17589

Attachment 20747: patch attempt
http://bugs.webkit.org/attachment.cgi?id=20747&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
Thanks for the patch! This is a good step in the right direction.

This patch won't fix the issue for overflow areas (e.g., <div style="overflow:
scroll">). Overflow areas use RenderLayer::scroll to do their scrolling. We'll
want to fix both at the same time, and hopefully in a way that avoids
duplicating the code you've added.

+	 if (!::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0,
&linesToScroll, 0))

On Vista there's also an SPI_GETWHEELSCROLLCHARS value that we should support
for horizontal scrolling. Since this value isn't present in pre-Vista headers,
you'll have to declare it at the top of this file, like this:

static const UINT SPI_GETWHEELSCROLLCHARS = <the value that Vista uses>;

+	     linesToScroll = 3;  // Windows default value 

This should be placed into a named constant at the top of the file, like this:

static const unsigned defaultLinesToScroll = 3;

+	     // Like Mozilla, use default font to determine the number of
pixels per line.
+	     const FrameView* frameView = static_cast<const FrameView*>(this);
+	     const Frame* frame = frameView->frame();
+	     int lineStep;
+	     if (frame && frame->contentRenderer() &&
frame->contentRenderer()->style())
+		 lineStep = linesToScroll *
frame->contentRenderer()->style()->font().lineSpacing();
+	     else
+		 lineStep = linesToScroll * LINE_STEP / 2; // fallback value
+
+	     scrollBy(-e.deltaX() * lineStep, -e.deltaY() * lineStep);

This is a layering violation. Classes in platform/ should not in general be
aware of classes outside of platform/, so it seems unfortunate to be
introducing RenderView usage here. We'd also want this change to be made on all
platforms, not just Windows, I believe. This change seems logically distinct
from supporting the system preferences for scrolling amount, so this should be
turned into a separate patch.