Don't track mouse events in HWNDMessageHandler when they are forwarded by the LegacyRenderWidgetHost

Don't track mouse events in HWNDMessageHandler when they are forwarded by the LegacyRenderWidgetHostHWND class.

We use the TrackMouseEvent API in HWNDMessageHandler when we receive a WM_MOUSEMOVE message. This is to ensure that Windows sends us a WM_MOUSELEAVE
message when the cursor leaves our window bounds. Tooltips use this message to dismiss the tooltip for e.g.

Technically when the mouse enters the child window (LegacyRenderWidgetHostHWND) it has left the parent window. We end up getting WM_MOUSELEAVE messages
for WM_MOUSEMOVES which are sent to the parent window by the LegacyRenderWidgetHostHWND class causing tooltips to not show up and some other bugs.

Fixes as below:-
1. Add a special marker in the hiword of the WPARAM in WM_MOUSEMOVE messages sent by the LegacyRenderWidgetHostHWND class. This is to indicate to the parent
   that it should not track these mouse moves.
2. When we lose activation in HWNDMessageHandler we post a dummy WM_MOUSELEAVE message. This is enable tooltips if they were visible to be dismissed for e.g
   Based on code inspection a WM_MOUSELEAVE message should not cause any issues.
3. Forward WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages via SendMessage to the parent without mucking with the parameters. The offsets are in screen coordinates in
   these messages.
4. Reposting events today fails when we click on the (LegacyRenderWidgetHostHWND) HWND. This is because the current code attempts to find an aura Window at the
   location which fails. I changed the RepostLocatedEvent function to allow a NULL window parameter on Windows. We attempt to forward the event to the HWND at the
   current location if it is on the same thread.

BUG=342323, 342298, 342299, 341879, 343246
R=cpu@chromium.org, sky@chromium.org, sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251190

[sky, 2014-02-12 14:36:31]

Superclassing like this makes me extremely nervous. Is there no other way?

https://codereview.chromium.org/159713012/diff/120001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/159713012/diff/120001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:106: if
(!g_parent_original_class_proc) {
Doesn't this assume the legacy hwnd always has a parent of the same class?

https://codereview.chromium.org/159713012/diff/120001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (left):

https://codereview.chromium.org/159713012/diff/120001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:791: DCHECK(!HasCapture());
What did the call stack look like when you hit this?

[ananta, 2014-02-12 22:17:05]

On 2014/02/12 14:36:31, sky wrote:
> Superclassing like this makes me extremely nervous. Is there no other way?
> 
>
https://codereview.chromium.org/159713012/diff/120001/content/browser/rendere...
> File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):
> 
>
https://codereview.chromium.org/159713012/diff/120001/content/browser/rendere...
> content/browser/renderer_host/legacy_render_widget_host_win.cc:106: if
> (!g_parent_original_class_proc) {
> Doesn't this assume the legacy hwnd always has a parent of the same class?
> 
>
https://codereview.chromium.org/159713012/diff/120001/ui/views/win/hwnd_messa...
> File ui/views/win/hwnd_message_handler.cc (left):
> 
>
https://codereview.chromium.org/159713012/diff/120001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:791: DCHECK(!HasCapture());
> What did the call stack look like when you hit this?

Reworked the patch to not use SetCapture. Instead we pass a special HIWORD in
WM_MOUSEMOVE messages from the LegacyRenderWidgetHostHWND 
class. This is to indicate that the parent should not track these moves.

[sky, 2014-02-12 22:51:29]

https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:136: // Mark the
WM_MOUSEMOVE message with a special flag 0xbeef in the high word
nit nuke 0xbeef, since you really mean SPECIAL_MOUSEMOVED_NOT_TO_B...

https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2143: if (view_tid !=
GetWindowThreadProcessId(HWNDForNativeView(window), NULL)) {
What is this code going to do with a NULL window?

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1028: ::PostMessage(hwnd(), WM_MOUSELEAVE,
0, 0);
If the legacy hwnd starts tracking mouse events, is this necessary any longer?

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1645: HIWORD(w_param) !=
SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED) {
Does this imply we need to track mouse mouse events in the child? If we don't,
doesn't that mean we may miss an exit?

[ananta, 2014-02-12 23:17:07]

https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:136: // Mark the
WM_MOUSEMOVE message with a special flag 0xbeef in the high word
On 2014/02/12 22:51:29, sky wrote:
> nit nuke 0xbeef, since you really mean SPECIAL_MOUSEMOVED_NOT_TO_B...

Done.

https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2143: if (view_tid !=
GetWindowThreadProcessId(HWNDForNativeView(window), NULL)) {
On 2014/02/12 22:51:29, sky wrote:
> What is this code going to do with a NULL window?

Fixed.

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1028: ::PostMessage(hwnd(), WM_MOUSELEAVE,
0, 0);
On 2014/02/12 22:51:29, sky wrote:
> If the legacy hwnd starts tracking mouse events, is this necessary any longer?

When it does. The TrackMouseEvent API succeeds on the legacy hwnd, but fails to
track the mouse. Until then this is necessary.

https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1645: HIWORD(w_param) !=
SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED) {
On 2014/02/12 22:51:29, sky wrote:
> Does this imply we need to track mouse mouse events in the child? If we don't,
> doesn't that mean we may miss an exit?

When the mouse leaves the bounds of the child we would get a WM_NCMOUSEMOVE or a
WM_MOUSEMOVE depending on where we would go. That should get us a WM_MOUSELEAVE.
Additionally if we lose activation we have a dummy WM_MOUSELEAVE posted to cover
for this.

[sky, 2014-02-12 23:45:10]

I thought that if you didn't do a TrackMouse you're not guaranteed to
get an exit? With this change doesn't it mean we're not tracking the
mouse over the legacy hwnd anymore?

  -Scott

On Wed, Feb 12, 2014 at 3:17 PM,  <ananta@chromium.org> wrote:
>
>
https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
> File content/browser/renderer_host/legacy_render_widget_host_win.cc
> (right):
>
>
https://codereview.chromium.org/159713012/diff/330001/content/browser/rendere...
> content/browser/renderer_host/legacy_render_widget_host_win.cc:136: //
> Mark the WM_MOUSEMOVE message with a special flag 0xbeef in the high
> word
> On 2014/02/12 22:51:29, sky wrote:
>>
>> nit nuke 0xbeef, since you really mean SPECIAL_MOUSEMOVED_NOT_TO_B...
>
>
> Done.
>
>
>
https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
> File ui/views/controls/menu/menu_controller.cc (right):
>
>
https://codereview.chromium.org/159713012/diff/330001/ui/views/controls/menu/...
> ui/views/controls/menu/menu_controller.cc:2143: if (view_tid !=
> GetWindowThreadProcessId(HWNDForNativeView(window), NULL)) {
> On 2014/02/12 22:51:29, sky wrote:
>>
>> What is this code going to do with a NULL window?
>
>
> Fixed.
>
>
>
https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:1028: ::PostMessage(hwnd(),
> WM_MOUSELEAVE, 0, 0);
> On 2014/02/12 22:51:29, sky wrote:
>>
>> If the legacy hwnd starts tracking mouse events, is this necessary any
>
> longer?
>
> When it does. The TrackMouseEvent API succeeds on the legacy hwnd, but
> fails to track the mouse. Until then this is necessary.
>
>
>
https://codereview.chromium.org/159713012/diff/330001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:1645: HIWORD(w_param) !=
> SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED) {
> On 2014/02/12 22:51:29, sky wrote:
>>
>> Does this imply we need to track mouse mouse events in the child? If
>
> we don't,
>>
>> doesn't that mean we may miss an exit?
>
>
> When the mouse leaves the bounds of the child we would get a
> WM_NCMOUSEMOVE or a WM_MOUSEMOVE depending on where we would go. That
> should get us a WM_MOUSELEAVE. Additionally if we lose activation we
> have a dummy WM_MOUSELEAVE posted to cover for this.
>
> https://codereview.chromium.org/159713012/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ananta, 2014-02-13 19:52:31]

Added code to track mouse events in the child. We send the WM_MOUSELEAVE to the
parent if the cursor is outside the bounds of the parent window.

The mouse move tagging has the added benefit of also fixing the DWM memory leak
which occurs because of tooltips showing up and hiding themselves constantly
leading to a memory spike in DWM

[cpu_(ooo_6.6-7.5), 2014-02-13 22:35:17]

lgtm

I think part of the confusion is that wm_mouseleave is not in the range of
mousefirst - mouselast so the previous CL take was that that tracking was broken
from child windows.

https://codereview.chromium.org/159713012/diff/1030001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/159713012/diff/1030001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1640: if (event.type() ==
ui::ET_MOUSE_MOVED && !HasCapture() &&
can you put a short comment here about why we test for
SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED

[sky, 2014-02-13 22:39:07]

LGTM

[ananta, 2014-02-13 22:47:34]

https://codereview.chromium.org/159713012/diff/1030001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/159713012/diff/1030001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1640: if (event.type() ==
ui::ET_MOUSE_MOVED && !HasCapture() &&
On 2014/02/13 22:35:18, cpu wrote:
> can you put a short comment here about why we test for
> SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED

Done.

https://codereview.chromium.org/159713012/diff/1030001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1640: if (event.type() ==
ui::ET_MOUSE_MOVED && !HasCapture() &&
On 2014/02/13 22:35:18, cpu wrote:
> can you put a short comment here about why we test for
> SPECIAL_MOUSEMOVE_NOT_TO_BE_TRACKED

Done.

[ananta, 2014-02-13 23:49:27]

Committed patchset #7 manually as r251190 (presubmit successful).