Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262483003:
Implementation of the Touch Exploration Mode - Part I (ui) (Closed)
|
Created:
6 years, 7 months ago by mfomitchev Modified:
6 years, 7 months ago CC:
chromium-reviews, dtseng+watch_chromium.org, sadrul, nkostylev+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, ben+aura_chromium.org, yuzo+watch_chromium.org, ben+ash_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, Rick Byers Base URL:
https://chromium.googlesource.com/chromium/src.git@event_source Visibility:
Public. |
DescriptionImplementation of the ui/ portion of the Touch Exploration Mode
Unofficial design doc: https://docs.google.com/a/google.com/document/d/1uOphhlascPTfNjsI2QTdeVTp-ANEz-g2rqGli5yVv34/edit#heading=h.a67n1i6wy36k
Reviews of the previous uncommitted implementation iterations:
https://codereview.chromium.org/240333007
https://codereview.chromium.org/225143007
BUG=368828
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268224
Patch Set 1 #
Total comments: 17
Patch Set 2 : Implementing review feedback from dmazzoni@ #
Total comments: 19
Patch Set 3 : Implementing review feedback from sadrul@ #Patch Set 4 : Moving the unit tests to ui/chromeos, changing the exploration mode to be disabled by default. #Patch Set 5 : Getting rid of unused method. #Patch Set 6 : Reverting all non- ui/ changes from this CL - they will be submitted separately. #
Total comments: 30
Patch Set 7 : Implementing review feedback. #
Total comments: 6
Patch Set 8 : Addressing nits. #
Total comments: 13
Patch Set 9 : Addressing oshima@'s feedback #
Messages
Total messages: 34 (0 generated)
Final Implementation of the Touch Exploration Mode.
lgtm https://codereview.chromium.org/262483003/diff/1/ash/default_accessibility_de... File ash/default_accessibility_delegate.cc (right): https://codereview.chromium.org/262483003/diff/1/ash/default_accessibility_de... ash/default_accessibility_delegate.cc:8: #include "base/logging.h" No longer needed? https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), I don't think we want browser tests navigating to real urls, right? https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:52: std::find(touch_ids_.begin(), touch_ids_.end(), touch_id); This can be written as just: std::vector<int>::iterator it = touch_ids_.find(touch_id); https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:60: touch_locations_.erase(touch_id); Assert / dcheck that touch_locations contains touch_id? https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:137: NOTREACHED(); Should you switch on |type| instead? Then not handling one of the possible values would be a compile error.
https://codereview.chromium.org/262483003/diff/1/ash/default_accessibility_de... File ash/default_accessibility_delegate.cc (right): https://codereview.chromium.org/262483003/diff/1/ash/default_accessibility_de... ash/default_accessibility_delegate.cc:8: #include "base/logging.h" On 2014/04/30 06:08:35, dmazzoni wrote: > No longer needed? Done. https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), What I am actually trying to do here is wait for the browser window to get resized (which happens if this is the first test to run I think). Mouse events are blocked during a resize via WindowEventDispatcher::HoldPointerMoves(), which causes my test to fail. There's probably a much better way to do that. I've tried a couple of variations of RunAllPending, but couldn't make it work. Any advice? https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:52: std::find(touch_ids_.begin(), touch_ids_.end(), touch_id); vector doesn't have find(), does it? I get a compile error: ../../ui/chromeos/touch_exploration_controller.cc:51:48: error: ‘class std::vector<int>’ has no member named ‘find’ https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:60: touch_locations_.erase(touch_id); On 2014/04/30 06:08:35, dmazzoni wrote: > Assert / dcheck that touch_locations contains touch_id? Done. https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:137: NOTREACHED(); Problem is, there's over 40 of values in the EventType enum, and I am only interested in five of them that are touch types. There'd be a lot of false positives with this approach - whenever a new event or gesture is added... Not sure what the best practice is here.
https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), On 2014/04/30 17:06:19, mfomitchev wrote: > What I am actually trying to do here is wait for the browser window to get > resized (which happens if this is the first test to run I think). Mouse events > are blocked during a resize via WindowEventDispatcher::HoldPointerMoves(), which > causes my test to fail. There's probably a much better way to do that. I've > tried a couple of variations of RunAllPending, but couldn't make it work. Any > advice? Does it work if you navigate to about:blank or chrome://version or some other internal page? If so that seems fine. What's special about the resize? If there's a notification that fires when the window resizes or some other notification you want to wait for, you probably want to use one of the utilities in ui_test_utils to keep running RunAllPending until a certain notification fires. https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:52: std::find(touch_ids_.begin(), touch_ids_.end(), touch_id); On 2014/04/30 17:06:19, mfomitchev wrote: > vector doesn't have find(), does it? I get a compile error: > ../../ui/chromeos/touch_exploration_controller.cc:51:48: error: ‘class > std::vector<int>’ has no member named ‘find’ Nevermind, I was thinking of other STL classes that do have it. https://codereview.chromium.org/262483003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:137: NOTREACHED(); On 2014/04/30 17:06:19, mfomitchev wrote: > Problem is, there's over 40 of values in the EventType enum, and I am only > interested in five of them that are touch types. There'd be a lot of false > positives with this approach - whenever a new event or gesture is added... Not > sure what the best practice is here. Got it - the rest are thrown away with event.IsTouchEvent(). This is fine.
+oshima@ My suggestion would be to split this CL into several pieces: 1. The code in ui/chromeos/. It should be possible to write tests in ui/chromeos/ that don't depend on ash/ or chromeos/ bits. I am not sure what test-suite this should be included in though. ui_unittests? oshima@: do you have a suggestion for this? 2. Use the controller introduced in 1. in ash/ and chromeos/, with the feature disabled-by-default (the tests can trivially turn on the feature). 3. Toggle the flag to enabled-by-defauled. It will make it much easier to review, revert etc. https://codereview.chromium.org/262483003/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/262483003/diff/1/ash/root_window_controller.c... ash/root_window_controller.cc:349: switches::kAshDisableTouchExplorationMode)) { We should land with the feature disabled-by-default, and turn it on in a subsequent CL, so that when we disable it again, we can simply revert the second CL. https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:859: break; We currently never generate STATIONARY events. I suspect we would need to do additional work if we do start generating STATIONARY events. So remove this (in fact, it may make sense to just remove the STATIONARY event-type altogether). https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:35: const gfx::Point location = touch_event.location(); Use location_f() instead? https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:104: CreateMouseMoveEvent(mouse_move_location, flags)); return DISPATCH_ANOTHER from here? https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:149: if ((*new_event)->IsMouseEvent()) Should you also check if last_event.IsTouchEvent()? https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:31: class UI_CHROMEOS_EXPORT TouchExplorationController : What is the expected ownership of this controller (i.e. is the caller expected to retain ownership)? Does this controller requires the owner to destroy it before the |root_window| is destroyed? https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:38: // Overridden from ui::EventRewriter Overrides usually come at the end of the non-override functions in the same block. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:44: ui::Event* CreateMouseMoveEvent(gfx::Point location, int flags); const &Point(F) This require the owner to take ownership of the created event, right? In that case, the return type should be a scoped_ptr<Event> https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:52: std::map<int, gfx::Point> touch_locations_; Can you use PointF here? https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.gyp File ui/chromeos/ui_chromeos.gyp (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.... ui/chromeos/ui_chromeos.gyp:15: '../../skia/skia.gyp:skia', What brings in skia?
https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:859: break; On 2014/04/30 18:51:15, sadrul wrote: > We currently never generate STATIONARY events. I suspect we would need to do > additional work if we do start generating STATIONARY events. So remove this (in > fact, it may make sense to just remove the STATIONARY event-type altogether). (Removing in https://codereview.chromium.org/268483004/)
https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), Looks like it's flaky with chrome://version. This probably means that it's flaky with google.com as well, although I couldn't make it fail. RenderWidgetHostViewAura::OnBoundsChanged() creates "resize lock" which calls WindowEventDispatcher::HoldPointerMoves(). Then RenderWidgetHostViewAura::OnCompositingDidCommit() destroys the lock when the resize is done. I think I can use ui::DrawWaiterForTest::WaitForCommit(compositor) to wait for the resize to complete, but I haven't found a good way to find out whether a resize in in progress. I think if I do Compositor->ScheduleDraw() and then DrawWaiterForTest::WaitForCommit it would work, but that seems dirty. Sadrul, do you have any thoughts on this?
On 2014/04/30 18:51:15, sadrul wrote: > +oshima@ > > My suggestion would be to split this CL into several pieces: > 1. The code in ui/chromeos/. It should be possible to write tests in > ui/chromeos/ that don't depend on ash/ or chromeos/ bits. I am not sure what > test-suite this should be included in though. ui_unittests? oshima@: do you have > a suggestion for this? ui_unittests sounds fine to me. > > 2. Use the controller introduced in 1. in ash/ and chromeos/, with the feature > disabled-by-default (the tests can trivially turn on the feature). > > 3. Toggle the flag to enabled-by-defauled. > > It will make it much easier to review, revert etc. > > https://codereview.chromium.org/262483003/diff/1/ash/root_window_controller.cc > File ash/root_window_controller.cc (right): > > https://codereview.chromium.org/262483003/diff/1/ash/root_window_controller.c... > ash/root_window_controller.cc:349: switches::kAshDisableTouchExplorationMode)) { > We should land with the feature disabled-by-default, and turn it on in a > subsequent CL, so that when we disable it again, we can simply revert the second > CL. > > https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... > File ui/aura/window_event_dispatcher.cc (right): > > https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... > ui/aura/window_event_dispatcher.cc:859: break; > We currently never generate STATIONARY events. I suspect we would need to do > additional work if we do start generating STATIONARY events. So remove this (in > fact, it may make sense to just remove the STATIONARY event-type altogether). > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > File ui/chromeos/touch_exploration_controller.cc (right): > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.cc:35: const gfx::Point location = > touch_event.location(); > Use location_f() instead? > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.cc:104: > CreateMouseMoveEvent(mouse_move_location, flags)); > return DISPATCH_ANOTHER from here? > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.cc:149: if > ((*new_event)->IsMouseEvent()) > Should you also check if last_event.IsTouchEvent()? > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > File ui/chromeos/touch_exploration_controller.h (right): > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.h:31: class UI_CHROMEOS_EXPORT > TouchExplorationController : > What is the expected ownership of this controller (i.e. is the caller expected > to retain ownership)? Does this controller requires the owner to destroy it > before the |root_window| is destroyed? > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.h:38: // Overridden from > ui::EventRewriter > Overrides usually come at the end of the non-override functions in the same > block. > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.h:44: ui::Event* > CreateMouseMoveEvent(gfx::Point location, int flags); > const &Point(F) > > This require the owner to take ownership of the created event, right? In that > case, the return type should be a scoped_ptr<Event> > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.h:52: std::map<int, gfx::Point> > touch_locations_; > Can you use PointF here? > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.gyp > File ui/chromeos/ui_chromeos.gyp (right): > > https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.... > ui/chromeos/ui_chromeos.gyp:15: '../../skia/skia.gyp:skia', > What brings in skia?
https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), On 2014/04/30 19:19:30, mfomitchev wrote: > Looks like it's flaky with chrome://version. This probably means that it's flaky > with http://google.com as well, although I couldn't make it fail. You can just load a data url (e.g. 'data:text/html;charset=utf8,<html></html>');
by the way, please file a bug and put the design doc and description of the featuer there, instead of putting everything here.
I will split up the CL into three and change the flag's value tomorrow. The rest of the feedback has been implemented.
https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/262483003/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:44: GURL("http://www.google.com/"), Will it always work though? I am concerned it could be flaky since chrome://version is flaky. WHat I really need is wait for the resizing to completing if one is in progress. https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/262483003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:859: break; On 2014/04/30 18:51:15, sadrul wrote: > We currently never generate STATIONARY events. I suspect we would need to do > additional work if we do start generating STATIONARY events. So remove this (in > fact, it may make sense to just remove the STATIONARY event-type altogether). Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:35: const gfx::Point location = touch_event.location(); On 2014/04/30 18:51:15, sadrul wrote: > Use location_f() instead? Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:104: CreateMouseMoveEvent(mouse_move_location, flags)); On 2014/04/30 18:51:15, sadrul wrote: > return DISPATCH_ANOTHER from here? Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:149: if ((*new_event)->IsMouseEvent()) Hmm.. As it stands, RewriteEvent() returns EVENT_REWRITE_CONTINUE for non-touch events, so here last_event would always be a touch event. I guess a DCHECK won't hurt... (As for next_dispatch_event_ - it could be a mouse event or a touch event, at least until the TODO in RewriteEvent() isn't done.) https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:31: class UI_CHROMEOS_EXPORT TouchExplorationController : Comment added to clarify. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:38: // Overridden from ui::EventRewriter On 2014/04/30 18:51:15, sadrul wrote: > Overrides usually come at the end of the non-override functions in the same > block. Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:44: ui::Event* CreateMouseMoveEvent(gfx::Point location, int flags); On 2014/04/30 18:51:15, sadrul wrote: > const &Point(F) > > This require the owner to take ownership of the created event, right? In that > case, the return type should be a scoped_ptr<Event> Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:52: std::map<int, gfx::Point> touch_locations_; On 2014/04/30 18:51:15, sadrul wrote: > Can you use PointF here? Done. https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.gyp File ui/chromeos/ui_chromeos.gyp (right): https://codereview.chromium.org/262483003/diff/20001/ui/chromeos/ui_chromeos.... ui/chromeos/ui_chromeos.gyp:15: '../../skia/skia.gyp:skia', On 2014/04/30 18:51:15, sadrul wrote: > What brings in skia? ui/gfx/transform.h includes SkMatrix44.h
All review feedback has been implemented. All non-ui changes have been reverted from this CL and will be submitted separately.
https://codereview.chromium.org/262483003/diff/90001/ui/base/DEPS File ui/base/DEPS (right): https://codereview.chromium.org/262483003/diff/90001/ui/base/DEPS#newcode13 ui/base/DEPS:13: "+ui/gl" My apologies, I didn't know that this requires new dependency to ui/base. Can you consult with ben/sky if this is ok?
+sky@ - mainly for the question re ui/base/DEPS. If adding a ui/gl dependency here is undersirable, I can move the unit test to aura_unittests instead of ui_unittests.
On 2014/05/01 21:28:34, mfomitchev wrote: > +sky@ - mainly for the question re ui/base/DEPS. If adding a ui/gl dependency > here is undersirable, I can move the unit test to aura_unittests instead of > ui_unittests. Can you move the initialization to your test?
https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:35: const gfx::PointF location = touch_event.location_f(); const gfx::PointF& https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:66: *rewritten_event = CreateMouseMoveEvent(location, flags).Pass(); I don't think you need Pass() here? https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:83: gfx::PointF rewritten_release_location = const & https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:90: rewritten_event->reset(rewritten_release_event); Do we need to copy over the flags from original event? https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:102: gfx::PointF mouse_move_location = touch_locations_[touch_ids_[0]]; const & https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:106: } else { // first_finger_released == true If the 'if' block ends in a return, then we don't need else. (also in lines 118-128) https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:56: std::vector<ui::LocatedEvent*> events_; Use a ScopedVector<> instead. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:118: EXPECT_TRUE(e2->IsTouchEvent()); These should probably be ASSERTs since otherwise the following code can cause crashes. Also, if you use ASSERT here, I think the callers will need to use the ASSERT_NO_FATAL_FAILURE() macro from the callsites. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:132: EXPECT_TRUE(e2->IsMouseEvent()); ditto https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:156: gfx::Point(13, 14), Did you mean to use |location_mouse_move| here? https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:169: if (type == ui::ET_MOUSE_ENTERED || type == ui::ET_MOUSE_EXITED) Do the enter/exit events happen in a predictable sequence? Can we test that these happen in the correct sequence instead of ignoring them? https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:171: EXPECT_EQ((*it)->type(), ui::ET_MOUSE_MOVED); EXPECT_EQ(<expectation>, <test case>) https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:226: EXPECT_FALSE(it == captured_events.end()); EXPECT_NE? https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:388: EXPECT_EQ(captured_events.size(), 7u); ASSERT_EQ
All review feedback has been implemented. Removed the ui/gl dependency from ui/base by moving initialization to the test. https://codereview.chromium.org/262483003/diff/90001/ui/base/DEPS File ui/base/DEPS (right): https://codereview.chromium.org/262483003/diff/90001/ui/base/DEPS#newcode13 ui/base/DEPS:13: "+ui/gl" Removed this, moved initialization to the test as sky@ recommended and added ui/gl to ui/chromeos/DEPS https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:35: const gfx::PointF location = touch_event.location_f(); On 2014/05/02 01:30:15, sadrul wrote: > const gfx::PointF& Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:66: *rewritten_event = CreateMouseMoveEvent(location, flags).Pass(); On 2014/05/02 01:30:15, sadrul wrote: > I don't think you need Pass() here? Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:83: gfx::PointF rewritten_release_location = On 2014/05/02 01:30:15, sadrul wrote: > const & Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:90: rewritten_event->reset(rewritten_release_event); On 2014/05/02 01:30:15, sadrul wrote: > Do we need to copy over the flags from original event? Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:102: gfx::PointF mouse_move_location = touch_locations_[touch_ids_[0]]; On 2014/05/02 01:30:15, sadrul wrote: > const & Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:106: } else { // first_finger_released == true On 2014/05/02 01:30:15, sadrul wrote: > If the 'if' block ends in a return, then we don't need else. (also in lines > 118-128) Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:56: std::vector<ui::LocatedEvent*> events_; On 2014/05/02 01:30:15, sadrul wrote: > Use a ScopedVector<> instead. Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:118: EXPECT_TRUE(e2->IsTouchEvent()); On 2014/05/02 01:30:15, sadrul wrote: > These should probably be ASSERTs since otherwise the following code can cause > crashes. Also, if you use ASSERT here, I think the callers will need to use the > ASSERT_NO_FATAL_FAILURE() macro from the callsites. Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:132: EXPECT_TRUE(e2->IsMouseEvent()); On 2014/05/02 01:30:15, sadrul wrote: > ditto Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:156: gfx::Point(13, 14), Removing location_mouse_move - it's a leftover from an older version https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:169: if (type == ui::ET_MOUSE_ENTERED || type == ui::ET_MOUSE_EXITED) It appears mouse enter/exit events aren't generated when run as part of ui_unittests (they are generated when running as part of ash_unittests) As per our discussion added a TODO for this. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:171: EXPECT_EQ((*it)->type(), ui::ET_MOUSE_MOVED); On 2014/05/02 01:30:15, sadrul wrote: > EXPECT_EQ(<expectation>, <test case>) Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:226: EXPECT_FALSE(it == captured_events.end()); On 2014/05/02 01:30:15, sadrul wrote: > EXPECT_NE? Done. https://codereview.chromium.org/262483003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:388: EXPECT_EQ(captured_events.size(), 7u); Done - switched to ASSERT everywhere we are checking size() to be safe.
A couple more nits. Other than that, LGTM https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:48: } else { No else. https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:123: } else if (touch_id == touch_ids_.front()) { You can have just 'if ' https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:40: scoped_ptr<ui::Event> CreateMouseMoveEvent(const gfx::PointF location, const &
https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:48: } else { On 2014/05/02 18:02:26, sadrul wrote: > No else. Done. https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:123: } else if (touch_id == touch_ids_.front()) { On 2014/05/02 18:02:26, sadrul wrote: > You can have just 'if ' Done. https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/262483003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:40: scoped_ptr<ui::Event> CreateMouseMoveEvent(const gfx::PointF location, On 2014/05/02 18:02:26, sadrul wrote: > const & Done.
mostly nits. just one q about InitializeOneOffForTests https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:130: NOTREACHED(); nit: can you add brief message to describe what might have went wrong here using NOTREACHED() << "xxx"? https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:22: class EventCapturer : public ui::EventHandler { nit: put this class into anonymous anmespace https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:34: event_copy = new ui::MouseEvent(static_cast<ui::MouseEvent&>(*event)); nit: can you just add it to events_ here? https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:64: gfx::GLSurface::InitializeOneOffForTests(); Is it safe if there is another test calling the same? https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:86: aura::client::CursorClient* cursor_client() {return cursor_client_.get();} nit: space between { and r, and ; and } you can just run git cl format https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:96: bool IsInTouchToMouseMode() { nit: const
On 2014/05/02 19:53:37, oshima wrote: > https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller_unittest.cc:64: > gfx::GLSurface::InitializeOneOffForTests(); > Is it safe if there is another test calling the same? I haven't looked too much into how these methods work, but the following makes me think this is okay: 1. sky@ recommended moving this initialization into the test 2. There is a couple of precedents in the existing code, and they don't even check if the surface is already initialized before calling InitializeOneOffForTests: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/corewm/de... https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/ash_native_... If you are satisfied with this explanation, can you pleae LGTM with nits? Thanks!
On 2014/05/02 20:22:56, mfomitchev wrote: > On 2014/05/02 19:53:37, oshima wrote: > > > https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... > > ui/chromeos/touch_exploration_controller_unittest.cc:64: > > gfx::GLSurface::InitializeOneOffForTests(); > > Is it safe if there is another test calling the same? > > I haven't looked too much into how these methods work, but the following makes > me think this is okay: > 1. sky@ recommended moving this initialization into the test > 2. There is a couple of precedents in the existing code, and they don't even > check if the surface is already initialized before calling > InitializeOneOffForTests: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/corewm/de... > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/ash_native_... Well, they do not make this CL safe. I looked at the code, I think you're doing the right think (by checking the impl). I think desktop_capture_controller_unittest.cc needs to do the same because simply calling twice will hit DCHECK, but that'd be outside of CL, so lgtm once you addressed nits. I was asking because this can introduce inter test dependency, which can cause a problem later and it's hard to troubleshoot it when it happens. (we just had such failure yesterday). > > If you are satisfied with this explanation, can you pleae LGTM with nits? > Thanks!
https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:130: NOTREACHED(); On 2014/05/02 19:53:38, oshima wrote: > nit: can you add brief message to describe what might have went wrong here using > > NOTREACHED() << "xxx"? Done. https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:22: class EventCapturer : public ui::EventHandler { On 2014/05/02 19:53:38, oshima wrote: > nit: put this class into anonymous anmespace Done. https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:34: event_copy = new ui::MouseEvent(static_cast<ui::MouseEvent&>(*event)); On 2014/05/02 19:53:38, oshima wrote: > nit: can you just add it to events_ here? Done. https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:64: gfx::GLSurface::InitializeOneOffForTests(); I think so - answered in detail separately https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:86: aura::client::CursorClient* cursor_client() {return cursor_client_.get();} On 2014/05/02 19:53:38, oshima wrote: > nit: space between { and r, and ; and } > you can just run git cl format Done. https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:96: bool IsInTouchToMouseMode() { root_window() is not const. Do you think it should be? If so, do you mind if I add a TODO for this and take care after I come back?
still lgtm https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/262483003/diff/130001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:96: bool IsInTouchToMouseMode() { On 2014/05/02 21:27:58, mfomitchev wrote: > root_window() is not const. Do you think it should be? If so, do you mind if I > add a TODO for this and take care after I come back? if that's the case, never mind. It probably should have had const version of it, but not big deal.
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/262483003/40002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
@sky - can you please lgtm the DEPS changes?
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/262483003/40002
Message was sent while issue was closed.
Change committed as 268224 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
