Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394833004:
Split View Mode: Support for the 2-finger bezel scroll. (Closed)
|
Created:
6 years, 5 months ago by mfomitchev Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSplit View Mode: Support for the 2-finger bezel scroll.
Implementing the BezelController class responsible for detecting bezel gestures.
Adding the bare skeleton of the SplitViewController class which is going to be
responsible for the split view mode.
BUG=383421
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285166
Patch Set 1 #Patch Set 2 : Removing accidental edit. #
Total comments: 18
Patch Set 3 : Addressing review feedback + "git cl format" #Patch Set 4 : Fixing scroll update. #
Total comments: 17
Patch Set 5 : Rebase #Patch Set 6 : Addressing oshima's feedback. #Patch Set 7 : Fixing a rebase issue. #
Total comments: 25
Patch Set 8 : Addressing feedback. #
Total comments: 4
Patch Set 9 : Getting rid of OnTouchEvent, num_fingers_down_ #
Messages
Total messages: 35 (0 generated)
https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:36: nit: remove this blank line https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:74: nit: remove this blank line https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:80: // TODO: Currently we aren't retargetting or consuming any of the touch TODO should have the owner name. TODO(mfomitchev): ... for example https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:113: //NOTREACHED(); I think it's okay to be NOTREACHED(). Anyways, please keep either. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:127: return; unnecessary to return; Anyways no further code exists. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:130: nit: remove this blank line https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:145: state_== BEZEL_SCROLLING_ONE_FINGER) { space between _ and == https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:161: return; you don't have to return here https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:29: // BEZEL_BOTTOM Uncomment-out them (it's okay unused at all), or just remove them for now.
https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:36: On 2014/07/21 17:18:52, Jun Mukai wrote: > nit: remove this blank line Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:74: On 2014/07/21 17:18:52, Jun Mukai wrote: > nit: remove this blank line Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:80: // TODO: Currently we aren't retargetting or consuming any of the touch On 2014/07/21 17:18:52, Jun Mukai wrote: > TODO should have the owner name. > TODO(mfomitchev): ... for example Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:113: //NOTREACHED(); On 2014/07/21 17:18:52, Jun Mukai wrote: > I think it's okay to be NOTREACHED(). Anyways, please keep either. Done. Also replaced all CHECKs with DCHECKs - meant to do that before submitting but forgot. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:127: return; On 2014/07/21 17:18:52, Jun Mukai wrote: > unnecessary to return; Anyways no further code exists. Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:130: On 2014/07/21 17:18:52, Jun Mukai wrote: > nit: remove this blank line Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:145: state_== BEZEL_SCROLLING_ONE_FINGER) { On 2014/07/21 17:18:52, Jun Mukai wrote: > space between _ and == Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:161: return; On 2014/07/21 17:18:52, Jun Mukai wrote: > you don't have to return here Done. https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/20001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:29: // BEZEL_BOTTOM On 2014/07/21 17:18:52, Jun Mukai wrote: > Uncomment-out them (it's okay unused at all), or just remove them for now. Done.
hm, I've started wondering what will happen if the user swipes from bezel at overview mode. Should SplitViewController care about the current window manager status and decide to ignore? or should BezelController care about that?
> hm, I've started wondering what will happen if the user swipes from bezel at > overview mode. Should SplitViewController care about the current window manager > status and decide to ignore? or should BezelController care about that? My current thinking is that BezelController will be responsible for the interpretation of the bezel gestures, but whether and how they are handled is up to the delegate(s). So SplitViewController will handle the case you've pointed out in SplitViewController::CanScroll(). CanScroll will return false in overview mode, in split view mode, when there's less than 2 windows, and also if the device is in portrait mode.
cool, lgtm
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/394833004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31201) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/35613)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31211)
https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:13: // Using bezel swipes on Nexus 10, the first touch that is registered is please remove the reference to specific hw https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:20: gfx::PointF(-100, -100); won't this create static initializer? https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:53: } nuke {} https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:29: class ScrollDelegate { I guess it'd be useful to promote this (or something like this) to input/public so that other modules in athena (like home card) can use? Not now, but something we should keep eye on it. https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:95: // Responsible for handling gestures started from the left and right bezels. document ownership. https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:97: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.h:14: } this isn't necessary? https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.h:32: // scoped_ptr<views::View> separator_; remove unused code. DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/394833004/diff/60001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:114: container_->AddPreTargetHandler(bezel_controller_.get()); remove the handler in dtor.
https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:13: // Using bezel swipes on Nexus 10, the first touch that is registered is On 2014/07/22 17:56:11, oshima wrote: > please remove the reference to specific hw Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:20: gfx::PointF(-100, -100); Done. Just constructing gfx::Point when I need it now. This changes in the next rev I am working on now - I don't need gfx::Point here at all anymore https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.cc:53: } On 2014/07/22 17:56:12, oshima wrote: > nuke {} Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:95: // Responsible for handling gestures started from the left and right bezels. On 2014/07/22 17:56:12, oshima wrote: > document ownership. Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/bezel_controll... athena/wm/bezel_controller.h:97: }; On 2014/07/22 17:56:12, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.h:14: } On 2014/07/22 17:56:12, oshima wrote: > this isn't necessary? Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.h:32: // scoped_ptr<views::View> separator_; On 2014/07/22 17:56:12, oshima wrote: > remove unused code. > > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/394833004/diff/60001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/394833004/diff/60001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:114: container_->AddPreTargetHandler(bezel_controller_.get()); On 2014/07/22 17:56:12, oshima wrote: > remove the handler in dtor. Done.
@oshima: I think I've addressed your feedback. I am going to try to commit this (again). If you have further comments, please feel free to post them here and I am going to address them in the next check-in implementing the split view mode itself. Thanks!
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/394833004/120001
It wasn't nits, so please wait until you get lgtm. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:12: // static remove this https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:65: bool BezelController::ShouldProcessGesture(ui::EventType event_type) { Move this to anonymouse namespace as it doesn't depend on BezelCOntroller https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:83: num_fingers_down_++; shouldn't you use event->finger_count() instead? https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:85: GetBezel(gfx::PointF(event->x(), event->y())); event->location() https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:136: gfx::PointF event_location = event->location_f(); const gfx::PointF& https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.h:69: static const float kScrollPositionNone; move these to anonymous namespace.
https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:12: // static On 2014/07/22 21:30:46, oshima wrote: > remove this Sorry for incomplete comments. What I meant is that if you move static consts to anonymous namespace, you can remove this.
On 2014/07/22 21:30:46, oshima wrote: > It wasn't nits, so please wait until you get lgtm. > oops, not lgtm > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > File athena/wm/bezel_controller.cc (right): > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.cc:12: // static > remove this > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.cc:65: bool > BezelController::ShouldProcessGesture(ui::EventType event_type) { > Move this to anonymouse namespace as it doesn't depend on BezelCOntroller > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.cc:83: num_fingers_down_++; > shouldn't you use event->finger_count() instead? > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.cc:85: GetBezel(gfx::PointF(event->x(), event->y())); > event->location() > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.cc:136: gfx::PointF event_location = > event->location_f(); > const gfx::PointF& > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > File athena/wm/bezel_controller.h (right): > > https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... > athena/wm/bezel_controller.h:69: static const float kScrollPositionNone; > move these to anonymous namespace.
The CQ bit was unchecked by mfomitchev@chromium.org
I looked at some of the touch/gesture-event related code. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:39: if (state != state_) { early outing here if state_ == state would make this less nested. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:79: // bezel. On chromeos/x11, we do this by using a --touch-calibration command-line flag. Have you considered using that for this too? https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:111: }; Remove ; https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:165: } Prefer early-outs where possible. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.h:47: BezelController(aura::Window* container); explicit
@oshima: Sorry, thought mukai's lgtm was enough to submit. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:12: // static On 2014/07/22 21:30:46, oshima wrote: > remove this Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:12: // static On 2014/07/22 21:32:03, oshima wrote: > On 2014/07/22 21:30:46, oshima wrote: > > remove this > > Sorry for incomplete comments. What I meant is that if you move static consts to > anonymous namespace, you can remove this. Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:39: if (state != state_) { On 2014/07/22 21:53:19, sadrul wrote: > early outing here if state_ == state would make this less nested. Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:65: bool BezelController::ShouldProcessGesture(ui::EventType event_type) { On 2014/07/22 21:30:46, oshima wrote: > Move this to anonymouse namespace as it doesn't depend on BezelCOntroller Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:79: // bezel. Thanks for the pointer. Discussed offline: Ideally we'd want to make this conditional on left_right_delegate_->CanScroll() so that we don't steal events in full screen mode, etc... so this is probably not the best option. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:83: num_fingers_down_++; finger_count() is defined for ScrollEvent, not for TouchEvent. I didn't find this in TouchEvent. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:85: GetBezel(gfx::PointF(event->x(), event->y())); On 2014/07/22 21:30:46, oshima wrote: > event->location() Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:111: }; On 2014/07/22 21:53:19, sadrul wrote: > Remove ; Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:136: gfx::PointF event_location = event->location_f(); On 2014/07/22 21:30:46, oshima wrote: > const gfx::PointF& Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:165: } On 2014/07/22 21:53:18, sadrul wrote: > Prefer early-outs where possible. Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.h:47: BezelController(aura::Window* container); On 2014/07/22 21:53:19, sadrul wrote: > explicit Done. https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.h:69: static const float kScrollPositionNone; On 2014/07/22 21:30:46, oshima wrote: > move these to anonymous namespace. Done.
My apologies: looks like this comment didn't make it through in the last review. https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:126: } Can you look into not overriding OnTouchEvent(), and use the corresponding OnGestureEvent()s instead? For example, you can use GESTURE_BEGIN/GESTURE_END to know when/where the touch-events are pressed/removed. I think that would make this code easier to read. Also, GestureEvent()s include the number of active touch-points, so you wouldn't have to maintain num_fingers_down_ yourself.
https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:126: } I tried doing that at first, but there is a a couple of problems. 1. The location of the gesture event is based on all the touch points, and I didn't find a way to extract the location of each individual touch point. This makes it tricky to determine whether the new touch point is at the bezel on the GESTURE_BEGIN event. 2. If you are performing more than one simultaneous gesture, the touch points are going to be split between these gestures. In order to execute the transition from IGNORE to NO_FINGERS_DOWN, you have to keep track of all the ongoing gestures, so you can't get rid of num_fingers_down_.
https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/120001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:83: num_fingers_down_++; On 2014/07/22 22:22:01, mfomitchev wrote: > finger_count() is defined for ScrollEvent, not for TouchEvent. I didn't find > this in TouchEvent. I see. sadrul@, does it make sense to have a finger count in TouchEvent? I'm worrying about relying on pressed/release event to count fingers as it's possible to lose it.
What are the scenarios when you can lose a release? If releases can be lost, we will have issues with touch exploration mode as well...
On 2014/07/23 15:34:08, mfomitchev wrote: > What are the scenarios when you can lose a release? If releases can be lost, we > will have issues with touch exploration mode as well... For example, when a screen is locked, the event will be blocked so that it won't reach windows behind lock window. Another example would be modal dialogs. It seems to be able to get the finger count from device, and that's much better than counting pressed events.
Very interesting. I think the touch exploration mode implementation would have the same problem (ui/chromeos/touch_exploration_controller.cc). If it looses a release, it will have a stale touch id stuck in current_touch_ids_ vector which it uses to keep track of state. Perhaps we could create an API which we could use to obtain all active touch ids for a given screen?
https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:126: } On 2014/07/22 22:51:26, mfomitchev wrote: > I tried doing that at first, but there is a a couple of problems. > 1. The location of the gesture event is based on all the touch points, and I > didn't find a way to extract the location of each individual touch point. This > makes it tricky to determine whether the new touch point is at the bezel on the > GESTURE_BEGIN event. If the location of the GESTURE_BEGIN event is not the location of the touch-point that was pressed, then that is a bug, and we should fix that. > 2. If you are performing more than one simultaneous gesture, the touch points > are going to be split between these gestures. In order to execute the transition > from IGNORE to NO_FINGERS_DOWN, you have to keep track of all the ongoing > gestures, so you can't get rid of num_fingers_down_. There can be a single gesture-sequence in an aura::Window. So you won't have to maintain a list of ongoing gestures.
https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/394833004/diff/140001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:126: } On 2014/07/23 16:56:19, sadrul wrote: > On 2014/07/22 22:51:26, mfomitchev wrote: > > I tried doing that at first, but there is a a couple of problems. > > 1. The location of the gesture event is based on all the touch points, and I > > didn't find a way to extract the location of each individual touch point. This > > makes it tricky to determine whether the new touch point is at the bezel on > the > > GESTURE_BEGIN event. > > If the location of the GESTURE_BEGIN event is not the location of the > touch-point that was pressed, then that is a bug, and we should fix that. > > > 2. If you are performing more than one simultaneous gesture, the touch points > > are going to be split between these gestures. In order to execute the > transition > > from IGNORE to NO_FINGERS_DOWN, you have to keep track of all the ongoing > > gestures, so you can't get rid of num_fingers_down_. > > There can be a single gesture-sequence in an aura::Window. So you won't have to > maintain a list of ongoing gestures. Done.
lgtm
cool. lgtm
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/394833004/160001
Message was sent while issue was closed.
Change committed as 285166 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
