Ensure on Windows AURA that the cursor stays within the bounds of the window during a LockMouse ope…

Ensure on Windows AURA that the cursor stays within the bounds of the window during a LockMouse operation and
is unclipped during the UnlockMouse operation.

On Windows we can ensure that the cursor stays within the bounds of a window via the ClipCursor API.

Changes in this CL are as below:-
1. The cursor is clipped in the RootWindowHost::ConfineCursorToRootWindow abstraction and is unclipped in the
   RootWindowHost::UnConfineCursor abstraction. Currently these functions are only implemented on Windows in
   the DesktopRootWindowHostWin class.

2. Added an accessor method UnConfineCursor to the RootWindow class which forwards this call to the RootWindowHost.
   This is on the same lines as the ConfineCursorToWindow method in the RootWindow class.

3. We need to explicitly hide the cursor on Windows via the ShowCursor API. This is done in the
   DesktopRootWindowHostWin::OnCursorVisibilityChanged function.

4. If we receive non client messages in the RenderWidgetHostViewAura class in the locked mouse state, we should move
   the cursor to the center and return. If we don't do this then it causes DCHECKs to fire in the webkit mouse event
   conversion code. In any case non client messages should not be sent to webkit.

Fixes bug https://code.google.com/p/chromium/issues/detail?id=305563

BUG=305563
R=scheib@chromium.org, sky@chromium.org

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

[jam, 2013-10-30 17:49:34]

I'm not familiar with this code, so I defer to scott

[ananta, 2013-10-30 18:06:14]

+scheib

[scheib, 2013-10-30 18:16:45]

https://codereview.chromium.org/49383004/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2200:
::ClipCursor(&window_rect);
Yes, clipping to the window is necessary. It's also necessary on window
resizing. See render_widget_host_view_win [1]. I'm unsure about a platform
#ifdef here though? I don't know aura, but is clipping not already abstracted,
or if not shouldn't it be?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

https://codereview.chromium.org/49383004/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2746: if
(event->flags() & ui::EF_IS_NON_CLIENT) {
I can't comment here because I don't understand NON CLIENT. The enum is not
documented, and its name+context alone isn't descriptive enough for me to
understand.

[ananta, 2013-10-30 20:41:26]

+sky for overall review.

[ananta, 2013-10-30 20:41:26]

+sky for overall review.

[ananta, 2013-10-30 20:47:29]

https://codereview.chromium.org/49383004/diff/50001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/50001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2200:
::ClipCursor(&window_rect);
On 2013/10/30 18:16:45, scheib wrote:
> Yes, clipping to the window is necessary. It's also necessary on window
> resizing. See render_widget_host_view_win [1]. I'm unsure about a platform
> #ifdef here though? I don't know aura, but is clipping not already abstracted,
> or if not shouldn't it be?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Good point. We have the ConfineCursorToWindow and UnConfineCursor functions on
the RootWindowHost interface. Moved this logic there.

[scheib, 2013-10-30 21:23:07]

Thumbs up on the use of ConfineCursor.

Please confirm you've checked that moving/resizing windows will work, this was
an earlier issue on windows when e.g. locking with a small window and then
maximizing while under lock.

https://codereview.chromium.org/49383004/diff/400001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/400001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2174:
static_cast<aura::RootWindow*>(window_->GetRootWindow());
Why doesn't GetRootWindow() return a RootWindow?

[ananta, 2013-10-30 21:43:15]

I verified that this change does not regress sizing and moving in any way

https://codereview.chromium.org/49383004/diff/400001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/400001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2174:
static_cast<aura::RootWindow*>(window_->GetRootWindow());
On 2013/10/30 21:23:07, scheib wrote:
> Why doesn't GetRootWindow() return a RootWindow?
Checked with sky and ben. The RootWindow class is on its way out and
its functionality will be consolidated in the WindowEventDispatcher
class. Changed the code to use GetDispatcher instead of GetRootWindow

[scheib, 2013-10-30 21:48:03]

lgtm

[sky, 2013-10-30 22:02:52]

https://codereview.chromium.org/49383004/diff/650002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/650002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2173:
aura::RootWindow* root_window = window_->GetDispatcher();
Use WindowEventDispatcher, not RootWindow (they are the same now, but won't be
in the long term). Also, I think you want
window_->GetRootWindow()->GetDispatcher(). And presumably you'll need to deal
with NULL.

https://codereview.chromium.org/49383004/diff/650002/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://codereview.chromium.org/49383004/diff/650002/ui/aura/root_window.h#ne...
ui/aura/root_window.h:140: void UnConfineCursor();
UnconfigCursor.

How did the old code unconfine?

[ananta, 2013-10-30 22:11:40]

https://codereview.chromium.org/49383004/diff/650002/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/49383004/diff/650002/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2173:
aura::RootWindow* root_window = window_->GetDispatcher();
On 2013/10/30 22:02:53, sky wrote:
> Use WindowEventDispatcher, not RootWindow (they are the same now, but won't be
> in the long term). Also, I think you want
> window_->GetRootWindow()->GetDispatcher(). And presumably you'll need to deal
> with NULL.

Changed to WindowEventDispatcher. Just checked the implementation of
Window::GetDispatcher. It first gets the root window and then gets the
dispatcher from there with NULL checks duly performed.

https://codereview.chromium.org/49383004/diff/650002/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://codereview.chromium.org/49383004/diff/650002/ui/aura/root_window.h#ne...
ui/aura/root_window.h:140: void UnConfineCursor();
On 2013/10/30 22:02:53, sky wrote:
> UnconfigCursor.
> 
> How did the old code unconfine?

The only consumer of ConfineCursorToWindow is the ash display controller.
UnConfineCursor was performed internally by the RootWindowHostX11 class.

[sky, 2013-10-31 01:42:22]

LGTM

[ananta, 2013-10-31 18:27:17]

Committed patchset #8 manually as r232176 (presubmit successful).