[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