Magnifier: Move the cursor directly to the root window host.

Magnifier: Move the cursor directly to the root window host.

- Add RootWindow::MoveCursorToInHost() to move the cursor to the specified location in the host. 
- Use it in MagnificationController to move the cursor instead of MoveCursorTo().

BUG=223983, 230979
TEST=ash_unittests passes.

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

[yoshiki, 2013-04-20 06:33:56]

@oshima, I wrote the patch as we discussed at http://crrev.com/13877008. PTAL?

[oshima, 2013-04-22 18:29:37]

Thanks.

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
ui/aura/root_window.cc:294:
host_->MoveCursorTo(gfx::ToFlooredPoint(point_3f.AsPointF()));
can you change to use MoveCursorToHostLocation

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
ui/aura/root_window.cc:304: void RootWindow::MoveCursorToInHost(const
gfx::Point& host_location) {
On second thought, MoveCursorToHostLoation is probably better.

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

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h#newcode132
ui/aura/root_window.h:132: // Moves the cursor to the specified location in the
host.
to the |host_location| given in host coordinates.

[yoshiki, 2013-04-22 18:43:29]

@oshima: PTAL, Thanks,

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
ui/aura/root_window.cc:294:
host_->MoveCursorTo(gfx::ToFlooredPoint(point_3f.AsPointF()));
On 2013/04/22 18:29:37, oshima wrote:
> can you change to use MoveCursorToHostLocation

Done.

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
ui/aura/root_window.cc:304: void RootWindow::MoveCursorToInHost(const
gfx::Point& host_location) {
On 2013/04/22 18:29:37, oshima wrote:
> On second thought, MoveCursorToHostLoation is probably better.

Done.

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

https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h#newcode132
ui/aura/root_window.h:132: // Moves the cursor to the specified location in the
host.
On 2013/04/22 18:29:37, oshima wrote:
> to the |host_location| given in host coordinates.

Done.

[oshima, 2013-04-22 18:45:04]

I forgot to ask in the first comment. Can you add test for the issue this CL
fixes?

On 2013/04/22 18:43:29, yoshiki wrote:
> @oshima: PTAL, Thanks,
> 
> https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
> 
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
> ui/aura/root_window.cc:294:
> host_->MoveCursorTo(gfx::ToFlooredPoint(point_3f.AsPointF()));
> On 2013/04/22 18:29:37, oshima wrote:
> > can you change to use MoveCursorToHostLocation
> 
> Done.
> 
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
> ui/aura/root_window.cc:304: void RootWindow::MoveCursorToInHost(const
> gfx::Point& host_location) {
> On 2013/04/22 18:29:37, oshima wrote:
> > On second thought, MoveCursorToHostLoation is probably better.
> 
> Done.
> 
> https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h
> File ui/aura/root_window.h (right):
> 
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h#newcode132
> ui/aura/root_window.h:132: // Moves the cursor to the specified location in
the
> host.
> On 2013/04/22 18:29:37, oshima wrote:
> > to the |host_location| given in host coordinates.
> 
> Done.

[yoshiki, 2013-04-24 01:00:37]

@oshima: Added tests. PTAL.

On 2013/04/22 18:45:04, oshima wrote:
> I forgot to ask in the first comment. Can you add test for the issue this CL
> fixes?
> 
> On 2013/04/22 18:43:29, yoshiki wrote:
> > @oshima: PTAL, Thanks,
> > 
> > https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc
> > File ui/aura/root_window.cc (right):
> > 
> >
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
> > ui/aura/root_window.cc:294:
> > host_->MoveCursorTo(gfx::ToFlooredPoint(point_3f.AsPointF()));
> > On 2013/04/22 18:29:37, oshima wrote:
> > > can you change to use MoveCursorToHostLocation
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.cc#newcod...
> > ui/aura/root_window.cc:304: void RootWindow::MoveCursorToInHost(const
> > gfx::Point& host_location) {
> > On 2013/04/22 18:29:37, oshima wrote:
> > > On second thought, MoveCursorToHostLoation is probably better.
> > 
> > Done.
> > 
> > https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h
> > File ui/aura/root_window.h (right):
> > 
> >
>
https://codereview.chromium.org/13947045/diff/1/ui/aura/root_window.h#newcode132
> > ui/aura/root_window.h:132: // Moves the cursor to the specified location in
> the
> > host.
> > On 2013/04/22 18:29:37, oshima wrote:
> > > to the |host_location| given in host coordinates.
> > 
> > Done.

[oshima, 2013-04-24 20:36:04]

lgtm if you addressed comments below

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:24: const float
kMagnificationFactor = 1.18920712f;
The significand of a floating value only has 7 digits precision, and specifying
more doesn't make sense.

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:53: aura::RootWindow*
GetRootWindow() {
can this (and other accessors) be constlike

const aura::RootWindow* GetRootWindow() const

? You may ignore if not possible.

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:58: void
MoveCursorWithEvent(gfx::Point point) {
const gfx::Point& point

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:199: aura::Env* env =
aura::Env::GetInstance();
const
same for other places

[yoshiki, 2013-04-24 23:30:23]

@sky: Could you do an owner review? Thanks,

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:24: const float
kMagnificationFactor = 1.18920712f;
On 2013/04/24 20:36:04, oshima wrote:
> The significand of a floating value only has 7 digits precision, and
specifying
> more doesn't make sense.

This was the same value used in magnifier. I move it to the header and use it
here.

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:53: aura::RootWindow*
GetRootWindow() {
On 2013/04/24 20:36:04, oshima wrote:
> can this (and other accessors) be constlike
> 
> const aura::RootWindow* GetRootWindow() const
> 
> ? You may ignore if not possible.

Done.

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:58: void
MoveCursorWithEvent(gfx::Point point) {
On 2013/04/24 20:36:04, oshima wrote:
> const gfx::Point& point

Done.

https://codereview.chromium.org/13947045/diff/31001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller_unittest.cc:199: aura::Env* env =
aura::Env::GetInstance();
On 2013/04/24 20:36:04, oshima wrote:
> const
> same for other places

Done.

[sky, 2013-04-25 14:41:35]

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
File ash/magnifier/magnification_controller.h (right):

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller.h:51: virtual gfx::Point
GetPointOfInterestForTesting() = 0;
point of interest? What is this?

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:305: host_->MoveCursorTo(host_location);
Shouldn't moving the cursor trigger all of the following code to execute? By
that I mean shouldn't moving the cursor result in a mouse move (or whatever) and
update everything accordingly?

[oshima, 2013-04-25 14:58:13]

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:295: SetLastMouseLocation(this, location_in_dip);
Sorry if I wasn't clear. Can you refactor this method and
MoveCursorToHostLocation so that it shares the same code?

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:305: host_->MoveCursorTo(host_location);
On 2013/04/25 14:41:35, sky wrote:
> Shouldn't moving the cursor trigger all of the following code to execute? By
> that I mean shouldn't moving the cursor result in a mouse move (or whatever)
and
> update everything accordingly?

Yes, but asynchronously. My understanding is that this is to make cursor move
synchronous
(useful for mouse control and testing). RootWindowHostLinux does similar thing
for window resize.

[yoshiki, 2013-04-25 16:05:19]

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
File ash/magnifier/magnification_controller.h (right):

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller.h:51: virtual gfx::Point
GetPointOfInterestForTesting() = 0;
On 2013/04/25 14:41:35, sky wrote:
> point of interest? What is this?

The value of point_of_interest is the last cursor position or the last touched
location. When changing the scale, the magnifier zooms in/out keeping this point
in the view port.

And here, this value is exposed only for unittest.

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:295: SetLastMouseLocation(this, location_in_dip);
On 2013/04/25 14:58:13, oshima wrote:
> Sorry if I wasn't clear. Can you refactor this method and
> MoveCursorToHostLocation so that it shares the same code?

We can't use MoveCursorToHostLoation from MoveCursorTo because we disables
synthesize_mouse_move in MoveCursorToHostLoation, but not in MoveCursorTo.
Instead, I added MoveCursorToInternal to share the common code. Do you think
it's good?

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:305: host_->MoveCursorTo(host_location);
On 2013/04/25 14:58:13, oshima wrote:
> On 2013/04/25 14:41:35, sky wrote:
> > Shouldn't moving the cursor trigger all of the following code to execute? By
> > that I mean shouldn't moving the cursor result in a mouse move (or whatever)
> and
> > update everything accordingly?
> 
> Yes, but asynchronously. My understanding is that this is to make cursor move
> synchronous
> (useful for mouse control and testing). RootWindowHostLinux does similar thing
> for window resize.

Right, I want to move the cursor synchronously.

[oshima, 2013-04-25 16:09:02]

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:295: SetLastMouseLocation(this, location_in_dip);
On 2013/04/25 16:05:19, yoshiki wrote:
> On 2013/04/25 14:58:13, oshima wrote:
> > Sorry if I wasn't clear. Can you refactor this method and
> > MoveCursorToHostLocation so that it shares the same code?
> 
> We can't use MoveCursorToHostLoation from MoveCursorTo because we disables
> synthesize_mouse_move in MoveCursorToHostLoation, but not in MoveCursorTo.

I think we should set this in both cases.

> Instead, I added MoveCursorToInternal to share the common code. Do you think
> it's good?

Yes, that's what I meant by refactoring. You need to move SetLastMouseLocation
out.

[yoshiki, 2013-04-25 16:23:27]

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:295: SetLastMouseLocation(this, location_in_dip);
On 2013/04/25 16:09:02, oshima wrote:
> On 2013/04/25 16:05:19, yoshiki wrote:
> > On 2013/04/25 14:58:13, oshima wrote:
> > > Sorry if I wasn't clear. Can you refactor this method and
> > > MoveCursorToHostLocation so that it shares the same code?
> > 
> > We can't use MoveCursorToHostLoation from MoveCursorTo because we disables
> > synthesize_mouse_move in MoveCursorToHostLoation, but not in MoveCursorTo.
> 
> I think we should set this in both cases.

Done. Moved 'synthesize_mouse_move_ = false' to MoveCursorToInternal.

> > Instead, I added MoveCursorToInternal to share the common code. Do you think
> > it's good?
> 
> Yes, that's what I meant by refactoring. You need to move SetLastMouseLocation
> out.

Sorry, I'm confusing. Do you mean we should move SetLastMouseLocation from
MoveCursorToInternal to somewhere?

[sky, 2013-04-25 16:46:59]

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
File ash/magnifier/magnification_controller.h (right):

https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
ash/magnifier/magnification_controller.h:51: virtual gfx::Point
GetPointOfInterestForTesting() = 0;
On 2013/04/25 16:05:19, yoshiki wrote:
> On 2013/04/25 14:41:35, sky wrote:
> > point of interest? What is this?
> 
> The value of point_of_interest is the last cursor position or the last touched
> location. When changing the scale, the magnifier zooms in/out keeping this
point
> in the view port.
> 
> And here, this value is exposed only for unittest.

My point is there is no description of the method and the name doesn't convey in
any way what is returned.

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:305: host_->MoveCursorTo(host_location);
On 2013/04/25 16:05:19, yoshiki wrote:
> On 2013/04/25 14:58:13, oshima wrote:
> > On 2013/04/25 14:41:35, sky wrote:
> > > Shouldn't moving the cursor trigger all of the following code to execute?
By
> > > that I mean shouldn't moving the cursor result in a mouse move (or
whatever)
> > and
> > > update everything accordingly?
> > 
> > Yes, but asynchronously. My understanding is that this is to make cursor
move
> > synchronous
> > (useful for mouse control and testing). RootWindowHostLinux does similar
thing
> > for window resize.
> 
> Right, I want to move the cursor synchronously.

Why is it important that the mouse is synchronous? And you mispelled Location
here.

[yoshiki, 2013-04-25 17:57:35]

On 2013/04/25 16:46:59, sky wrote:
>
https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
> File ash/magnifier/magnification_controller.h (right):
> 
>
https://codereview.chromium.org/13947045/diff/38001/ash/magnifier/magnificati...
> ash/magnifier/magnification_controller.h:51: virtual gfx::Point
> GetPointOfInterestForTesting() = 0;
> On 2013/04/25 16:05:19, yoshiki wrote:
> > On 2013/04/25 14:41:35, sky wrote:
> > > point of interest? What is this?
> > 
> > The value of point_of_interest is the last cursor position or the last
touched
> > location. When changing the scale, the magnifier zooms in/out keeping this
> point
> > in the view port.
> > 
> > And here, this value is exposed only for unittest.
> 
> My point is there is no description of the method and the name doesn't convey
in
> any way what is returned.

Added the description of the method.

https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
> 
>
https://codereview.chromium.org/13947045/diff/38001/ui/aura/root_window.cc#ne...
> ui/aura/root_window.cc:305: host_->MoveCursorTo(host_location);
> On 2013/04/25 16:05:19, yoshiki wrote:
> > On 2013/04/25 14:58:13, oshima wrote:
> > > On 2013/04/25 14:41:35, sky wrote:
> > > > Shouldn't moving the cursor trigger all of the following code to
execute?
> By
> > > > that I mean shouldn't moving the cursor result in a mouse move (or
> whatever)
> > > and
> > > > update everything accordingly?
> > > 
> > > Yes, but asynchronously. My understanding is that this is to make cursor
> move
> > > synchronous
> > > (useful for mouse control and testing). RootWindowHostLinux does similar
> thing
> > > for window resize.
> > 
> > Right, I want to move the cursor synchronously.
> 
> Why is it important that the mouse is synchronous? And you mispelled Location
> here.

Not so important as far as the magnifier. But I think it is better for
MoveCursorToHostLocation to use same logic as MoveCursorTo, since MoveCursorTo
is synchronous. Don't you think so?

And fixed the misspelling. Thanks.

[sky, 2013-04-25 18:16:24]

Agreed, they should all be consistent. Why do any of them need to be
synchronous?

https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:303: void RootWindow::MoveCursorToInternal(
Make position match header, and when you wrap each parameter on its own line.

[yoshiki, 2013-04-25 18:46:39]

On 2013/04/25 18:16:24, sky wrote:
> Agreed, they should all be consistent. Why do any of them need to be
> synchronous?

MoveCursorTo has been synchronous. I don't know the design. But I assume the
synchronous method was easier to handle. 

If we made MoveCursorTo asynchronous, a lot of tests should be broken and need
to be fixed. We should keep MoveCursorTo (and MoveCursorToHostLocation)
synchronous as for now.

https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc#ne...
ui/aura/root_window.cc:303: void RootWindow::MoveCursorToInternal(
On 2013/04/25 18:16:24, sky wrote:
> Make position match header, and when you wrap each parameter on its own line.

Done.

[sky, 2013-04-25 19:46:14]

Tests are easily updated. What is the compelling reason for assuming
the cursor moved? It's better to wait for the real event so we don't
have to duplicate logic.

  -Scott

On Thu, Apr 25, 2013 at 11:46 AM,  <yoshiki@chromium.org> wrote:
> On 2013/04/25 18:16:24, sky wrote:
>>
>> Agreed, they should all be consistent. Why do any of them need to be
>> synchronous?
>
>
> MoveCursorTo has been synchronous. I don't know the design. But I assume the
> synchronous method was easier to handle.
>
> If we made MoveCursorTo asynchronous, a lot of tests should be broken and
> need
> to be fixed. We should keep MoveCursorTo (and MoveCursorToHostLocation)
> synchronous as for now.
>
>
>
> https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
>
>
https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc#ne...
> ui/aura/root_window.cc:303: void RootWindow::MoveCursorToInternal(
> On 2013/04/25 18:16:24, sky wrote:
>>
>> Make position match header, and when you wrap each parameter on its
>
> own line.
>
> Done.
>
> https://codereview.chromium.org/13947045/

[oshima, 2013-04-25 20:37:56]

On 2013/04/25 19:46:14, sky wrote:
> Tests are easily updated. What is the compelling reason for assuming
> the cursor moved? It's better to wait for the real event so we don't
> have to duplicate logic.
> 
>   -Scott
> 
> On Thu, Apr 25, 2013 at 11:46 AM,  <mailto:yoshiki@chromium.org> wrote:
> > On 2013/04/25 18:16:24, sky wrote:
> >>
> >> Agreed, they should all be consistent. Why do any of them need to be
> >> synchronous?
> >
> >
> > MoveCursorTo has been synchronous. I don't know the design. But I assume the
> > synchronous method was easier to handle.

You added this actually :)

https://chromiumcodereview.appspot.com/10543174

Waiting for events in unit tests can make it slow and/or flaky IMO, but
what's more important to me is to me being synchronous. ChromeOS controls
mouse pointer, and it's important to know the latest designated
location to do the right thing. For example, we update the cursor's scale based
on the location, which allow us to switch mouse cursor without waiting
for next event.

> >
> > If we made MoveCursorTo asynchronous, a lot of tests should be broken and
> > need
> > to be fixed. We should keep MoveCursorTo (and MoveCursorToHostLocation)
> > synchronous as for now.
> >
> >
> >
> > https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc
> > File ui/aura/root_window.cc (right):
> >
> >
>
https://codereview.chromium.org/13947045/diff/51002/ui/aura/root_window.cc#ne...
> > ui/aura/root_window.cc:303: void RootWindow::MoveCursorToInternal(
> > On 2013/04/25 18:16:24, sky wrote:
> >>
> >> Make position match header, and when you wrap each parameter on its
> >
> > own line.
> >
> > Done.
> >
> > https://codereview.chromium.org/13947045/

[sky, 2013-04-25 20:59:46]

Makes sense, LGTM

[yoshiki, 2013-04-26 02:47:09]

@sky, @oshima, Thanks!

> You added this actually :)
> https://chromiumcodereview.appspot.com/10543174

Oh, I forgot it. :-(

[commit-bot: I haz the power, 2013-04-26 06:22:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/13947045/70006

[commit-bot: I haz the power, 2013-04-26 09:02:10]

Change committed as 196683