Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12087124:
Disable touch calibration on external touchscreen. (Closed)
|
Created:
7 years, 10 months ago by ynovikov Modified:
7 years, 9 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, ben+watch_chromium.org, tfarina, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, jochen+watch_chromium.org, Mr4D (OOO till 08-26) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionDisable touch calibration on external touchscreen.
Move CalibrateTouchCoordinates() code from events_x.cc into
TouchEventCalibrate::Calibrate() and call it from
RootWindowHostLinux::DispatchXI2Event() only on internal display.
This changes the order in handling of a touch event.
Used to be: 1. Calibrate, 2. Find matching window, 3. Relocate to window origin
Now: 1. Find matching window, 2. Relocate, 3. Calibrate with matching window
This change in order:
a) Makes finding matching window simpler and faster.
b) Makes calibration more correct, since matching window bounds are used.
To make the calibration even more correct,
calibration parameters have to be adjusted for non-native resolutions.
BUG=171310, 147605
TEST=Make sure touches are positioned correctly on internal and external
displays in single, extended desktop and mirror modes,
and when switching primary display.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189200
Patch Set 1 #Patch Set 2 : Fix compilation #Patch Set 3 : Fix Win compilation #Patch Set 4 : Fix Win compilation take 2 + rebase #
Total comments: 8
Patch Set 5 : Review + Rebase on top of 12321086 #
Total comments: 12
Patch Set 6 : Review #
Total comments: 2
Patch Set 7 : Review #
Total comments: 2
Patch Set 8 : Nits #Messages
Total messages: 22 (0 generated)
ben: Looks like you are the owner of most of this, either directly or indirectly, so you are my main reviewer. jochen: You might want to peek at content/shell. oshima: Please look at ash/display and ui/gfx. sadrul: Please look at ui/base/x, and you might also be interested in ui/base/events. skuhne: FYI
i defer to ben as the content shell change is purely mechanical
sadrul, please take a first glance.
ui/base/, i.e. the change to move calibration code from events_x.cc into event.cc LGTM. For the rest of the changes, how about: * Add the RootWindow::CreateParams::is_internal_display (as you have done in the CL). * Add RootWindowHost::SetIsInternalDisplaym, and call it from the RootWindow constructor. This seems cleaner than including is_internal_display in both RootWindow::CreateParams and RootWindowHost constructor list (e.g. what happens if one is set but the other isn't?) https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:443: class TouchCoordinatesCalibrator { Add doc https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:446: void Calibrate(TouchEvent& event, const gfx::Rect& bounds); You should either use a const ref, or a pointer. Also, add doc. https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:452: int left_border_touch_calibration; You can probably just call these left_, right_, top_ and bottom_
https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h File ui/aura/root_window.h (right): https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h#newc... ui/aura/root_window.h:67: bool is_internal_display); internal display <-> root window mapping is dynamic (it changes when you swap display), so this doesn't work. Let me take a look and get back to you about better approach.
On 2013/02/04 19:20:04, oshima wrote: > https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h > File ui/aura/root_window.h (right): > > https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h#newc... > ui/aura/root_window.h:67: bool is_internal_display); > internal display <-> root window mapping is dynamic > (it changes when you swap display), so this doesn't work. > > Let me take a look and get back to you about better approach. how about adding internal property to gfx::Display (see https://codereview.chromium.org/12194023 ), and check it when you receive ConfigureNotify?
https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h File ui/aura/root_window.h (right): https://codereview.chromium.org/12087124/diff/7005/ui/aura/root_window.h#newc... ui/aura/root_window.h:67: bool is_internal_display); On 2013/02/04 19:20:05, oshima wrote: > internal display <-> root window mapping is dynamic > (it changes when you swap display), so this doesn't work. > > Let me take a look and get back to you about better approach. Done. https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:443: class TouchCoordinatesCalibrator { On 2013/02/04 19:10:22, sadrul wrote: > Add doc Done. https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:446: void Calibrate(TouchEvent& event, const gfx::Rect& bounds); On 2013/02/04 19:10:22, sadrul wrote: > You should either use a const ref, or a pointer. > > Also, add doc. Done. https://codereview.chromium.org/12087124/diff/7005/ui/base/events/event.h#new... ui/base/events/event.h:452: int left_border_touch_calibration; On 2013/02/04 19:10:22, sadrul wrote: > You can probably just call these left_, right_, top_ and bottom_ Done.
SLGTM https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc File ui/base/events/event.cc (right): https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:441: DLOG(ERROR) << "Incorrect bottom border calibration value passed."; Should you check for valid (non-negative) values for left/right/top/bottom? https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:499: event->set_root_location(gfx::Point(x, y)); Use braces https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:502: #endif #endif // defined(USE_XI2_MT)
lgtm with nits https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc File ui/base/events/event.cc (right): https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:432: switches::kTouchCalibration), ',', &parts); please sync. This function is replaced by Tokenize https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h#ne... ui/base/events/event.h:454: public: indent https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h#ne... ui/base/events/event.h:463: private: indent
Ben, you are the only one left ... https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc File ui/base/events/event.cc (right): https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:432: switches::kTouchCalibration), ',', &parts); On 2013/03/14 01:23:27, oshima wrote: > please sync. This function is replaced by Tokenize Done. https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:441: DLOG(ERROR) << "Incorrect bottom border calibration value passed."; On 2013/03/14 00:20:13, sadrul wrote: > Should you check for valid (non-negative) values for left/right/top/bottom? Done. I don't think negative values are invalid. I can think of a hypothetical hardware where touchscreen will be smaller than screen, in which case negative values will shrink the coordinates appropriately. So, I've changed the comment that the values are positive, instead. https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:499: event->set_root_location(gfx::Point(x, y)); On 2013/03/14 00:20:13, sadrul wrote: > Use braces Done. https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.cc#n... ui/base/events/event.cc:502: #endif On 2013/03/14 00:20:13, sadrul wrote: > #endif // defined(USE_XI2_MT) Done. https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h#ne... ui/base/events/event.h:454: public: On 2013/03/14 01:23:27, oshima wrote: > indent Done. https://codereview.chromium.org/12087124/diff/31001/ui/base/events/event.h#ne... ui/base/events/event.h:463: private: On 2013/03/14 01:23:27, oshima wrote: > indent Done.
https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h#ne... ui/base/events/event.h:453: class TouchCoordinatesCalibrator { can this and the calibrate function below move to event_utils instead?
https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h File ui/base/events/event.h (right): https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h#ne... ui/base/events/event.h:453: class TouchCoordinatesCalibrator { On 2013/03/15 03:36:35, Ben Goodger (Google) wrote: > can this and the calibrate function below move to event_utils instead? Technically, they can, but I fail to see the advantage of that, while I can see many disadvantages: 1. event_utils deal with NativeEvent, while I need at least LocatedEvent, while the code is really specific only to TouchEvent. 2. Calibrate is currently a member of TouchEvent, which makes a lot of sense, since only TouchEvents need calibration. event_utils offer global functions, which is not as good an encapsulation as a class member. 3. This code has several dependencies, like LocatedEvent and Rect, which are not there in event_utils, but already are in event, so why add them in event_utils as well? 4. This code is ChromeOS specific (for example it uses a command line flag which will be there only if USE_XI2_MT is defined), while event_utils seem to be platform-independent.
OK let me rephrase a different way - I don't like seeing a bunch of platform specific stuff added to this file that is only tangentially related to event. What would you propose? -Ben On Fri, Mar 15, 2013 at 11:34 AM, <ynovikov@chromium.org> wrote: > > https://codereview.chromium.**org/12087124/diff/60001/ui/** > base/events/event.h<https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h> > File ui/base/events/event.h (right): > > https://codereview.chromium.**org/12087124/diff/60001/ui/** > base/events/event.h#newcode453<https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h#newcode453> > ui/base/events/event.h:453: class TouchCoordinatesCalibrator { > On 2013/03/15 03:36:35, Ben Goodger (Google) wrote: > >> can this and the calibrate function below move to event_utils instead? >> > > Technically, they can, but I fail to see the advantage of that, while I > can see many disadvantages: > 1. event_utils deal with NativeEvent, while I need at least > LocatedEvent, while the code is really specific only to TouchEvent. > 2. Calibrate is currently a member of TouchEvent, which makes a lot of > sense, since only TouchEvents need calibration. event_utils offer global > functions, which is not as good an encapsulation as a class member. > 3. This code has several dependencies, like LocatedEvent and Rect, which > are not there in event_utils, but already are in event, so why add them > in event_utils as well? > 4. This code is ChromeOS specific (for example it uses a command line > flag which will be there only if USE_XI2_MT is defined), while > event_utils seem to be platform-independent. > > https://codereview.chromium.**org/12087124/<https://codereview.chromium.org/1... >
On 2013/03/15 19:11:52, Ben Goodger (Google) wrote: > OK let me rephrase a different way - I don't like seeing a bunch of > platform specific stuff added to this file that is only tangentially > related to event. What would you propose? > > -Ben > > > On Fri, Mar 15, 2013 at 11:34 AM, <mailto:ynovikov@chromium.org> wrote: > > > > > https://codereview.chromium.**org/12087124/diff/60001/ui/** > > > base/events/event.h<https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h> > > File ui/base/events/event.h (right): > > > > https://codereview.chromium.**org/12087124/diff/60001/ui/** > > > base/events/event.h#newcode453<https://codereview.chromium.org/12087124/diff/60001/ui/base/events/event.h#newcode453> > > ui/base/events/event.h:453: class TouchCoordinatesCalibrator { > > On 2013/03/15 03:36:35, Ben Goodger (Google) wrote: > > > >> can this and the calibrate function below move to event_utils instead? > >> > > > > Technically, they can, but I fail to see the advantage of that, while I > > can see many disadvantages: > > 1. event_utils deal with NativeEvent, while I need at least > > LocatedEvent, while the code is really specific only to TouchEvent. > > 2. Calibrate is currently a member of TouchEvent, which makes a lot of > > sense, since only TouchEvents need calibration. event_utils offer global > > functions, which is not as good an encapsulation as a class member. > > 3. This code has several dependencies, like LocatedEvent and Rect, which > > are not there in event_utils, but already are in event, so why add them > > in event_utils as well? > > 4. This code is ChromeOS specific (for example it uses a command line > > flag which will be there only if USE_XI2_MT is defined), while > > event_utils seem to be platform-independent. > > > > > https://codereview.chromium.**org/12087124/%3Chttps://codereview.chromium.org...> > > Would RootWindowHostLinux be an appropriate place for this then? It seems to have platform specific stuff tangentially related to events already.
On Fri, Mar 15, 2013 at 4:17 PM, <ynovikov@chromium.org> wrote: > Would RootWindowHostLinux be an appropriate place for this then? It seems to > have platform specific stuff tangentially related to events already. > Please, when replying try to cut all the unnecessary stuff so it's easier to follow. You should have added your reply right after ">> related to event. What would you propose?" Thanks! -- Thiago
On 2013/03/15 20:20:17, tfarina wrote: > On Fri, Mar 15, 2013 at 4:17 PM, <mailto:ynovikov@chromium.org> wrote: > > Would RootWindowHostLinux be an appropriate place for this then? It seems to > > have platform specific stuff tangentially related to events already. > > > Please, when replying try to cut all the unnecessary stuff so it's > easier to follow. You should have added your reply right after ">> > related to event. What would you propose?" Sorry about that. So, the best I came up with is: 1. I leave CalibrateTouchCoordinates in events_x.cc, only I change the input to be Point instead of NativeEvent. 2. I expose this function in event_utils.h, and add empty implementations on other platforms. 3. I call CalibrateTouchCoordinates from RootWindowHostLinux and modify TouchEvent's location there as well. How does that sound? > > Thanks! > > -- > Thiago
> Would RootWindowHostLinux be an appropriate place for this then? It seems to > have platform specific stuff tangentially related to events already. If that's the only place that needs this then sure that sounds best.
On 2013/03/18 17:31:14, Ben Goodger (Google) wrote: > > Would RootWindowHostLinux be an appropriate place for this then? It seems to > > have platform specific stuff tangentially related to events already. > > If that's the only place that needs this then sure that sounds best. Done. Please review Patch Set 7.
lgtm https://codereview.chromium.org/12087124/diff/72001/ui/aura/root_window_host_... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12087124/diff/72001/ui/aura/root_window_host_... ui/aura/root_window_host_linux.cc:287: // The calibration values for the four border sides. What does calibration mean? What are these values relative to? What are the units?
Thanks for the review. https://codereview.chromium.org/12087124/diff/72001/ui/aura/root_window_host_... File ui/aura/root_window_host_linux.cc (right): https://codereview.chromium.org/12087124/diff/72001/ui/aura/root_window_host_... ui/aura/root_window_host_linux.cc:287: // The calibration values for the four border sides. On 2013/03/18 22:21:33, Ben Goodger (Google) wrote: > What does calibration mean? What are these values relative to? What are the > units? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ynovikov@chromium.org/12087124/87001
Message was sent while issue was closed.
Change committed as 189200 |
