chromeos: converts observed pointer events to DIPs

chromeos: converts observed pointer events to DIPs

They were in pixels and need to be in dips.

BUG=687700
TEST=covered by tests
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2685883003
Cr-Commit-Position: refs/heads/master@{#449453}
Committed: https://chromium.googlesource.com/chromium/src/+/907cde5ee78659803d8eb14736a2a00916c2fa35

[sky, 2017-02-08 21:50:07]

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

[sky, 2017-02-08 21:50:14]

Description was changed from

==========
chromeos: converts observed pointer events to DIPs

They were in pixels and need to be in dips.

BUG=687700
TEST=covered by tests
R=sadrul@chromium.org
==========

to

==========
chromeos: converts observed pointer events to DIPs

They were in pixels and need to be in dips.

BUG=687700
TEST=covered by tests
R=sadrul@chromium.org
==========

[sky, 2017-02-08 21:50:14]

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

[sky, 2017-02-08 21:50:24]

tsepez: mojom
sadrul: the rest

[commit-bot: I haz the power, 2017-02-08 21:51:00]

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-02-08 22:06:32]

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

[commit-bot: I haz the power, 2017-02-08 22:06:33]

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-02-08 23:17:50]

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

[commit-bot: I haz the power, 2017-02-08 23:18:37]

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-02-09 00:33:15]

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

[commit-bot: I haz the power, 2017-02-09 00:33:15]

Dry run: This issue passed the CQ dry run.

[riajiang, 2017-02-09 00:33:25]

riajiang@chromium.org changed reviewers:
+ riajiang@chromium.org

[riajiang, 2017-02-09 00:33:26]

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1151: DCHECK(display);
I thought there are checks in WindowTreeClient::OnWindowInputEvent to see if the
window it received from WindowTree is null? If the |target| here can be null,
then this |display| can be null? If the window cannot be null, would the
device_scale_factor we get that's associated with this display the same as if we
use the target window in WindowTreeClient::ScaleFactorForDisplay?

[sky, 2017-02-09 00:35:36]

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1151: DCHECK(display);
On 2017/02/09 00:33:26, riajiang wrote:
> I thought there are checks in WindowTreeClient::OnWindowInputEvent to see if
the
> window it received from WindowTree is null? If the |target| here can be null,
> then this |display| can be null? If the window cannot be null, would the
> device_scale_factor we get that's associated with this display the same as if
we
> use the target window in WindowTreeClient::ScaleFactorForDisplay?

In this function the window should never be null.
WindowTreeClient::OnWindowInputEvent() is asynchronously called though, so it's
entirely possible for the client to see a null window by the time it gets the
event.

[sadrul, 2017-02-09 02:27:46]

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:397:
EventDispatcher::AcceleratorMatchPhase::ANY);
Should we carry |display_id| through into EventDispatcher?

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:586: event_processing_display_id_);
Is |event_processing_display_id_| always going to be the correct display-id? For
example, if EventDispatcher is generating a pointer-exit event, that could be
going to a window that lives in a different display. Should we get the
display-id from |target| instead?

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:656: event_processing_display_id_);
Is this going to be correct? I assume an |event| that didn't find a target will
be in the coord-space of the screen. In that case, specifying a display-id would
probably be confusing? Perhaps just send -1 as the invalid/unknown display-id?

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:429: int64_t display_id) {
Can this figure out the display-id here from |target_window|, isntead of taking
it in as an input?

... although I guess since we already have this info from the calling code,
might as well use that instead of recomputing here?

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1205: std::unique_ptr<ui::Event>
event_in_dips(ui::Event::Clone(*event));
Is it necessary to clone here?

[riajiang, 2017-02-09 02:53:42]

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1151: DCHECK(display);
On 2017/02/09 00:35:36, sky wrote:
> On 2017/02/09 00:33:26, riajiang wrote:
> > I thought there are checks in WindowTreeClient::OnWindowInputEvent to see if
> the
> > window it received from WindowTree is null? If the |target| here can be
null,
> > then this |display| can be null? If the window cannot be null, would the
> > device_scale_factor we get that's associated with this display the same as
if
> we
> > use the target window in WindowTreeClient::ScaleFactorForDisplay?
> 
> In this function the window should never be null.
> WindowTreeClient::OnWindowInputEvent() is asynchronously called though, so
it's
> entirely possible for the client to see a null window by the time it gets the
> event.

Oh I see, thanks for your clarification!

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1154: ConvertEventLocationToDips(display_id,
event_in_dips->AsLocatedEvent());
event_in_dip and ConvertEventLocationToDIP to be consistent with other places?

[sky, 2017-02-09 17:04:36]

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

[commit-bot: I haz the power, 2017-02-09 17:05:08]

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

[sky, 2017-02-09 17:06:20]

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:397:
EventDispatcher::AcceleratorMatchPhase::ANY);
On 2017/02/09 02:27:45, sadrul wrote:
> Should we carry |display_id| through into EventDispatcher?

I was tempted to, but then EventDispatcher doesn't need it for anything and
EventDispatcher is entirely driven by WindowManagerState, so I didn't see a
reason to.

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:586: event_processing_display_id_);
On 2017/02/09 02:27:45, sadrul wrote:
> Is |event_processing_display_id_| always going to be the correct display-id?
For
> example, if EventDispatcher is generating a pointer-exit event, that could be
> going to a window that lives in a different display. Should we get the
> display-id from |target| instead?

With this patch all non-pointer-watcher events get the display id of the display
the window is on (see line 1153 in services/ui/ws/window_tree.cc). That leaves
pointer-watcher events. With this patch pointer-watcher events get the display
id of the display the event is generated on. This seems right given the client
uses that display to convert pixels to dips. That said, I don't know the state
of how we position multiple displays with differing densities. I believe
currently the positioning is done at the DIP level and I don't know if care is
taken to make sure the pixel boundaries line up nicely.

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:656: event_processing_display_id_);
On 2017/02/09 02:27:45, sadrul wrote:
> Is this going to be correct? I assume an |event| that didn't find a target
will
> be in the coord-space of the screen. In that case, specifying a display-id
would
> probably be confusing? Perhaps just send -1 as the invalid/unknown display-id?

See comment above. I believe this is right.

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:429: int64_t display_id) {
On 2017/02/09 02:27:45, sadrul wrote:
> Can this figure out the display-id here from |target_window|, isntead of
taking
> it in as an input?
> 
> ... although I guess since we already have this info from the calling code,
> might as well use that instead of recomputing here?

See my earlier comment as to why this passes in the display_id.

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1154: ConvertEventLocationToDips(display_id,
event_in_dips->AsLocatedEvent());
On 2017/02/09 02:53:42, riajiang wrote:
> event_in_dip and ConvertEventLocationToDIP to be consistent with other places?


Renamed to event_in_dip. For better or for worse current naming scheme says this
should be 'Dip'. If you're curious, dig up threads on chromium-dev.

https://codereview.chromium.org/2685883003/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1205: std::unique_ptr<ui::Event>
event_in_dips(ui::Event::Clone(*event));
On 2017/02/09 02:27:46, sadrul wrote:
> Is it necessary to clone here?

Indeed you are right it isn't. Updated.

[Tom Sepez, 2017-02-09 17:22:27]

mojom LGTM

[commit-bot: I haz the power, 2017-02-09 18:13:35]

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

[commit-bot: I haz the power, 2017-02-09 18:13:36]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-02-09 21:21:34]

lgtm

https://codereview.chromium.org/2685883003/diff/60001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.h (right):

https://codereview.chromium.org/2685883003/diff/60001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.h:156: struct InFlightEventDetails {
Btw: like this!

[sky, 2017-02-09 21:23:47]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:24:43]

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-02-09 21:33:58]

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

[commit-bot: I haz the power, 2017-02-09 21:34:02]

Failed to apply patch for services/ui/ws/window_manager_state_unittest.cc:
While running git apply --index -p1;
  error: patch failed: services/ui/ws/window_manager_state_unittest.cc:582
  error: services/ui/ws/window_manager_state_unittest.cc: patch does not apply

Patch:       services/ui/ws/window_manager_state_unittest.cc
Index: services/ui/ws/window_manager_state_unittest.cc
diff --git a/services/ui/ws/window_manager_state_unittest.cc
b/services/ui/ws/window_manager_state_unittest.cc
index
1de0f6047dcb267ad82715e0092276a5b8dd3949..8ed2a3301462f28859e2a557db6d1f02f9b77534
100644
--- a/services/ui/ws/window_manager_state_unittest.cc
+++ b/services/ui/ws/window_manager_state_unittest.cc
@@ -245,7 +245,7 @@ TEST_F(WindowManagerStateTest, PreTargetConsumed) {
 
   // Send an ensure only the pre accelerator is called.
   ui::KeyEvent key(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN);
-  window_manager_state()->ProcessEvent(key);
+  window_manager_state()->ProcessEvent(key, 0);
   EXPECT_TRUE(window_manager()->on_accelerator_called());
   EXPECT_EQ(accelerator_id, window_manager()->on_accelerator_id());
   EXPECT_TRUE(tracker->changes()->empty());
@@ -260,7 +260,7 @@ TEST_F(WindowManagerStateTest, PreTargetConsumed) {
   window_manager()->ClearAcceleratorCalled();
 
   // Repeat, but respond with UNHANDLED.
-  window_manager_state()->ProcessEvent(key);
+  window_manager_state()->ProcessEvent(key, 0);
   EXPECT_TRUE(window_manager()->on_accelerator_called());
   EXPECT_EQ(accelerator_id, window_manager()->on_accelerator_id());
   EXPECT_TRUE(tracker->changes()->empty());
@@ -582,7 +582,7 @@ TEST_F(WindowManagerStateTest, CursorResetOverNoTarget) {
       ui::ET_POINTER_MOVED, gfx::Point(), gfx::Point(), 0, 0, 0,
       ui::PointerDetails(EventPointerType::POINTER_TYPE_MOUSE),
       base::TimeTicks());
-  window_manager_state()->ProcessEvent(move);
+  window_manager_state()->ProcessEvent(move, 0);
   // The event isn't over a valid target, which should trigger resetting the
   // cursor to POINTER.
   EXPECT_EQ(ui::mojom::Cursor::POINTER, display_test_api.last_cursor());

[sky, 2017-02-09 22:19:09]

The CQ bit was checked by sky@chromium.org

[sky, 2017-02-09 22:19:12]

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

[commit-bot: I haz the power, 2017-02-09 22:19:44]

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-02-09 23:14:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486678749985080,
"parent_rev": "b54d63099501ccd240c5ab1d4e8f2ddbb0015b2d", "commit_rev":
"907cde5ee78659803d8eb14736a2a00916c2fa35"}

[commit-bot: I haz the power, 2017-02-09 23:14:47]

Description was changed from

==========
chromeos: converts observed pointer events to DIPs

They were in pixels and need to be in dips.

BUG=687700
TEST=covered by tests
R=sadrul@chromium.org
==========

to

==========
chromeos: converts observed pointer events to DIPs

They were in pixels and need to be in dips.

BUG=687700
TEST=covered by tests
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2685883003
Cr-Commit-Position: refs/heads/master@{#449453}
Committed:
https://chromium.googlesource.com/chromium/src/+/907cde5ee78659803d8eb14736a2...
==========

[commit-bot: I haz the power, 2017-02-09 23:14:51]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/907cde5ee78659803d8eb14736a2...