Bug 32029 – Mousewheel event delta has reversed sign on Linux Chrome

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Evan Stade]

http://code.google.com/p/chromium/issues/detail?id=29080

[Evan Stade]

Created attachment 44092 [details]
fix

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44092 [details] without any errors.

[Evan Martin]

[I am not a reviewer]
This looks good to me, but perhaps it would benefit from a comment on the counterintuitive notion that WebKit expect +X to mean to the left.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44092 [details]
fix

r=me, the windows version has this comment:

// Windows is <- -/+ ->, WebKit <- +/- ->.

[Eric Seidel (no email)]

Comment on attachment 44092 [details]
fix

I'm not sure why WebKit is reversed that way.

Probably due to:
    wkGetWheelEventDeltas(event, &m_deltaX, &m_deltaY, &continuous);
http://trac.webkit.org/browser/trunk/WebCore/platform/mac/WheelEventMac.mm#L46

I'll CC Hyatt, as he wrote that code.

[WebKit Commit Bot]

Comment on attachment 44092 [details]
fix

Clearing flags on attachment: 44092

Committed r51614: <http://trac.webkit.org/changeset/51614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

It is not sufficient to point to a website for a manual test. Manual tests should be in WebCore/manual-tests

So the website version of the test, should be rewritten to test exactly what this is fixing, and then be landed in the tree.

Further there is no reason that this should not be tested automatically.  The absence of existing automatic tests is not justification to not write an automated test, if necessary your patch should include additions to DRT to allow wheel event generation (i'm really surprised it doesn't already).  This is how DRT came to provide the functionality it does: the first time anyone encounters a untested area DRT is extended to make such testing possible.

[Evan Stade]

With respect to testing this automatically and DRT: DRT (or for chromium, Test Shell/EventSendingController) generates its own WebInputEvent objects, it does not start with a platform object (for gtk, GdkEvent*) and use this code to convert it, hence layout tests do not and can not cover this code without major structural changes. On the other hand, infrastructure to test event conversion code is largely in place in the Chromium tree in the form of the Automation/UIControls infrastructure, which is used in our automated test suite (interactive tests, browser tests, UI tests, etc.).

When I said "manual test", I simply meant "this is how I know the patch is correct", I didn't intend that this should be added to WebCore/manual-tests. In light of the fact that it can be automatically tested (within Chromium), I think it should not be a manual test.

[Eric Seidel (no email)]

I think the distinction being lost here is that this code appears only to affect the conversion between OS event objects and WebCore event objects.  It does not affect event objects which are synthesized wholly within WebCore, and thus untestable.

At some point we should either c++ unit test these sort of platform -> webcore event conversion routines or add layout tests for them.  Unfortunately we don't yet have a working c++ unit testing framework.  Bug 21010.

[Evan Stade]

yes, the c++ unit testing framework would be ideal

[Martin Robinson]

This seems to be an old Chromium bug.