Fix software mirror mode on Ozone part 2/2

Fix software mirror mode on Ozone part 2/2

This CL allows multiple touchscreens to be associated with one DriWindow.
In software mirror mode the displays are in extended mode, but
TouchTransformerController changes the touch event's coordinates to
be in the primary DriWindow's bounds.

BUG=444101
TEST=Manual, see bug

Committed: https://crrev.com/ae752c047c2ac5682340a7f0e719a5f542cfe552
Cr-Commit-Position: refs/heads/master@{#319929}

[pkotwicz, 2015-02-12 21:35:49]

pkotwicz@chromium.org changed reviewers:
+ spang@chromium.org

[pkotwicz, 2015-02-12 21:35:50]

spang@ can you please take a look?

I know that I need to change all of the PlatformWindow implementations

I think that it makes more sense to associate a PlatformWindow with the
touchscreens it should be accepting events from (than to associate both
touchscreens with one display as you suggested)

[spang, 2015-02-12 22:29:07]

On 2015/02/12 21:35:50, pkotwicz wrote:
> spang@ can you please take a look?
> 
> I know that I need to change all of the PlatformWindow implementations
> 
> I think that it makes more sense to associate a PlatformWindow with the
> touchscreens it should be accepting events from (than to associate both
> touchscreens with one display as you suggested)

I'm sorry for being to dense, but I still don't get why this is better. Can you
please elaborate on how it is better?

I don't think "accepting events from a device" is actually the right mental
model for dispatch to PlatformWindow. Windowing systems do not actually work the
way PlatformEventSource works way.

Secure windowing systems do not (and cannot) work that way. They figure out the
event location first, and find out which window to deliver it to second.

[pkotwicz, 2015-02-13 00:14:39]

DeviceDataManager::GetDisplayForTouchDevice() is the inverse of
DisplayInfo::touch_device_id().
It does not make sense for DeviceDataManager::GetDisplayForTouchDevice() to
return the same display for multiple touchscreens but for
DisplayInfo::touch_device_id() to return just one touchscreen.

It is necessary to know which display the touchscreen is actually associated
with to figure out insets for pillarboxing/letterboxing. In particular,
TouchTransformerController::GetTouchTransform() takes in two displays. (So
having Display::touch_device_id() return multiple touchscreen ids is a bad idea)

DriWindow could get events in coordinates relative to DriWindow::bounds_ in
software mirror mode.
AshWindowTreeHostOzone could forward TouchEvents to the primary WTH.
AshWindowTreeHostOzone would need to know:
- Which AshWindowTreeHostOzone the touch event needs to be forwarded to
- Whether it is in software mirroring mode. The transformation of the touch
event location is very special for software mirroring. Not as simple as what we
do for mouse events in AshWindowTreeHost::TranslateLocatedEvent()
This approach is a mess

[spang, 2015-02-13 16:53:02]

On 2015/02/13 00:14:39, pkotwicz wrote:
> DeviceDataManager::GetDisplayForTouchDevice() is the inverse of
> DisplayInfo::touch_device_id().
> It does not make sense for DeviceDataManager::GetDisplayForTouchDevice() to
> return the same display for multiple touchscreens but for
> DisplayInfo::touch_device_id() to return just one touchscreen.
> 
> It is necessary to know which display the touchscreen is actually associated
> with to figure out insets for pillarboxing/letterboxing. In particular,
> TouchTransformerController::GetTouchTransform() takes in two displays. (So
> having Display::touch_device_id() return multiple touchscreen ids is a bad
idea)

Does GetDisplayForTouchDevice have any purpose other than to be read by
DriWindow/WTHX11 during event dispatch? If not I don't see why it would matter.
Ash could always keep track of the real pairing.

Aside: touch_device_id() is something that may need to be adjusted anyway -
robert.brantford@intel.com was trying to bring up a device that had two touch
devices for one screen. 

> DriWindow could get events in coordinates relative to DriWindow::bounds_ in
> software mirror mode.
> AshWindowTreeHostOzone could forward TouchEvents to the primary WTH.
> AshWindowTreeHostOzone would need to know:
> - Which AshWindowTreeHostOzone the touch event needs to be forwarded to
> - Whether it is in software mirroring mode. The transformation of the touch
> event location is very special for software mirroring. Not as simple as what
we
> do for mouse events in AshWindowTreeHost::TranslateLocatedEvent()
> This approach is a mess

I would like to explore this further. Why wouldn't AshWindowTreeHostOzone know
that it is mirroring? Ash is already coordinating this.

[pkotwicz, 2015-02-13 17:06:18]

pkotwicz@chromium.org changed reviewers:
+ oshima@chromium.org

[pkotwicz, 2015-02-13 17:06:19]

+Oshima for his expert advice.

We are trying to figure out the correct architecture to fix software mirror mode
in ozone. Touches on the mirroring display while software mirroring is active
are ignored in Ozone. In order to fix this, we need to implement
AshWindowTreeHostOzone::UpdateDisplayID()

spang@ is suggesting that Ozone should not know about the display IDs which are
associated with a PlatformWindow but should get this information in a roundabout
way.

Oshima, do you have any suggestions as to how to make architecture "nice".
Please see the comments for more details on spang@'s opinion

[spang, 2015-02-13 17:17:46]

On 2015/02/13 17:06:19, pkotwicz wrote:
> +Oshima for his expert advice.
> 
> We are trying to figure out the correct architecture to fix software mirror
mode
> in ozone. Touches on the mirroring display while software mirroring is active
> are ignored in Ozone. In order to fix this, we need to implement
> AshWindowTreeHostOzone::UpdateDisplayID()
> 
> spang@ is suggesting that Ozone should not know about the display IDs which
are
> associated with a PlatformWindow but should get this information in a
roundabout
> way.
> 
> Oshima, do you have any suggestions as to how to make architecture "nice".
> Please see the comments for more details on spang@'s opinion

Let me elaborate on my stance.

One outcome I strongly do not want is to put touchscreen/display configuration
stuff on PlatformWindow. This is the windowing system client interface, if we
add methods like this then nobody else will be able to implement them because
these methods don't make sense on other platforms.

I would be less resistant to putting this on the display configuration or other
ChromeOS specific interface (like NativeDisplayDelegate). We could create a new
one interface if absolutely necessary.

But I also think that as long as ash implements SW mirroring by creating two
separate windows, it should be prepared to receive two separate event streams
for them. This seems like the cleanest way to me given the current layering.

[sadrul, 2015-02-13 17:22:44]

On 2015/02/13 17:17:46, spang wrote:
> On 2015/02/13 17:06:19, pkotwicz wrote:
> > +Oshima for his expert advice.
> > 
> > We are trying to figure out the correct architecture to fix software mirror
> mode
> > in ozone. Touches on the mirroring display while software mirroring is
active
> > are ignored in Ozone. In order to fix this, we need to implement
> > AshWindowTreeHostOzone::UpdateDisplayID()
> > 
> > spang@ is suggesting that Ozone should not know about the display IDs which
> are
> > associated with a PlatformWindow but should get this information in a
> roundabout
> > way.
> > 
> > Oshima, do you have any suggestions as to how to make architecture "nice".
> > Please see the comments for more details on spang@'s opinion
> 
> Let me elaborate on my stance.
> 
> One outcome I strongly do not want is to put touchscreen/display configuration
> stuff on PlatformWindow. This is the windowing system client interface, if we
> add methods like this then nobody else will be able to implement them because
> these methods don't make sense on other platforms.
> 
> I would be less resistant to putting this on the display configuration or
other
> ChromeOS specific interface (like NativeDisplayDelegate). We could create a
new
> one interface if absolutely necessary.
> 
> But I also think that as long as ash implements SW mirroring by creating two
> separate windows, it should be prepared to receive two separate event streams
> for them. This seems like the cleanest way to me given the current layering.

DeviceDataManager currently maintains a touch-device-id ==> display-id map. Can
that be useful here?

[spang, 2015-02-13 18:00:05]

On 2015/02/13 17:22:44, sadrul wrote:
> On 2015/02/13 17:17:46, spang wrote:
> > On 2015/02/13 17:06:19, pkotwicz wrote:
> > > +Oshima for his expert advice.
> > > 
> > > We are trying to figure out the correct architecture to fix software
mirror
> > mode
> > > in ozone. Touches on the mirroring display while software mirroring is
> active
> > > are ignored in Ozone. In order to fix this, we need to implement
> > > AshWindowTreeHostOzone::UpdateDisplayID()
> > > 
> > > spang@ is suggesting that Ozone should not know about the display IDs
which
> > are
> > > associated with a PlatformWindow but should get this information in a
> > roundabout
> > > way.
> > > 
> > > Oshima, do you have any suggestions as to how to make architecture "nice".
> > > Please see the comments for more details on spang@'s opinion
> > 
> > Let me elaborate on my stance.
> > 
> > One outcome I strongly do not want is to put touchscreen/display
configuration
> > stuff on PlatformWindow. This is the windowing system client interface, if
we
> > add methods like this then nobody else will be able to implement them
because
> > these methods don't make sense on other platforms.
> > 
> > I would be less resistant to putting this on the display configuration or
> other
> > ChromeOS specific interface (like NativeDisplayDelegate). We could create a
> new
> > one interface if absolutely necessary.
> > 
> > But I also think that as long as ash implements SW mirroring by creating two
> > separate windows, it should be prepared to receive two separate event
streams
> > for them. This seems like the cleanest way to me given the current layering.
> 
> DeviceDataManager currently maintains a touch-device-id ==> display-id map.
Can
> that be useful here?

That's what we're using today, but it's currently broken for SW mirroring
because it stores the real pairing (and Peter thinks it should stay that way).
So this conflicts with the requirement to virtualize the mirrored display's
touchscreen onto the primary display (so that they arrive on a different window
entirely).

I'm still seeing if we can't change that virtualization requirement - ash should
accept the full semantics of having two windows in SW mirroring and not ask for
subtle changes to dispatch.

[spang, 2015-02-13 18:20:56]

On 2015/02/13 17:06:19, pkotwicz wrote:
> +Oshima for his expert advice.
> 
> We are trying to figure out the correct architecture to fix software mirror
mode
> in ozone. Touches on the mirroring display while software mirroring is active
> are ignored in Ozone. In order to fix this, we need to implement
> AshWindowTreeHostOzone::UpdateDisplayID()
> 
> spang@ is suggesting that Ozone should not know about the display IDs which
are
> associated with a PlatformWindow but should get this information in a
roundabout
> way.

It's not a goal to do it in a roundabout way, but it is a goal to have clear
concepts. A window is not the same thing as a display. We can assume there is a
1:1 association in ash, and also in CrOS ozone code, but we cannot encode this
assumption into the windowing interface itself.

> 
> Oshima, do you have any suggestions as to how to make architecture "nice".
> Please see the comments for more details on spang@'s opinion

[pkotwicz, 2015-02-18 18:53:50]

Oshima, Ping!

[oshima, 2015-02-24 01:11:43]

On 2015/02/18 18:53:50, pkotwicz wrote:
> Oshima, Ping!

Sorry I was OOO last week. I'll take a look today.

[oshima, 2015-02-24 16:12:46]

On 2015/02/24 01:11:43, oshima wrote:
> On 2015/02/18 18:53:50, pkotwicz wrote:
> > Oshima, Ping!
> 
> Sorry I was OOO last week. I'll take a look today.

Am I correct that spang@ want to keep display/touch configuration stuff from
DriWindow while
pkotwicz@ want to keep touch event transformation stuff in
TouchTransformerController?

I understand spang@'s opinion about keeping DriWindow clean.

> Not as simple as what we
> do for mouse events in AshWindowTreeHost::TranslateLocatedEvent()
> This approach is a mess

While I can guess this can be messy, can you elaborate a bit about this?

Another way is to move the logic in CanDispatchEvent to delegate interface in
DriWindow (DriWindow::EventDelegate?).
Have you considered this approach?

[spang, 2015-02-24 18:40:54]

On 2015/02/24 16:12:46, oshima wrote:
> On 2015/02/24 01:11:43, oshima wrote:
> > On 2015/02/18 18:53:50, pkotwicz wrote:
> > > Oshima, Ping!
> > 
> > Sorry I was OOO last week. I'll take a look today.
> 
> Am I correct that spang@ want to keep display/touch configuration stuff from
> DriWindow while
> pkotwicz@ want to keep touch event transformation stuff in
> TouchTransformerController?
> 
> I understand spang@'s opinion about keeping DriWindow clean.
> 
> > Not as simple as what we
> > do for mouse events in AshWindowTreeHost::TranslateLocatedEvent()
> > This approach is a mess
> 
> While I can guess this can be messy, can you elaborate a bit about this?
> 
> Another way is to move the logic in CanDispatchEvent to delegate interface in
> DriWindow (DriWindow::EventDelegate?).
> Have you considered this approach?

I think any issue you could solve with an EventDelegate you could also solve by
just accepting events in PlatformWindowDelegate (i.e. [Ash]WindowTreeHost) and
then possibly retargeting them after the fact.

The reason doing that is a big win is that there are many more implementations
of PlatformWindow than there are of AshWindowTreeHost. Most of those are in
external projects - PlatformWindow is part of the portability interface and
should be kept small.

[pkotwicz, 2015-02-24 22:46:30]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2015-02-24 22:47:41]

spang@ can you please take another look?

I have added PlatformWindowDelegate::CanDispatchEvent() as you suggested. Thanks
for the suggestion!

[spang, 2015-02-24 22:58:08]

On 2015/02/24 22:47:41, pkotwicz wrote:
> spang@ can you please take another look?
> 
> I have added PlatformWindowDelegate::CanDispatchEvent() as you suggested.
Thanks
> for the suggestion!

My actual suggestion was to put any special behavior
PlatformWindowDelegate::DispatchEvent() not add a new one. Ash has global
knowledge and can do it.

Can you address the issue of why it's hard to do this entirely in ash without
adding stuff to the interface?

[oshima, 2015-02-25 17:47:10]

On 2015/02/24 22:58:08, spang wrote:
> On 2015/02/24 22:47:41, pkotwicz wrote:
> > spang@ can you please take another look?
> > 
> > I have added PlatformWindowDelegate::CanDispatchEvent() as you suggested.
> Thanks
> > for the suggestion!
> 
> My actual suggestion was to put any special behavior
> PlatformWindowDelegate::DispatchEvent() not add a new one. Ash has global
> knowledge and can do it.
> 
> Can you address the issue of why it's hard to do this entirely in ash without
> adding stuff to the interface?

Yes, I'd like to know.

I feel it's not unreasonable for ozone to have a generic framework to redirect
event,
but it'd be nice if we can do them all outside of ozone (ash in this case)
in clean way.

[pkotwicz, 2015-02-25 22:01:50]

I did some more investigation. We can have
PlatformWindowDelegate::DispatchEvent() retarget touch events in software mirror
mode. We would need to figure out how to tell AshWindowTreeHost (1) that it
needs to retarget touch events (2) who it needs to retarget touch events to

Option A: Introduce AshWindowTreeHost::RetargetTouchEventsTo(AshWindowTreeHost*
other_host)
Concerns:
- Where would the retargetting occur? WindowTargetter would be a logical place,
but option B might make more sense in this case
- We would need to associate a ScreenPositionClient with the mirrored window in
order to allow the retargetting to occur
- Other changes needed so that retargetting event works (e.g. my implementation
has trouble with one of the displays having a different device scale factor)

Option B: Introduce AshWindowTreeHost::SetTouchCapture()
Concerns:
- AshWindowTreeHost::SetTouchCapture() would call PlatformWindow::SetCapture()
- This would produce the correct behavior
- How would we track who has touch capture? A global variable somewhere?
- We would need to support nested capture (AshWindowTreeHost::SetCapture() and
AshWindowTreeHost::SetTouchCapture())
- We would need to figure out how to prevent one AshWindowTreeHost from having
regular capture and another having touch capture. (Regular capture applies to
touches too!)

Option C: Add a clear way of querying DeviceDataManager for which PlatformWindow
should process the event. spang@ suggested making 
DeviceDataManager::GetDisplayForTouchDevice() return the same display in mirror
mode.
Concerns:
- DeviceDataManager::GetDisplayForTouchDevice() is no longer reversible via
gfx::DisplayInfo::touch_device_id()
- This is just a way convoluted way of telling a PlatformWindow which device ids
to accept touches from. This  CL is a much cleaner way of doing this

[pkotwicz, 2015-02-25 22:04:07]

*gfx::DisplayInfo::touch_device_id() -> ash::DisplayInfo::touch_device_id()

[spang, 2015-02-25 22:19:39]

Couldn't you build a generic event reflector? It doesn't seem like this problem
is really even touch specific. Keyboard, mouse, etc can all be forwarded along
the same path.

It would just transforms located events for a WTH into another WTH's coordinate
space, and then forward them. Non-located events can be forwarded unmodified.

Doing that should work properly if you set up a SW mirroring test on your
workstation and clicked, touched, or otherwise interacted with the mirror
window.

On 2015/02/25 22:01:50, pkotwicz wrote:
> I did some more investigation. We can have
> PlatformWindowDelegate::DispatchEvent() retarget touch events in software
mirror
> mode. We would need to figure out how to tell AshWindowTreeHost (1) that it
> needs to retarget touch events (2) who it needs to retarget touch events to
> 
> Option A: Introduce
AshWindowTreeHost::RetargetTouchEventsTo(AshWindowTreeHost*
> other_host)
> Concerns:
> - Where would the retargetting occur? WindowTargetter would be a logical
place,
> but option B might make more sense in this case
> - We would need to associate a ScreenPositionClient with the mirrored window
in
> order to allow the retargetting to occur
> - Other changes needed so that retargetting event works (e.g. my
implementation
> has trouble with one of the displays having a different device scale factor)
> 
> Option B: Introduce AshWindowTreeHost::SetTouchCapture()
> Concerns:
> - AshWindowTreeHost::SetTouchCapture() would call PlatformWindow::SetCapture()
> - This would produce the correct behavior
> - How would we track who has touch capture? A global variable somewhere?
> - We would need to support nested capture (AshWindowTreeHost::SetCapture() and
> AshWindowTreeHost::SetTouchCapture())
> - We would need to figure out how to prevent one AshWindowTreeHost from having
> regular capture and another having touch capture. (Regular capture applies to
> touches too!)
> 
> Option C: Add a clear way of querying DeviceDataManager for which
PlatformWindow
> should process the event. spang@ suggested making 
> DeviceDataManager::GetDisplayForTouchDevice() return the same display in
mirror
> mode.
> Concerns:
> - DeviceDataManager::GetDisplayForTouchDevice() is no longer reversible via
> gfx::DisplayInfo::touch_device_id()
> - This is just a way convoluted way of telling a PlatformWindow which device
ids
> to accept touches from. This  CL is a much cleaner way of doing this

[robert.bradford, 2015-02-25 22:26:26]

> - DeviceDataManager::GetDisplayForTouchDevice() is no longer reversible via
> gfx::DisplayInfo::touch_device_id()


I've got a local patch where i'm experimenting with making the
DisplayInfo::touch_device_id() as vector of ids. So there is a N:1 mapping of
touchscreens to display screens in order to handle the situation that you have
an on-screen stylus that generates a touch events as well as a touchscreen. In
mirroring i'd expect the stylus "touchscreen" to behave like the main
touchscreen.

I do not recall observing any places that relied on the assumption on the
reversibility you mention above although I will need to check that further.

[pkotwicz, 2015-02-25 22:58:22]

This is an architecture problem not a coding problem. I do not know what the
best architecture is. The interesting problem is how to tell an
AshWindowTreeHost (1) that it needs to forward events (2) which
AshWindowTreeHost events should be forwarded to

An event reflector class can do the retargetting if it is passed the event,
source AWTH, and destination AWTH.

[spang, 2015-02-25 23:02:34]

On 2015/02/25 22:58:22, pkotwicz wrote:
> This is an architecture problem not a coding problem. I do not know what the
> best architecture is. The interesting problem is how to tell an
> AshWindowTreeHost (1) that it needs to forward events (2) which
> AshWindowTreeHost events should be forwarded to
> 
> An event reflector class can do the retargetting if it is passed the event,
> source AWTH, and destination AWTH.

Couldn't the input reflector be created in the same place we create the graphics
reflector?

I think MirrorWindowController. Although I am not too familiar with it.

[pkotwicz, 2015-02-25 23:20:34]

Oshima, what do you think about comments 19 to 25?

[oshima, 2015-02-26 18:30:43]

On 2015/02/25 22:01:50, pkotwicz wrote:
> I did some more investigation. We can have
> PlatformWindowDelegate::DispatchEvent() retarget touch events in software
mirror
> mode. We would need to figure out how to tell AshWindowTreeHost (1) that it
> needs to retarget touch events (2) who it needs to retarget touch events to
> 
> Option A: Introduce
AshWindowTreeHost::RetargetTouchEventsTo(AshWindowTreeHost*
> other_host)
> Concerns:
> - Where would the retargetting occur? WindowTargetter would be a logical
place,
> but option B might make more sense in this case
> - We would need to associate a ScreenPositionClient with the mirrored window
in
> order to allow the retargetting to occur
> - Other changes needed so that retargetting event works (e.g. my
implementation
> has trouble with one of the displays having a different device scale factor)
> 
> Option B: Introduce AshWindowTreeHost::SetTouchCapture()
> Concerns:
> - AshWindowTreeHost::SetTouchCapture() would call PlatformWindow::SetCapture()
> - This would produce the correct behavior
> - How would we track who has touch capture? A global variable somewhere?
> - We would need to support nested capture (AshWindowTreeHost::SetCapture() and
> AshWindowTreeHost::SetTouchCapture())
> - We would need to figure out how to prevent one AshWindowTreeHost from having
> regular capture and another having touch capture. (Regular capture applies to
> touches too!)
> 
> Option C: Add a clear way of querying DeviceDataManager for which
PlatformWindow
> should process the event. spang@ suggested making 
> DeviceDataManager::GetDisplayForTouchDevice() return the same display in
mirror
> mode.
> Concerns:
> - DeviceDataManager::GetDisplayForTouchDevice() is no longer reversible via
> gfx::DisplayInfo::touch_device_id()
> - This is just a way convoluted way of telling a PlatformWindow which device
ids
> to accept touches from. This  CL is a much cleaner way of doing this

option C) seems to be cleaner solution to me. You can rename/add method
GetLogicalDisplayForTouchDevice()
and may want to provide observer for client that needs to take action when
logical mapping has changed.

[pkotwicz, 2015-02-27 02:26:58]

Oshima, can you please take another look. I went ahead with option (C)

[oshima, 2015-02-27 02:37:43]

it looks much simpler and readable to me. thanks!

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.cc:79: return (touch_device_id > 0 &&
touch_device_id < kMaxDeviceNum);
optional: it may be safer to reset the table entry if it's not valid.

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetDestinationDisplayForTouchDevice(unsigned int touch_device_id);
I'll leave this to sadrul, but I feel that the name is a bit too long.

MappedDisplay, LogicalDisplay or DestDisplay may work.

[pkotwicz, 2015-02-27 02:47:06]

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.cc:79: return (touch_device_id > 0 &&
touch_device_id < kMaxDeviceNum);
My understanding is that there is no table entry if the touch device id is not
valid

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetDestinationDisplayForTouchDevice(unsigned int touch_device_id);
I'm leaving this to Sadrul too

[oshima, 2015-02-27 20:45:31]

ash/ lgtm

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.cc:79: return (touch_device_id > 0 &&
touch_device_id < kMaxDeviceNum);
On 2015/02/27 02:47:06, pkotwicz wrote:
> My understanding is that there is no table entry if the touch device id is not
> valid

this is just a array, not map right? anyway, if this isn't an issue, then please
ignore.

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetDestinationDisplayForTouchDevice(unsigned int touch_device_id);
On 2015/02/27 02:47:06, pkotwicz wrote:
> I'm leaving this to Sadrul too

GetTargetdisplayForTouchDevice could be a good candidate.

[pkotwicz, 2015-03-01 16:04:00]

pkotwicz@chromium.org changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2015-03-01 16:04:00]

Sadrul, can you please take a look at the changes in ui/events?

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/922843002/diff/60001/ui/events/devices/device...
ui/events/devices/device_data_manager.cc:79: return (touch_device_id > 0 &&
touch_device_id < kMaxDeviceNum);
I don't think I understand your concern

We check whether the touch device id is valid because we use the id as the index
into |touch_device_to_destination_display_map_|. If the touch device id is
invalid, there isn't an entry in |touch_device_to_destination_display_map_| for
the id.

[sadrul, 2015-03-02 16:56:18]

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
How is this different from GetDisplayForTouchDevice()?

[pkotwicz, 2015-03-02 17:55:53]

Sadrul, can you please take another look?

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
In mirror mode (either software or hardware):
- GetDisplayForTouchDevice() returns the display that the touch device is on.
This is a different display for each touch device.
- GetTargetDisplayForTouchDevice() returns the display that touches should be
forwarded to. This is the same display for all touch devices.

GetDisplayForTouchDevice() still exists for the sake of
DeviceDataManagerX11::TouchEventNeedsCalibrate(). We could remove the need for
GetDisplayForTouchDevice() by checking whether the touch device (instead of the
display) is internal or external. This is what Ozone does in
TouchEventConverterEvdev.

[spang, 2015-03-02 19:12:36]

On 2015/03/02 17:55:53, pkotwicz wrote:
> Sadrul, can you please take another look?
> 
>
https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
> File ui/events/devices/device_data_manager.h (right):
> 
>
https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
> ui/events/devices/device_data_manager.h:47: int64_t
> GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
> In mirror mode (either software or hardware):
> - GetDisplayForTouchDevice() returns the display that the touch device is on.
> This is a different display for each touch device.
> - GetTargetDisplayForTouchDevice() returns the display that touches should be
> forwarded to. This is the same display for all touch devices.
> 
> GetDisplayForTouchDevice() still exists for the sake of
> DeviceDataManagerX11::TouchEventNeedsCalibrate(). We could remove the need for
> GetDisplayForTouchDevice() by checking whether the touch device (instead of
the
> display) is internal or external. This is what Ozone does in
> TouchEventConverterEvdev.

I'm ok with this esp. because otherwise we are going around in circles.

But I do think it is more subtle and less correct than the approach I outlined
above. So hopefully we can get there eventually.

lgtm

[sadrul, 2015-03-03 04:43:09]

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
On 2015/03/02 17:55:53, pkotwicz wrote:
> In mirror mode (either software or hardware):
> - GetDisplayForTouchDevice() returns the display that the touch device is on.
> This is a different display for each touch device.
> - GetTargetDisplayForTouchDevice() returns the display that touches should be
> forwarded to. This is the same display for all touch devices.
> 
> GetDisplayForTouchDevice() still exists for the sake of
> DeviceDataManagerX11::TouchEventNeedsCalibrate(). We could remove the need for
> GetDisplayForTouchDevice() by checking whether the touch device (instead of
the
> display) is internal or external. This is what Ozone does in
> TouchEventConverterEvdev.

We should have just one of these functions. Having two
pretty-similar-but-slightly-different information here is confusing.

> - GetTargetDisplayForTouchDevice() returns the display that touches should be
> forwarded to. This is the same display for all touch devices.

Why doesn't Display have this information? e.g. in addition to a
ui::MultipleDisplayState, Display could have a |mirror_display_| id that points
to the mirroring display. That feels like the better place for this information.

[pkotwicz, 2015-03-03 20:34:56]

Sadrul, is this what you had in mind?

Using gfx::Display is a bit awkward because PlatformWindow does not have access
to a gfx::NativeView, hence we can't get a gfx::Screen instance so we need to
cache the gfx::Display in ui::DeviceDataManager.

[oshima, 2015-03-04 00:54:37]

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
On 2015/03/03 04:43:09, sadrul wrote:
> On 2015/03/02 17:55:53, pkotwicz wrote:
> > In mirror mode (either software or hardware):
> > - GetDisplayForTouchDevice() returns the display that the touch device is
on.
> > This is a different display for each touch device.
> > - GetTargetDisplayForTouchDevice() returns the display that touches should
be
> > forwarded to. This is the same display for all touch devices.
> > 
> > GetDisplayForTouchDevice() still exists for the sake of
> > DeviceDataManagerX11::TouchEventNeedsCalibrate(). We could remove the need
for
> > GetDisplayForTouchDevice() by checking whether the touch device (instead of
> the
> > display) is internal or external. This is what Ozone does in
> > TouchEventConverterEvdev.
> 
> We should have just one of these functions. Having two
> pretty-similar-but-slightly-different information here is confusing.

How about using GetDisplayForTouchDevice() for all logical mapping,
and then let ash tell the device manager which device needs calibration? (looks
like TouchTransformerController can do this) This way, we don't need
two-pretty-similar-but-slightly-different functions, and can keep software
mirroring logic inside ash.
 
> > - GetTargetDisplayForTouchDevice() returns the display that touches should
be
> > forwarded to. This is the same display for all touch devices.
> 
> Why doesn't Display have this information? e.g. in addition to a
> ui::MultipleDisplayState, Display could have a |mirror_display_| id that
points
> to the mirroring display. That feels like the better place for this
information.

https://codereview.chromium.org/922843002/diff/100001/ui/gfx/display.h
File ui/gfx/display.h (right):

https://codereview.chromium.org/922843002/diff/100001/ui/gfx/display.h#newcod...
ui/gfx/display.h:118: int64 mirrored_display_id() const { return
mirrored_display_id_; }
This is low level information that none of chrome code (chrome/content stuff)
needs, so I'd like to avoid having this in gfx::Display. Can we at least put
this somewhere in ui/display?

[sadrul, 2015-03-05 11:07:14]

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/922843002/diff/80001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:47: int64_t
GetTargetDisplayForTouchDevice(unsigned int touch_device_id);
On 2015/03/04 00:54:36, oshima wrote:
> On 2015/03/03 04:43:09, sadrul wrote:
> > On 2015/03/02 17:55:53, pkotwicz wrote:
> > > In mirror mode (either software or hardware):
> > > - GetDisplayForTouchDevice() returns the display that the touch device is
> on.
> > > This is a different display for each touch device.
> > > - GetTargetDisplayForTouchDevice() returns the display that touches should
> be
> > > forwarded to. This is the same display for all touch devices.
> > > 
> > > GetDisplayForTouchDevice() still exists for the sake of
> > > DeviceDataManagerX11::TouchEventNeedsCalibrate(). We could remove the need
> for
> > > GetDisplayForTouchDevice() by checking whether the touch device (instead
of
> > the
> > > display) is internal or external. This is what Ozone does in
> > > TouchEventConverterEvdev.
> > 
> > We should have just one of these functions. Having two
> > pretty-similar-but-slightly-different information here is confusing.
> 
> How about using GetDisplayForTouchDevice() for all logical mapping,
> and then let ash tell the device manager which device needs calibration?
(looks
> like TouchTransformerController can do this) This way, we don't need
> two-pretty-similar-but-slightly-different functions, and can keep software
> mirroring logic inside ash.

I think that sounds good, yeah.

>  
> > > - GetTargetDisplayForTouchDevice() returns the display that touches should
> be
> > > forwarded to. This is the same display for all touch devices.
> > 
> > Why doesn't Display have this information? e.g. in addition to a
> > ui::MultipleDisplayState, Display could have a |mirror_display_| id that
> points
> > to the mirroring display. That feels like the better place for this
> information.
>

[pkotwicz, 2015-03-05 20:55:47]

Sadrul, can you please take another look?

There is only one "touch device to display" function in DeviceDataManager now as
per oshima@'s suggestion. I have kept
DeviceDataManager::GetTargetDisplayForTouchDevice() because I think that name is
more descriptive.

I have made DeviceDataManagerX11::TouchEventNeedsCalibrate() calibrate touches
for the internal touchscreen. This is consistent with Ozone's behavior

[sadrul, 2015-03-09 21:18:52]

LGTM (sorry about the delay)

[pkotwicz, 2015-03-10 17:34:21]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-03-10 17:34:22]

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

[commit-bot: I haz the power, 2015-03-10 17:34:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/922843002/140001

[commit-bot: I haz the power, 2015-03-10 18:31:32]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-03-10 18:32:08]

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