Ignore update cursor messages from blink when the cursor position lies outside our root window

Ignore update cursor messages from blink when the cursor position lies outside the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates affect the cursor even when it is not
the destination window.

I made a change to the ScreenWin class to make the GetHWNDFromNativeView and GetNativeWindowFromHWND methods public

BUG=534262
TBR=jam

Committed: https://crrev.com/3e1db186342622ab582b8e97efd12b44ffe427e5
Cr-Commit-Position: refs/heads/master@{#359976}

[ananta, 2015-11-11 02:35:04]

ananta@chromium.org changed reviewers:
+ jam@chromium.org, scottmg@chromium.org

[scottmg, 2015-11-11 05:37:53]

Our cursors are such a mess. :( We have all sorts of CursorManagers and stuff,
one of them doesn't tie the cursor updates to the proper window? sky or
tdanderson might know of a better place to fix this (and maybe for all platforms
then too).

https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:2493: if
(legacy_render_widget_host_HWND_) {
Couldn't this happen when the legacy hwnd is disabled too?

[ananta, 2015-11-11 19:35:08]

https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:2493: if
(legacy_render_widget_host_HWND_) {
On 2015/11/11 05:37:52, scottmg wrote:
> Couldn't this happen when the legacy hwnd is disabled too?

The legacy window is disabled only for metro mode and by a command line switch.
We are using the legacy window as a signal that this is not metro mode.

[ananta, 2015-11-11 19:38:16]

On 2015/11/11 19:35:08, ananta wrote:
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2493: if
> (legacy_render_widget_host_HWND_) {
> On 2015/11/11 05:37:52, scottmg wrote:
> > Couldn't this happen when the legacy hwnd is disabled too?
> 
> The legacy window is disabled only for metro mode and by a command line
switch.
> We are using the legacy window as a signal that this is not metro mode.

Cursors on windows are per thread. Cursor managers are per root window. I guess
we could have some logic in the cursor manager which
determines if it is safe to set the cursor based on which window has focus and
the bounds of the cursor position. In most cases we are ok. This
is a special case where blink is requesting a cursor change for a view. We don't
want it to affect other windows.

[tdanderson, 2015-11-11 20:22:33]

On 2015/11/11 19:38:16, ananta wrote:
> On 2015/11/11 19:35:08, ananta wrote:
> >
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:2493: if
> > (legacy_render_widget_host_HWND_) {
> > On 2015/11/11 05:37:52, scottmg wrote:
> > > Couldn't this happen when the legacy hwnd is disabled too?
> > 
> > The legacy window is disabled only for metro mode and by a command line
> switch.
> > We are using the legacy window as a signal that this is not metro mode.
> 
> Cursors on windows are per thread. Cursor managers are per root window. I
guess
> we could have some logic in the cursor manager which
> determines if it is safe to set the cursor based on which window has focus and
> the bounds of the cursor position. In most cases we are ok. This
> is a special case where blink is requesting a cursor change for a view. We
don't
> want it to affect other windows.

Shouldn't you be able to keep the change in UpdateCursorIfOverSelf() but make it
platform-independent by using the aura::client::FocusClient and performing a
similar check on aura::Window instead of HWND?

[ananta, 2015-11-13 01:54:20]

Description was changed from

==========
Ignore update cursor messages from blink when the cursor position lies within
the bounds of a different focus window.

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.

This only fixes the bug on Windows.

BUG=534262
==========

to

==========
Ignore update cursor messages from blink when the cursor position lies outside
the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.



BUG=534262
==========

[ananta, 2015-11-13 01:55:17]

Description was changed from

==========
Ignore update cursor messages from blink when the cursor position lies outside
the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.



BUG=534262
==========

to

==========
Ignore update cursor messages from blink when the cursor position lies outside
the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.

I made a change to the ScreenWin class to make the GetHWNDFromNativeView and
GetNativeWindowFromHWND methods public

BUG=534262
==========

[ananta, 2015-11-13 01:55:55]

On 2015/11/11 20:22:33, tdanderson wrote:
> On 2015/11/11 19:38:16, ananta wrote:
> > On 2015/11/11 19:35:08, ananta wrote:
> > >
> >
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> > > File content/browser/renderer_host/render_widget_host_view_aura.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1433153002/diff/1/content/browser/renderer_ho...
> > > content/browser/renderer_host/render_widget_host_view_aura.cc:2493: if
> > > (legacy_render_widget_host_HWND_) {
> > > On 2015/11/11 05:37:52, scottmg wrote:
> > > > Couldn't this happen when the legacy hwnd is disabled too?
> > > 
> > > The legacy window is disabled only for metro mode and by a command line
> > switch.
> > > We are using the legacy window as a signal that this is not metro mode.
> > 
> > Cursors on windows are per thread. Cursor managers are per root window. I
> guess
> > we could have some logic in the cursor manager which
> > determines if it is safe to set the cursor based on which window has focus
and
> > the bounds of the cursor position. In most cases we are ok. This
> > is a special case where blink is requesting a cursor change for a view. We
> don't
> > want it to affect other windows.
> 
> Shouldn't you be able to keep the change in UpdateCursorIfOverSelf() but make
it
> platform-independent by using the aura::client::FocusClient and performing a
> similar check on aura::Window instead of HWND?

Done PTAL

[ananta, 2015-11-13 01:56:11]

ananta@chromium.org changed reviewers:
+ sky@chromium.org

[ananta, 2015-11-13 01:56:20]

+sky

[sky, 2015-11-13 16:12:18]

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2482:
cursor_screen_point);;
nit: only one ';'.

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2494:
gfx::ScreenWin* screen_win = static_cast<gfx::ScreenWin*>(screen);
What happens if running in metro mode?

https://codereview.chromium.org/1433153002/diff/20001/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1433153002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:23: virtual HWND GetHWNDFromNativeView(NativeView window)
const;
Move definition to match declaration location.

[ananta, 2015-11-13 20:09:41]

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2482:
cursor_screen_point);;
On 2015/11/13 16:12:18, sky wrote:
> nit: only one ';'.

Done.

https://codereview.chromium.org/1433153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2494:
gfx::ScreenWin* screen_win = static_cast<gfx::ScreenWin*>(screen);
On 2015/11/13 16:12:18, sky wrote:
> What happens if running in metro mode?

In metro mode we should not be hitting the if (!window_at_screen_point)
check. I added a check for the legacy window also being present.

https://codereview.chromium.org/1433153002/diff/20001/ui/gfx/screen_win.h
File ui/gfx/screen_win.h (right):

https://codereview.chromium.org/1433153002/diff/20001/ui/gfx/screen_win.h#new...
ui/gfx/screen_win.h:23: virtual HWND GetHWNDFromNativeView(NativeView window)
const;
On 2015/11/13 16:12:18, sky wrote:
> Move definition to match declaration location.

Done.

[sky, 2015-11-14 15:26:02]

LGTM

[scottmg, 2015-11-16 20:15:58]

lgtm

[ananta, 2015-11-16 20:17:35]

Description was changed from

==========
Ignore update cursor messages from blink when the cursor position lies outside
the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.

I made a change to the ScreenWin class to make the GetHWNDFromNativeView and
GetNativeWindowFromHWND methods public

BUG=534262
==========

to

==========
Ignore update cursor messages from blink when the cursor position lies outside
the bounds of the current RWHVA window

This fixes the bug where in Dev tools style changes which cause cursor updates
affect the cursor even when it is not
the destination window.

I made a change to the ScreenWin class to make the GetHWNDFromNativeView and
GetNativeWindowFromHWND methods public

BUG=534262
TBR=jam
==========

[ananta, 2015-11-16 20:17:47]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-11-16 20:20:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1433153002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1433153002/60001

[commit-bot: I haz the power, 2015-11-16 20:47:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-16 20:47:44]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[ananta, 2015-11-17 00:21:00]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-11-17 00:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1433153002/#ps120001
(title: "Replace NULL with nullptr")

[commit-bot: I haz the power, 2015-11-17 00:22:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1433153002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1433153002/120001

[commit-bot: I haz the power, 2015-11-17 02:10:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-17 02:12:05]

Patchset 7 (id:??) landed as
https://crrev.com/3e1db186342622ab582b8e97efd12b44ffe427e5
Cr-Commit-Position: refs/heads/master@{#359976}