Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025843003:
Introduce aura::Env controller to abstract away changes to aura::Env
|
Created:
4 years, 6 months ago by Mark Dittmer Modified:
4 years, 6 months ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, Fady Samuel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce aura::Env controller to abstract away changes to aura::Env
R=sadrul@chromium.org
BUG=616101
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add missing files #
Total comments: 6
Patch Set 3 : Address review comments #Patch Set 4 : Add new files to gyp build #
Total comments: 2
Patch Set 5 : Refactor to UpdateStateFor...Event controller API #Patch Set 6 : Fix extant aura_unnitest failures: Re-introduce custom logic in two places. #
Total comments: 5
Patch Set 7 : Re-introduce SetLastMouseLocation in PreDispatchMouseEvent, but run it unconditionally #Patch Set 8 : Add EF_IS_SYNTHESIZED condition to call in PreDispatchMouseEvent; fixes WindowEventDispatcherTest.S… #
Depends on Patchset: Messages
Total messages: 34 (8 generated)
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2025843003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2025843003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Add missing files
https://codereview.chromium.org/2025843003/diff/1/ui/aura/BUILD.gn File ui/aura/BUILD.gn (right): https://codereview.chromium.org/2025843003/diff/1/ui/aura/BUILD.gn#newcode77 ui/aura/BUILD.gn:77: "window_event_dispatcher_delegate.h", You need to add the files to rietveld I wouldn't call these WEDDelegate though. Perhaps EnvInputStateController or something like that?
https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher.cc:865: touch_ids_down_ = (touch_ids_down_ | (1 << event->touch_id())) ^ |touch_ids_down_| should move into the <new-thing> https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher_delegate.h (right): https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher_delegate.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. We don't do (c) no more https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher_delegate.h:34: bool IsMouseButtonDown() const; I don't think this should have any of the getters. Its responsibility would be to update the state in Env. The rest of the code that currently reads the state from Env should continue to do so.
Address review comments
Comments addressed. sadrul@chromium.org: PTAL. https://codereview.chromium.org/2025843003/diff/1/ui/aura/BUILD.gn File ui/aura/BUILD.gn (right): https://codereview.chromium.org/2025843003/diff/1/ui/aura/BUILD.gn#newcode77 ui/aura/BUILD.gn:77: "window_event_dispatcher_delegate.h", On 2016/05/31 15:57:45, sadrul wrote: > You need to add the files to rietveld > > I wouldn't call these WEDDelegate though. Perhaps EnvInputStateController or > something like that? Done. https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher_delegate.h (right): https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher_delegate.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/31 16:36:32, sadrul wrote: > We don't do (c) no more Done. https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher_delegate.h:34: bool IsMouseButtonDown() const; On 2016/05/31 16:36:32, sadrul wrote: > I don't think this should have any of the getters. Its responsibility would be > to update the state in Env. The rest of the code that currently reads the state > from Env should continue to do so. Done.
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2025843003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2025843003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Add new files to gyp build
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2025843003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2025843003/60001
Description was changed from ========== Introduce WindowEventDispatcherDelegate to abstract away operations the require aura::Env R=sadrul@chromium.org BUG=616101 ========== to ========== Introduce WindowEventDispatcherDelegate to abstract away operations the require aura::Env R=sadrul@chromium.org BUG=616101 ==========
Description was changed from ========== Introduce WindowEventDispatcherDelegate to abstract away operations the require aura::Env R=sadrul@chromium.org BUG=616101 ========== to ========== Introduce aura::Env controller to abstract away changes to aura::Env R=sadrul@chromium.org BUG=616101 ==========
https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/2025843003/diff/20001/ui/aura/window_event_di... ui/aura/window_event_dispatcher.cc:865: touch_ids_down_ = (touch_ids_down_ | (1 << event->touch_id())) ^ On 2016/05/31 16:36:32, sadrul wrote: > |touch_ids_down_| should move into the <new-thing> ^ this https://codereview.chromium.org/2025843003/diff/60001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/60001/ui/aura/window_event_di... ui/aura/window_event_dispatcher.cc:213: } Let's keep this here, and remove that from env_controller_ https://codereview.chromium.org/2025843003/diff/60001/ui/aura/window_event_di... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/2025843003/diff/60001/ui/aura/window_event_di... ui/aura/window_event_dispatcher.cc:846: env_controller_->PreDispatchMouseEvent(event); Looking at all the various places that explicitly call SetLastMouseLocation(), I am thinking it would be tedious to replicate all of that in NativeWidgetMus. The ideal api for EnvInputStateController would be just 'UpdateStateForMouseEvent(mouse)' and 'UpdateStateForTouchEvent(touch)'. Can you try updating the |Env::last_mouse_location_| from EnvInputStateController::UpdateStateForMouseEvent(), and see which tests break?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Refactor to UpdateStateFor...Event controller API
Fix extant aura_unnitest failures: Re-introduce custom logic in two places.
On 2016/06/02 19:25:18, Mark Dittmer wrote: > Fix extant aura_unnitest failures: Re-introduce custom logic in two places. Looks like one test in aura_unittests is still failing with the most recent patchset. Do we understand that failure?
On 2016/06/07 03:14:48, sadrul wrote: > On 2016/06/02 19:25:18, Mark Dittmer wrote: > > Fix extant aura_unnitest failures: Re-introduce custom logic in two places. > > Looks like one test in aura_unittests is still failing with the most recent > patchset. Do we understand that failure? That test: WindowEventDispatcherTest.MouseMovesHeld, appears to fail on ToT.
On 2016/06/07 14:17:10, Mark Dittmer wrote: > On 2016/06/07 03:14:48, sadrul wrote: > > On 2016/06/02 19:25:18, Mark Dittmer wrote: > > > Fix extant aura_unnitest failures: Re-introduce custom logic in two places. > > > > Looks like one test in aura_unittests is still failing with the most recent > > patchset. Do we understand that failure? > > That test: WindowEventDispatcherTest.MouseMovesHeld, appears to fail on ToT. Any idea why the failure isn't showing up on the buildbots/other tries?
Discovered source of aura unit test failure, but still do not understand the meaning of the code and test well enough to suggest a more parsimonious solution. https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:779: if (!(event->flags() & ui::EF_IS_SYNTHESIZED) && I do not yet understand why this is the case, but removing this code broke WindowEventDispatcherTest.MouseMovesHeld. Adding it back (calling via env_controller_) fixes the test.
https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:779: if (!(event->flags() & ui::EF_IS_SYNTHESIZED) && On 2016/06/08 15:31:30, Mark Dittmer wrote: > I do not yet understand why this is the case, but removing this code broke > WindowEventDispatcherTest.MouseMovesHeld. Adding it back (calling via > env_controller_) fixes the test. Perhaps we can move the call to |env_controller_->UpdateStateForMouseEvent(window(), event);| before this, for example, in line ~776.
https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:779: if (!(event->flags() & ui::EF_IS_SYNTHESIZED) && On 2016/06/08 17:07:27, sadrul wrote: > On 2016/06/08 15:31:30, Mark Dittmer wrote: > > I do not yet understand why this is the case, but removing this code broke > > WindowEventDispatcherTest.MouseMovesHeld. Adding it back (calling via > > env_controller_) fixes the test. > > Perhaps we can move the call to > |env_controller_->UpdateStateForMouseEvent(window(), event);| before this, for > example, in line ~776. Not sure I quite understand. Are you advocating that we drop the three layers of conditionals around |SetLastMouseLocation|? Line 776 appears to mean "here, but outside the 3 if's".
https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:779: if (!(event->flags() & ui::EF_IS_SYNTHESIZED) && On 2016/06/09 15:17:10, Mark Dittmer wrote: > On 2016/06/08 17:07:27, sadrul wrote: > > On 2016/06/08 15:31:30, Mark Dittmer wrote: > > > I do not yet understand why this is the case, but removing this code broke > > > WindowEventDispatcherTest.MouseMovesHeld. Adding it back (calling via > > > env_controller_) fixes the test. > > > > Perhaps we can move the call to > > |env_controller_->UpdateStateForMouseEvent(window(), event);| before this, for > > example, in line ~776. > > Not sure I quite understand. Are you advocating that we drop the three layers of > conditionals around |SetLastMouseLocation|? Line 776 appears to mean "here, but > outside the 3 if's". Yeah. The rest of the early returns seem to be related to destruction of a window (or the dispatcher itself) on dispatching an enter/exit event, which should probably still update the system mouse-location. So yeah, I would try that out.
Re-introduce SetLastMouseLocation in PreDispatchMouseEvent, but run it unconditionally
https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... File ui/aura/window_event_dispatcher.cc (left): https://codereview.chromium.org/2025843003/diff/100001/ui/aura/window_event_d... ui/aura/window_event_dispatcher.cc:779: if (!(event->flags() & ui::EF_IS_SYNTHESIZED) && On 2016/06/09 15:21:40, sadrul wrote: > On 2016/06/09 15:17:10, Mark Dittmer wrote: > > On 2016/06/08 17:07:27, sadrul wrote: > > > On 2016/06/08 15:31:30, Mark Dittmer wrote: > > > > I do not yet understand why this is the case, but removing this code broke > > > > WindowEventDispatcherTest.MouseMovesHeld. Adding it back (calling via > > > > env_controller_) fixes the test. > > > > > > Perhaps we can move the call to > > > |env_controller_->UpdateStateForMouseEvent(window(), event);| before this, > for > > > example, in line ~776. > > > > Not sure I quite understand. Are you advocating that we drop the three layers > of > > conditionals around |SetLastMouseLocation|? Line 776 appears to mean "here, > but > > outside the 3 if's". > > Yeah. The rest of the early returns seem to be related to destruction of a > window (or the dispatcher itself) on dispatching an enter/exit event, which > should probably still update the system mouse-location. So yeah, I would try > that out. Looks like no luck (see recent upload). WindowEventDispatcherTest.SynthesizedLocatedEvent breaks, so the check for EF_IS_SYNTHESIZED, at the least, is still necessary.
Add EF_IS_SYNTHESIZED condition to call in PreDispatchMouseEvent; fixes WindowEventDispatcherTest.SynthesizedLocatedEvent |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
