Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696873002:
Change OnWindowInputEvent to use display_id to find the host and update event root_location in WS. (Closed)
DescriptionChange WindowTreeClient::OnWindowInputEvent() to always use display_id
to find the host and use root window when window cannot be found.
Update event root_location in window server to be from the coordinate
system of the WmRoot to the coordinate system of the top-level window
of the window tree that we are going to send that event to.
This might happen if the window has already been deleted by the time
WindowTreeClient::OnWindowInputEvent() is called.
BUG=664617
TEST=aura_unittests mus_ws_unittests
Patch Set 1 #Patch Set 2 : helper function #
Total comments: 2
Patch Set 3 : host #
Total comments: 2
Patch Set 4 : root location #
Total comments: 4
Patch Set 5 : sequence #
Total comments: 2
Patch Set 6 : convert root_location in ws #Patch Set 7 : root window #
Total comments: 2
Patch Set 8 : TODO #
Total comments: 14
Patch Set 9 : sadrul@ comments #
Total comments: 8
Patch Set 10 : nits #
Total comments: 1
Messages
Total messages: 41 (13 generated)
Description was changed from ========== Change WindowTreeClient::OnWindowInputEvent() to use display_id to find the host when window cannot be found. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests ========== to ========== Change WindowTreeClient::OnWindowInputEvent() to use display_id to find the host when window cannot be found. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests ==========
Patchset #2 (id:20001) has been deleted
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
Hi sadrul@ and sky@, PTAL, thanks!
https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1159: host = GetWindowTreeHostMus(window); I don't think you're guaranteed the window has a host. For example, if between the dispatch and this function being called the window was removed then the host will be null. Can we always lookup the host?
On 2017/02/14 23:31:15, sky wrote: > https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... > File ui/aura/mus/window_tree_client.cc (right): > > https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... > ui/aura/mus/window_tree_client.cc:1159: host = GetWindowTreeHostMus(window); > I don't think you're guaranteed the window has a host. For example, if between > the dispatch and this function being called the window was removed then the host > will be null. Can we always lookup the host? That should be, can we always lookup the host using the display id?
https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1159: host = GetWindowTreeHostMus(window); On 2017/02/14 23:31:14, sky wrote: > I don't think you're guaranteed the window has a host. For example, if between > the dispatch and this function being called the window was removed then the host > will be null. Can we always lookup the host? Oh makes sense. I changed it to always lookup the host with display_id and use the root window if window was removed before this?
Description was changed from ========== Change WindowTreeClient::OnWindowInputEvent() to use display_id to find the host when window cannot be found. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests ========== to ========== Change WindowTreeClient::OnWindowInputEvent() to always use display_id to find the host and use root window when window cannot be found. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests ==========
https://codereview.chromium.org/2696873002/diff/60001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/60001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1156: window = WindowMus::Get(host->window()); If this happens I think you need to reset the event location too. The location should be the root_location.
https://codereview.chromium.org/2696873002/diff/60001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/60001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1156: window = WindowMus::Get(host->window()); On 2017/02/15 21:16:39, sky wrote: > If this happens I think you need to reset the event location too. The location > should be the root_location. Done. And updated test.
The CQ bit was checked by riajiang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1143: DCHECK(host); Remove this DCHECK given the conditional for !host on line 1132. https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1145: if (event->IsKeyEvent()) { Move this above 1136 as the conditional on 1136 is for obtaining the window and the window isn't in processing key events.
https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1143: DCHECK(host); On 2017/02/23 00:56:27, sky wrote: > Remove this DCHECK given the conditional for !host on line 1132. Done. https://codereview.chromium.org/2696873002/diff/80001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1145: if (event->IsKeyEvent()) { On 2017/02/23 00:56:27, sky wrote: > Move this above 1136 as the conditional on 1136 is for obtaining the window and > the window isn't in processing key events. Done.
https://codereview.chromium.org/2696873002/diff/100001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1135: } This could be: if (!window) { ... host = ...; window = WindowMus::Get(host->window()); } https://codereview.chromium.org/2696873002/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1150: } Why do this?
Description was changed from ========== Change WindowTreeClient::OnWindowInputEvent() to always use display_id to find the host and use root window when window cannot be found. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests ========== to ========== Change WindowTreeClient::OnWindowInputEvent() to always use display_id to find the host and use root window when window cannot be found. Update event root_location in window server to be from the coordinate system of the WmRoot to the coordinate system of the top-level window of the window tree that we are going to send that event to. This might happen if the window has already been deleted by the time WindowTreeClient::OnWindowInputEvent() is called. BUG=664617 TEST=aura_unittests mus_ws_unittests ==========
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Made the following changes to the previous patch: 1. Update event root_location in WS after we've found the window tree to send the event to. Change that to be from the coordinate system of the WM root to the coordinate system of the top-level window of the window tree that event is going to be sent to. Added test in mus_ws_unittests. 2. client-lib uses the display_id to find the window_tree_host and leave event->target() field empty if target window has already been deleted at that time so that EventProcessor can find the next best target window starting from its root window. Added test in aura_unittests. Please take another look, thanks!
https://codereview.chromium.org/2696873002/diff/180001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/180001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1842: WindowTreeHostMus* WindowTreeClient::GetWindowTreeHostMusWithDisplayId( It looks like tonikitoo/fwang have external mode sort of working and when that lands this function needs to only be used with internal window mode (there isn't a display_id per WTH in external mode). Maybe add a TODO or if there is something we can DCHECK on to make sure it's not called in external window mode?
Patchset #8 (id:200001) has been deleted
https://codereview.chromium.org/2696873002/diff/180001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/180001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1842: WindowTreeHostMus* WindowTreeClient::GetWindowTreeHostMusWithDisplayId( On 2017/03/13 15:08:14, kylechar wrote: > It looks like tonikitoo/fwang have external mode sort of working and when that > lands this function needs to only be used with internal window mode (there isn't > a display_id per WTH in external mode). Maybe add a TODO or if there is > something we can DCHECK on to make sure it's not called in external window mode? Added a TODO to find the correct WTH in external mode. (As discussed, it should be possible to distinguish between internal and external mode with |in_external_window_mode_| after https://codereview.chromium.org/2712203002/ is landed.)
https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... File services/ui/ws/test_change_tracker.cc (right): https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... services/ui/ws/test_change_tracker.cc:109: if (change.event_root_location != gfx::Point()) { Use PointF::IsOrigin() instead https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... services/ui/ws/test_change_tracker.cc:228: event_root_location(gfx::Point()), Don't need this https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:593: CHECK_LE(1u, target_tree->roots().size()); DCHECK_GE https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:595: const ServerWindow* target_window_tree_root = *target_tree->roots().begin(); I don't think these will always give you the right results. You should do something more like: const ServerWindow* wm_root = window_tree_->FindRootFor(target); const ServerWindow* target_root = target_tree->FindRootFor(target); DCHECK(wm_root); DCHECK(target_root); DCHECK(wm_root->Contains(target_root)); Where: const ServerWindow* WindowTree:FindRootFor(const ServerWindow* window) { for (const ServerWindow* parent = window; parent; parent = parent->parent()) { if (roots_.find(parent)) return parent; } return nullptr; } This ensures that you are using the right root from each tree.
https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1140: WindowMus* window = GetWindowByServerId(window_id); // May be null. Update the comment to explain when this can be null. https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1173: } Actually, can this happen in DispatchEventToTarget() instead? https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1350: return; Don't need return. When it's removed, don't need the {}
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... File services/ui/ws/test_change_tracker.cc (right): https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... services/ui/ws/test_change_tracker.cc:109: if (change.event_root_location != gfx::Point()) { On 2017/03/14 16:08:32, sadrul wrote: > Use PointF::IsOrigin() instead Done. https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/test_ch... services/ui/ws/test_change_tracker.cc:228: event_root_location(gfx::Point()), On 2017/03/14 16:08:32, sadrul wrote: > Don't need this Done. https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:593: CHECK_LE(1u, target_tree->roots().size()); On 2017/03/14 16:08:32, sadrul wrote: > DCHECK_GE Done. https://codereview.chromium.org/2696873002/diff/220001/services/ui/ws/window_... services/ui/ws/window_manager_state.cc:595: const ServerWindow* target_window_tree_root = *target_tree->roots().begin(); On 2017/03/14 16:08:32, sadrul wrote: > I don't think these will always give you the right results. You should do > something more like: > > const ServerWindow* wm_root = window_tree_->FindRootFor(target); > const ServerWindow* target_root = target_tree->FindRootFor(target); > DCHECK(wm_root); > DCHECK(target_root); > DCHECK(wm_root->Contains(target_root)); > > Where: > > const ServerWindow* WindowTree:FindRootFor(const ServerWindow* window) { > for (const ServerWindow* parent = window; > parent; parent = parent->parent()) { > if (roots_.find(parent)) > return parent; > } > return nullptr; > } > > This ensures that you are using the right root from each tree. Done. https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1140: WindowMus* window = GetWindowByServerId(window_id); // May be null. On 2017/03/14 16:12:17, sadrul wrote: > Update the comment to explain when this can be null. Done. https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1173: } On 2017/03/14 16:12:16, sadrul wrote: > Actually, can this happen in DispatchEventToTarget() instead? Done. https://codereview.chromium.org/2696873002/diff/220001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1350: return; On 2017/03/14 16:12:16, sadrul wrote: > Don't need return. When it's removed, don't need the {} Right! Done.
Mostly nits. lgtm https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:169: // the event to in EventProcessor. Merge this with the comment in 161:162 https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:174: event->AsLocatedEvent()->set_location( Actually, move the comment about changing the event-location here. Maybe something like: """The target window specified by the window server no longer exists. So it is necessary to find the target using the local event targeters. For that to work correctly, the event-location needs to be in the root window's coordinate space.""" https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1155: // OnWindowInputEvent is called, so we use the |display_id| to find the host s/so// https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1157: WindowTreeHostMus* host = GetWindowTreeHostMusWithDisplayId(display_id); Should this be: host = window ? window->GetHost() : GetWindowTreeHostMusWithDisplayId(display_id);
Hi sky@, could you take a look at services/ui? Thanks! https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:169: // the event to in EventProcessor. On 2017/03/15 01:21:17, sadrul wrote: > Merge this with the comment in 161:162 Done. https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:174: event->AsLocatedEvent()->set_location( On 2017/03/15 01:21:17, sadrul wrote: > Actually, move the comment about changing the event-location here. Maybe > something like: > > """The target window specified by the window server no longer exists. So it is > necessary to find the target using the local event targeters. For that to work > correctly, the event-location needs to be in the root window's coordinate > space.""" Done. https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1155: // OnWindowInputEvent is called, so we use the |display_id| to find the host On 2017/03/15 01:21:17, sadrul wrote: > s/so// Done. https://codereview.chromium.org/2696873002/diff/260001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1157: WindowTreeHostMus* host = GetWindowTreeHostMusWithDisplayId(display_id); On 2017/03/15 01:21:17, sadrul wrote: > Should this be: > > host = window ? window->GetHost() : > GetWindowTreeHostMusWithDisplayId(display_id); Done.
I think we should continue to pass in screen coordinates. To do otherwise means pointer watcher only events and non-pointer watcher events get different coordinates, which is confusing. I'm unclear as to why we need to supply events in the coordinates of the root of the client's tree. Could you elaborate on why that is necessary?
On 2017/03/15 19:20:51, sky wrote: > I think we should continue to pass in screen coordinates. To do otherwise means > pointer watcher only events and non-pointer watcher events get different > coordinates, which is confusing. > > I'm unclear as to why we need to supply events in the coordinates of the root of > the client's tree. Could you elaborate on why that is necessary? I'm not sure I understand the reason why pointer watcher only events and non-pointer watcher events would get different coordinates? We are not changing event-location in WS and the change about event-root-location should apply to all located events? Could you explain more about this? For updating event-root-location to be in the coordinates of the root of the client's tree - after we've found the correct window tree that event is going to be sent to in WS, at that point event-root-location is still in the coordinate space of the root of the ash window tree, but the correct coordinate space for that event-root-location after we've sent it to WindowTreeClient would be the root of the window tree that it is going be sent to? In the case of that target window has already been deleted by the time OnWindowInputEvent is called, we are going to use root window as the starting point to look for the next best target window in EventProcessor so that event should be in the root window's coordinate space.
On Wed, Mar 15, 2017 at 3:40 PM, <riajiang@chromium.org> wrote: > On 2017/03/15 19:20:51, sky wrote: >> I think we should continue to pass in screen coordinates. To do otherwise > means >> pointer watcher only events and non-pointer watcher events get different >> coordinates, which is confusing. >> >> I'm unclear as to why we need to supply events in the coordinates of the >> root > of >> the client's tree. Could you elaborate on why that is necessary? > > I'm not sure I understand the reason why pointer watcher only events and > non-pointer watcher events would get different coordinates? We are not > changing > event-location in WS and the change about event-root-location should apply > to > all located events? Could you explain more about this? Your patch modifies the location of the event and then calls DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may pass the event to other WS (for point watchers). The location you converted to is specific to a particular window tree, so that if the event is sent to another client the coordinates are unusual. Does that make sense? > > For updating event-root-location to be in the coordinates of the root of the > client's tree - after we've found the correct window tree that event is > going to > be sent to in WS, at that point event-root-location is still in the > coordinate > space of the root of the ash window tree, but the correct coordinate space > for > that event-root-location after we've sent it to WindowTreeClient would be > the > root of the window tree that it is going be sent to? In the case of that > target > window has already been deleted by the time OnWindowInputEvent is called, we > are > going to use root window as the starting point to look for the next best > target > window in EventProcessor so that event should be in the root window's > coordinate > space. If the root location is in screen coordinates isn't it converted appropriately? -Scott > > https://codereview.chromium.org/2696873002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/15 23:44:12, sky wrote: > On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: > > On 2017/03/15 19:20:51, sky wrote: > >> I think we should continue to pass in screen coordinates. To do otherwise > > means > >> pointer watcher only events and non-pointer watcher events get different > >> coordinates, which is confusing. > >> > >> I'm unclear as to why we need to supply events in the coordinates of the > >> root > > of > >> the client's tree. Could you elaborate on why that is necessary? > > > > I'm not sure I understand the reason why pointer watcher only events and > > non-pointer watcher events would get different coordinates? We are not > > changing > > event-location in WS and the change about event-root-location should apply > > to > > all located events? Could you explain more about this? > > Your patch modifies the location of the event and then calls > DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may > pass the event to other WS (for point watchers). The location you > converted to is specific to a particular window tree, so that if the > event is sent to another client the coordinates are unusual. Does that > make sense? I missed this in my review, sorry. Yeah, the conversion needs to happen right before sending the event to the client. I have left a comment below. > > For updating event-root-location to be in the coordinates of the root of the > > client's tree - after we've found the correct window tree that event is > > going to > > be sent to in WS, at that point event-root-location is still in the > > coordinate > > space of the root of the ash window tree, but the correct coordinate space > > for > > that event-root-location after we've sent it to WindowTreeClient would be > > the > > root of the window tree that it is going be sent to? In the case of that > > target > > window has already been deleted by the time OnWindowInputEvent is called, we > > are > > going to use root window as the starting point to look for the next best > > target > > window in EventProcessor so that event should be in the root window's > > coordinate > > space. > > If the root location is in screen coordinates isn't it converted appropriately? I don't believe so. For example, on other platforms (x11, w32), the root-location (and also the location) of a LocatedEvent is set to the local WTH root-windows's coordinate space [1]. And if we want to use local targeting, we need location to be set in the root-window's coord-space. [1]: https://cs.chromium.org/chromium/src/ui/events/event.cc?type=cs&sq=package:ch... https://codereview.chromium.org/2696873002/diff/280001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2696873002/diff/280001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1175: client()->OnWindowInputEvent( This is where the conversion should happen.
On Wed, Mar 15, 2017 at 6:35 PM, <sadrul@chromium.org> wrote: > On 2017/03/15 23:44:12, sky wrote: >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: >> > On 2017/03/15 19:20:51, sky wrote: >> >> I think we should continue to pass in screen coordinates. To do >> >> otherwise >> > means >> >> pointer watcher only events and non-pointer watcher events get >> >> different >> >> coordinates, which is confusing. >> >> >> >> I'm unclear as to why we need to supply events in the coordinates of >> >> the >> >> root >> > of >> >> the client's tree. Could you elaborate on why that is necessary? >> > >> > I'm not sure I understand the reason why pointer watcher only events and >> > non-pointer watcher events would get different coordinates? We are not >> > changing >> > event-location in WS and the change about event-root-location should >> > apply >> > to >> > all located events? Could you explain more about this? >> >> Your patch modifies the location of the event and then calls >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may >> pass the event to other WS (for point watchers). The location you >> converted to is specific to a particular window tree, so that if the >> event is sent to another client the coordinates are unusual. Does that >> make sense? > > I missed this in my review, sorry. Yeah, the conversion needs to happen > right > before sending the event to the client. I have left a comment below. > >> > For updating event-root-location to be in the coordinates of the root of >> > the >> > client's tree - after we've found the correct window tree that event is >> > going to >> > be sent to in WS, at that point event-root-location is still in the >> > coordinate >> > space of the root of the ash window tree, but the correct coordinate >> > space >> > for >> > that event-root-location after we've sent it to WindowTreeClient would >> > be >> > the >> > root of the window tree that it is going be sent to? In the case of that >> > target >> > window has already been deleted by the time OnWindowInputEvent is >> > called, we >> > are >> > going to use root window as the starting point to look for the next best >> > target >> > window in EventProcessor so that event should be in the root window's >> > coordinate >> > space. >> >> If the root location is in screen coordinates isn't it converted > appropriately? > > I don't believe so. For example, on other platforms (x11, w32), the > root-location (and also the location) of a LocatedEvent is set to the local > WTH > root-windows's coordinate space [1]. And if we want to use local targeting, > we > need location to be set in the root-window's coord-space. By root-window do you mean display-root? I believe Ria's patch was converting to the connections root, which is not necessarily that of the display-root. This would matter for embedded windows. -Scott > > [1]: > https://cs.chromium.org/chromium/src/ui/events/event.cc?type=cs&sq=package:ch... > > > https://codereview.chromium.org/2696873002/diff/280001/services/ui/ws/window_... > File services/ui/ws/window_tree.cc (right): > > https://codereview.chromium.org/2696873002/diff/280001/services/ui/ws/window_... > services/ui/ws/window_tree.cc:1175: client()->OnWindowInputEvent( > This is where the conversion should happen. > > https://codereview.chromium.org/2696873002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/16 02:47:07, sky wrote: > On Wed, Mar 15, 2017 at 6:35 PM, <mailto:sadrul@chromium.org> wrote: > > On 2017/03/15 23:44:12, sky wrote: > >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: > >> > On 2017/03/15 19:20:51, sky wrote: > >> >> I think we should continue to pass in screen coordinates. To do > >> >> otherwise > >> > means > >> >> pointer watcher only events and non-pointer watcher events get > >> >> different > >> >> coordinates, which is confusing. > >> >> > >> >> I'm unclear as to why we need to supply events in the coordinates of > >> >> the > >> >> root > >> > of > >> >> the client's tree. Could you elaborate on why that is necessary? > >> > > >> > I'm not sure I understand the reason why pointer watcher only events and > >> > non-pointer watcher events would get different coordinates? We are not > >> > changing > >> > event-location in WS and the change about event-root-location should > >> > apply > >> > to > >> > all located events? Could you explain more about this? > >> > >> Your patch modifies the location of the event and then calls > >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may > >> pass the event to other WS (for point watchers). The location you > >> converted to is specific to a particular window tree, so that if the > >> event is sent to another client the coordinates are unusual. Does that > >> make sense? > > > > I missed this in my review, sorry. Yeah, the conversion needs to happen > > right > > before sending the event to the client. I have left a comment below. > > > >> > For updating event-root-location to be in the coordinates of the root of > >> > the > >> > client's tree - after we've found the correct window tree that event is > >> > going to > >> > be sent to in WS, at that point event-root-location is still in the > >> > coordinate > >> > space of the root of the ash window tree, but the correct coordinate > >> > space > >> > for > >> > that event-root-location after we've sent it to WindowTreeClient would > >> > be > >> > the > >> > root of the window tree that it is going be sent to? In the case of that > >> > target > >> > window has already been deleted by the time OnWindowInputEvent is > >> > called, we > >> > are > >> > going to use root window as the starting point to look for the next best > >> > target > >> > window in EventProcessor so that event should be in the root window's > >> > coordinate > >> > space. > >> > >> If the root location is in screen coordinates isn't it converted > > appropriately? > > > > I don't believe so. For example, on other platforms (x11, w32), the > > root-location (and also the location) of a LocatedEvent is set to the local > > WTH > > root-windows's coordinate space [1]. And if we want to use local targeting, > > we > > need location to be set in the root-window's coord-space. > > By root-window do you mean display-root? I believe Ria's patch was > converting to the connections root, which is not necessarily that of > the display-root. I mean the root of the local window tree, i.e. the 'connections root', and not 'display root'. (local event-targeting happens in the local root's coordinate space). > This would matter for embedded windows. I am not sure what you are referring to here. If you are referring to renderer, then it does expect the location in screen-space, but the browser takes care of computing that from the local window-tree's root coord-space for the rest of the aura platforms [1]. So this should not be a problem there. Are there other embedded windows that would be affected by this? 1: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
Prior to Ria's patch the root_location was screen-coordinates. After Ria's patch the root_location is relative to the nearest root of the client. For example, lets say you're in chrome and one of Chrome's windows is at a location of 10,20 in the root. For an event occurring at a location of 5,6 if Chrome gets this event before Ria's change it would see 5,6, after Ria's change it sees -5, -14. I would expect it to continue seeing 5,6. I could certainly see making the root_location relative to the display it's on, but making it relative to the root seems confusing, especially for clients that have multiple roots, in which case how do they know which root it was relative to? -Scott On Thu, Mar 16, 2017 at 10:25 AM, <sadrul@chromium.org> wrote: > On 2017/03/16 02:47:07, sky wrote: > >> On Wed, Mar 15, 2017 at 6:35 PM, <mailto:sadrul@chromium.org> wrote: >> > On 2017/03/15 23:44:12, sky wrote: >> >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: >> >> > On 2017/03/15 19:20:51, sky wrote: >> >> >> I think we should continue to pass in screen coordinates. To do >> >> >> otherwise >> >> > means >> >> >> pointer watcher only events and non-pointer watcher events get >> >> >> different >> >> >> coordinates, which is confusing. >> >> >> >> >> >> I'm unclear as to why we need to supply events in the coordinates of >> >> >> the >> >> >> root >> >> > of >> >> >> the client's tree. Could you elaborate on why that is necessary? >> >> > >> >> > I'm not sure I understand the reason why pointer watcher only events >> >> > and >> >> > non-pointer watcher events would get different coordinates? We are >> >> > not >> >> > changing >> >> > event-location in WS and the change about event-root-location should >> >> > apply >> >> > to >> >> > all located events? Could you explain more about this? >> >> >> >> Your patch modifies the location of the event and then calls >> >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may >> >> pass the event to other WS (for point watchers). The location you >> >> converted to is specific to a particular window tree, so that if the >> >> event is sent to another client the coordinates are unusual. Does that >> >> make sense? >> > >> > I missed this in my review, sorry. Yeah, the conversion needs to happen >> > right >> > before sending the event to the client. I have left a comment below. >> > >> >> > For updating event-root-location to be in the coordinates of the root >> >> > of >> >> > the >> >> > client's tree - after we've found the correct window tree that event >> >> > is >> >> > going to >> >> > be sent to in WS, at that point event-root-location is still in the >> >> > coordinate >> >> > space of the root of the ash window tree, but the correct coordinate >> >> > space >> >> > for >> >> > that event-root-location after we've sent it to WindowTreeClient >> >> > would >> >> > be >> >> > the >> >> > root of the window tree that it is going be sent to? In the case of >> >> > that >> >> > target >> >> > window has already been deleted by the time OnWindowInputEvent is >> >> > called, we >> >> > are >> >> > going to use root window as the starting point to look for the next >> >> > best >> >> > target >> >> > window in EventProcessor so that event should be in the root window's >> >> > coordinate >> >> > space. >> >> >> >> If the root location is in screen coordinates isn't it converted >> > appropriately? >> > >> > I don't believe so. For example, on other platforms (x11, w32), the >> > root-location (and also the location) of a LocatedEvent is set to the >> > local >> > WTH >> > root-windows's coordinate space [1]. And if we want to use local >> > targeting, >> > we >> > need location to be set in the root-window's coord-space. >> >> By root-window do you mean display-root? I believe Ria's patch was >> converting to the connections root, which is not necessarily that of >> the display-root. > > I mean the root of the local window tree, i.e. the 'connections root', and > not > 'display root'. (local event-targeting happens in the local root's > coordinate > space). > >> This would matter for embedded windows. > > I am not sure what you are referring to here. If you are referring to > renderer, > then it does expect the location in screen-space, but the browser takes care > of > computing that from the local window-tree's root coord-space for the rest of > the > aura platforms [1]. So this should not be a problem there. Are there other > embedded windows that would be affected by this? Currently > > 1: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > https://codereview.chromium.org/2696873002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
But if there's an event occurring at 15,25 (that chrome window is still at 10,20) and we are going to send this event to that chrome window, in order for local targeting to work, that event should be converted to that window's coordinate space so the root-location should be 5,5? For that event that's occurring at 5,6, if it's going to be sent to that window at 10,20, -5,-14 is the correct location relative to the root that it is going to be sent to? When we get to WindowManagerState::DispatchInputEventToWindow, we get the client_id of the window tree that event is going to (say WT1) so we can start from the target window and walk up WT1 to find the nearest root even if that client have multiple roots? On 2017/03/16 20:19:53, sky wrote: > Prior to Ria's patch the root_location was screen-coordinates. After > Ria's patch the root_location is relative to the nearest root of the > client. For example, lets say you're in chrome and one of Chrome's > windows is at a location of 10,20 in the root. For an event occurring > at a location of 5,6 if Chrome gets this event before Ria's change it > would see 5,6, after Ria's change it sees -5, -14. I would expect it > to continue seeing 5,6. > > I could certainly see making the root_location relative to the display > it's on, but making it relative to the root seems confusing, > especially for clients that have multiple roots, in which case how do > they know which root it was relative to? > > -Scott > > On Thu, Mar 16, 2017 at 10:25 AM, <mailto:sadrul@chromium.org> wrote: > > On 2017/03/16 02:47:07, sky wrote: > > > >> On Wed, Mar 15, 2017 at 6:35 PM, <mailto:sadrul@chromium.org> wrote: > >> > On 2017/03/15 23:44:12, sky wrote: > >> >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: > >> >> > On 2017/03/15 19:20:51, sky wrote: > >> >> >> I think we should continue to pass in screen coordinates. To do > >> >> >> otherwise > >> >> > means > >> >> >> pointer watcher only events and non-pointer watcher events get > >> >> >> different > >> >> >> coordinates, which is confusing. > >> >> >> > >> >> >> I'm unclear as to why we need to supply events in the coordinates of > >> >> >> the > >> >> >> root > >> >> > of > >> >> >> the client's tree. Could you elaborate on why that is necessary? > >> >> > > >> >> > I'm not sure I understand the reason why pointer watcher only events > >> >> > and > >> >> > non-pointer watcher events would get different coordinates? We are > >> >> > not > >> >> > changing > >> >> > event-location in WS and the change about event-root-location should > >> >> > apply > >> >> > to > >> >> > all located events? Could you explain more about this? > >> >> > >> >> Your patch modifies the location of the event and then calls > >> >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may > >> >> pass the event to other WS (for point watchers). The location you > >> >> converted to is specific to a particular window tree, so that if the > >> >> event is sent to another client the coordinates are unusual. Does that > >> >> make sense? > >> > > >> > I missed this in my review, sorry. Yeah, the conversion needs to happen > >> > right > >> > before sending the event to the client. I have left a comment below. > >> > > >> >> > For updating event-root-location to be in the coordinates of the root > >> >> > of > >> >> > the > >> >> > client's tree - after we've found the correct window tree that event > >> >> > is > >> >> > going to > >> >> > be sent to in WS, at that point event-root-location is still in the > >> >> > coordinate > >> >> > space of the root of the ash window tree, but the correct coordinate > >> >> > space > >> >> > for > >> >> > that event-root-location after we've sent it to WindowTreeClient > >> >> > would > >> >> > be > >> >> > the > >> >> > root of the window tree that it is going be sent to? In the case of > >> >> > that > >> >> > target > >> >> > window has already been deleted by the time OnWindowInputEvent is > >> >> > called, we > >> >> > are > >> >> > going to use root window as the starting point to look for the next > >> >> > best > >> >> > target > >> >> > window in EventProcessor so that event should be in the root window's > >> >> > coordinate > >> >> > space. > >> >> > >> >> If the root location is in screen coordinates isn't it converted > >> > appropriately? > >> > > >> > I don't believe so. For example, on other platforms (x11, w32), the > >> > root-location (and also the location) of a LocatedEvent is set to the > >> > local > >> > WTH > >> > root-windows's coordinate space [1]. And if we want to use local > >> > targeting, > >> > we > >> > need location to be set in the root-window's coord-space. > >> > >> By root-window do you mean display-root? I believe Ria's patch was > >> converting to the connections root, which is not necessarily that of > >> the display-root. > > > > I mean the root of the local window tree, i.e. the 'connections root', and > > not > > 'display root'. (local event-targeting happens in the local root's > > coordinate > > space). > > > >> This would matter for embedded windows. > > > > I am not sure what you are referring to here. If you are referring to > > renderer, > > then it does expect the location in screen-space, but the browser takes care > > of > > computing that from the local window-tree's root coord-space for the rest of > > the > > aura platforms [1]. So this should not be a problem there. Are there other > > embedded windows that would be affected by this? > > Currently > > > > > 1: > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > > > > https://codereview.chromium.org/2696873002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
The client should have all the information it needs to do this. Chrome knows the bounds of the Chrome window, right? Each WindowTreeHostMus has a bounds associated with it. This works for Chrome windows, because the parent is always in display coordinates. For deeper embeddings that isn't the case. That deeper embeddings don't know their screen bounds is a known bug: 661270. Once that's fixed clients should have enough information to do the conversion you mention while keeping the root in screen (or display) coordinates. I'm not sure I understand your example below. In the patch (window_manager_state.cc line 599) you are making the coordinates relative to one of the roots of the client. If the client can't find the window supplied to OnWindowInputEvent how does it know which root the coordinates are relative to? Please correct me if I'm wrong, but the bug you are trying to fix is how should a client handle an event when it no longer has the window. In that case the client needs to know the root window that contained the window the event was for. Your patch doesn't do that. Your patch converts coordinates, which isn't the same thing. If you change OnWindowInputMethod to include the id of the root of the window the event is targeted at, then I think WindowTreeClient has all the information it needs without changing the coordinates passed to the client. By that I specifically mean if there is no window then WindowTreeClient::OnWindowInputEvent uses the root window as the target, and resets the location (not the root location, just the location) based on converting the root_location (in screen coordinates) to the windows coordinates. To do that second operation for deeply embedded clients requires 661270 to be fixed, but that can be ignored at the moment. Does this make sense? I'm happy to have an IM or VC if this still doesn't make sense, or if I'm still confused. -Scott On Thu, Mar 16, 2017 at 2:36 PM, <riajiang@chromium.org> wrote: > But if there's an event occurring at 15,25 (that chrome window is still at > 10,20) and we are going to send this event to that chrome window, in order > for > local targeting to work, that event should be converted to that window's > coordinate space so the root-location should be 5,5? For that event that's > occurring at 5,6, if it's going to be sent to that window at 10,20, -5,-14 > is > the correct location relative to the root that it is going to be sent to? > > When we get to WindowManagerState::DispatchInputEventToWindow, we get the > client_id of the window tree that event is going to (say WT1) so we can > start > from the target window and walk up WT1 to find the nearest root even if that > client have multiple roots? > > On 2017/03/16 20:19:53, sky wrote: >> Prior to Ria's patch the root_location was screen-coordinates. After >> Ria's patch the root_location is relative to the nearest root of the >> client. For example, lets say you're in chrome and one of Chrome's >> windows is at a location of 10,20 in the root. For an event occurring >> at a location of 5,6 if Chrome gets this event before Ria's change it >> would see 5,6, after Ria's change it sees -5, -14. I would expect it >> to continue seeing 5,6. >> >> I could certainly see making the root_location relative to the display >> it's on, but making it relative to the root seems confusing, >> especially for clients that have multiple roots, in which case how do >> they know which root it was relative to? >> >> -Scott >> >> On Thu, Mar 16, 2017 at 10:25 AM, <mailto:sadrul@chromium.org> wrote: >> > On 2017/03/16 02:47:07, sky wrote: >> > >> >> On Wed, Mar 15, 2017 at 6:35 PM, <mailto:sadrul@chromium.org> wrote: >> >> > On 2017/03/15 23:44:12, sky wrote: >> >> >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> >> >> >> wrote: >> >> >> > On 2017/03/15 19:20:51, sky wrote: >> >> >> >> I think we should continue to pass in screen coordinates. To do >> >> >> >> otherwise >> >> >> > means >> >> >> >> pointer watcher only events and non-pointer watcher events get >> >> >> >> different >> >> >> >> coordinates, which is confusing. >> >> >> >> >> >> >> >> I'm unclear as to why we need to supply events in the coordinates >> >> >> >> of >> >> >> >> the >> >> >> >> root >> >> >> > of >> >> >> >> the client's tree. Could you elaborate on why that is necessary? >> >> >> > >> >> >> > I'm not sure I understand the reason why pointer watcher only >> >> >> > events >> >> >> > and >> >> >> > non-pointer watcher events would get different coordinates? We are >> >> >> > not >> >> >> > changing >> >> >> > event-location in WS and the change about event-root-location >> >> >> > should >> >> >> > apply >> >> >> > to >> >> >> > all located events? Could you explain more about this? >> >> >> >> >> >> Your patch modifies the location of the event and then calls >> >> >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() >> >> >> may >> >> >> pass the event to other WS (for point watchers). The location you >> >> >> converted to is specific to a particular window tree, so that if the >> >> >> event is sent to another client the coordinates are unusual. Does >> >> >> that >> >> >> make sense? >> >> > >> >> > I missed this in my review, sorry. Yeah, the conversion needs to >> >> > happen >> >> > right >> >> > before sending the event to the client. I have left a comment below. >> >> > >> >> >> > For updating event-root-location to be in the coordinates of the >> >> >> > root >> >> >> > of >> >> >> > the >> >> >> > client's tree - after we've found the correct window tree that >> >> >> > event >> >> >> > is >> >> >> > going to >> >> >> > be sent to in WS, at that point event-root-location is still in >> >> >> > the >> >> >> > coordinate >> >> >> > space of the root of the ash window tree, but the correct >> >> >> > coordinate >> >> >> > space >> >> >> > for >> >> >> > that event-root-location after we've sent it to WindowTreeClient >> >> >> > would >> >> >> > be >> >> >> > the >> >> >> > root of the window tree that it is going be sent to? In the case >> >> >> > of >> >> >> > that >> >> >> > target >> >> >> > window has already been deleted by the time OnWindowInputEvent is >> >> >> > called, we >> >> >> > are >> >> >> > going to use root window as the starting point to look for the >> >> >> > next >> >> >> > best >> >> >> > target >> >> >> > window in EventProcessor so that event should be in the root >> >> >> > window's >> >> >> > coordinate >> >> >> > space. >> >> >> >> >> >> If the root location is in screen coordinates isn't it converted >> >> > appropriately? >> >> > >> >> > I don't believe so. For example, on other platforms (x11, w32), the >> >> > root-location (and also the location) of a LocatedEvent is set to the >> >> > local >> >> > WTH >> >> > root-windows's coordinate space [1]. And if we want to use local >> >> > targeting, >> >> > we >> >> > need location to be set in the root-window's coord-space. >> >> >> >> By root-window do you mean display-root? I believe Ria's patch was >> >> converting to the connections root, which is not necessarily that of >> >> the display-root. >> > >> > I mean the root of the local window tree, i.e. the 'connections root', >> > and >> > not >> > 'display root'. (local event-targeting happens in the local root's >> > coordinate >> > space). >> > >> >> This would matter for embedded windows. >> > >> > I am not sure what you are referring to here. If you are referring to >> > renderer, >> > then it does expect the location in screen-space, but the browser takes >> > care >> > of >> > computing that from the local window-tree's root coord-space for the >> > rest of >> > the >> > aura platforms [1]. So this should not be a problem there. Are there >> > other >> > embedded windows that would be affected by this? >> >> Currently >> >> > >> > 1: >> > >> > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... >> > >> > https://codereview.chromium.org/2696873002/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> 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/2696873002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ria and I discussed this offline a little bit, and I have some additional concern: I think you are right that for the client to be able to reliably re-target (when the target-window sent by the window-server has already been locally destroyed), the server needs to send the root-window (instead of/in addition to the display-id) to the client too. However, if the root window is also destroyed, then this becomes a lot more difficult. For example, consider client A has two top-level windows W1, W2, and client B has a top-level window W3, and they exactly overlap each other, and the stacking order is W1 is at the top, and W2 at the bottom. In this config, if a user clicks on top of W1, then mus-ws sends W1's root to the client. If, however, the client has destroyed W1 locally, and it still tries to re-target the event, then it doesn't have enough information to know that W2 should not receive the event (because unbeknownst to A, there's a window W3 covering W2). So, I think the safest option is to use the event to clear out local states (e.g. notify Env() that the mouse is not down anymore on a mouse-release event, and so on), but not actually dispatch the event to any target, when the target window has been destroyed. Does that make sense? WDYT?
SGTM On Thu, Mar 16, 2017 at 1:19 PM, Scott Violet <sky@chromium.org> wrote: > Prior to Ria's patch the root_location was screen-coordinates. After > Ria's patch the root_location is relative to the nearest root of the > client. For example, lets say you're in chrome and one of Chrome's > windows is at a location of 10,20 in the root. For an event occurring > at a location of 5,6 if Chrome gets this event before Ria's change it > would see 5,6, after Ria's change it sees -5, -14. I would expect it > to continue seeing 5,6. > > I could certainly see making the root_location relative to the display > it's on, but making it relative to the root seems confusing, > especially for clients that have multiple roots, in which case how do > they know which root it was relative to? > > -Scott > > On Thu, Mar 16, 2017 at 10:25 AM, <sadrul@chromium.org> wrote: >> On 2017/03/16 02:47:07, sky wrote: >> >>> On Wed, Mar 15, 2017 at 6:35 PM, <mailto:sadrul@chromium.org> wrote: >>> > On 2017/03/15 23:44:12, sky wrote: >>> >> On Wed, Mar 15, 2017 at 3:40 PM, <mailto:riajiang@chromium.org> wrote: >>> >> > On 2017/03/15 19:20:51, sky wrote: >>> >> >> I think we should continue to pass in screen coordinates. To do >>> >> >> otherwise >>> >> > means >>> >> >> pointer watcher only events and non-pointer watcher events get >>> >> >> different >>> >> >> coordinates, which is confusing. >>> >> >> >>> >> >> I'm unclear as to why we need to supply events in the coordinates of >>> >> >> the >>> >> >> root >>> >> > of >>> >> >> the client's tree. Could you elaborate on why that is necessary? >>> >> > >>> >> > I'm not sure I understand the reason why pointer watcher only events >>> >> > and >>> >> > non-pointer watcher events would get different coordinates? We are >>> >> > not >>> >> > changing >>> >> > event-location in WS and the change about event-root-location should >>> >> > apply >>> >> > to >>> >> > all located events? Could you explain more about this? >>> >> >>> >> Your patch modifies the location of the event and then calls >>> >> DispatchInputEventToWindowImpl(). DispatchInputEventToWindowImpl() may >>> >> pass the event to other WS (for point watchers). The location you >>> >> converted to is specific to a particular window tree, so that if the >>> >> event is sent to another client the coordinates are unusual. Does that >>> >> make sense? >>> > >>> > I missed this in my review, sorry. Yeah, the conversion needs to happen >>> > right >>> > before sending the event to the client. I have left a comment below. >>> > >>> >> > For updating event-root-location to be in the coordinates of the root >>> >> > of >>> >> > the >>> >> > client's tree - after we've found the correct window tree that event >>> >> > is >>> >> > going to >>> >> > be sent to in WS, at that point event-root-location is still in the >>> >> > coordinate >>> >> > space of the root of the ash window tree, but the correct coordinate >>> >> > space >>> >> > for >>> >> > that event-root-location after we've sent it to WindowTreeClient >>> >> > would >>> >> > be >>> >> > the >>> >> > root of the window tree that it is going be sent to? In the case of >>> >> > that >>> >> > target >>> >> > window has already been deleted by the time OnWindowInputEvent is >>> >> > called, we >>> >> > are >>> >> > going to use root window as the starting point to look for the next >>> >> > best >>> >> > target >>> >> > window in EventProcessor so that event should be in the root window's >>> >> > coordinate >>> >> > space. >>> >> >>> >> If the root location is in screen coordinates isn't it converted >>> > appropriately? >>> > >>> > I don't believe so. For example, on other platforms (x11, w32), the >>> > root-location (and also the location) of a LocatedEvent is set to the >>> > local >>> > WTH >>> > root-windows's coordinate space [1]. And if we want to use local >>> > targeting, >>> > we >>> > need location to be set in the root-window's coord-space. >>> >>> By root-window do you mean display-root? I believe Ria's patch was >>> converting to the connections root, which is not necessarily that of >>> the display-root. >> >> I mean the root of the local window tree, i.e. the 'connections root', and >> not >> 'display root'. (local event-targeting happens in the local root's >> coordinate >> space). >> >>> This would matter for embedded windows. >> >> I am not sure what you are referring to here. If you are referring to >> renderer, >> then it does expect the location in screen-space, but the browser takes care >> of >> computing that from the local window-tree's root coord-space for the rest of >> the >> aura platforms [1]. So this should not be a problem there. Are there other >> embedded windows that would be affected by this? > > Currently > >> >> 1: >> https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... >> >> https://codereview.chromium.org/2696873002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
