Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779093004:
Update event states in Env when target window has been destroyed. (Closed)
DescriptionUpdate event states in Env when target window has been destroyed.
Based on discussions in https://codereview.chromium.org/2696873002, in
WindowTreeClient::OnWindowInputEvent(), if the target window provided by
window server has already been destroyed by the time client-lib got the
event, we should use the event to update global states related to
MouseEvent and TouchEvent kept in Env() and not dispatch the event to
any window.
BUG=664617
TEST=aura_unittests
Review-Url: https://codereview.chromium.org/2779093004
Cr-Commit-Position: refs/heads/master@{#468893}
Committed: https://chromium.googlesource.com/chromium/src/+/09e6e887d03585ba7080c3cbcd5222d119502fff
Patch Set 1 #Patch Set 2 : test #
Total comments: 6
Patch Set 3 : UNHANDLED; mapped_event #Patch Set 4 : rebase #Patch Set 5 : rebase #
Total comments: 4
Patch Set 6 : window null #
Total comments: 4
Patch Set 7 : get EnvInputStateController from Env #
Messages
Total messages: 42 (30 generated)
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...
riajiang@chromium.org changed reviewers: + sadrul@chromium.org
Hi sadrul@, could you take a look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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.
Looks good. A couple of comments https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1219: tree_->OnWindowInputEventAck(event_id, ui::mojom::EventResult::HANDLED); I actually think this should be UNHANDLED. (because otherwise, if there are some post-target accelerators, those will not be triggered). https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1258: } Two comments: . We should not receive MouseEvent or TouchEvent from the window server, right? So you shouldn't need those two cases. . Let's move this code into EnvInputStateController::UpdateStateForPointerEvent(), and do the necessary work in there. https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1259: tree_->OnWindowInputEventAck(event_id, ui::mojom::EventResult::HANDLED); UNHANDLED.
https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1219: tree_->OnWindowInputEventAck(event_id, ui::mojom::EventResult::HANDLED); On 2017/03/31 05:10:51, sadrul wrote: > I actually think this should be UNHANDLED. (because otherwise, if there are some > post-target accelerators, those will not be triggered). Right! Done. https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1258: } On 2017/03/31 05:10:51, sadrul wrote: > Two comments: > . We should not receive MouseEvent or TouchEvent from the window server, right? > So you shouldn't need those two cases. > > . Let's move this code into > EnvInputStateController::UpdateStateForPointerEvent(), and do the necessary work > in there. As discussed offline, going to use the MapEvent helper function from CL https://codereview.chromium.org/2792573003. https://codereview.chromium.org/2779093004/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:1259: tree_->OnWindowInputEventAck(event_id, ui::mojom::EventResult::HANDLED); On 2017/03/31 05:10:51, sadrul wrote: > UNHANDLED. Done.
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.
The CQ bit was checked by riajiang@chromium.org to run a CQ dry run
ping (rebased)
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.
lgtm https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... File ui/aura/env_input_state_controller.cc (right): https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... ui/aura/env_input_state_controller.cc:30: // we shouldn't update mouse location. Can you move this into SetLastMouseLocation() instead? https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... ui/aura/env_input_state_controller.cc:68: client::ScreenPositionClient* client = Early return here if window is nullptr (but only for MUS).
https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... File ui/aura/env_input_state_controller.cc (right): https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... ui/aura/env_input_state_controller.cc:30: // we shouldn't update mouse location. On 2017/05/01 15:27:55, sadrul wrote: > Can you move this into SetLastMouseLocation() instead? Done. https://codereview.chromium.org/2779093004/diff/80001/ui/aura/env_input_state... ui/aura/env_input_state_controller.cc:68: client::ScreenPositionClient* client = On 2017/05/01 15:27:55, sadrul wrote: > Early return here if window is nullptr (but only for MUS). Done.
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.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1313: GetWindowTreeHostMus(*roots_.begin())->dispatcher()->env_controller(); How do you know roots_ is not empty here?
https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1309: // aura::EnvInputStateController from since these are global states. Also, it may make sense to have Env own a single instance of EnvInputStateController.
https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1309: // aura::EnvInputStateController from since these are global states. On 2017/05/01 17:39:36, sadrul wrote: > Also, it may make sense to have Env own a single instance of > EnvInputStateController. Done in https://codereview.chromium.org/2854663002/. https://codereview.chromium.org/2779093004/diff/100001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1313: GetWindowTreeHostMus(*roots_.begin())->dispatcher()->env_controller(); On 2017/05/01 17:37:27, sky wrote: > How do you know roots_ is not empty here? Yes sorry didn't consider that case. Changed EnvInputStateController to be owned by Env so we can get EnvInputStateController from Env here.
LGTM
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.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2779093004/#ps120001 (title: "get EnvInputStateController from Env")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493785809084140,
"parent_rev": "602bf8bca1a8b785948667a07f5782288c0ab160", "commit_rev":
"09e6e887d03585ba7080c3cbcd5222d119502fff"}
Message was sent while issue was closed.
Description was changed from ========== Update event states in Env when target window has been destroyed. Based on discussions in https://codereview.chromium.org/2696873002, in WindowTreeClient::OnWindowInputEvent(), if the target window provided by window server has already been destroyed by the time client-lib got the event, we should use the event to update global states related to MouseEvent and TouchEvent kept in Env() and not dispatch the event to any window. BUG=664617 TEST=aura_unittests ========== to ========== Update event states in Env when target window has been destroyed. Based on discussions in https://codereview.chromium.org/2696873002, in WindowTreeClient::OnWindowInputEvent(), if the target window provided by window server has already been destroyed by the time client-lib got the event, we should use the event to update global states related to MouseEvent and TouchEvent kept in Env() and not dispatch the event to any window. BUG=664617 TEST=aura_unittests Review-Url: https://codereview.chromium.org/2779093004 Cr-Commit-Position: refs/heads/master@{#468893} Committed: https://chromium.googlesource.com/chromium/src/+/09e6e887d03585ba7080c3cbcd52... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/09e6e887d03585ba7080c3cbcd52... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
