Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
191223007:
Move touch CTM from X into Chrome (Closed)
|
Created:
6 years, 9 months ago by Yufeng Shen (Slow to review) Modified:
6 years, 7 months ago CC:
chromium-reviews, ben+aura_chromium.org, sadrul, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, ben+ash_chromium.org, dnicoara, Daniel Erat, Rick Byers Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMove touch CTM from X into Chrome
Currently we compute the touch CTM in OutputConfigurator
and push that into X. This CL makes computing the touch CTM
in DisplayController, and pushing it
into WindowTreeHostX11. This moves the functionality of
touch CTM from X into Chrome.
Basically, when there is output configuration change, we
compute the TouchCTM for each touch device, and push the
TouchCTM into the WindowTreeHostX11 that is associated
with the touchscreen. Then when X events reaching root
window, we use the CTM to map the events coordinate in
framebuffer space into the root window's coordinate space.
BUG=351019, chrome-os-partner:25788
TEST=tested on Pixel/Clapper with external touch/non-touch displays
on both extended/mirror mode. Touch events are correctly mapped to
chrome window or discarded if it is from blank region from letterboxing/pillarboxing mirror mode.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269371
Patch Set 1 #Patch Set 2 : aura & ash unittests pass #Patch Set 3 : add file ui/aura/touch_ctm.h(cc) #
Total comments: 6
Patch Set 4 : restructuring #
Total comments: 3
Patch Set 5 : move CTM update code into a separate file ash/touch/touch_ctm_controller.cc #
Total comments: 70
Patch Set 6 : rework #
Total comments: 5
Patch Set 7 : move the logic of if a touch event should be dispatched to a root window into CanDispatchEvent() #
Total comments: 26
Patch Set 8 : address comments #Patch Set 9 : move display_ids_ into AshWindowTreeHostX11 #
Total comments: 3
Patch Set 10 : prettify the mirror mode ctm formula #
Total comments: 3
Patch Set 11 : fix all the unittests #
Total comments: 18
Patch Set 12 : address Sadrul's comments #Patch Set 13 : rebase #Patch Set 14 : fix compile error #Patch Set 15 : rework events.gyp #Patch Set 16 : fix events/BUILD.gn #
Messages
Total messages: 55 (0 generated)
https://codereview.chromium.org/191223007/diff/40001/ui/aura/touch_ctm.h File ui/aura/touch_ctm.h (right): https://codereview.chromium.org/191223007/diff/40001/ui/aura/touch_ctm.h#newc... ui/aura/touch_ctm.h:12: class AURA_EXPORT TouchCTM { Document. https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:820: if (touch_ctm_map_.find(xiev->deviceid) == touch_ctm_map_.end()) It looks like the display-controller in ash is the only place that sets this? Does that mean we will never dispatch touch events in non-ash environments? If that is the case, then we need to change this. https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:857: touch_calibrate_->Calibrate(&touchev, bounds_); We should get rid of this code, and TouchEventCalibrate altogether. This new code should make it much easier to detect touch-events on bezels without us having to do all kinds of weird things in here. We should stuff the 'CTM' stuff (with a more descriptive name) into DeviceDataManager, and have ui::EventLocationFromNative() use that information to return the proper location. This code in here should essentially look something like this: ... [early return because of the unfortunate touch-release issue] ui::TouchEvent touchev(xev); SendEventToProcessor(&touchev);
https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... File ash/display/display_controller.cc (right): https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... ash/display/display_controller.cc:798: FOR_EACH_OBSERVER(Observer, observers_, OnDisplayConfigurationChanged()); Looks like new code don't have to be in DisplayController. Can you use DisplayController::Observer::OnDisplayConfigurationChanged?
https://codereview.chromium.org/191223007/diff/40001/ui/aura/touch_ctm.h File ui/aura/touch_ctm.h (right): https://codereview.chromium.org/191223007/diff/40001/ui/aura/touch_ctm.h#newc... ui/aura/touch_ctm.h:12: class AURA_EXPORT TouchCTM { On 2014/03/10 20:20:12, sadrul wrote: > Document. Done. https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:820: if (touch_ctm_map_.find(xiev->deviceid) == touch_ctm_map_.end()) On 2014/03/10 20:20:12, sadrul wrote: > It looks like the display-controller in ash is the only place that sets this? > Does that mean we will never dispatch touch events in non-ash environments? If > that is the case, then we need to change this. As discussed, in the new patch I am firstly using mapping from touch_event -> touch_device_id -> display_id -> root_window to see if the touch event is targeting this root window. If the mapping information is not available, it falls back to check if the event is within the bounds of the root window. https://codereview.chromium.org/191223007/diff/40001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:857: touch_calibrate_->Calibrate(&touchev, bounds_); On 2014/03/10 20:20:12, sadrul wrote: > We should get rid of this code, and TouchEventCalibrate altogether. This new > code should make it much easier to detect touch-events on bezels without us > having to do all kinds of weird things in here. We should stuff the 'CTM' stuff > (with a more descriptive name) into DeviceDataManager, and have > ui::EventLocationFromNative() use that information to return the proper > location. > > This code in here should essentially look something like this: > > ... [early return because of the unfortunate touch-release issue] > ui::TouchEvent touchev(xev); > SendEventToProcessor(&touchev); so the new patch is like: if (!IsTouchEventTargetingThisRootWindow(xev)) break; ui::TouchEvent touchev(xev); CalibrateTouchEvent(xev, &touchev); SendEventToProcessor(&touchev); As you suggested, I put the TouchCTM stuff into DeviceDataManager. And in EventLocationFromNative(), TouchCTM is applied. As for the TouchEventCalibrate, I still need it here for 1. It is a MessagePumpObserver, and it copies the root_x/y into event_x/y when it gets touch event. I found that event_x/y not always has the same value as root_x/y and it is the root_x/y value that we need. So it seems we still need this trick/hack. 2. For bezel calibration, the root window's bounds_ information is needed. I don't have a good way to access that information in EventLocationFromNative(). 3. The CL is getting too big now. Leaving the calibration code intact here adds more confident that the CL does not break existing behavior. If we want to get rid of the calibration code for cleanness, lets do it in a followup CL. https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... File ash/display/display_controller.cc (right): https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... ash/display/display_controller.cc:798: FOR_EACH_OBSERVER(Observer, observers_, OnDisplayConfigurationChanged()); On 2014/03/13 20:43:54, oshima wrote: > Looks like new code don't have to be in DisplayController. Can you use > DisplayController::Observer::OnDisplayConfigurationChanged? but is OnDisplayConfigurationChanged() is always called if say the system boots with 2 displays ? or is it only called when the config change after initial boot ?
https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... File ash/display/display_controller.cc (right): https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... ash/display/display_controller.cc:798: FOR_EACH_OBSERVER(Observer, observers_, OnDisplayConfigurationChanged()); On 2014/03/13 20:55:46, Yufeng Shen wrote: > On 2014/03/13 20:43:54, oshima wrote: > > Looks like new code don't have to be in DisplayController. Can you use > > DisplayController::Observer::OnDisplayConfigurationChanged? > > but is OnDisplayConfigurationChanged() is always called if say the system boots > with 2 displays ? It's always called when display configuration changes. > or is it only called when the config change after initial boot > ? For startup time, you can just call your function in Shell::Init, or we can add explicit Observer::OnDisplaysInitialized.
On 2014/03/13 21:34:48, oshima wrote: > https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... > File ash/display/display_controller.cc (right): > > https://codereview.chromium.org/191223007/diff/60001/ash/display/display_cont... > ash/display/display_controller.cc:798: FOR_EACH_OBSERVER(Observer, observers_, > OnDisplayConfigurationChanged()); > On 2014/03/13 20:55:46, Yufeng Shen wrote: > > On 2014/03/13 20:43:54, oshima wrote: > > > Looks like new code don't have to be in DisplayController. Can you use > > > DisplayController::Observer::OnDisplayConfigurationChanged? Done. Move the TouchCTM update code into a separate TouchCTMController which is a DisplayController::Observer. > > > > but is OnDisplayConfigurationChanged() is always called if say the system > boots > > with 2 displays ? > > It's always called when display configuration changes. > > > or is it only called when the config change after initial boot > > ? > > For startup time, you can just call your function in Shell::Init, or we can add > explicit Observer::OnDisplaysInitialized.
+ben@ for his opinion about change in WTH. https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... File ash/display/display_controller.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... ash/display/display_controller.h:95: } can you use GetAllRootWindows instead? https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info.h File ash/display/display_info.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info... ash/display/display_info.h:175: int touch_device_id_; This needs documentation. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:13: #if defined(OS_CHROMEOS) && defined(USE_X11) Since this make sense only on chromeos + x11, it's better to exclude these files on other platforms in gyp and ifdef in Shell. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:22: // Overrides DisplayController::Observer: just // DisplayController::Obserer: is new style https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:26: nit: remove extra new line https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; Instead of adding these information and code in the WTH, how about adding aura::client::TouchCalibrateClient ? https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:757: // is targeting this RootWindow. then you can do something like if (GetTouchCalibrateClient(window())->CalibrateTouchEvent(xev, bounds_, &touchev)) SendEventToProcessor(&touchev); ?
What happens when the touch-support in a display isn't turned on when the display is available, but it's turned on after some time? (e.g. you connect an external monitor, but don't plugin its usb for touch at first) https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... File ash/display/display_change_observer_chromeos.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... ash/display/display_change_observer_chromeos.cc:173: displays.back().set_touch_device_id(output.touch_device_id); Can this block be like: DisplayInfo display(id, name, has_overscan); display.set_device_scale_factor(...); display.SetBounds(....); ... displays.push_back(display); ? https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:32: // and the sceond monitor is translated to Point (0, 828) in the second https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:175: std::map<int64, aura::Window*>::const_iterator it = root_windows.begin(); In this case, is root_windows.size() == 1? https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:187: } This block of code should be in DeviceDataManager, e.g. DeviceDataManager::UpdateTouchInfoForDisplay(const gfx::Display& display, const gfx::Transform& x) { if (display.touch_device_id() == 0) return; .... } https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:14: public: indent one space https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:152: void Calibrate(ui::TouchEvent* event, const gfx::Rect& bounds) { Why do we still need this? Shouldn't this be part of the CTM? We shouldn't need to calibrate twice. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:877: const base::NativeEvent& event) { Just use XEvent* here. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:885: // fall backt to check if this touch event is within the bounds of *back https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:891: // If we do have record of the display id for this touch device, I don't think this comment is necessary here. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:899: return true; non-CHROMEOS case should do the bounds check. This function could look like: #if chromeos touch_display_id = ... if (touch_display_id != invalid) { return (touch_display_id == display_ids().first || touch_display_id == display_ids().second)); } #endif // check bounds https://codereview.chromium.org/191223007/diff/80001/ui/events/touch_ctm.h File ui/events/touch_ctm.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/touch_ctm.h#ne... ui/events/touch_ctm.h:21: class EVENTS_BASE_EXPORT TouchCTM { Do we need this? Could we use a gfx::Transform instead? https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.cc:621: else Don't need the else here. https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... File ui/events/x/device_data_manager.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.h:219: void ClearTouchCTM(); Use something more descriptive than CTM https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.h:224: void ClearTouchDeviceToDisplayMap(); We should have one 'Clear<...>' instead of two separate 'Clear<...>'s https://codereview.chromium.org/191223007/diff/80001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/events_x.cc#... ui/events/x/events_x.cc:441: switch (type) { Let's do this for xievent->evtype == XI_Touch* events only.
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:34: // X will first map input event location to [0, 2560) x [0, 2428), Is this really a half-open interval? AFAIK aura events can be anywhere within the bounding rectangle of the window, including on the border. So a closed interval. Rob was telling me X is similar, and (0,0) is actually on the bounding rectangle. I think all of this math may need to be updated.
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:34: // X will first map input event location to [0, 2560) x [0, 2428), On 2014/03/24 18:52:31, spang wrote: > Is this really a half-open interval? No, but I think the numbers make more sense this way. > > AFAIK aura events can be anywhere within the bounding rectangle of the window, > including on the border. So a closed interval. > > Rob was telling me X is similar, and (0,0) is actually on the bounding > rectangle. > > I think all of this math may need to be updated. Would you prefer the comment to say [0, 2559] x [0, 2427]?
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:34: // X will first map input event location to [0, 2560) x [0, 2428), On 2014/03/24 19:02:41, ynovikov wrote: > On 2014/03/24 18:52:31, spang wrote: > > Is this really a half-open interval? > No, but I think the numbers make more sense this way. > > > > AFAIK aura events can be anywhere within the bounding rectangle of the window, > > including on the border. So a closed interval. > > > > Rob was telling me X is similar, and (0,0) is actually on the bounding > > rectangle. > > > > I think all of this math may need to be updated. > Would you prefer the comment to say [0, 2559] x [0, 2427]? No it's [0, 2560] x [0, 2428]. That's the point.
On Mon, Mar 24, 2014 at 3:05 PM, <spang@chromium.org> wrote: > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc > File ash/touch/touch_ctm_controller.cc (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode34 > ash/touch/touch_ctm_controller.cc:34: // X will first map input event > location to [0, 2560) x [0, 2428), > On 2014/03/24 19:02:41, ynovikov wrote: > >> On 2014/03/24 18:52:31, spang wrote: >> > Is this really a half-open interval? >> No, but I think the numbers make more sense this way. >> > >> > AFAIK aura events can be anywhere within the bounding rectangle of >> > the window, > >> > including on the border. So a closed interval. >> > >> > Rob was telling me X is similar, and (0,0) is actually on the >> > bounding > >> > rectangle. >> > >> > I think all of this math may need to be updated. >> Would you prefer the comment to say [0, 2559] x [0, 2427]? >> > > No it's [0, 2560] x [0, 2428]. That's the point. > But is X sending out events that can have max_width or max_height ? We are carrying a X patch locally http://lists.x.org/archives/xorg-devel/2012-September/033600.html and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + height - 1]. > > https://codereview.chromium.org/191223007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/24 19:07:26, miletus1 wrote: > On Mon, Mar 24, 2014 at 3:05 PM, <mailto:spang@chromium.org> wrote: > > > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > > touch/touch_ctm_controller.cc > > File ash/touch/touch_ctm_controller.cc (right): > > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > > touch/touch_ctm_controller.cc#newcode34 > > ash/touch/touch_ctm_controller.cc:34: // X will first map input event > > location to [0, 2560) x [0, 2428), > > On 2014/03/24 19:02:41, ynovikov wrote: > > > >> On 2014/03/24 18:52:31, spang wrote: > >> > Is this really a half-open interval? > >> No, but I think the numbers make more sense this way. > >> > > >> > AFAIK aura events can be anywhere within the bounding rectangle of > >> > > the window, > > > >> > including on the border. So a closed interval. > >> > > >> > Rob was telling me X is similar, and (0,0) is actually on the > >> > > bounding > > > >> > rectangle. > >> > > >> > I think all of this math may need to be updated. > >> Would you prefer the comment to say [0, 2559] x [0, 2427]? > >> > > > > No it's [0, 2560] x [0, 2428]. That's the point. > > > > But is X sending out events that can have max_width or max_height ? The touchscreen driver does send events that can have max_width and max_height. But it doesn't matter, as they are converted to [0, width - 1] x [0, height - 1] in scale_to_desktop() (which BTW has this code also in upstream, our patch is local because our X is old). To add to the confusion, I've seen touchscreens with both 1920 x 1080 and 1919 x 1079 axis sizes. > > We are carrying a X patch locally > http://lists.x.org/archives/xorg-devel/2012-September/033600.html > > and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + > height - 1]. > > > > > > https://codereview.chromium.org/191223007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/03/24 19:07:26, miletus1 wrote: > On Mon, Mar 24, 2014 at 3:05 PM, <mailto:spang@chromium.org> wrote: > > > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > > touch/touch_ctm_controller.cc > > File ash/touch/touch_ctm_controller.cc (right): > > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > > touch/touch_ctm_controller.cc#newcode34 > > ash/touch/touch_ctm_controller.cc:34: // X will first map input event > > location to [0, 2560) x [0, 2428), > > On 2014/03/24 19:02:41, ynovikov wrote: > > > >> On 2014/03/24 18:52:31, spang wrote: > >> > Is this really a half-open interval? > >> No, but I think the numbers make more sense this way. > >> > > >> > AFAIK aura events can be anywhere within the bounding rectangle of > >> > > the window, > > > >> > including on the border. So a closed interval. > >> > > >> > Rob was telling me X is similar, and (0,0) is actually on the > >> > > bounding > > > >> > rectangle. > >> > > >> > I think all of this math may need to be updated. > >> Would you prefer the comment to say [0, 2559] x [0, 2427]? > >> > > > > No it's [0, 2560] x [0, 2428]. That's the point. > > > > But is X sending out events that can have max_width or max_height ? > > We are carrying a X patch locally > http://lists.x.org/archives/xorg-devel/2012-September/033600.html > Why isn't the patch upstream? > and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + > height - 1]. I think the coordinate systems may not match, then. Or X11 is buggy for dropping the event. Is the location the distance from the middle of the first pixel or the origin of the first pixel? I suppose it's not super critical, because it's only a difference of 1/2 a pixel. But I am not sure why the coordinate space of the window for events would not include the outermost 1/2 pixel on all sides. Someone should clarify what chrome expects here inside aura for event targeting. > > > > > > https://codereview.chromium.org/191223007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; And if this code is only used from ash, some code that lives there? i.e. no aura client interface at all.
https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; On 2014/03/24 19:36:55, Ben Goodger (Google) wrote: > And if this code is only used from ash, some code that lives there? i.e. no aura > client interface at all. Sorry I probably wasn't clear. This is used from aura in this CL (see WindowTreeHostX11::IsTouchEventTargetingThisRootWindow), and my suggestion is to move entire calibration code (TouchEventCalibrate etc) out from aura because it's very chromeos specific.
I wondered a while ago at what point the WTHX11 code became ChromeOS-specific enough to be better off living in Ash. I don't think it ends up being used on Desktop-Aura on Linux (which provides its own impl in Views). The pro for keeping this where it is is that it at least forces you to have something of an API between it and the client code in Ash. I guess my assertion might be that you could still have that API division within Ash and that might be fine. -Ben On Mon, Mar 24, 2014 at 1:00 PM, <oshima@chromium.org> wrote: > > https://codereview.chromium.org/191223007/diff/80001/ui/ > aura/window_tree_host.h > File ui/aura/window_tree_host.h (right): > > https://codereview.chromium.org/191223007/diff/80001/ui/ > aura/window_tree_host.h#newcode235 > ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; > On 2014/03/24 19:36:55, Ben Goodger (Google) wrote: > >> And if this code is only used from ash, some code that lives there? >> > i.e. no aura > >> client interface at all. >> > > Sorry I probably wasn't clear. This is used from aura in this CL (see > WindowTreeHostX11::IsTouchEventTargetingThisRootWindow), and > my suggestion is to move entire calibration code (TouchEventCalibrate > etc) > out from aura because it's very chromeos specific. > > https://codereview.chromium.org/191223007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/24 20:05:44, Ben Goodger (Google) wrote: > I wondered a while ago at what point the WTHX11 code became > ChromeOS-specific enough to be better off living in Ash. I don't think it > ends up being used on Desktop-Aura on Linux (which provides its own impl in > Views). > > The pro for keeping this where it is is that it at least forces you to have > something of an API between it and the client code in Ash. I guess my > assertion might be that you could still have that API division within Ash > and that might be fine. > > -Ben > > > On Mon, Mar 24, 2014 at 1:00 PM, <mailto:oshima@chromium.org> wrote: > > > > > https://codereview.chromium.org/191223007/diff/80001/ui/ > > aura/window_tree_host.h > > File ui/aura/window_tree_host.h (right): > > > > https://codereview.chromium.org/191223007/diff/80001/ui/ > > aura/window_tree_host.h#newcode235 > > ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; > > On 2014/03/24 19:36:55, Ben Goodger (Google) wrote: > > > >> And if this code is only used from ash, some code that lives there? > >> > > i.e. no aura > > > >> client interface at all. > >> > > > > Sorry I probably wasn't clear. This is used from aura in this CL (see > > WindowTreeHostX11::IsTouchEventTargetingThisRootWindow), and > > my suggestion is to move entire calibration code (TouchEventCalibrate > > etc) > > out from aura because it's very chromeos specific. > > > > https://codereview.chromium.org/191223007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I thought WTHX11 was still used for unit tests. I might be wrong though; I haven't poked at this in a very long time.
Yeah I think you need a minimal WTH in aura still for its tests. And maybe ash isn't the right place for cros-specific stuff either since Ash is consumed by Metro too. Perhaps src/chromeos. Food for thought. -Ben On Mon, Mar 24, 2014 at 1:16 PM, <erg@chromium.org> wrote: > On 2014/03/24 20:05:44, Ben Goodger (Google) wrote: > >> I wondered a while ago at what point the WTHX11 code became >> ChromeOS-specific enough to be better off living in Ash. I don't think it >> ends up being used on Desktop-Aura on Linux (which provides its own impl >> in >> Views). >> > > The pro for keeping this where it is is that it at least forces you to >> have >> something of an API between it and the client code in Ash. I guess my >> assertion might be that you could still have that API division within Ash >> and that might be fine. >> > > -Ben >> > > > On Mon, Mar 24, 2014 at 1:00 PM, <mailto:oshima@chromium.org> wrote: >> > > > >> > https://codereview.chromium.org/191223007/diff/80001/ui/ >> > aura/window_tree_host.h >> > File ui/aura/window_tree_host.h (right): >> > >> > https://codereview.chromium.org/191223007/diff/80001/ui/ >> >> > aura/window_tree_host.h#newcode235 >> > ui/aura/window_tree_host.h:235: std::pair<int64, int64> display_ids_; >> > On 2014/03/24 19:36:55, Ben Goodger (Google) wrote: >> > >> >> And if this code is only used from ash, some code that lives there? >> >> >> > i.e. no aura >> > >> >> client interface at all. >> >> >> > >> > Sorry I probably wasn't clear. This is used from aura in this CL (see >> > WindowTreeHostX11::IsTouchEventTargetingThisRootWindow), and >> > my suggestion is to move entire calibration code (TouchEventCalibrate >> > etc) >> > out from aura because it's very chromeos specific. >> > >> > https://codereview.chromium.org/191223007/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I thought WTHX11 was still used for unit tests. I might be wrong though; I > haven't poked at this in a very long time. > > https://codereview.chromium.org/191223007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Mar 24, 2014 at 3:24 PM, <spang@chromium.org> wrote: > On 2014/03/24 19:07:26, miletus1 wrote: > >> On Mon, Mar 24, 2014 at 3:05 PM, <mailto:spang@chromium.org> wrote: >> > > > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ >> > touch/touch_ctm_controller.cc >> > File ash/touch/touch_ctm_controller.cc (right): >> > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ >> > touch/touch_ctm_controller.cc#newcode34 >> > ash/touch/touch_ctm_controller.cc:34: // X will first map input event >> > location to [0, 2560) x [0, 2428), >> > On 2014/03/24 19:02:41, ynovikov wrote: >> > >> >> On 2014/03/24 18:52:31, spang wrote: >> >> > Is this really a half-open interval? >> >> No, but I think the numbers make more sense this way. >> >> > >> >> > AFAIK aura events can be anywhere within the bounding rectangle of >> >> >> > the window, >> > >> >> > including on the border. So a closed interval. >> >> > >> >> > Rob was telling me X is similar, and (0,0) is actually on the >> >> >> > bounding >> > >> >> > rectangle. >> >> > >> >> > I think all of this math may need to be updated. >> >> Would you prefer the comment to say [0, 2559] x [0, 2427]? >> >> >> > >> > No it's [0, 2560] x [0, 2428]. That's the point. >> > >> > > But is X sending out events that can have max_width or max_height ? >> > > We are carrying a X patch locally >> http://lists.x.org/archives/xorg-devel/2012-September/033600.html >> > > > Why isn't the patch upstream? > > It is upstreamed just not in our version of X server. > > and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + >> height - 1]. >> > > I think the coordinate systems may not match, then. Or X11 is buggy for > dropping > the event. > > Yes, X11 is dropping the events with max_height or max_width. But I think we can live with that the code is written to align with X11's output range, until ozone takes over. > Is the location the distance from the middle of the first pixel or the > origin of > the first pixel? > > I suppose it's not super critical, because it's only a difference of 1/2 a > pixel. But I am not sure why the coordinate space of the window for events > would > not include the outermost 1/2 pixel on all sides. > > Someone should clarify what chrome expects here inside aura for event > targeting. > > > > > > >> > https://codereview.chromium.org/191223007/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/191223007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/24 20:49:57, miletus1 wrote: > On Mon, Mar 24, 2014 at 3:24 PM, <mailto:spang@chromium.org> wrote: > > > On 2014/03/24 19:07:26, miletus1 wrote: > > > >> On Mon, Mar 24, 2014 at 3:05 PM, <mailto:spang@chromium.org> wrote: > >> > > > > > > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ > >> > touch/touch_ctm_controller.cc > >> > File ash/touch/touch_ctm_controller.cc (right): > >> > > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ > >> > touch/touch_ctm_controller.cc#newcode34 > >> > ash/touch/touch_ctm_controller.cc:34: // X will first map input event > >> > location to [0, 2560) x [0, 2428), > >> > On 2014/03/24 19:02:41, ynovikov wrote: > >> > > >> >> On 2014/03/24 18:52:31, spang wrote: > >> >> > Is this really a half-open interval? > >> >> No, but I think the numbers make more sense this way. > >> >> > > >> >> > AFAIK aura events can be anywhere within the bounding rectangle of > >> >> > >> > the window, > >> > > >> >> > including on the border. So a closed interval. > >> >> > > >> >> > Rob was telling me X is similar, and (0,0) is actually on the > >> >> > >> > bounding > >> > > >> >> > rectangle. > >> >> > > >> >> > I think all of this math may need to be updated. > >> >> Would you prefer the comment to say [0, 2559] x [0, 2427]? > >> >> > >> > > >> > No it's [0, 2560] x [0, 2428]. That's the point. > >> > > >> > > > > But is X sending out events that can have max_width or max_height ? > >> > > > > We are carrying a X patch locally > >> http://lists.x.org/archives/xorg-devel/2012-September/033600.html > >> > > > > > > Why isn't the patch upstream? > > > > > It is upstreamed just not in our version of X server. > > > > > > and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + > >> height - 1]. > >> > > > > I think the coordinate systems may not match, then. Or X11 is buggy for > > dropping > > the event. > > > > > Yes, X11 is dropping the events with max_height or max_width. But I think > we can live > with that the code is written to align with X11's output range, until ozone > takes over. Yes obviously we have work with X11 behavior here. The axis scaling is apparently quite subtle as evidence here: http://lists.x.org/archives/xorg-devel/2013-May/036113.html "It's a bug, but we'll have to live with it because with all this scaling, we just cannot win." > > > > > Is the location the distance from the middle of the first pixel or the > > origin of > > the first pixel? > > > > I suppose it's not super critical, because it's only a difference of 1/2 a > > pixel. But I am not sure why the coordinate space of the window for events > > would > > not include the outermost 1/2 pixel on all sides. > > > > Someone should clarify what chrome expects here inside aura for event > > targeting. > > > > > > > > > > > > >> > https://codereview.chromium.org/191223007/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > https://codereview.chromium.org/191223007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/03/24 20:49:57, miletus1 wrote: > On Mon, Mar 24, 2014 at 3:24 PM, <mailto:spang@chromium.org> wrote: > > > On 2014/03/24 19:07:26, miletus1 wrote: > > > >> On Mon, Mar 24, 2014 at 3:05 PM, <mailto:spang@chromium.org> wrote: > >> > > > > > > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ > >> > touch/touch_ctm_controller.cc > >> > File ash/touch/touch_ctm_controller.cc (right): > >> > > >> > https://codereview.chromium.org/191223007/diff/80001/ash/ > >> > touch/touch_ctm_controller.cc#newcode34 > >> > ash/touch/touch_ctm_controller.cc:34: // X will first map input event > >> > location to [0, 2560) x [0, 2428), > >> > On 2014/03/24 19:02:41, ynovikov wrote: > >> > > >> >> On 2014/03/24 18:52:31, spang wrote: > >> >> > Is this really a half-open interval? > >> >> No, but I think the numbers make more sense this way. > >> >> > > >> >> > AFAIK aura events can be anywhere within the bounding rectangle of > >> >> > >> > the window, > >> > > >> >> > including on the border. So a closed interval. > >> >> > > >> >> > Rob was telling me X is similar, and (0,0) is actually on the > >> >> > >> > bounding > >> > > >> >> > rectangle. > >> >> > > >> >> > I think all of this math may need to be updated. > >> >> Would you prefer the comment to say [0, 2559] x [0, 2427]? > >> >> > >> > > >> > No it's [0, 2560] x [0, 2428]. That's the point. > >> > > >> > > > > But is X sending out events that can have max_width or max_height ? > >> > > > > We are carrying a X patch locally > >> http://lists.x.org/archives/xorg-devel/2012-September/033600.html > >> > > > > > > Why isn't the patch upstream? > > > > > It is upstreamed just not in our version of X server. > The upstream patch looks like it was reverted; the current rescaling does not subtract 1 [1]. I wonder if this change is compatible with the current behavior of X11? [1] http://cgit.freedesktop.org/xorg/xserver/tree/dix/getevents.c#n293 > > > > > and it maps events to only [min_x, min_x + width - 1] and [min_y, min_y + > >> height - 1]. > >> > > > > I think the coordinate systems may not match, then. Or X11 is buggy for > > dropping > > the event. > > > > > Yes, X11 is dropping the events with max_height or max_width. But I think > we can live > with that the code is written to align with X11's output range, until ozone > takes over. > > > > > Is the location the distance from the middle of the first pixel or the > > origin of > > the first pixel? > > > > I suppose it's not super critical, because it's only a difference of 1/2 a > > pixel. But I am not sure why the coordinate space of the window for events > > would > > not include the outermost 1/2 pixel on all sides. > > > > Someone should clarify what chrome expects here inside aura for event > > targeting. > > > > > > > > > > > > >> > https://codereview.chromium.org/191223007/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > https://codereview.chromium.org/191223007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:34: // X will first map input event location to [0, 2560) x [0, 2428), On 2014/03/24 19:05:31, spang wrote: > On 2014/03/24 19:02:41, ynovikov wrote: > > On 2014/03/24 18:52:31, spang wrote: > > > Is this really a half-open interval? > > No, but I think the numbers make more sense this way. > > > > > > AFAIK aura events can be anywhere within the bounding rectangle of the > window, > > > including on the border. So a closed interval. > > > > > > Rob was telling me X is similar, and (0,0) is actually on the bounding > > > rectangle. > > > > > > I think all of this math may need to be updated. > > Would you prefer the comment to say [0, 2559] x [0, 2427]? > > No it's [0, 2560] x [0, 2428]. That's the point. sadrul has fixed my understanding of the coordinate system. It is indeed half-open [0, width) x [0, height). The points on the left & top border are inside the screen, while the points on the bottom & right borders are outside the screen. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / (framebuffer_width - 1); I think this should be: width / framebuffer_width Right? We scale from the old width to new; subtracting 1 introduces a small amount of error here. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:63: ctm.y_scale = (height -1) / (framebuffer_height - 1); Same here.
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:46: // y_offset = 0 This used to be: // y_offset = 828 / (2428 -1) The original code mapped touches for the second monitor from (0,0)-(2559,2427) rectangle into (0,828)-(2559,2427) rectangle. Now, combined with a change in WindowTreeHostX11, you actually map the second monitor into (0,0)-(2560,1599) rectangle instead. So, the second monitor computation is not different from the first one. You should simplify the comment, saying that it scales the events from X display size to the root window / monitor size, and you don't need to distinguish between monitor 1 and 2. The original code also used to offset the event to the position of the root window inside X's display, thus the handling was different, but since you match the touch to window based on touch device id and not coordinate, we can simplify this now. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:55: // float origin_y = touch_display.bounds_in_native().origin().y(); This comment is misleading, please remove. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:60: second_display.bounds_in_native().height(); I don't think this computation is appropriate here. It made sense when this code was in OutputConfigurator, since it was the one that configured X's display in this fashion. It would be better to pass X's display size to this function instead of second_display, otherwise changes in OutputConfigurator might have unexpected effect here. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / (framebuffer_width - 1); On 2014/03/26 17:30:34, spang wrote: > I think this should be: > > width / framebuffer_width > > Right? We scale from the old width to new; subtracting 1 introduces a small > amount of error here. No, as the comment states for Monitor 2, in case where width==framebuffer_width x_scale should be 1. Think of the transformation matrix like this: We map from [0,framebuffer_width-1] range into [0,1] range by dividing by (framebuffer_width-1) factor, and then we map from [0,1] range into [0,width-1] range by multiplying by (width-1) factor. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:63: ctm.y_scale = (height -1) / (framebuffer_height - 1); On 2014/03/26 17:30:34, spang wrote: > Same here. Nit, add space in (height -1) https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:79: // and just use the default TouchCTM. Why? Double checking that aspect ratio hasn't changed is trivial. In fact, the code already does that. But, something that you should check is whether the monitor "is_aspect_preserving_scaling" - if it stretches to full screen, no need for CTM. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:234: // If the display is a touch display, we add a deafult TouchCTM to the default https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (left): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:786: touchev.Relocate(bounds_.origin()); I think this is the only use of Relocate(), could you remove the function? https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:152: void Calibrate(ui::TouchEvent* event, const gfx::Rect& bounds) { On 2014/03/15 19:32:51, sadrul wrote: > Why do we still need this? Shouldn't this be part of the CTM? We shouldn't need > to calibrate twice. I agree. Originally the second calibration was needed, since setting X's CTM to account for bezels would have resulted in negative coordinates, which would have been clipped. Something that we need to think about, though, is how to determine whether the touch is in bezels or not. The problem is that bezel size in pixels is different in different resolutions. And when we set up mirror mode with pillarboxing, we want to ignore the pillar area but respond to touches in bezel area (and both will have negative coordinates). One solution is to apply CTM to "native resolution - bezels" rectangle, i.e., assuming bezel sizes 20 on all sides, the (20,20)-(2539,1679) rectangle. If we are in 1024x768 resolution, the picture would be in (164,20)-(2395,1679) touchscreen coordinates, so the transformation would be x = (x - 164) / (2395 - 164) * 1023 y = (y - 20) / (1679 - 20) * 767 So, "native resolution - bezels" would be translated to (-66,0)-(1089,767) and everything outside that rectangle can be treated as bezels, for example top left corner, which would be (-75,-9). Note: In case we are mirroring to a touchscreen, we should use bezels for internal touchscreen CTM, but not use them for external. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:889: !bounds_.Contains(ui::EventLocationFromNative(xev))) As I understand it, this check will be wrong most of the time. If we don't have display <-> touchscreen mapping, we also don't have CTM. So, EventLocationFromNative will return touches in X's display range. Meaning, if we have 2 monitors and one touchscreen, touches at the top will go to first monitor, and touches at the bottom will go to the second monitor. The only case this would work properly, is if we have 1 monitor which is also 1 touchscreen. Perhaps add a check here whether we are in this situation? And discard the event if we have more than 1 monitor? Also, please document this behavior. It would probably also be useful to add UMA stat or something to track how often do we end up in a situation where we don't know where touchscreen event should go to, with perhaps some information (keeping privacy in mind) which will help us fix it.
https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / (framebuffer_width - 1); On 2014/03/27 23:35:53, ynovikov wrote: > On 2014/03/26 17:30:34, spang wrote: > > I think this should be: > > > > width / framebuffer_width > > > > Right? We scale from the old width to new; subtracting 1 introduces a small > > amount of error here. > No, as the comment states for Monitor 2, in case where width==framebuffer_width > x_scale should be 1. Agree completely. However note x/x == (x-1)/(x-1) == 1 > Think of the transformation matrix like this: We map from > [0,framebuffer_width-1] range into [0,1] range by dividing by > (framebuffer_width-1) factor, and then we map from [0,1] range into [0,width-1] > range by multiplying by (width-1) factor. It is wrong to try to do geometry with these integer bounds because it doesn't make sense. Let's work in floats. We're trying to map from the [0, framebuffer_width) range to the [0, width) range. Consider a very small example. Say we're scaling a 4x4 rectangle onto a 2x2 rectangle. Then framebuffer_width = 4 width = 2 Then the current code scale factor is (2 - 1) / (4 - 1) = 1/3. Should we really scale by 1/3 onto a rectangle that is only 1/2 the size? I don't think so; this is flawed geometry. Don't worry about the integer upper bound. The integer upper bound is the left side of the rightmost pixel, and does not correspond to the border of the screen. For scaling, the width must be used. In particular, we absolutely do not need to maintain the invariant 0 <= x <= width -1 The invariant is actually: 0 <= x < width If we truncate the float coordinate into the integers, we will certainly get a number in the range [0, width-1], but that's only because we've lost a lot of precision.
Thanks for all your comments. Finally get time to resume the work. PTAL. https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... File ash/display/display_change_observer_chromeos.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... ash/display/display_change_observer_chromeos.cc:173: displays.back().set_touch_device_id(output.touch_device_id); On 2014/03/15 19:32:51, sadrul wrote: > Can this block be like: > > DisplayInfo display(id, name, has_overscan); > display.set_device_scale_factor(...); > display.SetBounds(....); > ... > displays.push_back(display); > > ? seems that it is already reworked this way. https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... File ash/display/display_controller.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... ash/display/display_controller.h:95: } On 2014/03/14 21:53:53, oshima wrote: > can you use GetAllRootWindows instead? removed. https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info.h File ash/display/display_info.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info... ash/display/display_info.h:175: int touch_device_id_; On 2014/03/14 21:53:53, oshima wrote: > This needs documentation. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.cc (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:13: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/03/14 21:53:53, oshima wrote: > Since this make sense only on chromeos + x11, it's better to exclude these files > on other platforms in gyp and ifdef in Shell. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:32: // and the sceond monitor is translated to Point (0, 828) in the On 2014/03/15 19:32:51, sadrul wrote: > second comments removed. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:46: // y_offset = 0 On 2014/03/27 23:35:53, ynovikov wrote: > This used to be: > // y_offset = 828 / (2428 -1) > > The original code mapped touches for the second monitor from (0,0)-(2559,2427) > rectangle into (0,828)-(2559,2427) rectangle. > Now, combined with a change in WindowTreeHostX11, you actually map the second > monitor into (0,0)-(2560,1599) rectangle instead. > So, the second monitor computation is not different from the first one. > > You should simplify the comment, saying that it scales the events from X display > size to the root window / monitor size, and you don't need to distinguish > between monitor 1 and 2. > The original code also used to offset the event to the position of the root > window inside X's display, thus the handling was different, but since you match > the touch to window based on touch device id and not coordinate, we can simplify > this now. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:55: // float origin_y = touch_display.bounds_in_native().origin().y(); On 2014/03/27 23:35:53, ynovikov wrote: > This comment is misleading, please remove. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:60: second_display.bounds_in_native().height(); On 2014/03/27 23:35:53, ynovikov wrote: > I don't think this computation is appropriate here. > It made sense when this code was in OutputConfigurator, since it was the one > that configured X's display in this fashion. > It would be better to pass X's display size to this function instead of > second_display, otherwise changes in OutputConfigurator might have unexpected > effect here. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / (framebuffer_width - 1); On 2014/03/28 16:49:31, spang wrote: > On 2014/03/27 23:35:53, ynovikov wrote: > > On 2014/03/26 17:30:34, spang wrote: > > > I think this should be: > > > > > > width / framebuffer_width > > > > > > Right? We scale from the old width to new; subtracting 1 introduces a small > > > amount of error here. > > No, as the comment states for Monitor 2, in case where > width==framebuffer_width > > x_scale should be 1. > > Agree completely. However note x/x == (x-1)/(x-1) == 1 > > > Think of the transformation matrix like this: We map from > > [0,framebuffer_width-1] range into [0,1] range by dividing by > > (framebuffer_width-1) factor, and then we map from [0,1] range into > [0,width-1] > > range by multiplying by (width-1) factor. > > It is wrong to try to do geometry with these integer bounds because it doesn't > make sense. Let's work in floats. We're trying to map from the [0, > framebuffer_width) range to the [0, width) range. > > Consider a very small example. Say we're scaling a 4x4 rectangle onto a 2x2 > rectangle. Then > > framebuffer_width = 4 > width = 2 > > Then the current code scale factor is (2 - 1) / (4 - 1) = 1/3. Should we really > scale by 1/3 onto a rectangle that is only 1/2 the size? I don't think so; this > is flawed geometry. > > Don't worry about the integer upper bound. The integer upper bound is the left > side of the rightmost pixel, and does not correspond to the border of the > screen. For scaling, the width must be used. > > In particular, we absolutely do not need to maintain the invariant > > 0 <= x <= width -1 > > The invariant is actually: > > 0 <= x < width > > If we truncate the float coordinate into the integers, we will certainly get a > number in the range [0, width-1], but that's only because we've lost a lot of > precision. OK, changed to use width / framebuffer_width. The concern I had was I wanted to maintain the invariant 0 <= integer_x <= width -1 I mean, it is definitely right we want to maintain 0 <= float_x < width, but eventually when we convert the xevent's position from float into ui::Event's location which is in integer, we don't want the integer_x gets accidentally rounded to |width|. We are doing truncating in EventLocationFromNative() in events_x.cc using static_cast<int>(xievent->event_x), so we are ok there. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:79: // and just use the default TouchCTM. On 2014/03/27 23:35:53, ynovikov wrote: > Why? Double checking that aspect ratio hasn't changed is trivial. > In fact, the code already does that. > > But, something that you should check is whether the monitor > "is_aspect_preserving_scaling" - if it stretches to full screen, no need for > CTM. Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:175: std::map<int64, aura::Window*>::const_iterator it = root_windows.begin(); On 2014/03/15 19:32:51, sadrul wrote: > In this case, is root_windows.size() == 1? reworked, ptal. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:187: } On 2014/03/15 19:32:51, sadrul wrote: > This block of code should be in DeviceDataManager, e.g. > > DeviceDataManager::UpdateTouchInfoForDisplay(const gfx::Display& display, > const gfx::Transform& x) { > if (display.touch_device_id() == 0) > return; > .... > } done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.cc:234: // If the display is a touch display, we add a deafult TouchCTM to the On 2014/03/27 23:35:53, ynovikov wrote: > default Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... File ash/touch/touch_ctm_controller.h (right): https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:14: public: On 2014/03/15 19:32:51, sadrul wrote: > indent one space Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:22: // Overrides DisplayController::Observer: On 2014/03/14 21:53:53, oshima wrote: > just > > // DisplayController::Obserer: > > is new style Done. https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... ash/touch/touch_ctm_controller.h:26: On 2014/03/14 21:53:53, oshima wrote: > nit: remove extra new line Done. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (left): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:786: touchev.Relocate(bounds_.origin()); On 2014/03/27 23:35:53, ynovikov wrote: > I think this is the only use of Relocate(), could you remove the function? Done. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:152: void Calibrate(ui::TouchEvent* event, const gfx::Rect& bounds) { On 2014/03/27 23:35:53, ynovikov wrote: > On 2014/03/15 19:32:51, sadrul wrote: > > Why do we still need this? Shouldn't this be part of the CTM? We shouldn't > need > > to calibrate twice. > > I agree. Originally the second calibration was needed, since setting X's CTM to > account for bezels would have resulted in negative coordinates, which would have > been clipped. > > Something that we need to think about, though, is how to determine whether the > touch is in bezels or not. The problem is that bezel size in pixels is different > in different resolutions. And when we set up mirror mode with pillarboxing, we > want to ignore the pillar area but respond to touches in bezel area (and both > will have negative coordinates). > One solution is to apply CTM to "native resolution - bezels" rectangle, i.e., > assuming bezel sizes 20 on all sides, the (20,20)-(2539,1679) rectangle. > If we are in 1024x768 resolution, the picture would be in (164,20)-(2395,1679) > touchscreen coordinates, so the transformation would be > x = (x - 164) / (2395 - 164) * 1023 > y = (y - 20) / (1679 - 20) * 767 > So, "native resolution - bezels" would be translated to (-66,0)-(1089,767) and > everything outside that rectangle can be treated as bezels, for example top left > corner, which would be (-75,-9). > > Note: In case we are mirroring to a touchscreen, we should use bezels for > internal touchscreen CTM, but not use them for external. Lets do this in a separate CL if necessary. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:757: // is targeting this RootWindow. On 2014/03/14 21:53:53, oshima wrote: > then you can do something like > > if (GetTouchCalibrateClient(window())->CalibrateTouchEvent(xev, bounds_, > &touchev)) > SendEventToProcessor(&touchev); > > ? > I moved TouchCalibrate into DeviceDataManager. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:877: const base::NativeEvent& event) { On 2014/03/15 19:32:51, sadrul wrote: > Just use XEvent* here. Done. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:885: // fall backt to check if this touch event is within the bounds of On 2014/03/15 19:32:51, sadrul wrote: > *back Done. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:889: !bounds_.Contains(ui::EventLocationFromNative(xev))) On 2014/03/27 23:35:53, ynovikov wrote: > As I understand it, this check will be wrong most of the time. > If we don't have display <-> touchscreen mapping, we also don't have CTM. > So, EventLocationFromNative will return touches in X's display range. > Meaning, if we have 2 monitors and one touchscreen, touches at the top will go > to first monitor, and touches at the bottom will go to the second monitor. > > The only case this would work properly, is if we have 1 monitor which is also 1 > touchscreen. > Perhaps add a check here whether we are in this situation? And discard the event > if we have more than 1 monitor? > I am more interested in not regress the 1 monitor case so that's why the behavior of if no record of display <-> touchscreen map, I want to keep the old logic. As for the 2 monitor case, if the mapping is wrong then we will get complaints and we go fix it. > Also, please document this behavior. > > It would probably also be useful to add UMA stat or something to track how often > do we end up in a situation where we don't know where touchscreen event should > go to, with perhaps some information (keeping privacy in mind) which will help > us fix it. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:891: // If we do have record of the display id for this touch device, On 2014/03/15 19:32:51, sadrul wrote: > I don't think this comment is necessary here. Done. https://codereview.chromium.org/191223007/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_x11.cc:899: return true; On 2014/03/15 19:32:51, sadrul wrote: > non-CHROMEOS case should do the bounds check. > > This function could look like: > > #if chromeos > touch_display_id = ... > if (touch_display_id != invalid) { > return (touch_display_id == display_ids().first || > touch_display_id == display_ids().second)); > } > #endif > // check bounds The original code does not do the bounds check in non-chromeos case. I would like to keep the original logic the same in this CL. And add the bounds check for non-chromeos case in a separate CL if we have a concrete bug. https://codereview.chromium.org/191223007/diff/80001/ui/events/touch_ctm.h File ui/events/touch_ctm.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/touch_ctm.h#ne... ui/events/touch_ctm.h:21: class EVENTS_BASE_EXPORT TouchCTM { On 2014/03/15 19:32:51, sadrul wrote: > Do we need this? Could we use a gfx::Transform instead? Looking at Transform::TransformPointInternal() there is some rounding happening which worries me. I don't want the event that's close to the boundary gets unexpectedly rounded up and gets out of the boundary. While if we just use the plain float operation when applying TouchCTM, we can rely on the static_cast<int> in EventLocationFromeNative() to truncate the event position so it won't get out of bound. https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.cc:621: else On 2014/03/15 19:32:51, sadrul wrote: > Don't need the else here. Done. https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... File ui/events/x/device_data_manager.h (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.h:219: void ClearTouchCTM(); On 2014/03/15 19:32:51, sadrul wrote: > Use something more descriptive than CTM renamed to TouchTransformer. https://codereview.chromium.org/191223007/diff/80001/ui/events/x/device_data_... ui/events/x/device_data_manager.h:224: void ClearTouchDeviceToDisplayMap(); On 2014/03/15 19:32:51, sadrul wrote: > We should have one 'Clear<...>' instead of two separate 'Clear<...>'s Done. https://codereview.chromium.org/191223007/diff/80001/ui/events/x/events_x.cc File ui/events/x/events_x.cc (right): https://codereview.chromium.org/191223007/diff/80001/ui/events/x/events_x.cc#... ui/events/x/events_x.cc:441: switch (type) { On 2014/03/15 19:32:51, sadrul wrote: > Let's do this for xievent->evtype == XI_Touch* events only. Done.
On 2014/04/29 20:34:17, Yufeng Shen wrote: > Thanks for all your comments. > > Finally get time to resume the work. PTAL. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... > File ash/display/display_change_observer_chromeos.cc (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... > ash/display/display_change_observer_chromeos.cc:173: > displays.back().set_touch_device_id(output.touch_device_id); > On 2014/03/15 19:32:51, sadrul wrote: > > Can this block be like: > > > > DisplayInfo display(id, name, has_overscan); > > display.set_device_scale_factor(...); > > display.SetBounds(....); > > ... > > displays.push_back(display); > > > > ? > > seems that it is already reworked this way. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... > File ash/display/display_controller.h (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... > ash/display/display_controller.h:95: } > On 2014/03/14 21:53:53, oshima wrote: > > can you use GetAllRootWindows instead? > > removed. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info.h > File ash/display/display_info.h (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info... > ash/display/display_info.h:175: int touch_device_id_; > On 2014/03/14 21:53:53, oshima wrote: > > This needs documentation. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > File ash/touch/touch_ctm_controller.cc (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:13: #if defined(OS_CHROMEOS) && > defined(USE_X11) > On 2014/03/14 21:53:53, oshima wrote: > > Since this make sense only on chromeos + x11, it's better to exclude these > files > > on other platforms in gyp and ifdef in Shell. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:32: // and the sceond monitor is translated to > Point (0, 828) in the > On 2014/03/15 19:32:51, sadrul wrote: > > second > > comments removed. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:46: // y_offset = 0 > On 2014/03/27 23:35:53, ynovikov wrote: > > This used to be: > > // y_offset = 828 / (2428 -1) > > > > The original code mapped touches for the second monitor from (0,0)-(2559,2427) > > rectangle into (0,828)-(2559,2427) rectangle. > > Now, combined with a change in WindowTreeHostX11, you actually map the second > > monitor into (0,0)-(2560,1599) rectangle instead. > > So, the second monitor computation is not different from the first one. > > > > You should simplify the comment, saying that it scales the events from X > display > > size to the root window / monitor size, and you don't need to distinguish > > between monitor 1 and 2. > > The original code also used to offset the event to the position of the root > > window inside X's display, thus the handling was different, but since you > match > > the touch to window based on touch device id and not coordinate, we can > simplify > > this now. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:55: // float origin_y = > touch_display.bounds_in_native().origin().y(); > On 2014/03/27 23:35:53, ynovikov wrote: > > This comment is misleading, please remove. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:60: > second_display.bounds_in_native().height(); > On 2014/03/27 23:35:53, ynovikov wrote: > > I don't think this computation is appropriate here. > > It made sense when this code was in OutputConfigurator, since it was the one > > that configured X's display in this fashion. > > It would be better to pass X's display size to this function instead of > > second_display, otherwise changes in OutputConfigurator might have unexpected > > effect here. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / > (framebuffer_width - 1); > On 2014/03/28 16:49:31, spang wrote: > > On 2014/03/27 23:35:53, ynovikov wrote: > > > On 2014/03/26 17:30:34, spang wrote: > > > > I think this should be: > > > > > > > > width / framebuffer_width > > > > > > > > Right? We scale from the old width to new; subtracting 1 introduces a > small > > > > amount of error here. > > > No, as the comment states for Monitor 2, in case where > > width==framebuffer_width > > > x_scale should be 1. > > > > Agree completely. However note x/x == (x-1)/(x-1) == 1 > > > > > Think of the transformation matrix like this: We map from > > > [0,framebuffer_width-1] range into [0,1] range by dividing by > > > (framebuffer_width-1) factor, and then we map from [0,1] range into > > [0,width-1] > > > range by multiplying by (width-1) factor. > > > > It is wrong to try to do geometry with these integer bounds because it doesn't > > make sense. Let's work in floats. We're trying to map from the [0, > > framebuffer_width) range to the [0, width) range. > > > > Consider a very small example. Say we're scaling a 4x4 rectangle onto a 2x2 > > rectangle. Then > > > > framebuffer_width = 4 > > width = 2 > > > > Then the current code scale factor is (2 - 1) / (4 - 1) = 1/3. Should we > really > > scale by 1/3 onto a rectangle that is only 1/2 the size? I don't think so; > this > > is flawed geometry. > > > > Don't worry about the integer upper bound. The integer upper bound is the left > > side of the rightmost pixel, and does not correspond to the border of the > > screen. For scaling, the width must be used. > > > > In particular, we absolutely do not need to maintain the invariant > > > > 0 <= x <= width -1 > > > > The invariant is actually: > > > > 0 <= x < width > > > > If we truncate the float coordinate into the integers, we will certainly get a > > number in the range [0, width-1], but that's only because we've lost a lot of > > precision. > > OK, changed to use width / framebuffer_width. > > The concern I had was I wanted to maintain the invariant > 0 <= integer_x <= width -1 > > I mean, it is definitely right we want to maintain > 0 <= float_x < width, > > but eventually when we convert the xevent's position from float > into ui::Event's location which is in integer, we don't want the > integer_x gets accidentally rounded to |width|. ui::TouchEvent/ui::LocatedEvent does store float x/y internally. I wonder why ui::EventLocationFromNative doesn't also use floats. On LocatedEvent, there's .location() and .location_f(). The .location() method floors the floats. > > We are doing truncating in EventLocationFromNative() in events_x.cc using > static_cast<int>(xievent->event_x), so we are ok there. >
On 2014/04/29 20:34:17, Yufeng Shen wrote: > Thanks for all your comments. > > Finally get time to resume the work. PTAL. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... > File ash/display/display_change_observer_chromeos.cc (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_chan... > ash/display/display_change_observer_chromeos.cc:173: > displays.back().set_touch_device_id(output.touch_device_id); > On 2014/03/15 19:32:51, sadrul wrote: > > Can this block be like: > > > > DisplayInfo display(id, name, has_overscan); > > display.set_device_scale_factor(...); > > display.SetBounds(....); > > ... > > displays.push_back(display); > > > > ? > > seems that it is already reworked this way. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... > File ash/display/display_controller.h (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_cont... > ash/display/display_controller.h:95: } > On 2014/03/14 21:53:53, oshima wrote: > > can you use GetAllRootWindows instead? > > removed. > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info.h > File ash/display/display_info.h (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/display/display_info... > ash/display/display_info.h:175: int touch_device_id_; > On 2014/03/14 21:53:53, oshima wrote: > > This needs documentation. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > File ash/touch/touch_ctm_controller.cc (right): > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:13: #if defined(OS_CHROMEOS) && > defined(USE_X11) > On 2014/03/14 21:53:53, oshima wrote: > > Since this make sense only on chromeos + x11, it's better to exclude these > files > > on other platforms in gyp and ifdef in Shell. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:32: // and the sceond monitor is translated to > Point (0, 828) in the > On 2014/03/15 19:32:51, sadrul wrote: > > second > > comments removed. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:46: // y_offset = 0 > On 2014/03/27 23:35:53, ynovikov wrote: > > This used to be: > > // y_offset = 828 / (2428 -1) > > > > The original code mapped touches for the second monitor from (0,0)-(2559,2427) > > rectangle into (0,828)-(2559,2427) rectangle. > > Now, combined with a change in WindowTreeHostX11, you actually map the second > > monitor into (0,0)-(2560,1599) rectangle instead. > > So, the second monitor computation is not different from the first one. > > > > You should simplify the comment, saying that it scales the events from X > display > > size to the root window / monitor size, and you don't need to distinguish > > between monitor 1 and 2. > > The original code also used to offset the event to the position of the root > > window inside X's display, thus the handling was different, but since you > match > > the touch to window based on touch device id and not coordinate, we can > simplify > > this now. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:55: // float origin_y = > touch_display.bounds_in_native().origin().y(); > On 2014/03/27 23:35:53, ynovikov wrote: > > This comment is misleading, please remove. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:60: > second_display.bounds_in_native().height(); > On 2014/03/27 23:35:53, ynovikov wrote: > > I don't think this computation is appropriate here. > > It made sense when this code was in OutputConfigurator, since it was the one > > that configured X's display in this fashion. > > It would be better to pass X's display size to this function instead of > > second_display, otherwise changes in OutputConfigurator might have unexpected > > effect here. > > Done. > > https://codereview.chromium.org/191223007/diff/80001/ash/touch/touch_ctm_cont... > ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / > (framebuffer_width - 1); > On 2014/03/28 16:49:31, spang wrote: > > On 2014/03/27 23:35:53, ynovikov wrote: > > > On 2014/03/26 17:30:34, spang wrote: > > > > I think this should be: > > > > > > > > width / framebuffer_width > > > > > > > > Right? We scale from the old width to new; subtracting 1 introduces a > small > > > > amount of error here. > > > No, as the comment states for Monitor 2, in case where > > width==framebuffer_width > > > x_scale should be 1. > > > > Agree completely. However note x/x == (x-1)/(x-1) == 1 > > > > > Think of the transformation matrix like this: We map from > > > [0,framebuffer_width-1] range into [0,1] range by dividing by > > > (framebuffer_width-1) factor, and then we map from [0,1] range into > > [0,width-1] > > > range by multiplying by (width-1) factor. > > > > It is wrong to try to do geometry with these integer bounds because it doesn't > > make sense. Let's work in floats. We're trying to map from the [0, > > framebuffer_width) range to the [0, width) range. > > > > Consider a very small example. Say we're scaling a 4x4 rectangle onto a 2x2 > > rectangle. Then > > > > framebuffer_width = 4 > > width = 2 > > > > Then the current code scale factor is (2 - 1) / (4 - 1) = 1/3. Should we > really > > scale by 1/3 onto a rectangle that is only 1/2 the size? I don't think so; > this > > is flawed geometry. > > > > Don't worry about the integer upper bound. The integer upper bound is the left > > side of the rightmost pixel, and does not correspond to the border of the > > screen. For scaling, the width must be used. > > > > In particular, we absolutely do not need to maintain the invariant > > > > 0 <= x <= width -1 > > > > The invariant is actually: > > > > 0 <= x < width > > > > If we truncate the float coordinate into the integers, we will certainly get a > > number in the range [0, width-1], but that's only because we've lost a lot of > > precision. > > OK, changed to use width / framebuffer_width. > > The concern I had was I wanted to maintain the invariant > 0 <= integer_x <= width -1 > > I mean, it is definitely right we want to maintain > 0 <= float_x < width, > > but eventually when we convert the xevent's position from float > into ui::Event's location which is in integer, we don't want the > integer_x gets accidentally rounded to |width|. ui::TouchEvent/ui::LocatedEvent does store float x/y internally. I wonder why ui::EventLocationFromNative doesn't also use floats. On LocatedEvent, there's .location() and .location_f(). The .location() method floors the floats. > > We are doing truncating in EventLocationFromNative() in events_x.cc using > static_cast<int>(xievent->event_x), so we are ok there. >
On Tue, Apr 29, 2014 at 4:53 PM, <spang@chromium.org> wrote: > On 2014/04/29 20:34:17, Yufeng Shen wrote: > >> Thanks for all your comments. >> > > Finally get time to resume the work. PTAL. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_change_observer_chromeos.cc > >> File ash/display/display_change_observer_chromeos.cc (right): >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_change_observer_chromeos.cc#newcode173 > >> ash/display/display_change_observer_chromeos.cc:173: >> displays.back().set_touch_device_id(output.touch_device_id); >> On 2014/03/15 19:32:51, sadrul wrote: >> > Can this block be like: >> > >> > DisplayInfo display(id, name, has_overscan); >> > display.set_device_scale_factor(...); >> > display.SetBounds(....); >> > ... >> > displays.push_back(display); >> > >> > ? >> > > seems that it is already reworked this way. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_controller.h > >> File ash/display/display_controller.h (right): >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_controller.h#newcode95 > >> ash/display/display_controller.h:95: } >> On 2014/03/14 21:53:53, oshima wrote: >> > can you use GetAllRootWindows instead? >> > > removed. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_info.h > >> File ash/display/display_info.h (right): >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > display/display_info.h#newcode175 > >> ash/display/display_info.h:175: int touch_device_id_; >> On 2014/03/14 21:53:53, oshima wrote: >> > This needs documentation. >> > > Done. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc > >> File ash/touch/touch_ctm_controller.cc (right): >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode13 > >> ash/touch/touch_ctm_controller.cc:13: #if defined(OS_CHROMEOS) && >> defined(USE_X11) >> On 2014/03/14 21:53:53, oshima wrote: >> > Since this make sense only on chromeos + x11, it's better to exclude >> these >> files >> > on other platforms in gyp and ifdef in Shell. >> > > Done. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode32 > >> ash/touch/touch_ctm_controller.cc:32: // and the sceond monitor is >> translated >> > to > >> Point (0, 828) in the >> On 2014/03/15 19:32:51, sadrul wrote: >> > second >> > > comments removed. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode46 > >> ash/touch/touch_ctm_controller.cc:46: // y_offset = 0 >> On 2014/03/27 23:35:53, ynovikov wrote: >> > This used to be: >> > // y_offset = 828 / (2428 -1) >> > >> > The original code mapped touches for the second monitor from >> > (0,0)-(2559,2427) > >> > rectangle into (0,828)-(2559,2427) rectangle. >> > Now, combined with a change in WindowTreeHostX11, you actually map the >> > second > >> > monitor into (0,0)-(2560,1599) rectangle instead. >> > So, the second monitor computation is not different from the first one. >> > >> > You should simplify the comment, saying that it scales the events from X >> display >> > size to the root window / monitor size, and you don't need to >> distinguish >> > between monitor 1 and 2. >> > The original code also used to offset the event to the position of the >> root >> > window inside X's display, thus the handling was different, but since >> you >> match >> > the touch to window based on touch device id and not coordinate, we can >> simplify >> > this now. >> > > Done. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode55 > >> ash/touch/touch_ctm_controller.cc:55: // float origin_y = >> touch_display.bounds_in_native().origin().y(); >> On 2014/03/27 23:35:53, ynovikov wrote: >> > This comment is misleading, please remove. >> > > Done. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode60 > >> ash/touch/touch_ctm_controller.cc:60: >> second_display.bounds_in_native().height(); >> On 2014/03/27 23:35:53, ynovikov wrote: >> > I don't think this computation is appropriate here. >> > It made sense when this code was in OutputConfigurator, since it was >> the one >> > that configured X's display in this fashion. >> > It would be better to pass X's display size to this function instead of >> > second_display, otherwise changes in OutputConfigurator might have >> > unexpected > >> > effect here. >> > > Done. >> > > > https://codereview.chromium.org/191223007/diff/80001/ash/ > touch/touch_ctm_controller.cc#newcode61 > >> ash/touch/touch_ctm_controller.cc:61: ctm.x_scale = (width - 1) / >> (framebuffer_width - 1); >> On 2014/03/28 16:49:31, spang wrote: >> > On 2014/03/27 23:35:53, ynovikov wrote: >> > > On 2014/03/26 17:30:34, spang wrote: >> > > > I think this should be: >> > > > >> > > > width / framebuffer_width >> > > > >> > > > Right? We scale from the old width to new; subtracting 1 introduces >> a >> small >> > > > amount of error here. >> > > No, as the comment states for Monitor 2, in case where >> > width==framebuffer_width >> > > x_scale should be 1. >> > >> > Agree completely. However note x/x == (x-1)/(x-1) == 1 >> > >> > > Think of the transformation matrix like this: We map from >> > > [0,framebuffer_width-1] range into [0,1] range by dividing by >> > > (framebuffer_width-1) factor, and then we map from [0,1] range into >> > [0,width-1] >> > > range by multiplying by (width-1) factor. >> > >> > It is wrong to try to do geometry with these integer bounds because it >> > doesn't > >> > make sense. Let's work in floats. We're trying to map from the [0, >> > framebuffer_width) range to the [0, width) range. >> > >> > Consider a very small example. Say we're scaling a 4x4 rectangle onto a >> 2x2 >> > rectangle. Then >> > >> > framebuffer_width = 4 >> > width = 2 >> > >> > Then the current code scale factor is (2 - 1) / (4 - 1) = 1/3. Should we >> really >> > scale by 1/3 onto a rectangle that is only 1/2 the size? I don't think >> so; >> this >> > is flawed geometry. >> > >> > Don't worry about the integer upper bound. The integer upper bound is >> the >> > left > >> > side of the rightmost pixel, and does not correspond to the border of >> the >> > screen. For scaling, the width must be used. >> > >> > In particular, we absolutely do not need to maintain the invariant >> > >> > 0 <= x <= width -1 >> > >> > The invariant is actually: >> > >> > 0 <= x < width >> > >> > If we truncate the float coordinate into the integers, we will >> certainly get >> > a > >> > number in the range [0, width-1], but that's only because we've lost a >> lot >> > of > >> > precision. >> > > OK, changed to use width / framebuffer_width. >> > > The concern I had was I wanted to maintain the invariant >> 0 <= integer_x <= width -1 >> > > I mean, it is definitely right we want to maintain >> 0 <= float_x < width, >> > > but eventually when we convert the xevent's position from float >> into ui::Event's location which is in integer, we don't want the >> integer_x gets accidentally rounded to |width|. >> > > ui::TouchEvent/ui::LocatedEvent does store float x/y internally. I wonder > why > ui::EventLocationFromNative doesn't also use floats. > Yes, the float version was added by Tim recently for crbug.com/337824. anyway, for my CL, I am changing the logic of determining if a touch event belongs to a root window from using bounds check to matching display_id so this potential off-by-one error is no longer a concern. On LocatedEvent, there's .location() and .location_f(). The .location() > method > floors the floats. > > > > We are doing truncating in EventLocationFromNative() in events_x.cc using >> static_cast<int>(xievent->event_x), so we are ok there. >> > > > https://codereview.chromium.org/191223007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:612: } Can you move chromeos specific code to ash/host/ash_window_tree_host_x11 ?
https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:612: } On 2014/04/30 16:19:05, oshima wrote: > Can you move chromeos specific code to ash/host/ash_window_tree_host_x11 ? The thing is IsTouchEventTargetingThisRootWindow(xev) has to be called before ui::TouchEvent touchev(xev) in DispatchXI2Event() Maybe I should make IsTouchEventTargetingThisRootWindow virtual and move this into the AshWindowTreeHostX11's implementation of IsTouchEventTargetingThisRootWindow ?
On 2014/04/30 16:42:42, Yufeng Shen wrote: > https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... > File ui/aura/window_tree_host_x11.cc (right): > > https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... > ui/aura/window_tree_host_x11.cc:612: } > On 2014/04/30 16:19:05, oshima wrote: > > Can you move chromeos specific code to ash/host/ash_window_tree_host_x11 ? > > The thing is IsTouchEventTargetingThisRootWindow(xev) has to be called before > ui::TouchEvent touchev(xev) in DispatchXI2Event() > > Maybe I should make IsTouchEventTargetingThisRootWindow virtual > and move this into the AshWindowTreeHostX11's implementation of > IsTouchEventTargetingThisRootWindow ? Yes, that'd be better.
https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:191: return target == xwindow_ || target == x_root_window_; You should update this function to decide whether or not the touch-event should be dispatched to this window-tree. (or the Ash implementation should override this to make that decision)
https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:191: return target == xwindow_ || target == x_root_window_; On 2014/04/30 16:58:58, sadrul wrote: > You should update this function to decide whether or not the touch-event should > be dispatched to this window-tree. (or the Ash implementation should override > this to make that decision) Perfect. Added the logic in AshWindowTreeHostX11::CanDispatchEvent() https://codereview.chromium.org/191223007/diff/100001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:612: } On 2014/04/30 16:42:42, Yufeng Shen wrote: > On 2014/04/30 16:19:05, oshima wrote: > > Can you move chromeos specific code to ash/host/ash_window_tree_host_x11 ? > > The thing is IsTouchEventTargetingThisRootWindow(xev) has to be called before > ui::TouchEvent touchev(xev) in DispatchXI2Event() > > Maybe I should make IsTouchEventTargetingThisRootWindow virtual > and move this into the AshWindowTreeHostX11's implementation of > IsTouchEventTargetingThisRootWindow ? as Sadrul suggested, moved this into AshWindowTreeHostX11::CanDispatchEvent()
https://codereview.chromium.org/191223007/diff/120001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/shell.h#newcode374 ash/shell.h:374: #if defined(OS_CHROMEOS) && defined(USE_X11) can this be just USE_X11? https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:146: Shell::GetInstance()->display_configurator()->display_state(); what value does this return for software mirror mode? https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:154: return; I believe this should not happen. Did you hit this? maybe DCHECK? https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:160: return; same here https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nuke (c) same for other files https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:37: const DisplayInfo& touch_display); const? (same for other methods) https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_hos... ui/aura/window_tree_host.h:215: std::pair<int64, int64> display_ids_; this can move to ash_window_tree_host_x11 now? https://codereview.chromium.org/191223007/diff/120001/ui/events/touch_transfo... File ui/events/touch_transformer.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/events/touch_transfo... ui/events/touch_transformer.h:20: class EVENTS_BASE_EXPORT TouchTransformer { does this have to be class? can be struct? https://codereview.chromium.org/191223007/diff/120001/ui/events/x/device_data... File ui/events/x/device_data_manager.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/events/x/device_data... ui/events/x/device_data_manager.h:229: int64 GetDisplayForTouchDevice(int touch_device_id); const?
https://codereview.chromium.org/191223007/diff/120001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/shell.h#newcode374 ash/shell.h:374: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/05/01 09:35:42, oshima wrote: > can this be just USE_X11? Could it also be non X11? I'm looking at the code and the only X11 specific bits are the touch event processing in DeviceDataManager::TouchEventCalibrate and it looks like those bits can be standalone.
https://codereview.chromium.org/191223007/diff/120001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/shell.h#newcode374 ash/shell.h:374: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/05/01 09:35:42, oshima wrote: > can this be just USE_X11? I am not sure. Within shell.h it seems all the USE_X11 are wrapped inside OS_CHROMEOS. So I am wondering what's the case that there is USE_X11 but !OS_CHROMEOS ? https://codereview.chromium.org/191223007/diff/120001/ash/shell.h#newcode374 ash/shell.h:374: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/05/01 13:47:43, dnicoara wrote: > On 2014/05/01 09:35:42, oshima wrote: > > can this be just USE_X11? > > Could it also be non X11? I'm looking at the code and the only X11 specific bits > are the touch event processing in DeviceDataManager::TouchEventCalibrate and it > looks like those bits can be standalone. TouchTransformerController uses DeviceDataManager to save the touchscreen's TouchTransform. DeviceDataManager itself is now depending on X11. I feel if we can have a generic DeviceDataManager then we can remove the USE_X11 ? https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:146: Shell::GetInstance()->display_configurator()->display_state(); On 2014/05/01 09:35:42, oshima wrote: > what value does this return for software mirror mode? hmm, I actually don't know what is software mirror mode. Looking at the code DisplayConfigurator::EnterStateOrFallBackToSoftwareMirroring() It seems that if the system fails to enter DUAL_MIRROR, it will try to enter DUAL_EXTENDED mode, and if that succeeds then software mirroring is enabled. So in this case, the return value would be MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ? https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:154: return; On 2014/05/01 09:35:42, oshima wrote: > I believe this should not happen. Did you hit this? maybe DCHECK? Done. https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:160: return; On 2014/05/01 09:35:42, oshima wrote: > same here Done. https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/01 09:35:42, oshima wrote: > nuke (c) > same for other files Done. https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:37: const DisplayInfo& touch_display); On 2014/05/01 09:35:42, oshima wrote: > const? > > (same for other methods) Done. https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_hos... ui/aura/window_tree_host.h:215: std::pair<int64, int64> display_ids_; On 2014/05/01 09:35:42, oshima wrote: > this can move to ash_window_tree_host_x11 now? In touch_transformer_controller.cc I need to get a hold of WindowTreeHost* to call UpdateDisplayID(). The way I do is calling display_controller->GetPrimaryRootWindow()/GetRootWindowForDisplayID(id)->GetHost() How can I get a hold of AshWindowTreeHost* ? Do you think we should expose similar getter to query DisplayController::window_tree_hosts_ to return AshWindowTreeHost* ? https://codereview.chromium.org/191223007/diff/120001/ui/events/touch_transfo... File ui/events/touch_transformer.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/events/touch_transfo... ui/events/touch_transformer.h:20: class EVENTS_BASE_EXPORT TouchTransformer { On 2014/05/01 09:35:42, oshima wrote: > does this have to be class? can be struct? Done. https://codereview.chromium.org/191223007/diff/120001/ui/events/x/device_data... File ui/events/x/device_data_manager.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/events/x/device_data... ui/events/x/device_data_manager.h:229: int64 GetDisplayForTouchDevice(int touch_device_id); On 2014/05/01 09:35:42, oshima wrote: > const? Done.
https://codereview.chromium.org/191223007/diff/120001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/191223007/diff/120001/ash/shell.h#newcode374 ash/shell.h:374: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/05/01 22:43:48, Yufeng Shen wrote: > On 2014/05/01 13:47:43, dnicoara wrote: > > On 2014/05/01 09:35:42, oshima wrote: > > > can this be just USE_X11? > > > > Could it also be non X11? I'm looking at the code and the only X11 specific > bits > > are the touch event processing in DeviceDataManager::TouchEventCalibrate and > it > > looks like those bits can be standalone. > > TouchTransformerController uses DeviceDataManager to save the touchscreen's > TouchTransform. DeviceDataManager itself is now depending on X11. I feel if we > can have a generic DeviceDataManager then we can remove the USE_X11 ? If we will remove USE_X11, then keeping as is now and remove USE_X11 later is fine with me. https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:146: Shell::GetInstance()->display_configurator()->display_state(); On 2014/05/01 22:43:48, Yufeng Shen wrote: > On 2014/05/01 09:35:42, oshima wrote: > > what value does this return for software mirror mode? > > hmm, I actually don't know what is software mirror mode. > Looking at the code > DisplayConfigurator::EnterStateOrFallBackToSoftwareMirroring() > > It seems that if the system fails to enter DUAL_MIRROR, it will try > to enter DUAL_EXTENDED mode, and if that succeeds then software mirroring > is enabled. So in this case, the return value would be > MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ? I just want to make sure things doesn't break in software mirror mode, and ideally consistent (a user do not have to know how mirror mode is implemetned). What happens if both displays are touch capable and switch to mirror mode now? Do we ignore touch events from second display? https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_hos... ui/aura/window_tree_host.h:215: std::pair<int64, int64> display_ids_; On 2014/05/01 22:43:48, Yufeng Shen wrote: > On 2014/05/01 09:35:42, oshima wrote: > > this can move to ash_window_tree_host_x11 now? > > In touch_transformer_controller.cc I need to get a hold > of WindowTreeHost* to call UpdateDisplayID(). > The way I do is calling > display_controller->GetPrimaryRootWindow()/GetRootWindowForDisplayID(id)->GetHost() > > How can I get a hold of AshWindowTreeHost* ? > Do you think we should expose similar getter to query > DisplayController::window_tree_hosts_ to return AshWindowTreeHost* ? You can use RootWindowController()->ash_host()
https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:146: Shell::GetInstance()->display_configurator()->display_state(); On 2014/05/01 23:07:56, oshima wrote: > On 2014/05/01 22:43:48, Yufeng Shen wrote: > > On 2014/05/01 09:35:42, oshima wrote: > > > what value does this return for software mirror mode? > > > > hmm, I actually don't know what is software mirror mode. > > Looking at the code > > DisplayConfigurator::EnterStateOrFallBackToSoftwareMirroring() > > > > It seems that if the system fails to enter DUAL_MIRROR, it will try > > to enter DUAL_EXTENDED mode, and if that succeeds then software mirroring > > is enabled. So in this case, the return value would be > > MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ? > > I just want to make sure things doesn't break in software mirror mode, > and ideally consistent (a user do not have to know how mirror mode is > implemetned). > > What happens if both displays are touch capable and switch to mirror mode now? > Do we ignore touch events from second display? AFAIK, the old code would treat software mirror mode as a regular extended desktop mode, so if software mirror mode did aspect preserving scaling and not full screen stretching, the touches in the pillarboxed / letterboxed areas would register as if they appeared on the visible image, and not ignored as appropriate. The touches on the visible image would also be offset. I think it's better to fix this in another patch. Maybe add a TODO in this patch?
https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/120001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:146: Shell::GetInstance()->display_configurator()->display_state(); On 2014/05/01 23:07:56, oshima wrote: > On 2014/05/01 22:43:48, Yufeng Shen wrote: > > On 2014/05/01 09:35:42, oshima wrote: > > > what value does this return for software mirror mode? > > > > hmm, I actually don't know what is software mirror mode. > > Looking at the code > > DisplayConfigurator::EnterStateOrFallBackToSoftwareMirroring() > > > > It seems that if the system fails to enter DUAL_MIRROR, it will try > > to enter DUAL_EXTENDED mode, and if that succeeds then software mirroring > > is enabled. So in this case, the return value would be > > MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ? > > I just want to make sure things doesn't break in software mirror mode, > and ideally consistent (a user do not have to know how mirror mode is > implemetned). > > What happens if both displays are touch capable and switch to mirror mode now? > Do we ignore touch events from second display? > no, touch events from second display also gets dispatched. But there could be bugs in the complicated case of extended mode with software mirroring, which I have not tested. Added a TODO per Yuly's request. https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_host.h File ui/aura/window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/120001/ui/aura/window_tree_hos... ui/aura/window_tree_host.h:215: std::pair<int64, int64> display_ids_; On 2014/05/01 23:07:56, oshima wrote: > On 2014/05/01 22:43:48, Yufeng Shen wrote: > > On 2014/05/01 09:35:42, oshima wrote: > > > this can move to ash_window_tree_host_x11 now? > > > > In touch_transformer_controller.cc I need to get a hold > > of WindowTreeHost* to call UpdateDisplayID(). > > The way I do is calling > > > display_controller->GetPrimaryRootWindow()/GetRootWindowForDisplayID(id)->GetHost() > > > > How can I get a hold of AshWindowTreeHost* ? > > Do you think we should expose similar getter to query > > DisplayController::window_tree_hosts_ to return AshWindowTreeHost* ? > > You can use RootWindowController()->ash_host() Done.
https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:110: ctm.y_offset = (1.0 / mirror_ar - 1.0 / native_ar) * 0.5 * mirror_width; Nit: I think the formula looks prettier (symmetric with the pillarboxed one) and consistent with the old code in this form: ctm.y_offset = (1.0 - mirror_ar / native_ar) * 0.5 * mirror_height; https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:116: ctm.x_offset = (mirror_ar - native_ar) * 0.5 * mirror_height; And ctm.x_offset = (1.0 - native_ar / mirror_ar) * 0.5 * mirror_width;
https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:110: ctm.y_offset = (1.0 / mirror_ar - 1.0 / native_ar) * 0.5 * mirror_width; Nit: I think the formula looks prettier (symmetric with the pillarboxed one) and consistent with the old code in this form: ctm.y_offset = (1.0 - mirror_ar / native_ar) * 0.5 * mirror_height; https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:116: ctm.x_offset = (mirror_ar - native_ar) * 0.5 * mirror_height; And ctm.x_offset = (1.0 - native_ar / mirror_ar) * 0.5 * mirror_width;
https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/191223007/diff/160001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:110: ctm.y_offset = (1.0 / mirror_ar - 1.0 / native_ar) * 0.5 * mirror_width; On 2014/05/02 00:43:20, ynovikov wrote: > Nit: I think the formula looks prettier (symmetric with the pillarboxed one) and > consistent with the old code in this form: > ctm.y_offset = (1.0 - mirror_ar / native_ar) * 0.5 * mirror_height; of course :)
oshima@ and sadrul@, do you have more comments ? I would like to have this in for M36. Partners are complaining about the broken touch mapping in dual clone monitor mode and I have promised them to get this fixed in M36.
ash, ui/display lgtm with a nit and q. https://codereview.chromium.org/191223007/diff/180001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/191223007/diff/180001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:19: DisplayInfo info(id, "", false); "" -> std::string() https://codereview.chromium.org/191223007/diff/180001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/180001/ui/aura/window_tree_hos... ui/aura/window_tree_host_x11.cc:520: ui::TouchEvent touchev(xev); I vaguely remember that creating TouchEvent before checking if it can be dispatched caused a failure in a test. Maybe you have already taken care of it, but wanted to mention it just in case.
fixed all the unittests. https://codereview.chromium.org/191223007/diff/180001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/191223007/diff/180001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:19: DisplayInfo info(id, "", false); On 2014/05/06 01:47:57, oshima wrote: > "" -> std::string() Done.
https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host.h:46: virtual void UpdateDisplayID(int64 id1, int64 id2) = 0; What are id1 and id2? Document please. https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_x11.cc:123: display_ids_.first = id1; indent is off https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_x11.cc:214: switch (event->type()) { Remove the switch. Use if (!event->IsTouchEvent()) instead. https://codereview.chromium.org/191223007/diff/200001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/191223007/diff/200001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:14: class ASH_EXPORT TouchTransformerController Document https://codereview.chromium.org/191223007/diff/200001/ui/events/test/events_t... File ui/events/test/events_test_utils_x11.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/test/events_t... ui/events/test/events_test_utils_x11.cc:113: xiev->event = target; Can we always set this to DefaultRootWindow() without need the target being passed in? https://codereview.chromium.org/191223007/diff/200001/ui/events/touch_transfo... File ui/events/touch_transformer.h (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/touch_transfo... ui/events/touch_transformer.h:20: struct EVENTS_BASE_EXPORT TouchTransformer { [copying over the comments from the earlier TouchCTM file] On 2014/04/29 20:34:18, Yufeng Shen wrote: > On 2014/03/15 19:32:51, sadrul wrote: > > Do we need this? Could we use a gfx::Transform instead? > > Looking at Transform::TransformPointInternal() there is some > rounding happening which worries me. I don't want the event that's close to the > boundary gets unexpectedly rounded up and gets out of > the boundary. > While if we just use the plain float operation when applying TouchCTM, we can > rely on the static_cast<int> in EventLocationFromeNative() to truncate the event > position so it won't get out of bound. Surely we can have Transform operate on PointFs without losing precision? That would be better than adding this similar-but-different thing. (and we use device/display id to find the target, so events close to the boundary shouldn't still go to a different window, right?) https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:111: // where they can be calibrated later. Is this step still necessary? https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:802: if (touch_device_id > 0 && touch_device_id < kMaxDeviceNum) { Have a separate IsTouchDevideIdValid() and use that in these functions.
https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host.h (right): https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host.h:46: virtual void UpdateDisplayID(int64 id1, int64 id2) = 0; On 2014/05/07 20:00:27, sadrul wrote: > What are id1 and id2? Document please. Done. https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_x11.cc (right): https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_x11.cc:123: display_ids_.first = id1; On 2014/05/07 20:00:27, sadrul wrote: > indent is off Done. https://codereview.chromium.org/191223007/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_x11.cc:214: switch (event->type()) { On 2014/05/07 20:00:27, sadrul wrote: > Remove the switch. Use if (!event->IsTouchEvent()) instead. Done. https://codereview.chromium.org/191223007/diff/200001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/191223007/diff/200001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:14: class ASH_EXPORT TouchTransformerController On 2014/05/07 20:00:27, sadrul wrote: > Document Done. https://codereview.chromium.org/191223007/diff/200001/ui/events/test/events_t... File ui/events/test/events_test_utils_x11.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/test/events_t... ui/events/test/events_test_utils_x11.cc:113: xiev->event = target; On 2014/05/07 20:00:27, sadrul wrote: > Can we always set this to DefaultRootWindow() without need the target being > passed in? Done. https://codereview.chromium.org/191223007/diff/200001/ui/events/touch_transfo... File ui/events/touch_transformer.h (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/touch_transfo... ui/events/touch_transformer.h:20: struct EVENTS_BASE_EXPORT TouchTransformer { On 2014/05/07 20:00:27, sadrul wrote: > [copying over the comments from the earlier TouchCTM file] > > On 2014/04/29 20:34:18, Yufeng Shen wrote: > > On 2014/03/15 19:32:51, sadrul wrote: > > > Do we need this? Could we use a gfx::Transform instead? > > > > Looking at Transform::TransformPointInternal() there is some > > rounding happening which worries me. I don't want the event that's close to > the > > boundary gets unexpectedly rounded up and gets out of > > the boundary. > > While if we just use the plain float operation when applying TouchCTM, we can > > rely on the static_cast<int> in EventLocationFromeNative() to truncate the > event > > position so it won't get out of bound. > > Surely we can have Transform operate on PointFs without losing precision? That > would be better than adding this similar-but-different thing. (and we use > device/display id to find the target, so events close to the boundary shouldn't > still go to a different window, right?) Done. https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:111: // where they can be calibrated later. On 2014/05/07 20:00:27, sadrul wrote: > Is this step still necessary? I saw you filed another bug on this. Let's address the issue there. My last experiment shows that we still need to modify the event position as " xievent->event_x = xievent->root_x; xievent->event_y = xievent->root_y; " https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:802: if (touch_device_id > 0 && touch_device_id < kMaxDeviceNum) { On 2014/05/07 20:00:27, sadrul wrote: > Have a separate IsTouchDevideIdValid() and use that in these functions. Done.
The build issues in the trybots look real. Can you take a look?
LGTM
The CQ bit was checked by miletus@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/191223007/300001
Message was sent while issue was closed.
Change committed as 269371
Message was sent while issue was closed.
https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:111: // where they can be calibrated later. On 2014/05/07 22:02:58, Yufeng Shen wrote: > On 2014/05/07 20:00:27, sadrul wrote: > > Is this step still necessary? > > I saw you filed another bug on this. Let's address the issue > there. My last experiment shows that we still need to modify > the event position as > " > xievent->event_x = xievent->root_x; > xievent->event_y = xievent->root_y; > " What bug? Please CC if you try to move this code. It should be done in a way that does not make X11-specific assumptions about the axis ranges, except under X11 builds.
Message was sent while issue was closed.
https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... File ui/events/x/device_data_manager.cc (right): https://codereview.chromium.org/191223007/diff/200001/ui/events/x/device_data... ui/events/x/device_data_manager.cc:111: // where they can be calibrated later. On 2014/05/20 16:38:16, spang wrote: > On 2014/05/07 22:02:58, Yufeng Shen wrote: > > On 2014/05/07 20:00:27, sadrul wrote: > > > Is this step still necessary? > > > > I saw you filed another bug on this. Let's address the issue > > there. My last experiment shows that we still need to modify > > the event position as > > " > > xievent->event_x = xievent->root_x; > > xievent->event_y = xievent->root_y; > > " > > What bug? > > Please CC if you try to move this code. It should be done in a way that does not > make X11-specific assumptions about the axis ranges, except under X11 builds. crbug.com/371060 I think. I am not worrying about the calibration part, but more about ui::PlatformEventObserver:WillProcessEvent() part which sets the root_x/y value to event_x/y. It is a X specific hack which should not affect ozone though. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
