Calibrating touch input

Calibrating touch input


BUG=116890
TEST=None, manually tested


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135747

[Mr4D (OOO till 08-26), 2012-05-03 22:00:45]

This is the perquisite for the bezel detection. Need to properly stow the bezel
pixel dimensions somewhere else. Note: The gesture detection seems to be fine
with the out of bounds values.

[sadrul, 2012-05-04 14:02:55]

Looks good. A few nits:

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

http://codereview.chromium.org/10306014/diff/1/ui/aura/root_window.cc#newcode314
ui/aura/root_window.cc:314: // We only come here when the first contact was
within the root window.
I don't think this is necessary. But if we want to ensure our assumption that
'target != this' is always correct, then let's add a DCHECK_NE(this, target)
here instead.

http://codereview.chromium.org/10306014/diff/1/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

http://codereview.chromium.org/10306014/diff/1/ui/base/ui_base_switches.cc#ne...
ui/base/ui_base_switches.cc:32: const char kDisableTouchCalibration[]       =
"no-touch-calibration";
This should either be kNoTouchCalibration or disable-touch-calibration.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode684
ui/base/x/events_x.cc:684: gfx::Point CalibrateTouchCoordinates(
This should be inside the anonymous namespace above.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode697
ui/base/x/events_x.cc:697: gfx::Rect bounds =
gfx::Screen::GetPrimaryMonitor().bounds_in_pixel();
Perhaps using GetMonitorNearestPoint would be better here.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode698
ui/base/x/events_x.cc:698: int kLeftBorder = 50;
const these

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode720
ui/base/x/events_x.cc:720: // Offset the x position to the real
the y position

[Mr4D (OOO till 08-26), 2012-05-04 15:01:06]

Addressed! Please have another look!

In regards of the monitor selection - see my comment there. Not sure if the
suggestion is wise, but then again I am not a Linux expert.

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

http://codereview.chromium.org/10306014/diff/1/ui/aura/root_window.cc#newcode314
ui/aura/root_window.cc:314: // We only come here when the first contact was
within the root window.
Actually... It *is* needed. The "touch down" works well, but each consecutive
call will then enter this loop and crash a little bit further down because the
target seems to cause trouble. If I remember it right the ProcessTouchEvent was
the function crashing. If that part is needed for the processing I will have to
dig further down where exactly it crashes (but without debugger that is.. oh
well).

http://codereview.chromium.org/10306014/diff/1/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

http://codereview.chromium.org/10306014/diff/1/ui/base/ui_base_switches.cc#ne...
ui/base/ui_base_switches.cc:32: const char kDisableTouchCalibration[]       =
"no-touch-calibration";
On 2012/05/04 14:02:56, sadrul wrote:
> This should either be kNoTouchCalibration or disable-touch-calibration.

Done.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode684
ui/base/x/events_x.cc:684: gfx::Point CalibrateTouchCoordinates(
On 2012/05/04 14:02:56, sadrul wrote:
> This should be inside the anonymous namespace above.

Done.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode697
ui/base/x/events_x.cc:697: gfx::Rect bounds =
gfx::Screen::GetPrimaryMonitor().bounds_in_pixel();
First I thought this is a good idea, but then I thought:
a.) The coordinate is (unless I am wrong) given in touch device coordinates - so
not in "Global coordinates" - or?
b.) It only makes sense with the built in monitor - which is always the primary
- unless turned off in which case the touch screen wouldn't make sense anyways.
c.) It would only make sense in multi mon & multi touch screen scenarios. But in
that case this wouldn't work anyways (no calibration for the second touch screen
- which we can tackle if we ever get there - or have at least a Calibration DB
behind it).
Well - I leave it up to you. If you still want it I'll change!

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode698
ui/base/x/events_x.cc:698: int kLeftBorder = 50;
On 2012/05/04 14:02:56, sadrul wrote:
> const these

Done.

http://codereview.chromium.org/10306014/diff/1/ui/base/x/events_x.cc#newcode720
ui/base/x/events_x.cc:720: // Offset the x position to the real
On 2012/05/04 14:02:56, sadrul wrote:
> the y position

Done.

[Mr4D (OOO till 08-26), 2012-05-04 16:04:05]

Sky,

this should be only an 'owners' review for you.

Many thanks in advance!
-Stefan

[sky, 2012-05-04 16:31:41]

LGTM, but add a test for the RootWindow code.

http://codereview.chromium.org/10306014/diff/7001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10306014/diff/7001/ui/aura/root_window.cc#newc...
ui/aura/root_window.cc:313: } else if (target != this) {
Can you add a test case for this?

http://codereview.chromium.org/10306014/diff/7001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

http://codereview.chromium.org/10306014/diff/7001/ui/base/ui_base_switches.h#...
ui/base/ui_base_switches.h:22: UI_EXPORT extern const char
kDisableTouchCalibration[];
Keep sorted.

[Mr4D (OOO till 08-26), 2012-05-07 16:28:21]

Addressed and corrected the crash case.

http://codereview.chromium.org/10306014/diff/7001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://codereview.chromium.org/10306014/diff/7001/ui/aura/root_window.cc#newc...
ui/aura/root_window.cc:313: } else if (target != this) {
After discussion with Sadrul I changed this to the way it should be (there was a
"bug" in the current code).

http://codereview.chromium.org/10306014/diff/7001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

http://codereview.chromium.org/10306014/diff/7001/ui/base/ui_base_switches.h#...
ui/base/ui_base_switches.h:22: UI_EXPORT extern const char
kDisableTouchCalibration[];
On 2012/05/04 16:31:41, sky wrote:
> Keep sorted.

Done.

[sadrul, 2012-05-07 17:40:46]

LGTM (with two more nits)

http://codereview.chromium.org/10306014/diff/12002/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

http://codereview.chromium.org/10306014/diff/12002/ui/base/ui_base_switches.c...
ui/base/ui_base_switches.cc:32: const char kDisableTouchCalibration[]       =
"disable-touch-calibration";
You want to move this up to be the same order as in the header file.

http://codereview.chromium.org/10306014/diff/12002/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10306014/diff/12002/ui/base/x/events_x.cc#newc...
ui/base/x/events_x.cc:572: #endif
#endif  // defined(USE_XI2_MT)

[Mr4D (OOO till 08-26), 2012-05-07 18:30:40]

Addressed. Do one more test if the changes did break the build or not and submit
then.

http://codereview.chromium.org/10306014/diff/12002/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

http://codereview.chromium.org/10306014/diff/12002/ui/base/ui_base_switches.c...
ui/base/ui_base_switches.cc:32: const char kDisableTouchCalibration[]       =
"disable-touch-calibration";
On 2012/05/07 17:40:47, sadrul wrote:
> You want to move this up to be the same order as in the header file.

Done.

http://codereview.chromium.org/10306014/diff/12002/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/10306014/diff/12002/ui/base/x/events_x.cc#newc...
ui/base/x/events_x.cc:572: #endif
On 2012/05/07 17:40:47, sadrul wrote:
> #endif  // defined(USE_XI2_MT)

Done.

[commit-bot: I haz the power, 2012-05-07 20:55:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10306014/14001

[commit-bot: I haz the power, 2012-05-07 22:45:01]

Change committed as 135747