mash: make Env::last_mouse_location() accurate for Mus

mash: make Env::last_mouse_location() accurate for Mus

This patch makes last_mouse_location() reflect the location of the
current event while processing an event, otherwise calls to
WindowTreeClient::GetCursorScreenPoint().

BUG=679402
TEST=none
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2657283003
Cr-Commit-Position: refs/heads/master@{#447570}
Committed: https://chromium.googlesource.com/chromium/src/+/c19010b9bd9c531283b1157ed51a8af22ae665f4

[sky, 2017-01-27 16:10:04]

I need to write a test for this, and likely rename Env::last_mouse_location(),
GetLastMouseLocation(), but I wanted to make sure you couldn't think of any
problems with this approach.

[sky, 2017-01-27 16:10:11]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 16:10:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 16:35:42]

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

[commit-bot: I haz the power, 2017-01-27 16:35:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-01-27 19:25:52]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 19:26:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 20:20:42]

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

[commit-bot: I haz the power, 2017-01-27 20:20:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2017-01-27 21:08:36]

I assume this is because the read in WindowTreeClient::GetCursorScreenPoint() is
a little expensive? I can't think of any red flags

https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
File ui/aura/mus/mus_mouse_location_updater.cc (right):

https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
ui/aura/mus/mus_mouse_location_updater.cc:49:
Env::GetInstance()->get_last_mouse_location_from_mus_ = false;
Maybe call this |last_mouse_location_is_valid_| (and set to true from here)?
That I think would make the code in Env read a little easier.

https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
ui/aura/mus/mus_mouse_location_updater.cc:51:
base::MessageLoop::current()->AddNestingObserver(this);
Is it better to just leave it always added as a nesting observer instead of
add/removing for each event?

[sadrul, 2017-01-27 21:24:53]

On 2017/01/27 21:08:36, sadrul wrote:
> I assume this is because the read in WindowTreeClient::GetCursorScreenPoint()
is
> a little expensive? I can't think of any red flags

I guess this also necessary so two consecutive calls to last_mouse_location()
doesn't return different values.

> 
>
https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
> File ui/aura/mus/mus_mouse_location_updater.cc (right):
> 
>
https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
> ui/aura/mus/mus_mouse_location_updater.cc:49:
> Env::GetInstance()->get_last_mouse_location_from_mus_ = false;
> Maybe call this |last_mouse_location_is_valid_| (and set to true from here)?
> That I think would make the code in Env read a little easier.
> 
>
https://codereview.chromium.org/2657283003/diff/20001/ui/aura/mus/mus_mouse_l...
> ui/aura/mus/mus_mouse_location_updater.cc:51:
> base::MessageLoop::current()->AddNestingObserver(this);
> Is it better to just leave it always added as a nesting observer instead of
> add/removing for each event?

[sky, 2017-01-31 20:25:01]

I considered not doing this, and always calling through to WindowTreeClient, but
then that seems risky for observed events. For observed events the server
doesn't wait for the the client to process them, so it's entirely likely the
value returned from WindowTreeClient is different from that of the event.

I've update this and added a test. I'll rename last_mouse_location() separately.
Take another look?

[sky, 2017-01-31 20:25:07]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:26:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-01-31 20:40:27]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:40:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 21:43:31]

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

[commit-bot: I haz the power, 2017-01-31 21:43:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-01-31 22:11:35]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 22:12:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 23:40:29]

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

[commit-bot: I haz the power, 2017-01-31 23:40:30]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-01 00:41:56]

Description was changed from

==========
WIP: mash: make Env::last_mouse_location() accurate for Mus

This patch makes last_mouse_location() reflect the location of the
current event while processing an event, otherwise calls to
WindowTreeClient::GetCursorScreenPoint().

BUG=679402
TEST=none
R=sadrul@chromium.org
==========

to

==========
mash: make Env::last_mouse_location() accurate for Mus

This patch makes last_mouse_location() reflect the location of the
current event while processing an event, otherwise calls to
WindowTreeClient::GetCursorScreenPoint().

BUG=679402
TEST=none
R=sadrul@chromium.org
==========

[sadrul, 2017-02-01 17:30:14]

lgtm

https://codereview.chromium.org/2657283003/diff/100001/ui/aura/env.h
File ui/aura/env.h (right):

https://codereview.chromium.org/2657283003/diff/100001/ui/aura/env.h#newcode78
ui/aura/env.h:78: const gfx::Point& last_mouse_location();
Alternatively, maybe we could keep this const, and make |last_mouse_location_|
mutable?

[sky, 2017-02-01 17:50:20]

The CQ bit was checked by sky@chromium.org

[sky, 2017-02-01 17:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2657283003/#ps140001
(title: "merge")

[commit-bot: I haz the power, 2017-02-01 17:50:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-02-01 17:58:09]

https://codereview.chromium.org/2657283003/diff/100001/ui/aura/env.h
File ui/aura/env.h (right):

https://codereview.chromium.org/2657283003/diff/100001/ui/aura/env.h#newcode78
ui/aura/env.h:78: const gfx::Point& last_mouse_location();
On 2017/02/01 17:30:14, sadrul wrote:
> Alternatively, maybe we could keep this const, and make |last_mouse_location_|
> mutable?

Done.

[commit-bot: I haz the power, 2017-02-01 19:17:31]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485971420904020,
"parent_rev": "f28533e16f132079d4e88a4dde87895351d2c866", "commit_rev":
"c19010b9bd9c531283b1157ed51a8af22ae665f4"}

[commit-bot: I haz the power, 2017-02-01 19:18:10]

Description was changed from

==========
mash: make Env::last_mouse_location() accurate for Mus

This patch makes last_mouse_location() reflect the location of the
current event while processing an event, otherwise calls to
WindowTreeClient::GetCursorScreenPoint().

BUG=679402
TEST=none
R=sadrul@chromium.org
==========

to

==========
mash: make Env::last_mouse_location() accurate for Mus

This patch makes last_mouse_location() reflect the location of the
current event while processing an event, otherwise calls to
WindowTreeClient::GetCursorScreenPoint().

BUG=679402
TEST=none
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2657283003
Cr-Commit-Position: refs/heads/master@{#447570}
Committed:
https://chromium.googlesource.com/chromium/src/+/c19010b9bd9c531283b1157ed51a...
==========

[commit-bot: I haz the power, 2017-02-01 19:18:11]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c19010b9bd9c531283b1157ed51a...