Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
756643006:
Remote assistance on Chrome OS Part IX - Input injection on Ozone (Closed)
|
Created:
6 years ago by kelvinp Modified:
6 years ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org, chromoting-reviews_chromium.org, kpschoedel, Jamie Base URL:
https://chromium.googlesource.com/chromium/src.git@Ozone_input_injection Project:
chromium Visibility:
Public. |
DescriptionRemote assistance on Chrome OS Part IX - Input injection on Ozone
This CL implements InputInjectorChromeOS with the SystemInputInjector API.
It also introduce a new class, DisplayRotationFilter, on the input pipeline to handle
display rotation settings on Ozone.
BUG=426716
Committed: https://crrev.com/cc35531cfdcfb7df829c5dce693ee77726f8ef88
Cr-Commit-Position: refs/heads/master@{#307558}
Patch Set 1 #Patch Set 2 : Add UsbCode to DomCode API #
Total comments: 21
Patch Set 3 : Address feedback (wez) #Patch Set 4 : Fix build error #Patch Set 5 : Keep the coordinate system sane #Patch Set 6 : Fix rotation on X11 devices #
Total comments: 30
Patch Set 7 : Address wez's feedback #
Total comments: 37
Patch Set 8 : Address feedback (wez) #
Total comments: 6
Patch Set 9 : Rebase on ToT #Patch Set 10 : Address feedback (wez) #
Messages
Total messages: 26 (8 generated)
kelvinp@chromium.org changed reviewers: + dcaiafa@chromium.org, jamiewalch@chromium.org, spang@chromium.org, wez@chromium.org
kelvinp@chromium.org changed reviewers: - jamiewalch@chromium.org
spang@, can you take a look on the display rotation change under remoting/host/chromeos/display_rotation_filter_ozone.cc wez@ and dcaiafa@, can you take a look at the rest of the change? Thank you.
Initial comments. https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... File remoting/host/chromeos/display_rotation_filter_ozone.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:5: #include "remoting/host/display_rotation_filter.h" nit: We typically leave the platform-specific implementations of cross-platform interfaces alongside the cross-platform parts; I'm fine with us moving away from that model, but we should make a deliberate decision to do so, or not. https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:24: // the original ones in Ozone. This is confusingly worded; IIUC what you're saying is that if the physical display is 1280x850, but its rotated 90deg then it is effectively an 850x1200 display and that's the coordinate system that Chromoting will send events for, while Ozone expects raw events to match the native resolution? https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:40: DCHECK(root_window_); Why DCHECK here, if the rest of the code always has to cope w/ there being no |root_window_|? What is it that ash::Shell::HasInstance() does that is different from just checking for a non-null result from GetPrimaryRootWindow? https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:59: transform.Scale(1 / scale, 1 / scale); You're scaling by the device scale factor, applying a transform, then scaling back - the comment doesn't clearly explain why this is required (i.e. that rotation moves the origin off of (0,0)?) https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... File remoting/host/client_session.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.cc:59: CreateDisplayRotationFilter(&remote_input_filter_)), This feels like a property of the DesktopEnvironment, not a process-global property - it may even be better to push creation of this filter into the DesktopEnvironment for CrOS. https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.cc:60: mouse_clamping_filter_(display_rotation_filter_.get()), Use *display_rotation_filter_ here. In general, avoid using scoped_ptr<>::get() unless you really really need to; operator* and operator-> will both DCHECK if the scoped_ptr<> is NULL, to give clean failures. https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... File remoting/host/client_session.h (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.h:187: // |remote_input_filter| so that the local mouse movements matches the nit: "... the coordinate system of injected events matches those returned by the local input monitor"? https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... File remoting/host/display_rotation_filter.h (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... remoting/host/display_rotation_filter.h:15: // custom transformation. This comment should describe the method's function, e.g: "Returns an InputFilter that rotates input event coordinates appropriately for the current display rotation settings." https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... remoting/host/display_rotation_filter.h:16: scoped_ptr<protocol::InputFilter> CreateDisplayRotationFilter( This is an InputFilter that rotates the coordinates of input events, so let's call it RotatingInputFilter or InputRotator, i.e. CreateRotatingInputFilter here()?
On 2014/11/25 05:02:47, kelvinp wrote: > spang@, can you take a look on the display rotation change > under remoting/host/chromeos/display_rotation_filter_ozone.cc > How are you handling the rotation on X11 builds? I'm a little surprised to see an _ozone here. This coordinate transformation problem really applies to all ChromeOS builds.
I'm not sure you need a whole new file for fixing up coordinates. https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... remoting/host/input_injector_chromeos.cc:102: DCHECK(event.has_x() && event.has_y()); I think just do: aura::WindowTreeHost* host = ash::Shell::GetPrimaryRootWindow()->GetHost(); gfx::Point location(event.x(), event.y()); host->ConvertPointToHost(&location); // after some pending ozone changes this will change to: // host->ConvertPointToNativeScreen(&location); delegate_->MoveCursorTo(location);
Patchset #3 (id:40001) has been deleted
@wez, PTAL @spang. Good catch, input rotation is now correctly handled on chromeos x11 as well. https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... File remoting/host/chromeos/display_rotation_filter_ozone.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:5: #include "remoting/host/display_rotation_filter.h" On 2014/11/26 02:28:34, Wez wrote: > nit: We typically leave the platform-specific implementations of cross-platform > interfaces alongside the cross-platform parts; I'm fine with us moving away from > that model, but we should make a deliberate decision to do so, or not. Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:40: DCHECK(root_window_); On 2014/11/26 02:28:33, Wez wrote: > Why DCHECK here, if the rest of the code always has to cope w/ there being no > |root_window_|? > > What is it that ash::Shell::HasInstance() does that is different from just > checking for a non-null result from GetPrimaryRootWindow? This is mainly to allow the test not to crash in Unit tests. https://codereview.chromium.org/756643006/diff/20001/remoting/host/chromeos/d... remoting/host/chromeos/display_rotation_filter_ozone.cc:59: transform.Scale(1 / scale, 1 / scale); On 2014/11/26 02:28:33, Wez wrote: > You're scaling by the device scale factor, applying a transform, then scaling > back - the comment doesn't clearly explain why this is required (i.e. that > rotation moves the origin off of (0,0)?) Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... File remoting/host/client_session.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.cc:59: CreateDisplayRotationFilter(&remote_input_filter_)), On 2014/11/26 02:28:34, Wez wrote: > This feels like a property of the DesktopEnvironment, not a process-global > property - it may even be better to push creation of this filter into the > DesktopEnvironment for CrOS. Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.cc:59: CreateDisplayRotationFilter(&remote_input_filter_)), On 2014/11/26 02:28:34, Wez wrote: > This feels like a property of the DesktopEnvironment, not a process-global > property - it may even be better to push creation of this filter into the > DesktopEnvironment for CrOS. Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.cc:60: mouse_clamping_filter_(display_rotation_filter_.get()), On 2014/11/26 02:28:34, Wez wrote: > Use *display_rotation_filter_ here. In general, avoid using scoped_ptr<>::get() > unless you really really need to; operator* and operator-> will both DCHECK if > the scoped_ptr<> is NULL, to give clean failures. *display_rotation_filter_ returns an InputStub&, but the constructor of of MouseClampingFilter expects a InputStub* https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... File remoting/host/client_session.h (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/client_ses... remoting/host/client_session.h:187: // |remote_input_filter| so that the local mouse movements matches the On 2014/11/26 02:28:34, Wez wrote: > nit: "... the coordinate system of injected events matches those returned by the > local input monitor"? Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... File remoting/host/display_rotation_filter.h (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... remoting/host/display_rotation_filter.h:15: // custom transformation. On 2014/11/26 02:28:34, Wez wrote: > This comment should describe the method's function, e.g: > > "Returns an InputFilter that rotates input event coordinates appropriately for > the current display rotation settings." Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/display_ro... remoting/host/display_rotation_filter.h:16: scoped_ptr<protocol::InputFilter> CreateDisplayRotationFilter( On 2014/11/26 02:28:34, Wez wrote: > This is an InputFilter that rotates the coordinates of input events, so let's > call it RotatingInputFilter or InputRotator, i.e. CreateRotatingInputFilter > here()? Done. https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... remoting/host/input_injector_chromeos.cc:102: DCHECK(event.has_x() && event.has_y()); On 2014/11/26 17:06:42, spang wrote: > I think just do: > > aura::WindowTreeHost* host = ash::Shell::GetPrimaryRootWindow()->GetHost(); > gfx::Point location(event.x(), event.y()); > > host->ConvertPointToHost(&location); > // after some pending ozone changes this will change to: > // host->ConvertPointToNativeScreen(&location); > > delegate_->MoveCursorTo(location); We need a class to handle the rotation for a few reasons: 1. Chromoting monitors local mouse movement such that when both the local and the remote user moves the mouse in the same time, the local user will always win. The local mouse movement is obtained by subtracting all mouse movements by the injected positions. As a result, we need to apply the co-ordinate rotation early in the pipeline such that the coordinates system of injected events matches those returned by the monitor. 2. Using a separate class allows us to remove the rotation during unittest and on non chromeos platforms.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/20001/remoting/host/input_inje... remoting/host/input_injector_chromeos.cc:102: DCHECK(event.has_x() && event.has_y()); On 2014/12/02 00:12:26, kelvinp wrote: > On 2014/11/26 17:06:42, spang wrote: > > I think just do: > > > > aura::WindowTreeHost* host = ash::Shell::GetPrimaryRootWindow()->GetHost(); > > gfx::Point location(event.x(), event.y()); > > > > host->ConvertPointToHost(&location); > > // after some pending ozone changes this will change to: > > // host->ConvertPointToNativeScreen(&location); Those pending changes are now landed. You should use ConvertPointToNativeScreen and inject a screen location (although since you only support one display, the native screen coordinates are probably probably the same as the host coordinates because the display origin is 0,0). > > > > delegate_->MoveCursorTo(location); > > We need a class to handle the rotation for a few reasons: > 1. Chromoting monitors local mouse movement such that when both the local and > the remote user moves the mouse in the same time, the local user will always > win. The local mouse movement is obtained by subtracting all mouse movements by > the injected positions. As a result, we need to apply the co-ordinate rotation > early in the pipeline such that the coordinates system of injected events > matches those returned by the monitor. I don't completely follow. Can't you use the root window or ash screen coordinates to implement that feature? The host / native screen space is an implementation detail and is basically nonsense to work in. If possible, I'd suggesting trying hard to avoid it, and also to avoid reproducing ash's root transformer in the remoting code. Why wouldn't the following work: (1) Right before calling InjectMouseEvent, convert your (sensible) root window coordinates to (nonsense) native screen coordinates using the aura API (ConvertPointToNativeScreen). (2) If you ever get host or native screen coordinates somewhere (where?), immediately convert them to root window or ash screen so that they're in a sensible space. Use the sensible space for any mouse-related logic. The idea is to sweep these platform-dependent rotations under the rug. I don't think it will affect your tests either way; the test won't set a rotation in the first place. > 2. Using a separate class allows us to remove the rotation during unittest and > on non chromeos platforms.
Patchset #4 (id:100001) has been deleted
PTAL @spang. I like the idea of keeping the remoting code as far away as possible from the platform specific rotations. In this way, if we need to manipulate mouse location in the remoting code in the future, we don't need to remember that we have to do special logic on ChromeOS. I have updated the CL. However, I can't use ConvertPointToNativeScreen. This is because ConvertPointToNativeScreen has rounding problems as it uses Point instead of PointF underneath the hood. When I convert the coordinates from pixels to DIP prior to the rotation transform, it is rounded down. And when I uncover the local movement into the host coordinate system, it is rounded down again. The result is that the local movements and the injected locations will always be off by one when the coordinate is an odd number.
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... File remoting/host/chromeos/display_transform_util.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:13: gfx::Transform GetRootTransform(bool inverse) { This needs to be static, or in an anonymous namespace. https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:15: float scale = ui::GetDeviceScaleFactor(layer); nit: Blank line after this, before the comment, to break things up for the reader https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:21: // rotation transform. This paragraph isn't very helpful; it's not clear how the second sentence relates to the first? https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:33: transform.Scale(1/scale, 1/scale); // Converts to device independent pixels. As I commented previously, you need to explain *why* you are doing this scale/unscale hack here. e.g. Is the issue that |layer->transform()| returns a transform consisting of a rotation and translation, but in device pixels, so we need to switch DIPS->device pixels, apply it, then switch device->DIPS? If so, why can't we just do: gfx::Transform rotation = layer->transform(); ... inverse logic ... // Convert the rotation from device pixels to DIPs. rotation.Scale(1/scale, 1/scale); return rotation; i.e. Do we need the separate |transform| variable? Note that DIPs are Density Independent Pixels, not Device Independent. :) https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:41: return screen_location.AsPointF(); It seems expensive to keep fetching the root layer transform and scaling it on each and every point - is there a way we can cache the DIPs-scaled rotation transform to re-use? https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... File remoting/host/chromeos/display_transform_util.h (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.h:13: // screen coordinate system. |location| is in device pixels. Is there some design doc we can refer to that explains what the difference between the "window tree host" and "native screen" coordinate systems is? https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.h:13: // screen coordinate system. |location| is in device pixels. It's confusing to say that |location| is in "window tree host" coordinates, but then that it is "in device pixels" - which is it? Do you mean to say that "window tree host" coordinates are always expressed in "device pixels" rather than DIPs? Looking at the implementation, it seems that |location| and the return value are both expressed in DIPs, not device pixels? https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:43: class InputInjectorChromeos::Core { nit: Suggest comment to clarify that this class is run exclusively on <whatever-thread> https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:63: clipboard_(Clipboard::Create()) { nit: Clarify what sort of Clipboard we're creating here; is it going to be the usual X11 one? https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:69: // (See crbug.com/425201). This comment belongs in InjectKeyEvent, I think? https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:78: if (!event.has_pressed() || !event.has_usb_keycode()) { Why do you need these tests? Events are received via the IPC channel or EventDispatcher, both of which suppress events that are missing these fields, don't they? If so then you should just DCHECK https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:93: NOTIMPLEMENTED(); Doesn't that mean that the Android client won't work properly? In which case refer to the bug for getting this properly implemented. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:160: return make_scoped_ptr(new InputInjectorChromeos(ui_task_runner)); nit: Explain why the Core runs on the ui task runner. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.h (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.h:15: class InputInjectorChromeos : public InputInjector { nit: I think this should strictly be InputInjectorChromeOs :P https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:417: new_mouse_position.set(screen_location.x(), screen_location.y()); Can we do this rotation in the InputInjectorChromeOS, before we pass the event on to the X11 injector, rather than polluting it?
@wez, it is ready for another iteration. Can you take another look? https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... File remoting/host/chromeos/display_transform_util.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:13: gfx::Transform GetRootTransform(bool inverse) { On 2014/12/04 01:28:25, Wez wrote: > This needs to be static, or in an anonymous namespace. No longer applied. https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:15: float scale = ui::GetDeviceScaleFactor(layer); On 2014/12/04 01:28:25, Wez wrote: > nit: Blank line after this, before the comment, to break things up for the > reader Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:21: // rotation transform. On 2014/12/04 01:28:25, Wez wrote: > This paragraph isn't very helpful; it's not clear how the second sentence > relates to the first? Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:33: transform.Scale(1/scale, 1/scale); // Converts to device independent pixels. On 2014/12/04 01:28:25, Wez wrote: > As I commented previously, you need to explain *why* you are doing this > scale/unscale hack here. > > e.g. Is the issue that |layer->transform()| returns a transform consisting of a > rotation and translation, but in device pixels, so we need to switch > DIPS->device pixels, apply it, then switch device->DIPS? > > If so, why can't we just do: > > gfx::Transform rotation = layer->transform(); > ... inverse logic ... > > // Convert the rotation from device pixels to DIPs. > rotation.Scale(1/scale, 1/scale); > return rotation; > > i.e. Do we need the separate |transform| variable? > > Note that DIPs are Density Independent Pixels, not Device Independent. :) Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.cc:41: return screen_location.AsPointF(); On 2014/12/04 01:28:25, Wez wrote: > It seems expensive to keep fetching the root layer transform and scaling it on > each and every point - is there a way we can cache the DIPs-scaled rotation > transform to re-use? Done https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... File remoting/host/chromeos/display_transform_util.h (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/chromeos/... remoting/host/chromeos/display_transform_util.h:13: // screen coordinate system. |location| is in device pixels. On 2014/12/04 01:28:25, Wez wrote: > It's confusing to say that |location| is in "window tree host" coordinates, but > then that it is "in device pixels" - which is it? Do you mean to say that > "window tree host" coordinates are always expressed in "device pixels" rather > than DIPs? > > Looking at the implementation, it seems that |location| and the return value are > both expressed in DIPs, not device pixels? Unfortunately, we don't have a design docs. The comments is updated to make it clearer for the reader. Both coordinate system is in DIP https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:43: class InputInjectorChromeos::Core { On 2014/12/04 01:28:25, Wez wrote: > nit: Suggest comment to clarify that this class is run exclusively on > <whatever-thread> Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:63: clipboard_(Clipboard::Create()) { On 2014/12/04 01:28:25, Wez wrote: > nit: Clarify what sort of Clipboard we're creating here; is it going to be the > usual X11 one? Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:69: // (See crbug.com/425201). On 2014/12/04 01:28:25, Wez wrote: > This comment belongs in InjectKeyEvent, I think? Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:78: if (!event.has_pressed() || !event.has_usb_keycode()) { On 2014/12/04 01:28:25, Wez wrote: > Why do you need these tests? Events are received via the IPC channel or > EventDispatcher, both of which suppress events that are missing these fields, > don't they? If so then you should just DCHECK Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:93: NOTIMPLEMENTED(); On 2014/12/04 01:28:25, Wez wrote: > Doesn't that mean that the Android client won't work properly? In which case > refer to the bug for getting this properly implemented. We only support It2me on ChromeOS, which is not supported on Android. So we are fine for now. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:160: return make_scoped_ptr(new InputInjectorChromeos(ui_task_runner)); On 2014/12/04 01:28:25, Wez wrote: > nit: Explain why the Core runs on the ui task runner. Done. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.h (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.h:15: class InputInjectorChromeos : public InputInjector { On 2014/12/04 01:28:25, Wez wrote: > nit: I think this should strictly be InputInjectorChromeOs :P Are you referring to the casing of Chromeos in the class name? From code search suggests that half of the classes refer ChromeOS as "Chromeos" while the other half refer it as "ChromeOS". Since the remoting codebase already uses "Chromeos" as the convention, I am leaving it as it is. https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:417: new_mouse_position.set(screen_location.x(), screen_location.y()); On 2014/12/04 01:28:25, Wez wrote: > Can we do this rotation in the InputInjectorChromeOS, before we pass the event > on to the X11 injector, rather than polluting it? This is going to be an interim hack, that will be removed when all ChromeOS devices is moving away from x11. So, i don't think there is much value is writing a class that we will throw away later. I have filed a bug to remove the hack as discussed.
https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.h (right): https://codereview.chromium.org/756643006/diff/180001/remoting/host/input_inj... remoting/host/input_injector_chromeos.h:15: class InputInjectorChromeos : public InputInjector { On 2014/12/05 03:17:28, kelvinp wrote: > On 2014/12/04 01:28:25, Wez wrote: > > nit: I think this should strictly be InputInjectorChromeOs :P > > Are you referring to the casing of Chromeos in the class name? > From code search suggests that half of the classes refer ChromeOS as "Chromeos" > while the other half refer it as "ChromeOS". Since the remoting codebase > already uses "Chromeos" as the convention, I am leaving it as it is. Acknowledged. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... File remoting/host/chromeos/pixel_rotator.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:14: : root_window_(nullptr) { nit: no need to init to nullptr only to then overwrite it; consider moving the GetPrimaryRootWindow call into the initializer. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:24: if (window != root_window_) { Since you only observed the root window, can you ever get other windows listed here? Consider a DCHECK or CHECK if not. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:32: // translation, but in DIP, so we need to switch device pixels to DIPs https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:39: ignore_result(rotation.GetInverse(&inverse_rotation)); Do we really want to ignore the result? Consider saving it to a bool and DCHECKing it if it should never fail https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:49: gfx::PointF PixelRotator::ToScreenPixel(const gfx::PointF& window_location) { nit: suggest root_location, here and elsewhere; otherwise it reads as though this might just be a simple window-to/from-screen converter? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... File remoting/host/chromeos/pixel_rotator.h (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:18: // A class that performs coordinates system transformation between the root coordinate transformations https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:22: // The root window coordinate of a pixel is its relative location to the top Root window coordinates are expressed relative to the top left... https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:24: // physical location of the pixel that is rendered on the display. Both What does "physical location" mean? Presumably you mean relative to the top-left of the framebuffer, which may not match the root window either in origin nor orientation? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:30: // (height - y, x) on the display. This comment just seems a more specific example of the previous paragraph? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:31: class PixelRotator : public aura::WindowObserver { This doesn't touch pixels, and it may translate as well as rotate - it transforms points so call it e.g. OzoneCoordinateTransformer? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:33: PixelRotator(); Presumably the root window is bound when the ctor is invoked? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:34: ~PixelRotator(); override https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:37: void OnWindowTransformed(aura::Window* window) override; Is this part of the PixelRotator interface? If not then move it into the private section. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:49: gfx::Transform transform_; suggest root_to_screen or root_to_native https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:50: gfx::Transform inverse_; similarly here https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:59: // Rotate the input coordinates appropriately based on the current display nit: "Used to rotate..." https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:70: DCHECK(delegate); nit: Do you need to check that |clipboard_| succeeded, as well? https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:97: // Chrome OS only supports It2me, which is not supported on mobile clients, so nit: It2Me or IT2Me https://codereview.chromium.org/756643006/diff/200001/remoting/host/local_inp... File remoting/host/local_input_monitor_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/local_inp... remoting/host/local_input_monitor_chromeos.cc:56: // Rotate the local mouse positions appropriately based on the current nit: Used to rotate... Here and elsewhere, does it also translate, e.g in the Ash-on-X11 case?
Patchset #8 (id:220001) has been deleted
@wez. The CL is ready for another iteration. I would like to land this before I am gone for vacation (Wednesday 12/9) if possible. Do you have any specific blocking feedback that needs to be included in this cl rather than in a follow up? https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... File remoting/host/chromeos/pixel_rotator.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:14: : root_window_(nullptr) { On 2014/12/06 01:55:16, Wez wrote: > nit: no need to init to nullptr only to then overwrite it; consider moving the > GetPrimaryRootWindow call into the initializer. Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:24: if (window != root_window_) { On 2014/12/06 01:55:16, Wez wrote: > Since you only observed the root window, can you ever get other windows listed > here? Consider a DCHECK or CHECK if not. Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:32: // translation, but in DIP, so we need to switch device pixels to On 2014/12/06 01:55:16, Wez wrote: > DIPs Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:39: ignore_result(rotation.GetInverse(&inverse_rotation)); On 2014/12/06 01:55:16, Wez wrote: > Do we really want to ignore the result? Consider saving it to a bool and > DCHECKing it if it should never fail Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.cc:49: gfx::PointF PixelRotator::ToScreenPixel(const gfx::PointF& window_location) { On 2014/12/06 01:55:17, Wez wrote: > nit: suggest root_location, here and elsewhere; otherwise it reads as though > this might just be a simple window-to/from-screen converter? Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... File remoting/host/chromeos/pixel_rotator.h (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:18: // A class that performs coordinates system transformation between the root On 2014/12/06 01:55:17, Wez wrote: > coordinate transformations Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:22: // The root window coordinate of a pixel is its relative location to the top On 2014/12/06 01:55:17, Wez wrote: > Root window coordinates are expressed relative to the top left... Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:24: // physical location of the pixel that is rendered on the display. Both On 2014/12/06 01:55:17, Wez wrote: > What does "physical location" mean? Presumably you mean relative to the top-left > of the framebuffer, which may not match the root window either in origin nor > orientation? Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:30: // (height - y, x) on the display. On 2014/12/06 01:55:17, Wez wrote: > This comment just seems a more specific example of the previous paragraph? Yes, it is always better to illustrate with an example https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:33: PixelRotator(); On 2014/12/06 01:55:17, Wez wrote: > Presumably the root window is bound when the ctor is invoked? Yes https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:34: ~PixelRotator(); On 2014/12/06 01:55:17, Wez wrote: > override Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:37: void OnWindowTransformed(aura::Window* window) override; On 2014/12/06 01:55:17, Wez wrote: > Is this part of the PixelRotator interface? If not then move it into the private > section. Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:49: gfx::Transform transform_; On 2014/12/06 01:55:17, Wez wrote: > suggest root_to_screen or root_to_native Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/chromeos/... remoting/host/chromeos/pixel_rotator.h:50: gfx::Transform inverse_; On 2014/12/06 01:55:17, Wez wrote: > similarly here Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:59: // Rotate the input coordinates appropriately based on the current display On 2014/12/06 01:55:17, Wez wrote: > nit: "Used to rotate..." Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:70: DCHECK(delegate); On 2014/12/06 01:55:17, Wez wrote: > nit: Do you need to check that |clipboard_| succeeded, as well? Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/input_inj... remoting/host/input_injector_chromeos.cc:97: // Chrome OS only supports It2me, which is not supported on mobile clients, so On 2014/12/06 01:55:17, Wez wrote: > nit: It2Me or IT2Me Done. https://codereview.chromium.org/756643006/diff/200001/remoting/host/local_inp... File remoting/host/local_input_monitor_chromeos.cc (right): https://codereview.chromium.org/756643006/diff/200001/remoting/host/local_inp... remoting/host/local_input_monitor_chromeos.cc:56: // Rotate the local mouse positions appropriately based on the current On 2014/12/06 01:55:17, Wez wrote: > nit: Used to rotate... > > Here and elsewhere, does it also translate, e.g in the Ash-on-X11 case? Done. No, it doesn't translate AFAIK.
LGTM w/ nits. https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... File remoting/host/chromeos/point_transformer.h (right): https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... remoting/host/chromeos/point_transformer.h:18: // A class that performs coordinates transformations between the root window coordinate https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... remoting/host/chromeos/point_transformer.h:25: // either in origin nor orientation. Both coordinates systems are always in coordinate https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... File ui/events/keycodes/dom4/keycode_converter.cc (right): https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... ui/events/keycodes/dom4/keycode_converter.cc:210: return static_cast<DomCode>(usb_keycode); Return usb_keycode_map[i].id rather than the static cast here.
FYI https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... File remoting/host/chromeos/point_transformer.h (right): https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... remoting/host/chromeos/point_transformer.h:18: // A class that performs coordinates transformations between the root window On 2014/12/09 01:36:44, Wez wrote: > coordinate Done. https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... remoting/host/chromeos/point_transformer.h:25: // either in origin nor orientation. Both coordinates systems are always in On 2014/12/09 01:36:44, Wez wrote: > coordinate Done. https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... File ui/events/keycodes/dom4/keycode_converter.cc (right): https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... ui/events/keycodes/dom4/keycode_converter.cc:210: return static_cast<DomCode>(usb_keycode); On 2014/12/09 01:36:44, Wez wrote: > Return usb_keycode_map[i].id rather than the static cast here. Unfortunately, the id field does not exist on Linux. See line 19 on the same file #elif defined(OS_LINUX) #define USB_KEYMAP(usb, xkb, win, mac, code, id) {usb, xkb, code}
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/756643006/280001
Message was sent while issue was closed.
Committed patchset #10 (id:280001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cc35531cfdcfb7df829c5dce693ee77726f8ef88 Cr-Commit-Position: refs/heads/master@{#307558}
Message was sent while issue was closed.
On 9 Dec 2014 12:24, <kelvinp@chromium.org> wrote: > > FYI > > > > https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... > File remoting/host/chromeos/point_transformer.h (right): > > https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... > remoting/host/chromeos/point_transformer.h:18: // A class that performs > coordinates transformations between the root window > On 2014/12/09 01:36:44, Wez wrote: >> >> coordinate > > > Done. > > > https://codereview.chromium.org/756643006/diff/240001/remoting/host/chromeos/... > remoting/host/chromeos/point_transformer.h:25: // either in origin nor > orientation. Both coordinates systems are always in > On 2014/12/09 01:36:44, Wez wrote: >> >> coordinate > > > Done. > > > https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... > File ui/events/keycodes/dom4/keycode_converter.cc (right): > > https://codereview.chromium.org/756643006/diff/240001/ui/events/keycodes/dom4... > ui/events/keycodes/dom4/keycode_converter.cc:210: return > static_cast<DomCode>(usb_keycode); > On 2014/12/09 01:36:44, Wez wrote: >> >> Return usb_keycode_map[i].id rather than the static cast here. > > > Unfortunately, the id field does not exist on Linux. See line 19 on the > same file > #elif defined(OS_LINUX) > #define USB_KEYMAP(usb, xkb, win, mac, code, id) {usb, xkb, code} OK, please fix that, then - we definitely don't want a static cast spitting out enums. > > https://codereview.chromium.org/756643006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
