Bug 81938

Created attachment 133301 [details] Patch

Comment on attachment 133301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133301&action=review > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:154 > - deltaX *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > - deltaY *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > + static const int cWheelScrollLines = 3; > + deltaX *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; > + deltaY *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; Make sure to keep the same code in WebCore/platform/qt/WheelEventQt.cpp up to date. But is there any good reason not to use the input delta value instead of QApplication::wheelScrollLines()? The value of a wheel-event should always either be the lines or pixels to be scrolled, and this can change if multiple wheel-events have been coalezed for instance.

(In reply to comment #2) > (From update of attachment 133301 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133301&action=review > > > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:154 > > - deltaX *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > > - deltaY *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > > + static const int cWheelScrollLines = 3; > > + deltaX *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; > > + deltaY *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; > > Make sure to keep the same code in WebCore/platform/qt/WheelEventQt.cpp up to date. > > But is there any good reason not to use the input delta value instead of QApplication::wheelScrollLines()? The value of a wheel-event should always either be the lines or pixels to be scrolled, and this can change if multiple wheel-events have been coalezed for instance. I don't really know the history of this logic, so I'm just going to explain how I understand it. In the case of a fine-resolution device we use the delta as it is. In the case of fulltick device we need to compute the pixel values because WebCore expects that. I don't think that it can change: we either get fine-resolution or fulltick values but never both, at least for one device. So I think the logic we have here is necessary and correct.

Created attachment 133470 [details] Patch

(In reply to comment #4) > Created an attachment (id=133470) [details] > Patch Applied the change to WheelEventQt.cpp as well.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 133301 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=133301&action=review > > > > > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:154 > > > - deltaX *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > > > - deltaY *= (fullTick) ? QApplication::wheelScrollLines() * cDefaultQtScrollStep : 1; > > > + static const int cWheelScrollLines = 3; > > > + deltaX *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; > > > + deltaY *= (fullTick) ? cWheelScrollLines * cDefaultQtScrollStep : 1; > > > > Make sure to keep the same code in WebCore/platform/qt/WheelEventQt.cpp up to date. > > > > But is there any good reason not to use the input delta value instead of QApplication::wheelScrollLines()? The value of a wheel-event should always either be the lines or pixels to be scrolled, and this can change if multiple wheel-events have been coalezed for instance. > > I don't really know the history of this logic, so I'm just going to explain how I understand it. In the case of a fine-resolution device we use the delta as it is. In the case of fulltick device we need to compute the pixel values because WebCore expects that. I don't think that it can change: we either get fine-resolution or fulltick values but never both, at least for one device. So I think the logic we have here is necessary and correct. Hmm, thinking it over again, now I don't see the evidence why we need to compute another delta with these constants. The bug that the comment is referring to is about fixing behavior for yahoo.com. It does some strange with the wheel events. Anyway, for now I think it's better to check in this typo fix and keep the behavior, later we can take a try with simplifying the logic.

Comment on attachment 133470 [details] Patch I realized that we don't even build WheelEventQt.cpp. Seems like it has just left over. So, the firts patch is sufficient.

Comment on attachment 133301 [details] Patch A similar change was applied in r112093 - clearing review :)

*** This bug has been marked as a duplicate of bug 79458 ***

(In reply to comment #9) > > *** This bug has been marked as a duplicate of bug 79458 *** Fix compilation with QtWidget? What does that have to do with this bug?

(In reply to comment #10) > (In reply to comment #9) > > > > *** This bug has been marked as a duplicate of bug 79458 *** > > Fix compilation with QtWidget? What does that have to do with this bug? Nevermind. I found it.