Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
657603002:
ash: ozone: apply transformation to events outside the root window (Closed)
|
Created:
6 years, 2 months ago by llandwerlin-old Modified:
6 years ago CC:
chromium-reviews, kalyank, ben+aura_chromium.org, sadrul, ben+ash_chromium.org, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionash: ozone: apply transformation to events outside the root window
When dragging windows from one screen to another, events need to be
captured and send to the window where the drag started. As a result we
also need to translate event back into the original window's location.
BUG=423383
TEST=none
Committed: https://crrev.com/164046d3753535889d3ba1f0060a0280a94bc1c7
Cr-Commit-Position: refs/heads/master@{#306623}
Patch Set 1 #Patch Set 2 : Remove whitespace in toplevel_window_event_handler.cc #Patch Set 3 : Use SetCapture/ReleaseCapture on aura Window #Patch Set 4 : Remove grab_flags_ from DriWindow #Patch Set 5 : coding style #
Total comments: 3
Patch Set 6 : Add root location on DriCursor #Patch Set 7 : Rebase on ToT #
Total comments: 1
Patch Set 8 : Fix event unit test #Patch Set 9 : Rebase on ToT #
Total comments: 8
Patch Set 10 : Update after second round of review #Patch Set 11 : Change GetBounds() visibility from public to protected #
Total comments: 15
Patch Set 12 : Remove DCHECKs from DriWindowManager #
Total comments: 7
Patch Set 13 : Only capture specific events during the grab #Patch Set 14 : Capture everything #Patch Set 15 : Fix tablet_event_converter_evdev_unittest #
Total comments: 1
Patch Set 16 : Remove explicit gfx::Rect to gfx::RectF conversion #
Total comments: 8
Patch Set 17 : Drop Ozone platform changes already on master #Patch Set 18 : Drop Ozone platform changes already on master #
Total comments: 1
Patch Set 19 : Change visibility of TranslateLocatedEvent() #
Total comments: 2
Patch Set 20 : Flatten TranslateLocatedEvent() #
Messages
Total messages: 53 (6 generated)
lionel.g.landwerlin@intel.com changed reviewers: + darin@chromium.org, dnicoara@chromium.org, spang@chromium.org
darin@chromium.org: Please review changes in AshWindowTreeHostOzone dnicoara@chromium.org: Please review changes in DriWindow/DriWindowManager spang@chromium.org: Please review changes in DriWindow/DriWindowManager Thanks
On 2014/10/14 15:51:04, llandwerlin wrote: > mailto:darin@chromium.org: Please review changes in AshWindowTreeHostOzone > > mailto:dnicoara@chromium.org: Please review changes in DriWindow/DriWindowManager > > mailto:spang@chromium.org: Please review changes in DriWindow/DriWindowManager > > Thanks Can you look at ash/drag_drop/drag_drop_tracker.h and implement PlatformWindow::SetCapture/ReleaseCapture?
On 2014/10/14 17:31:01, spang wrote: > On 2014/10/14 15:51:04, llandwerlin wrote: > > mailto:darin@chromium.org: Please review changes in AshWindowTreeHostOzone > > > > mailto:dnicoara@chromium.org: Please review changes in > DriWindow/DriWindowManager > > > > mailto:spang@chromium.org: Please review changes in DriWindow/DriWindowManager > > > > Thanks > > Can you look at ash/drag_drop/drag_drop_tracker.h and implement > PlatformWindow::SetCapture/ReleaseCapture? The DragDropTracker isn't used for moving windows. Is that something that needs to be changed?
Thanks for pointing that out. Removed the bit of logic from DriWindow and now using SetCapture/ReleaseCapture on aura::Window in the DragWindowResizer. I hope it's the way you wanted this.
On 2014/10/15 11:10:24, llandwerlin wrote: > Thanks for pointing that out. > Removed the bit of logic from DriWindow and now using SetCapture/ReleaseCapture > on aura::Window in the DragWindowResizer. I hope it's the way you wanted this. What I want is simply a straightforward API. The Set/ReleaseCapture calls seem to provide a way to make this interaction more explicit, which is a win.
On 2014/10/15 12:32:21, spang (OOO Oct 15 - Oct 17) wrote: > On 2014/10/15 11:10:24, llandwerlin wrote: > > Thanks for pointing that out. > > Removed the bit of logic from DriWindow and now using > SetCapture/ReleaseCapture > > on aura::Window in the DragWindowResizer. I hope it's the way you wanted this. > > What I want is simply a straightforward API. The Set/ReleaseCapture calls seem > to provide a way to make this interaction more explicit, which is a win. Sure, I'm just hoping DragWindowResizer was the right place to call that API. PTAL, thanks.
sorry for the delay https://codereview.chromium.org/657603002/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_ozone.cc (right): https://codereview.chromium.org/657603002/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_ozone.cc:35: if (event->IsMouseEvent()) Can you move this entirely into AshWindowTreeHostOzone? i.e. // ui/aura/window_tree_host_ozone.h void DispatchEvent(ui::Event* event) override; // ui/aura/window_tree_host_ozone.cc void AshWindowTreeHostOzone::DispatchEvent(ui::Event* event) { if (event->IsMouseEvent() || event->IsScrollEvent()) TranslateMouseEvent(static_cast<ui::LocatedEvent*>(event)); SendEventToProcessor(event); } https://codereview.chromium.org/657603002/diff/80001/ui/aura/window_tree_host... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/80001/ui/aura/window_tree_host... ui/aura/window_tree_host_ozone.h:31: const gfx::Rect bounds() const { return bounds_; } Please use the existing function GetBounds(). https://codereview.chromium.org/657603002/diff/80001/ui/ozone/platform/dri/dr... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/80001/ui/ozone/platform/dri/dr... ui/ozone/platform/dri/dri_window.cc:118: // We need to convert the cursor's position into coordinates Can you change DriCursor to generate the event coordinates relative to the screen, and then just offset here? So this becomes: if (event->IsMouseEvent() || event->IsScrollEvent()) { LocatedEvent* located_event = static_cast<LocatedEvent*>(event); gfx::PointF location = located_event->location(); location.Offset(-bounds_.Origin()); located_event->set_location(location); located_event->set_location(root_location); }
Added the root location on the dri cursor as you suggested and realized I could set it on native events. PTAL, Thanks.
lgtm
lionel.g.landwerlin@intel.com changed reviewers: + sky@chromium.org - darin@chromium.org
Sky, Could you review the changes in ash? Thank you.
spang@chromium.org changed reviewers: + derat@chromium.org, sadrul@chromium.org - sky@chromium.org
+sadrul for ui/events + ui/aura +derat for ash https://codereview.chromium.org/657603002/diff/120001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/120001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:43: // ui::PlatformWindowDelegate: is there a reason to move these?
lionel.g.landwerlin@intel.com changed reviewers: + sky@chromium.org - derat@chromium.org
On 2014/11/07 22:38:45, spang wrote: > +sadrul for ui/events + ui/aura > +derat for ash > > https://codereview.chromium.org/657603002/diff/120001/ui/aura/window_tree_hos... > File ui/aura/window_tree_host_ozone.h (right): > > https://codereview.chromium.org/657603002/diff/120001/ui/aura/window_tree_hos... > ui/aura/window_tree_host_ozone.h:43: // ui::PlatformWindowDelegate: > is there a reason to move these? Yes, they had a private visibility, which makes in impossible to use GetBounds() from AshWindowTreeHostOzone. I made these methods public like in WindowTreeHost and WindowTreeHostX11.
spang@chromium.org changed reviewers: + derat@chromium.org
Sadrul, could you review the changes in ui/events & ui/aura? Thank you.
i'm fine with the ash/ changes as an owner, but sadrul should review that directory for correctness too (i.e. i don't know much about this code).
https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_cursor.cc (right): https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_cursor.cc:73: cursor_root_location_.Offset(bounds.origin().x(), bounds.origin().y()); I noticed a bug. This is using the original location prior to clamping it to the window bounds. So the location is wrong. Please just do: cursor_root_location_ = cursor_location_ + bounds.OffsetFromOrigin(); after clamping.
https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: Do all of these need to be public? I think you only really need GetBounds() in AshWindowTreeHostOzone. Could you just make that protected and leave the rest private? https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window_manager.cc (right): https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ == gfx::kNullAcceleratedWidget) Should/Is it possible that this is false? Should we just DCHECK? https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ == widget) Is it possible that this is false? Should we just DCHECK instead?
https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: On 2014/11/10 21:44:06, dnicoara wrote: > Do all of these need to be public? I think you only really need GetBounds() in > AshWindowTreeHostOzone. Could you just make that protected and leave the rest > private? Done. https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_cursor.cc (right): https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_cursor.cc:73: cursor_root_location_.Offset(bounds.origin().x(), bounds.origin().y()); On 2014/11/10 20:30:47, spang wrote: > I noticed a bug. This is using the original location prior to clamping it to the > window bounds. So the location is wrong. > > Please just do: > > cursor_root_location_ = cursor_location_ + bounds.OffsetFromOrigin(); > > after clamping. Done. https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window_manager.cc (right): https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ == gfx::kNullAcceleratedWidget) On 2014/11/10 21:44:06, dnicoara wrote: > Should/Is it possible that this is false? Should we just DCHECK? The idea was to prevent 2 overlapping grabs, by making the second a no-op. I agree it's probably a good idea to have a DCHECK instead. https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ == widget) On 2014/11/10 21:44:06, dnicoara wrote: > Is it possible that this is false? Should we just DCHECK instead? Replacing by DCHECK.
PTAL, Thanks.
https://codereview.chromium.org/657603002/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_ozone.cc (right): https://codereview.chromium.org/657603002/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_ozone.cc:40: void TranslateMouseEvent(ui::LocatedEvent* event); non-override before overrides https://codereview.chromium.org/657603002/diff/200001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:28: virtual gfx::Rect GetBounds() const override; + // WindowTreeHost: (you should update the .cc file to match the function order in .h) https://codereview.chromium.org/657603002/diff/200001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/events/event.h#newco... ui/events/event.h:775: int finger_count); We should have one of these. Remove the first one. https://codereview.chromium.org/657603002/diff/200001/ui/events/ozone/evdev/c... File ui/events/ozone/evdev/cursor_delegate_evdev.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/events/ozone/evdev/c... ui/events/ozone/evdev/cursor_delegate_evdev.h:31: virtual gfx::PointF root_location() = 0; virtual functions shouldn't be in unix_hacker_style(). Call this GetRootLocation(). (location() above is also wrong) https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:112: gfx::AcceleratedWidget grabber = window_manager_->mouse_events_grabber(); Should you look at the capture window for other events too (e.g. key events)? I think that's how x11 behaves? https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:126: if (event->IsMouseEvent() || event->IsScrollEvent()) { You shouldn't need a grab for scroll events. https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window_manager.cc (right): https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:82: DCHECK(mouse_events_grabber_ == gfx::kNullAcceleratedWidget); DCHECK_EQ Have you tested this with nested message loops? e.g. click on the wrench menu to bring up the menu, then click on the menu. Or click on a folder in the bookmark bar, then start dragging a bookmark out of it, etc. https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:87: DCHECK(mouse_events_grabber_ == widget); ditto
On 2014/11/10 21:44:07, dnicoara wrote: > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > File ui/aura/window_tree_host_ozone.h (right): > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: > Do all of these need to be public? I think you only really need GetBounds() in > AshWindowTreeHostOzone. Could you just make that protected and leave the rest > private? > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > File ui/ozone/platform/dri/dri_window_manager.cc (right): > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ == > gfx::kNullAcceleratedWidget) > Should/Is it possible that this is false? Should we just DCHECK? > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ == > widget) > Is it possible that this is false? Should we just DCHECK instead? DCHECK is wrong here. If I am a window and ask to capture the mouse, how can I tell if a different window has already captured it? I can't. We should never DCHECK things like this. These operations on PlatformWindow usually cross a process boundary (think x11, wayland, mir). Crashing the server if the client misbehaves is not OK. I think, even though we are doing these operations in-process, we shouldn't have a stronger contract for the client code.
On 2014/11/11 16:59:49, spang wrote: > On 2014/11/10 21:44:07, dnicoara wrote: > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > File ui/aura/window_tree_host_ozone.h (right): > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: > > Do all of these need to be public? I think you only really need GetBounds() in > > AshWindowTreeHostOzone. Could you just make that protected and leave the rest > > private? > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > File ui/ozone/platform/dri/dri_window_manager.cc (right): > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ == > > gfx::kNullAcceleratedWidget) > > Should/Is it possible that this is false? Should we just DCHECK? > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ == > > widget) > > Is it possible that this is false? Should we just DCHECK instead? > > DCHECK is wrong here. If I am a window and ask to capture the mouse, how can I > tell if a different window has already captured it? I can't. We should never > DCHECK things like this. > > These operations on PlatformWindow usually cross a process boundary (think x11, > wayland, mir). Crashing the server if the client misbehaves is not OK. I think, > even though we are doing these operations in-process, we shouldn't have a > stronger contract for the client code. Ok, do you think we should make SetCapture return a boolean telling whether the capture has succeeded?
On 2014/11/11 17:28:56, llandwerlin wrote: > On 2014/11/11 16:59:49, spang wrote: > > On 2014/11/10 21:44:07, dnicoara wrote: > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > > File ui/aura/window_tree_host_ozone.h (right): > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > > ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: > > > Do all of these need to be public? I think you only really need GetBounds() > in > > > AshWindowTreeHostOzone. Could you just make that protected and leave the > rest > > > private? > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > File ui/ozone/platform/dri/dri_window_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ == > > > gfx::kNullAcceleratedWidget) > > > Should/Is it possible that this is false? Should we just DCHECK? > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ == > > > widget) > > > Is it possible that this is false? Should we just DCHECK instead? > > > > DCHECK is wrong here. If I am a window and ask to capture the mouse, how can I > > tell if a different window has already captured it? I can't. We should never > > DCHECK things like this. > > > > These operations on PlatformWindow usually cross a process boundary (think > x11, > > wayland, mir). Crashing the server if the client misbehaves is not OK. I > think, > > even though we are doing these operations in-process, we shouldn't have a > > stronger contract for the client code. > > Ok, do you think we should make SetCapture return a boolean telling whether the > capture has succeeded? No, that would require it to be synchronous. The API this is based on is this: http://msdn.microsoft.com/en-us/library/windows/desktop/ms646262(v=vs.85).aspx
On 2014/11/11 17:45:23, spang wrote: > On 2014/11/11 17:28:56, llandwerlin wrote: > > On 2014/11/11 16:59:49, spang wrote: > > > On 2014/11/10 21:44:07, dnicoara wrote: > > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > > > File ui/aura/window_tree_host_ozone.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/aura/window_tree_hos... > > > > ui/aura/window_tree_host_ozone.h:27: // WindowTreeHost: > > > > Do all of these need to be public? I think you only really need > GetBounds() > > in > > > > AshWindowTreeHostOzone. Could you just make that protected and leave the > > rest > > > > private? > > > > > > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > > File ui/ozone/platform/dri/dri_window_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > > ui/ozone/platform/dri/dri_window_manager.cc:82: if (mouse_events_grabber_ > == > > > > gfx::kNullAcceleratedWidget) > > > > Should/Is it possible that this is false? Should we just DCHECK? > > > > > > > > > > > > > > https://codereview.chromium.org/657603002/diff/160001/ui/ozone/platform/dri/d... > > > > ui/ozone/platform/dri/dri_window_manager.cc:87: if (mouse_events_grabber_ > == > > > > widget) > > > > Is it possible that this is false? Should we just DCHECK instead? > > > > > > DCHECK is wrong here. If I am a window and ask to capture the mouse, how can > I > > > tell if a different window has already captured it? I can't. We should never > > > DCHECK things like this. > > > > > > These operations on PlatformWindow usually cross a process boundary (think > > x11, > > > wayland, mir). Crashing the server if the client misbehaves is not OK. I > > think, > > > even though we are doing these operations in-process, we shouldn't have a > > > stronger contract for the client code. > > > > Ok, do you think we should make SetCapture return a boolean telling whether > the > > capture has succeeded? > > No, that would require it to be synchronous. The API this is based on is this: > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms646262(v=vs.85).aspx I think your original "first to capture" wins was fine, instead of the windows spec which has some restrictions. In any case, it can fail to capture and it does so silently.
https://codereview.chromium.org/657603002/diff/200001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_ozone.cc (right): https://codereview.chromium.org/657603002/diff/200001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_ozone.cc:40: void TranslateMouseEvent(ui::LocatedEvent* event); On 2014/11/11 14:41:14, sadrul wrote: > non-override before overrides Done. https://codereview.chromium.org/657603002/diff/200001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:28: virtual gfx::Rect GetBounds() const override; On 2014/11/11 14:41:14, sadrul wrote: > + // WindowTreeHost: > > (you should update the .cc file to match the function order in .h) Done. https://codereview.chromium.org/657603002/diff/200001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/events/event.h#newco... ui/events/event.h:775: int finger_count); On 2014/11/11 14:41:14, sadrul wrote: > We should have one of these. Remove the first one. Done. https://codereview.chromium.org/657603002/diff/200001/ui/events/ozone/evdev/c... File ui/events/ozone/evdev/cursor_delegate_evdev.h (right): https://codereview.chromium.org/657603002/diff/200001/ui/events/ozone/evdev/c... ui/events/ozone/evdev/cursor_delegate_evdev.h:31: virtual gfx::PointF root_location() = 0; On 2014/11/11 14:41:14, sadrul wrote: > virtual functions shouldn't be in unix_hacker_style(). Call this > GetRootLocation(). (location() above is also wrong) Done. https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:112: gfx::AcceleratedWidget grabber = window_manager_->mouse_events_grabber(); On 2014/11/11 14:41:14, sadrul wrote: > Should you look at the capture window for other events too (e.g. key events)? I > think that's how x11 behaves? Done. https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window_manager.cc (right): https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:82: DCHECK(mouse_events_grabber_ == gfx::kNullAcceleratedWidget); On 2014/11/11 14:41:14, sadrul wrote: > DCHECK_EQ Done. > > Have you tested this with nested message loops? e.g. click on the wrench menu to > bring up the menu, then click on the menu. Or click on a folder in the bookmark > bar, then start dragging a bookmark out of it, etc. All the cases you mentioned are working properly across/within display boundaries. https://codereview.chromium.org/657603002/diff/200001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window_manager.cc:87: DCHECK(mouse_events_grabber_ == widget); On 2014/11/11 14:41:14, sadrul wrote: > ditto Moving back to if()s after spang's comment.
PTAL, Thanks.
https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; Let's talk about this more. Touch events should not be grabbed, they neither use the mouse cursor nor respect keyboard focus. Mouse press, drag, release events should. Key events probably should, since you typically cannot remove keyboard focus from the capturing window anyway. We haven't really implemented keyboard focus separately from what's under the cursor in this function, because ash doesn't care about it. Dunno about scroll events. Sadrul suggested they should not. It makes some sense that scrolling always acts at the location under the cursor. However: What about mouse wheel? It's a mouse event. Should it be consistent with scroll events, or with mouse events? Gesture events are N/A don't go through this code path. https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:127: if (event->IsLocatedEvent()) { As above, touch events must not be affected by the cursor location.
On 2014/11/11 19:00:24, spang wrote: > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > File ui/ozone/platform/dri/dri_window.cc (right): > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; > Let's talk about this more. > > Touch events should not be grabbed, they neither use the mouse cursor nor > respect keyboard focus. > > Mouse press, drag, release events should. > > Key events probably should, since you typically cannot remove keyboard focus > from the capturing window anyway. We haven't really implemented keyboard focus > separately from what's under the cursor in this function, because ash doesn't > care about it. > > Dunno about scroll events. Sadrul suggested they should not. It makes some sense > that scrolling always acts at the location under the cursor. However: What about > mouse wheel? It's a mouse event. Should it be consistent with scroll events, or > with mouse events? > Chromium ScrollEvent seem to higher level kinds of events (built up on top of the lower level ones like motion/press/release). I wrote a small test on top of a toolkit running on X11. It shows the X11 equivalent to MouseWheel events (ButtonPress with button number >= 4) are sent during the implicit grab. So I would apply the same processing as mouse events. > > Gesture events are N/A don't go through this code path. > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window.cc:127: if (event->IsLocatedEvent()) { > As above, touch events must not be affected by the cursor location. Done.
https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; On 2014/11/11 19:00:24, spang wrote: > Let's talk about this more. > > Touch events should not be grabbed, they neither use the mouse cursor nor > respect keyboard focus. > > Mouse press, drag, release events should. > > Key events probably should, since you typically cannot remove keyboard focus > from the capturing window anyway. We haven't really implemented keyboard focus > separately from what's under the cursor in this function, because ash doesn't > care about it. > > Dunno about scroll events. Sadrul suggested they should not. It makes some sense > that scrolling always acts at the location under the cursor. However: What about > mouse wheel? It's a mouse event. Should it be consistent with scroll events, or > with mouse events? > > Gesture events are N/A don't go through this code path. Acknowledged. https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:127: if (event->IsLocatedEvent()) { On 2014/11/11 19:00:24, spang wrote: > As above, touch events must not be affected by the cursor location. They shouldn't be, because they will be delivered to the window on which the cursor is, so the conditional statement (window_manager_->cursor()->GetCursorWindow() != widget_) will fail.
PTAL, Thanks.
On 2014/11/12 14:40:37, llandwerlin wrote: > On 2014/11/11 19:00:24, spang wrote: > > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > > File ui/ozone/platform/dri/dri_window.cc (right): > > > > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > > ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; > > Let's talk about this more. > > > > Touch events should not be grabbed, they neither use the mouse cursor nor > > respect keyboard focus. > > > > Mouse press, drag, release events should. > > > > Key events probably should, since you typically cannot remove keyboard focus > > from the capturing window anyway. We haven't really implemented keyboard focus > > separately from what's under the cursor in this function, because ash doesn't > > care about it. > > > > Dunno about scroll events. Sadrul suggested they should not. It makes some > sense > > that scrolling always acts at the location under the cursor. However: What > about > > mouse wheel? It's a mouse event. Should it be consistent with scroll events, > or > > with mouse events? > > > > Chromium ScrollEvent seem to higher level kinds of events (built up on top of > the lower level ones like motion/press/release). > I wrote a small test on top of a toolkit running on X11. It shows the X11 > equivalent to MouseWheel events (ButtonPress with button number >= 4) are sent > during the implicit grab. So I would apply the same processing as mouse events. On ChromeOS touchpads directly generate ScrollEvents for 2 and 3-finger swipe gestures. sadrul: Can you clarify what you meant when you said we shouldn't grab scroll events? Why not? > > > > > > Gesture events are N/A don't go through this code path. > > > > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > > ui/ozone/platform/dri/dri_window.cc:127: if (event->IsLocatedEvent()) { > > As above, touch events must not be affected by the cursor location. > > Done.
https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:127: if (event->IsLocatedEvent()) { On 2014/11/12 15:47:24, llandwerlin wrote: > On 2014/11/11 19:00:24, spang wrote: > > As above, touch events must not be affected by the cursor location. > > They shouldn't be, because they will be delivered to the window on which the > cursor is, so the conditional statement > (window_manager_->cursor()->GetCursorWindow() != widget_) will fail. That conditional is only for mouse events. Touch events are for touchscreen devices and must be delivered to a window even if it is not under the cursor. The multi-window case wasn't implemented until recently but if you rebase, there's more code here to handle it. Anyway, you will break touch unless you to one of two things. (1) Rebase and update the new function RewriteTouchEvent to set root_location properly. (2) Change the conditional for this block to exclude touch events. https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:130: if (window_manager_->cursor()->GetCursorWindow() != widget_) { I don't think you need this conditional as the calculation is still correct when the cursor is over the window.
https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; On 2014/11/11 19:00:24, spang wrote: > Let's talk about this more. > > Touch events should not be grabbed, they neither use the mouse cursor nor > respect keyboard focus. I think we do grab for touch-events. Can you verify our behaviour on X11? > > Mouse press, drag, release events should. > > Key events probably should, since you typically cannot remove keyboard focus > from the capturing window anyway. We haven't really implemented keyboard focus > separately from what's under the cursor in this function, because ash doesn't > care about it. > > Dunno about scroll events. Sadrul suggested they should not. It makes some sense > that scrolling always acts at the location under the cursor. However: What about > mouse wheel? It's a mouse event. Should it be consistent with scroll events, or > with mouse events? We should grab scroll events too. I am not sure what I meant with my earlier comment. Perhaps I misread the code/comment and thought we were doing an implicit grab when we receive a scroll event. > > Gesture events are N/A don't go through this code path.
On 2014/11/19 21:07:10, sadrul wrote: > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > File ui/ozone/platform/dri/dri_window.cc (right): > > https://codereview.chromium.org/657603002/diff/220001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window.cc:114: return grabber == widget_; > On 2014/11/11 19:00:24, spang wrote: > > Let's talk about this more. > > > > Touch events should not be grabbed, they neither use the mouse cursor nor > > respect keyboard focus. > > I think we do grab for touch-events. Can you verify our behaviour on X11? Ah, you're totally right, we'll want the same behaviour for a touch drag. I was thinking of this as a cursor thing but that's not right at all. Ok, if we have a capturer, let's capture everything. > > > > > Mouse press, drag, release events should. > > > > Key events probably should, since you typically cannot remove keyboard focus > > from the capturing window anyway. We haven't really implemented keyboard focus > > separately from what's under the cursor in this function, because ash doesn't > > care about it. > > > > Dunno about scroll events. Sadrul suggested they should not. It makes some > sense > > that scrolling always acts at the location under the cursor. However: What > about > > mouse wheel? It's a mouse event. Should it be consistent with scroll events, > or > > with mouse events? > > We should grab scroll events too. I am not sure what I meant with my earlier > comment. Perhaps I misread the code/comment and thought we were doing an > implicit grab when we receive a scroll event. > > > > > Gesture events are N/A don't go through this code path.
As requested, let's capture everything. PTAL, Thanks.
lgtm https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:179: gfx::PointF(bounds_.origin().x(), bounds_.origin().y())); can't you just pass bounds_.origin()?
On 2014/11/21 19:20:26, spang wrote: > lgtm > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > File ui/ozone/platform/dri/dri_window.cc (right): > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > ui/ozone/platform/dri/dri_window.cc:179: gfx::PointF(bounds_.origin().x(), > bounds_.origin().y())); > can't you just pass bounds_.origin()? Sadly no, because bounds_ is gfx::Rect not gfx::RectF.
On 2014/11/21 19:22:29, llandwerlin wrote: > On 2014/11/21 19:20:26, spang wrote: > > lgtm > > > > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > > File ui/ozone/platform/dri/dri_window.cc (right): > > > > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > > ui/ozone/platform/dri/dri_window.cc:179: gfx::PointF(bounds_.origin().x(), > > bounds_.origin().y())); > > can't you just pass bounds_.origin()? > > Sadly no, because bounds_ is gfx::Rect not gfx::RectF. Sorry if I stir the waters again. You should be able to gfx::Point defines an operator for that.
On 2014/11/21 19:38:32, dnicoara wrote: > On 2014/11/21 19:22:29, llandwerlin wrote: > > On 2014/11/21 19:20:26, spang wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > > > File ui/ozone/platform/dri/dri_window.cc (right): > > > > > > > > > https://codereview.chromium.org/657603002/diff/280001/ui/ozone/platform/dri/d... > > > ui/ozone/platform/dri/dri_window.cc:179: gfx::PointF(bounds_.origin().x(), > > > bounds_.origin().y())); > > > can't you just pass bounds_.origin()? > > > > Sadly no, because bounds_ is gfx::Rect not gfx::RectF. > > Sorry if I stir the waters again. You should be able to gfx::Point defines an > operator for that. Thanks, I got confused with the +/- operators...
https://codereview.chromium.org/657603002/diff/300001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host_ozone.cc (right): https://codereview.chromium.org/657603002/diff/300001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_ozone.cc:72: } You should refactor this so you are not duplicating code from AshWindowTreeHostX11. https://codereview.chromium.org/657603002/diff/300001/ash/host/ash_window_tre... ash/host/ash_window_tree_host_ozone.cc:118: TranslateMouseEvent(static_cast<ui::LocatedEvent*>(event)); We do this translation for touch-events too on x11. Do we not need it here? https://codereview.chromium.org/657603002/diff/300001/ash/wm/drag_window_resi... File ash/wm/drag_window_resizer.cc (right): https://codereview.chromium.org/657603002/diff/300001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer.cc:151: GetTarget()->ReleaseCapture(); DnD and capture is relatively tricky. The changes in this file should be a separate CL (explaining why this is necessary) with sufficient test coverage. https://codereview.chromium.org/657603002/diff/300001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_ozone.h (right): https://codereview.chromium.org/657603002/diff/300001/ui/aura/window_tree_hos... ui/aura/window_tree_host_ozone.h:28: virtual gfx::Rect GetBounds() const override; Add // WindowTreeHost: before this line. https://codereview.chromium.org/657603002/diff/300001/ui/events/ozone/evdev/l... File ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc (right): https://codereview.chromium.org/657603002/diff/300001/ui/events/ozone/evdev/l... ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc:428: const gfx::PointF& root_loc = cursor_->GetRootLocation(); Do these need to be &? https://codereview.chromium.org/657603002/diff/300001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.cc (right): https://codereview.chromium.org/657603002/diff/300001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:179: if (event->IsLocatedEvent()) { Why do we rewrite the location of the touch-event twice? (once in RewriteTouchEvent(), and again here) https://codereview.chromium.org/657603002/diff/300001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.cc:184: location.Offset(-bounds_.origin().x(), -bounds_.origin().y()); Just bounds_.x(), bounds_.y(); https://codereview.chromium.org/657603002/diff/300001/ui/ozone/platform/dri/d... File ui/ozone/platform/dri/dri_window.h (right): https://codereview.chromium.org/657603002/diff/300001/ui/ozone/platform/dri/d... ui/ozone/platform/dri/dri_window.h:21: class Event; Doesn't look like this is necessary here?
PTAL Following spang's refactoring on master I can drop most of the stuff in ui/ and just add the missing translation of located events which is already used on X11.
lgtm https://codereview.chromium.org/657603002/diff/340001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host.h (right): https://codereview.chromium.org/657603002/diff/340001/ash/host/ash_window_tre... ash/host/ash_window_tree_host.h:66: virtual void TranslateLocatedEvent(ui::LocatedEvent* event); This should be non-virtual (and maybe protected too?)
Daniel, could you give your lgtm if you're still okay with the change? Thanks.
lgtm for ash with a suggestion https://codereview.chromium.org/657603002/diff/360001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host.cc (right): https://codereview.chromium.org/657603002/diff/360001/ash/host/ash_window_tre... ash/host/ash_window_tree_host.cc:15: if (!event->IsTouchEvent()) { nit: how about changing this to: if (event->IsTouchEvent()) return; // rest of method goes here ?
https://codereview.chromium.org/657603002/diff/360001/ash/host/ash_window_tre... File ash/host/ash_window_tree_host.cc (right): https://codereview.chromium.org/657603002/diff/360001/ash/host/ash_window_tre... ash/host/ash_window_tree_host.cc:15: if (!event->IsTouchEvent()) { On 2014/12/03 14:44:17, Daniel Erat wrote: > nit: how about changing this to: > > if (event->IsTouchEvent()) > return; > > // rest of method goes here > > ? Done.
The CQ bit was checked by lionel.g.landwerlin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657603002/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/164046d3753535889d3ba1f0060a0280a94bc1c7 Cr-Commit-Position: refs/heads/master@{#306623} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
