Associate internal touchscreen to internal display

Associate internal touchscreen to internal display

Currently we associate touchscreen to display based on matching
resolution. But there could be cases the internal touchscreen
reports a resolution that does not match any display resolution,
or the internal display and external display have the same resolution
so we don't know which display to match the touchscreen to.

This patch adds a logic that if we have internal touchscreen and
internal display, we should match them first regardless of the
resolution.

We already have the information about which display is internal display.
For deciding internal touchscreen, we query X to get the touchscreen's
/dev/input/eventX  node, and call a script is_touchscreen_internal
(which checks if the device is a I2C device) to decide whether the
touchscreen is internal or not.

BUG=chrome-os-partner:29398
TEST=touch position is correctly mapped on Blaze in extending mode, with
     external monitor having 1920x1280 resolution.

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

[Yufeng Shen (Slow to review), 2014-07-16 02:41:18]

Danial, please help to take a look.

The is_touchscreen_internal script is from here
https://chromium-review.googlesource.com/#/c/208260/

[Daniel Erat, 2014-07-16 02:59:46]

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.cc:27: int internal_ts = -1;
s/ts/touchscreen/

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.cc:35:
DisplayConfigurator::DisplayState* internal_dpy_state = NULL;
s/dpy/display/, or just name this |internal_state|

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/touchscreen_device_manager_x11.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:22: bool ret = false;
i don't see any benefit from declaring this so early -- just return false in all
of the failure conditions below and declare this variable right before you
actually need it

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:33: int  act_format;
nit: remove double-space between int and act_format

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:35: unsigned char
*data;
* goes on left side of space

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:46:
base::FilePath("/opt/google/touchscreen/is_touchscreen_internal"));
why is this a separate script? putting the logic in chrome seems like it'll make
the code easier to read and be faster...

https://codereview.chromium.org/394063004/diff/20001/ui/display/types/chromeo...
File ui/display/types/chromeos/touchscreen_device.h (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/types/chromeo...
ui/display/types/chromeos/touchscreen_device.h:25: // If this is an internal
touchscreen.
nit: "True if this is an internal touchscreen." or "Is this an internal
touchscreen?"

[dnicoara, 2014-07-16 13:42:35]

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl_unittest.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:51: // Internal
display. Must not be matched to a touch screen unless the size
nit: Please update the comment.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:169:
device_manager_->AddDevice(TouchscreenDevice(1, gfx::Size(9999, 888), true));
Could you add another external touchscreen before the internal one? Just to test
that mapping the internal display takes precedence.

[Yufeng Shen (Slow to review), 2014-07-16 20:20:28]

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.cc:27: int internal_ts = -1;
On 2014/07/16 02:59:46, Daniel Erat wrote:
> s/ts/touchscreen/

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.cc:35:
DisplayConfigurator::DisplayState* internal_dpy_state = NULL;
On 2014/07/16 02:59:46, Daniel Erat wrote:
> s/dpy/display/, or just name this |internal_state|

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl_unittest.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:51: // Internal
display. Must not be matched to a touch screen unless the size
On 2014/07/16 13:42:35, dnicoara wrote:
> nit: Please update the comment.

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:169:
device_manager_->AddDevice(TouchscreenDevice(1, gfx::Size(9999, 888), true));
On 2014/07/16 13:42:35, dnicoara wrote:
> Could you add another external touchscreen before the internal one? Just to
test
> that mapping the internal display takes precedence.

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/touchscreen_device_manager_x11.cc (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:22: bool ret = false;
On 2014/07/16 02:59:46, Daniel Erat wrote:
> i don't see any benefit from declaring this so early -- just return false in
all
> of the failure conditions below and declare this variable right before you
> actually need it

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:33: int  act_format;
On 2014/07/16 02:59:46, Daniel Erat wrote:
> nit: remove double-space between int and act_format

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:35: unsigned char
*data;
On 2014/07/16 02:59:46, Daniel Erat wrote:
> * goes on left side of space

Done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:46:
base::FilePath("/opt/google/touchscreen/is_touchscreen_internal"));
On 2014/07/16 02:59:46, Daniel Erat wrote:
> why is this a separate script? putting the logic in chrome seems like it'll
make
> the code easier to read and be faster...

yeah, why not.
done.

https://codereview.chromium.org/394063004/diff/20001/ui/display/types/chromeo...
File ui/display/types/chromeos/touchscreen_device.h (right):

https://codereview.chromium.org/394063004/diff/20001/ui/display/types/chromeo...
ui/display/types/chromeos/touchscreen_device.h:25: // If this is an internal
touchscreen.
On 2014/07/16 02:59:46, Daniel Erat wrote:
> nit: "True if this is an internal touchscreen." or "Is this an internal
> touchscreen?"

Done.

[Daniel Erat, 2014-07-16 20:31:24]

lgtm with a few comments

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/touchscreen_device_manager_x11.cc (right):

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:25: // We consider the
touchscreen is internal if it is an I2c device.
nit: s/touchscreen is internal/touchscreen to be internal/

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:29: bool
IsTouchscreenInternal(Display* dpy, int device_id) {
nit: s/Display/XDisplay/ to match x11_types.h

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:42: Atom act_type;
avoid abbreviations: rename this to |actual_type| and |act_format| to
|actual_format|

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:53: return false;
do you need to call XCloseDevice() before returning here?

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:90: }
nit: }  // namespace

[Yufeng Shen (Slow to review), 2014-07-16 20:42:07]

The CQ bit was checked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2014-07-16 20:42:24]

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/touchscreen_device_manager_x11.cc (right):

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:25: // We consider the
touchscreen is internal if it is an I2c device.
On 2014/07/16 20:31:24, Daniel Erat wrote:
> nit: s/touchscreen is internal/touchscreen to be internal/

Done.

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:29: bool
IsTouchscreenInternal(Display* dpy, int device_id) {
On 2014/07/16 20:31:23, Daniel Erat wrote:
> nit: s/Display/XDisplay/ to match x11_types.h

Done.

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:42: Atom act_type;
On 2014/07/16 20:31:24, Daniel Erat wrote:
> avoid abbreviations: rename this to |actual_type| and |act_format| to
> |actual_format|

Done.

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:53: return false;
On 2014/07/16 20:31:23, Daniel Erat wrote:
> do you need to call XCloseDevice() before returning here?

Done.

https://codereview.chromium.org/394063004/diff/40001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/touchscreen_device_manager_x11.cc:90: }
On 2014/07/16 20:31:24, Daniel Erat wrote:
> nit: }  // namespace

Done.

[commit-bot: I haz the power, 2014-07-16 20:48:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miletus@chromium.org/394063004/60001

[commit-bot: I haz the power, 2014-07-17 04:26:22]

Change committed as 283641