[webkit-reviews] review denied: [Bug 17589] Scroll wheel sensitivity ignored : [Attachment 22015] Patch attempt
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 2 17:28:42 PDT 2008
Darin Adler <darin at apple.com> has denied Maxime Britto <britto at apple.com>'s
request for review:
Bug 17589: Scroll wheel sensitivity ignored
https://bugs.webkit.org/show_bug.cgi?id=17589
Attachment 22015: Patch attempt
https://bugs.webkit.org/attachment.cgi?id=22015&action=edit
------- Additional Comments from Darin Adler <darin at apple.com>
Looks good!
Is "sensitivity" really the right name for this? The Windows APIs don't seem to
call it that. Is that the end-user name for this feature? Where does the term
come from.
The magic number 150 appears multiple times in this patch. It needs to be a
named constant if we're going to keep it.
But I don't think that using a magic number is a clean way to communicate that
we're in page scroll mode -- I suggest a separate boolean for that.
1364 //if (deltaX && node->renderer()->scroll(deltaX < 0 ?
ScrollRight : ScrollLeft, e.isContinuous() ? ScrollByPixel : ScrollByLine,
1365 // deltaX < 0 ?
-deltaX : deltaX))
1366 // e.accept();
We don't want to check in commented-out code.
1368 int pixelsToScroll = abs(deltaX);
1369 if (!e.isContinuous() && abs(deltaX) != 150) {
You're repeating abs(deltaX) a number of times here. Can you use pixelsToScroll
instead?
You say "sensibility" in a few places where you mean "sensitivity".
74 //TODO: retrieve the user setting for the number of lines to scroll on
each wheel event
We don't normally use "TODO:" -- consider FIXME instead.
39 , m_charScrollSensitivity(1) //Values needed on other platforms to let
the user to choose the numer of lines to scroll on each wheel event
I don't think this comment is helpful. You should just leave it out.
68 if(! ::SystemParametersInfo(SPI_GETWHEELSCROLLCHARS, 0,
&scrollChars, 0))
69 scrollChars = 1;
There should be a space after the if. There should not be a space after the
"(". We don't use the "::" when calling global functions.
71 //If we are in a page scroll mode we choose an arbitrary high value
72 //which can be recongnized easily later when we'll choose the
scroll granularity.
There should be spaces after the "//" and before the comments. Typo here:
"recongnized" instead of "recognized".
82 if(! ::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0,
&scrollLines, 0))
83 scrollLines = 3;
Where does this number 3 come from?
76 m_charScrollSensitivity = (int) scrollChars;
We normally use static_cast rather than C-style casts.
More information about the webkit-reviews
mailing list