Use root window's coordinates when updating cursor in Aura

Use root window's coordinates when updating cursor in Aura

In RenderWidgetHostViewAura::UpdateCursorIfOverSelf(),
the point used for the hit test against the root window
needs to be in the root window's coordinate space.

This CL also removes some Windows-specific code
originally added for issue 292379 which is no longer
needed.

BUG=295125
TEST=RenderWidgetHostViewAuraTest.UpdateCursorIfOverSelf

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

[sadrul, 2013-10-01 14:33:36]

Preemptive drive-by review!

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

https://codereview.chromium.org/25336003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:3190:
local_point.Offset(-screen_rect.x(), -screen_rect.y());
I think this is incorrect. This should use ScreenPositionClient to convert
|screen_point| to |local_point|.

https://codereview.chromium.org/25336003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:3200: if
(root_window->GetEventHandlerForPoint(screen_point) != window_)
Window::GetEventHandlerForPoint() expects the location to be in the Window's
local coordinates. So both of these calls are incorrect.

[tdanderson, 2013-10-17 18:23:46]

Drive-by comments addressed. It looks as though the
coordinate space of the point used in the hit test was
the problem, as pointed out by Sadrul. In CrOS Aura,
the root window's coordinate space and the screen's
coordinate space are the same, so this is the reason why
using |screen_point| in Patch Set 1 fixed the bug (since
|screen_point| and |root_window_point| are the same).

Sadrul, can you have another quick look before I pass
this along to an OWNER?

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

https://codereview.chromium.org/25336003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:3190:
local_point.Offset(-screen_rect.x(), -screen_rect.y());
On 2013/10/01 14:33:36, sadrul wrote:
> I think this is incorrect. This should use ScreenPositionClient to convert
> |screen_point| to |local_point|.

Done.

https://codereview.chromium.org/25336003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:3200: if
(root_window->GetEventHandlerForPoint(screen_point) != window_)
On 2013/10/01 14:33:36, sadrul wrote:
> Window::GetEventHandlerForPoint() expects the location to be in the Window's
> local coordinates. So both of these calls are incorrect.

Done.

[sadrul, 2013-10-21 02:37:06]

LGTM

[sadrul, 2013-10-21 02:38:20]

On 2013/10/21 02:37:06, sadrul wrote:
> LGTM

Let me know if you want me to take a look at the test when you add it.

[sadrul, 2013-12-17 17:25:56]

ping. Any update on this one?

[tdanderson, 2013-12-18 00:27:58]

Sadrul, can you please take another look? The test I added is unsatisfying for
two reasons:

- There is no ScreenPositionClient associated with the root window in the test,
so some of the code I added is not being tested.
- My test is for a private helper function. Is it better to instead write a test
for a public function which calls UpdateCursorIfOverSelf() such as
RWHVA::UpdateCursor()?

[tdanderson, 2013-12-19 17:31:48]

Scott, can you please take a look?

[sky, 2013-12-19 17:48:34]

LGTM

[commit-bot: I haz the power, 2013-12-19 18:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/25336003/90001

[commit-bot: I haz the power, 2013-12-19 19:53:49]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-19 19:57:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/25336003/90001

[tdanderson, 2013-12-19 22:54:27]

Removing some Windows-specific code which I believe is no longer necessary, and
was causing my new unit test to fail on Windows.

@scottmg, can you please take a look? Is there is a test in place to detect
regressions for the bug you originally fixed (cursor shape should be an arrow
after right clicking and hovering over a context menu?)

[scottmg, 2013-12-19 23:51:51]

On 2013/12/19 22:54:27, tdanderson wrote:
> Removing some Windows-specific code which I believe is no longer necessary,
and
> was causing my new unit test to fail on Windows.
> 
> @scottmg, can you please take a look? Is there is a test in place to detect
> regressions for the bug you originally fixed (cursor shape should be an arrow
> after right clicking and hovering over a context menu?)

Diff between ps5 & ps6 lgtm if right-click on a link is ok, and tests pass.
Thanks for cleaning it up!

[scottmg, 2013-12-19 23:55:53]

On 2013/12/19 23:51:51, scottmg OoO Dec21-Jan5 wrote:
> On 2013/12/19 22:54:27, tdanderson wrote:
> > Removing some Windows-specific code which I believe is no longer necessary,
> and
> > was causing my new unit test to fail on Windows.
> > 
> > @scottmg, can you please take a look? Is there is a test in place to detect
> > regressions for the bug you originally fixed (cursor shape should be an
arrow
> > after right clicking and hovering over a context menu?)
> 
> Diff between ps5 & ps6 lgtm if right-click on a link is ok, and tests pass.
> Thanks for cleaning it up!

Oh, and middle mouse pan scrolling -- the cursor should change on mouse down,
that was the other thing.

[tdanderson, 2013-12-20 18:05:22]

I have verified that the latest patch does not break the context menu or the
middle-click to scroll cases on Windows, so I am going to go ahead and put this
in the CQ.

I have a bug to add some more test coverage for cursors on Windows
(crbug.com/328549), and as part of that I will add coverage for the cases
described above so we can detect any further regressions.

[commit-bot: I haz the power, 2013-12-20 18:08:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/25336003/110001

[commit-bot: I haz the power, 2013-12-20 20:43:21]

Change committed as 242168