Bug 81938 - [Qt] Scrolling with mouse wheel does not work in MiniBrowser with --desktop
Summary: [Qt] Scrolling with mouse wheel does not work in MiniBrowser with --desktop
Status: RESOLVED DUPLICATE of bug 79458
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-03-22 11:18 PDT by Balazs Kelemen
Modified: 2012-03-30 02:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.05 KB, patch)
2012-03-22 11:20 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2012-03-23 06:22 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-03-22 11:18:16 PDT
WebEventFactoryQt.cpp is using QApplication::wheelScrollLines() which is wrong since we don't instantiate a QApplication.
Comment 1 Balazs Kelemen 2012-03-22 11:20:30 PDT
Created attachment 133301 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-03-22 11:44:08 PDT
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.
Comment 3 Balazs Kelemen 2012-03-23 06:12:15 PDT
(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.
Comment 4 Balazs Kelemen 2012-03-23 06:22:11 PDT
Created attachment 133470 [details]
Patch
Comment 5 Balazs Kelemen 2012-03-23 06:22:39 PDT
(In reply to comment #4)
> Created an attachment (id=133470) [details]
> Patch

Applied the change to WheelEventQt.cpp as well.
Comment 6 Balazs Kelemen 2012-03-25 14:45:59 PDT
(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 7 Balazs Kelemen 2012-03-27 08:18:38 PDT
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 8 Simon Hausmann 2012-03-30 00:23:22 PDT
Comment on attachment 133301 [details]
Patch

A similar change was applied in r112093 - clearing review :)
Comment 9 Simon Hausmann 2012-03-30 00:23:33 PDT

*** This bug has been marked as a duplicate of bug 79458 ***
Comment 10 Allan Sandfeld Jensen 2012-03-30 02:15:03 PDT
(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?
Comment 11 Allan Sandfeld Jensen 2012-03-30 02:17:16 PDT
(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.