Implement WM_MOUSEHWHEEL events....

Implements a WM_MOUSEHWHEEL message handler.
A change r2266 removed fake scroll bars used to handle WM_MOUSEWHEEL messages. This change prevents Windows from sending WM_HSCROLL messages when tilting a mouse wheel (Windows sends WM_HSCROLL messages only to a window which has scroll bars.) and this causes a regression problem that we cannot use tilt-wheel to scroll horizontally.
To solve this problem, this change implements a WM_MOUSEHWHEEL (0x020E) message handler to dispatch tilt-wheel events to the RenderWidgetHostViewWin::OnWheelEvents() function. Also, It changes scroll direction in creating a WebMouseWheelEvent instance.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5095

[Peter Kasting, 2008-11-05 05:33:52]

I'm opposed to use the key_state |= MK_SHIFT hack long-term, although perhaps
it's expedient now if you're trying to solve this immediately.  Instead the
input event type should be augmented to distinguish between vertical and
horizontal wheel scrolls.  We shouldn't lie about the key state in hopes it will
do what we want on the other end.

[Hironori Bono, 2008-11-05 06:40:49]

Thank you for your review and comment.

On 2008/11/05 05:33:52, pkasting wrote:
> I'm opposed to use the key_state |= MK_SHIFT hack long-term, although perhaps
> it's expedient now if you're trying to solve this immediately.  Instead the
> input event type should be augmented to distinguish between vertical and
> horizontal wheel scrolls.  We shouldn't lie about the key state in hopes it
will
> do what we want on the other end.

Done. I have noticed jackson wrote a TODO note for WM_MOUSEHWHEEL messages below
and I don't have to use the said key_state hack. I have updated my fix to comply
with his note.

[Peter Kasting, 2008-11-05 07:03:23]

Don't wait for LGTM from me on this one, as after tonight I'll be gone for a few
days.

http://codereview.chromium.org/9319/diff/4/5
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/4/5#newcode183
Line 183: key_state |= MK_SHIFT;
Can this key_state hack (and the comment about adding a shift above it) be
removed in favor of adding a WM_HSCROLL condition to the conditional below that
you're modifying?

http://codereview.chromium.org/9319/diff/4/5#newcode235
Line 235: // TODO(jackson): Support WM_MOUSEHWHEEL = 0x020E event as well.
Nit: Take out this TODO

[Hironori Bono, 2008-11-05 11:22:14]

Thank you for your quick review and comments.
I wish you have happy holidays.

http://codereview.chromium.org/9319/diff/4/5
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/4/5#newcode183
Line 183: key_state |= MK_SHIFT;
On 2008/11/05 07:03:23, pkasting wrote:
> Can this key_state hack (and the comment about adding a shift above it) be
> removed in favor of adding a WM_HSCROLL condition to the conditional below
that
> you're modifying?

Done. As far as I have tested with Microsoft and Logicool Mice, we don't need
this hack any longer. So, I also removed this code.

http://codereview.chromium.org/9319/diff/4/5#newcode235
Line 235: // TODO(jackson): Support WM_MOUSEHWHEEL = 0x020E event as well.
On 2008/11/05 07:03:23, pkasting wrote:
> Nit: Take out this TODO

Done. Thank you for noticing this.

[Peter Kasting, 2008-11-05 15:52:07]

http://codereview.chromium.org/9319/diff/4/5
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/4/5#newcode183
Line 183: key_state |= MK_SHIFT;
On 2008/11/05 11:22:14, hbono wrote:
> On 2008/11/05 07:03:23, pkasting wrote:
> > Can this key_state hack (and the comment about adding a shift above it) be
> > removed in favor of adding a WM_HSCROLL condition to the conditional below
> that
> > you're modifying?
> 
> Done. As far as I have tested with Microsoft and Logicool Mice, we don't need
> this hack any longer. So, I also removed this code.

I wouldn't remove it entirely.  This was for various forms of trackpad/touchpad,
it seems unlikely to be triggered by normal mice.  Since the block this was
contained in is still handling WM_HSCROLL, the effect hereis that WM_HSCROLL is
now going to get treated like WM_VSCROLL, which is certainly wrong, instead of
ignored, which is perhaps what you intended (and probably wrong).  If you're not
sure how to test this you should work with joshia.  I think the only change to
make here is removing the key_state set and adding WM_HSCROLL to the
WM_MOUSEHWHEEL code below, but perhaps this entire huge block can be removed.

[amit, 2008-11-05 23:53:05]

http://codereview.chromium.org/9319/diff/8/207
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/8/207#newcode173
Line 173: }
Good that you removed this!

http://codereview.chromium.org/9319/diff/8/207#newcode223
Line 223: if (key_state & MK_SHIFT || WM_MOUSEHWHEEL == message) {
I think it's better to just add MK_SHIFT to the key_state before this check,
possibly somewhere in the beginning. This way any further processing (here or
anywhere else) will know that this is a horizontal scroll operation.

[Hironori Bono, 2008-11-06 05:09:29]

Thank you for your review and comments.

http://codereview.chromium.org/9319/diff/8/207
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/8/207#newcode223
Line 223: if (key_state & MK_SHIFT || WM_MOUSEHWHEEL == message) {
On 2008/11/05 23:53:05, amit wrote:
> I think it's better to just add MK_SHIFT to the key_state before this check,
> possibly somewhere in the beginning. This way any further processing (here or
> anywhere else) will know that this is a horizontal scroll operation.

My first fix just added MK_SHIFT before Line 192 "global_x =
static_cast<short>(LOWORD(lparam));" as you noted.
But I changed the fix to this one to comply with a comment from pkasting that he
did not like adding MK_SHIFT.
Is it possible to give me advices about which option I should take?

[amit, 2008-11-06 05:26:42]

http://codereview.chromium.org/9319/diff/8/207
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/8/207#newcode223
Line 223: if (key_state & MK_SHIFT || WM_MOUSEHWHEEL == message) {
I think the ideal solution is to have a bool is_horizontal to indicate
horizontal scrolling. pkasting probably means so (correct me if I am wrong
here). 

However, the current way to indicate horizontal scrolling is to have MK_SHIFT
and it's better to use that so that the code using this information can detect
horizontal scrolling semantics.

Makes sense?

On 2008/11/06 05:09:29, hbono wrote:
> On 2008/11/05 23:53:05, amit wrote:
> > I think it's better to just add MK_SHIFT to the key_state before this check,
> > possibly somewhere in the beginning. This way any further processing (here
or
> > anywhere else) will know that this is a horizontal scroll operation.
> 
> My first fix just added MK_SHIFT before Line 192 "global_x =
> static_cast<short>(LOWORD(lparam));" as you noted.
> But I changed the fix to this one to comply with a comment from pkasting that
he
> did not like adding MK_SHIFT.
> Is it possible to give me advices about which option I should take?

[Hironori Bono, 2008-11-07 11:09:07]

Thank you for your comments.
Is it possible to review the updated one?

http://codereview.chromium.org/9319/diff/8/207
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/9319/diff/8/207#newcode223
Line 223: if (key_state & MK_SHIFT || WM_MOUSEHWHEEL == message) {
On 2008/11/06 05:26:43, amit wrote:
> I think the ideal solution is to have a bool is_horizontal to indicate
> horizontal scrolling. pkasting probably means so (correct me if I am wrong
> here). 
> 
> However, the current way to indicate horizontal scrolling is to have MK_SHIFT
> and it's better to use that so that the code using this information can detect
> horizontal scrolling semantics.
> Makes sense?

Done. I have added the original MK_SHIFT hack and a TODO comment which notes
this.

[amit, 2008-11-07 15:44:33]

LGTM

[Peter Kasting, 2008-11-10 19:00:29]

Please open a bug for this if there is not already one.  Use of the shift hack
aside, this doesn't seem like it does the right thing.

http://codereview.chromium.org/9319/diff/801/405
File webkit/glue/webinputevent_win.cc (left):

http://codereview.chromium.org/9319/diff/801/405#oldcode182
Line 182: if (WM_HSCROLL == message) {
I still don't think this can possibly be the right change.

We're inside a block handling WM_HSCROLL, but we've now removed the shift hack
for WM_HSCROLL.  It seems like only two things are possible:
* We never receive WM_HSCROLL now that we have WM_MOUSEHWEEL support, and all
WM_HSCROLL code can disappear.  This seems unlikely because I thought this code
also got triggered by some trackpad drivers.
* Removing this code was wrong.

We need to know precisely when this code is triggered and be able to test it to
know what should happen; we can't just change it based on theories about what
might be right.  Does anyone have this info?