Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
385073009:
Side Slide Gestures for Accessibility (Closed)
|
Created:
6 years, 5 months ago by lisayin Modified:
6 years, 5 months ago CC:
evy_chromium.com, chromium-reviews, kalyank, stevenjb+watch_chromium.org, oshima+watch_chromium.org, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdded side slide gestures to touch exploration controller.
The user can control settings that might be normally changed using sliders by sliding along the edge of the screen when ChromeVox is on. For example, the user can slide along the right edge of the screen and adjust the volume.
If the user enters this mode and leaves the boundaries without releasing their touch, they will stop adjusting the setting, however, they will not enter touch exploration. If they return to the given boundaries, they will be able to modify the setting again. If the user does not touch a "hot edge" of the screen, they will not enter this state if they move to the an of the screen.
BUG=393326
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284819
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285149
Patch Set 1 #Patch Set 2 : Added Tests for Slide Gestures #
Total comments: 24
Patch Set 3 : Addressed comments except for making new files #Patch Set 4 : Separated CrosAcessibilityObserver into own files #
Total comments: 25
Patch Set 5 : Addressed Comments #
Total comments: 18
Patch Set 6 : Added Unittest for touch exploration manager #
Total comments: 15
Patch Set 7 : Addressed comments #
Total comments: 32
Patch Set 8 : Renamed unittest file #Patch Set 9 : Addressed Comments #Patch Set 10 : Running try tests #Patch Set 11 : Fixed TouchExploration Not Turning On #
Total comments: 8
Patch Set 12 : Rebase Off Master #Patch Set 13 : Addressed Comments #
Total comments: 28
Patch Set 14 : Addressed Comments #
Total comments: 8
Patch Set 15 : Addressed Comments #Patch Set 16 : Nit fix #Patch Set 17 : Fixed Memory leak #Messages
Total messages: 45 (0 generated)
Implementation for side slide gestures. Does not include tests and test refactoring yet.
Added Tests for Side Slide gestures.
+James Cook for reviewer
I only looked at ash/*. dmazzoni might be a better reviewer for the touch_exploration* parts. Let me know if you want me to review those pieces as well. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:55: #include "chromeos/audio/chromeos_sounds.h" Ash code is compile on non-ChromeOS platforms (e.g. Windows) so this and all code that uses it needs to be in #if defined(OS_CHROMEOS). https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:271: class CrosAccessibilityObserver Please extract this class into its own file. It's a little too big for a platform-specific local class. This will also let you use GYP to exclude it on non-Chrome OS platforms and reduce the number of headers this file needs to include. This would also make it easier to add a small test for this, especially if you inject your own mock CrasAudioHandler. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:291: bool VolumeAdjustSoundEnabled() { const? https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:296: // Overridden from TouchExplorationControllerDelegate nit: end with colon https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:306: // Overridden from TouchExplorationControllerDelegate nit: We usually only have one of these lines https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:315: if (audio_handler_->IsOutputVolumeBelowDefaultMuteLvel()) Not your typo, but could you use this CL to change the name of this function to "IsOutputVolumeBelowDefaultMuteLevel"?
This is really cool. Some initial comments. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:271: class CrosAccessibilityObserver On 2014/07/14 17:19:30, James Cook wrote: > Please extract this class into its own file. It's a little too big for a > platform-specific local class. This will also let you use GYP to exclude it on > non-Chrome OS platforms and reduce the number of headers this file needs to > include. > > This would also make it easier to add a small test for this, especially if you > inject your own mock CrasAudioHandler. That sounds good to me. I'd call it something like AshTouchExplorationManager or something like that, since "accessibility observer" doesn't really describe its primary purpose. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:57: delegate_.reset(delegate); Do this as an initializer before the initial {, as in: ... delegate_(delegate), ... VLOG_on_(true) { https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:95: gfx::Rect TouchExplorationController::BoundsOfWindowInDIPForTesting() { This should be BoundsOfRootWindow...? https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:581: // then wait until all fingers have been released. This comment doesn't seem to describe what the code below does. Why does it enter two-to-one passthrough state? https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:688: SideSlideControl(gesture); Why are you using the gesture detector for slides? It's not clear what it's computing for you that you couldn't get just from the x, y location of the finger. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:29: // A delegate to access ash methods. Ideally the comment shouldn't mention ash - describe what it's for - a delegate to handle commands in response to detected accessibility gestures. We will probably handle a lot of other gestures using this class - maybe even replacing the ones that currently generate keys. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:109: // along an edge of the screen and discretely control a setting. Once the user "discretely" doesn't sound right since it controls continuous-valued parameters
https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:55: #include "chromeos/audio/chromeos_sounds.h" On 2014/07/14 17:19:31, James Cook wrote: > Ash code is compile on non-ChromeOS platforms (e.g. Windows) so this and all > code that uses it needs to be in #if defined(OS_CHROMEOS). Done. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:291: bool VolumeAdjustSoundEnabled() { On 2014/07/14 17:19:30, James Cook wrote: > const? Done. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:296: // Overridden from TouchExplorationControllerDelegate On 2014/07/14 17:19:30, James Cook wrote: > nit: end with colon Done. https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:306: // Overridden from TouchExplorationControllerDelegate On 2014/07/14 17:19:30, James Cook wrote: > nit: We usually only have one of these lines Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:57: delegate_.reset(delegate); On 2014/07/14 18:00:34, dmazzoni wrote: > Do this as an initializer before the initial {, as in: > > ... > delegate_(delegate), > ... > VLOG_on_(true) { Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:95: gfx::Rect TouchExplorationController::BoundsOfWindowInDIPForTesting() { On 2014/07/14 18:00:33, dmazzoni wrote: > This should be BoundsOfRootWindow...? Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:581: // then wait until all fingers have been released. On 2014/07/14 18:00:34, dmazzoni wrote: > This comment doesn't seem to describe what the code below does. Why does it > enter two-to-one passthrough state? Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:688: SideSlideControl(gesture); On 2014/07/14 18:00:33, dmazzoni wrote: > Why are you using the gesture detector for slides? It's not clear what it's > computing for you that you couldn't get just from the x, y location of the > finger. Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:29: // A delegate to access ash methods. On 2014/07/14 18:00:34, dmazzoni wrote: > Ideally the comment shouldn't mention ash - describe what it's for - a delegate > to handle commands in response to detected accessibility gestures. > > We will probably handle a lot of other gestures using this class - maybe even > replacing the ones that currently generate keys. Done. https://codereview.chromium.org/385073009/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:109: // along an edge of the screen and discretely control a setting. Once the user On 2014/07/14 18:00:34, dmazzoni wrote: > "discretely" doesn't sound right since it controls continuous-valued parameters Done.
https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:271: class CrosAccessibilityObserver On 2014/07/14 18:00:33, dmazzoni wrote: > On 2014/07/14 17:19:30, James Cook wrote: > > Please extract this class into its own file. It's a little too big for a > > platform-specific local class. This will also let you use GYP to exclude it on > > non-Chrome OS platforms and reduce the number of headers this file needs to > > include. > > > > This would also make it easier to add a small test for this, especially if you > > inject your own mock CrasAudioHandler. > > That sounds good to me. I'd call it something like AshTouchExplorationManager or > something like that, since "accessibility observer" doesn't really describe its > primary purpose. Should the file be in the same folder as this file?
On 2014/07/14 22:40:36, lisayin wrote: > https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... > File ash/root_window_controller.cc (right): > > https://codereview.chromium.org/385073009/diff/20001/ash/root_window_controll... > ash/root_window_controller.cc:271: class CrosAccessibilityObserver > On 2014/07/14 18:00:33, dmazzoni wrote: > > On 2014/07/14 17:19:30, James Cook wrote: > > > Please extract this class into its own file. It's a little too big for a > > > platform-specific local class. This will also let you use GYP to exclude it > on > > > non-Chrome OS platforms and reduce the number of headers this file needs to > > > include. > > > > > > This would also make it easier to add a small test for this, especially if > you > > > inject your own mock CrasAudioHandler. > > > > That sounds good to me. I'd call it something like AshTouchExplorationManager > or > > something like that, since "accessibility observer" doesn't really describe > its > > primary purpose. > > Should the file be in the same folder as this file? Yes, please put it in the same directory (ash/) for now. dmazzoni, do you think we'll eventually want this and ash/autoclick in the same directory, maybe introducing an ash/accessibility?
Separated CrosAccessibilityObserver into its own files.
On 2014/07/14 22:55:35, James Cook wrote: > dmazzoni, do you think we'll eventually want this and ash/autoclick in the same > directory, maybe introducing an ash/accessibility? Possibly, it depends on how much real code we end up with here vs glue code.
https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:23: class CrosAccessibilityObserver This class name should match the file name. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:30: // Overridden from TouchExplorationControllerDelegate: Newline before this. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:36: // Overridden from AccessibilityObserver. Separate any overridden functions from others with newlines - group the ones that aren't overridden together. https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... ash/root_window_controller.cc:12: #include "ash/audio/sounds.h" No longer needed? https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... ash/root_window_controller.cc:764: cros_accessibility_observer_.reset(new CrosAccessibilityObserver(this)); Rename this to match the new name of the class.
https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:9: #include "ash/shell_delegate.h" Do you need this include? Or ash/accessibility_delegate.h? https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:30: if (system_tray_notifier) Is this NULL in normal operation, in tests, in both? I'm worried you might be leaking an observer. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:32: no blank line https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:48: void CrosAccessibilityObserver::AdjustSound(float volume) { Please add a unit test for this functionality. (In general, it's good to add at least one unit test for a new class. This ensures that the class can be used in a unit test harness and make it much easier for later maintainers to add additional tests. The hardest test to write is always the first one. :-) https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:8: #include "ash/root_window_controller.h" Do you need this #include? The forward declaration below seems sufficient. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:10: #include "chromeos/audio/cras_audio_handler.h" Can you forward declare the class you need here? https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:12: nit: one blank line only https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:30: // Overridden from TouchExplorationControllerDelegate: nit: blank line above Also, while this comment meets the style guide, I prefer versions where the overridden interface name appears first, because that's the important part of the line. For example: // TouchExplorationControllerDelegate overrides: https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:36: // Overridden from AccessibilityObserver. nit: blank line above Also, I tend to put all overridden interfaces in the same section (public vs. private), and keep them in the same order as the declaration above (public A, public B has overrides in the order A B). https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... File ash/root_window_controller.cc (left): https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... ash/root_window_controller.cc:267: class CrosAccessibilityObserver : public AccessibilityObserver { Hooray for making root_window_controller.cc shorter!
https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:23: class CrosAccessibilityObserver On 2014/07/15 18:04:01, dmazzoni wrote: > This class name should match the file name. Done. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:30: // Overridden from TouchExplorationControllerDelegate: On 2014/07/15 18:04:01, dmazzoni wrote: > Newline before this. Done. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:36: // Overridden from AccessibilityObserver. On 2014/07/15 18:04:01, dmazzoni wrote: > Separate any overridden functions from others with newlines - group the ones > that aren't overridden together. Done. https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... ash/root_window_controller.cc:12: #include "ash/audio/sounds.h" On 2014/07/15 18:04:02, dmazzoni wrote: > No longer needed? Done.
+derat I've modified the name of a function in cras_audio_handler.
lgtm heh. don't think that that was my typo, but i heartily approve of changes under chromeos/
On 2014/07/15 19:48:04, Daniel Erat wrote: > lgtm > > heh. don't think that that was my typo, but i heartily approve of changes under > chromeos/ If things are ready for me to look again, please put something like "jamescook, PTAL" in the mail message. I'm presuming you want me to look again, so looking now. :-)
Almost there! https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. "Copyright 2014" please (and no (c) for 2014) https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:30: if (system_tray_notifier) Why is this needed? Are you sure you are always removing the observer? https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:72: // Overridden from AccessibilityObserver. Either remove this comment or add one for the other interface. Also, reorder functions in .cc file to match .h https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:8: #include "ash/root_window_controller.h" not needed? https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:10: #include "chromeos/audio/cras_audio_handler.h" not needed? https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:13: one blank line https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:31: // AccessibilityObserver overrides: hooray, this looks nice https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:36: virtual void const PlayVolumeAdjustSound() OVERRIDE; virtual void PlayVolumeAdjustSound() const OVERRIDE (const goes last) Or make it not const, since playing a sound has a side effect https://codereview.chromium.org/385073009/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:35: virtual void const PlayVolumeAdjustSound() = 0; either const at end or not const Also, please document each function. This is especially important in an interface declaration, as the concrete implementations often won't have comments.
https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:9: #include "ash/shell_delegate.h" On 2014/07/15 18:21:57, James Cook wrote: > Do you need this include? Or ash/accessibility_delegate.h? Done. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:32: On 2014/07/15 18:21:57, James Cook wrote: > no blank line Done. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:8: #include "ash/root_window_controller.h" On 2014/07/15 18:21:57, James Cook wrote: > Do you need this #include? The forward declaration below seems sufficient. Done. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:10: #include "chromeos/audio/cras_audio_handler.h" On 2014/07/15 18:21:57, James Cook wrote: > Can you forward declare the class you need here? To clarify, do you mean forward declare CrasAudioHandler? I think it's already forward declared. https://codereview.chromium.org/385073009/diff/60001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:12: On 2014/07/15 18:21:57, James Cook wrote: > nit: one blank line only Done. https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/60001/ash/root_window_controll... ash/root_window_controller.cc:764: cros_accessibility_observer_.reset(new CrosAccessibilityObserver(this)); On 2014/07/15 18:04:01, dmazzoni wrote: > Rename this to match the new name of the class. Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/15 19:58:10, James Cook wrote: > "Copyright 2014" please (and no (c) for 2014) Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:30: if (system_tray_notifier) On 2014/07/15 19:58:10, James Cook wrote: > Why is this needed? Are you sure you are always removing the observer? The original code included this in its deconstructor. Should it belong in a deconstructor in the root window instead? https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:72: // Overridden from AccessibilityObserver. On 2014/07/15 19:58:10, James Cook wrote: > Either remove this comment or add one for the other interface. > > Also, reorder functions in .cc file to match .h Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:8: #include "ash/root_window_controller.h" On 2014/07/15 19:58:10, James Cook wrote: > not needed? Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:10: #include "chromeos/audio/cras_audio_handler.h" On 2014/07/15 19:58:11, James Cook wrote: > not needed? Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:13: On 2014/07/15 19:58:10, James Cook wrote: > one blank line Done. https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:36: virtual void const PlayVolumeAdjustSound() OVERRIDE; On 2014/07/15 19:58:10, James Cook wrote: > virtual void PlayVolumeAdjustSound() const OVERRIDE > > (const goes last) > > Or make it not const, since playing a sound has a side effect Done. https://codereview.chromium.org/385073009/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:35: virtual void const PlayVolumeAdjustSound() = 0; On 2014/07/15 19:58:11, James Cook wrote: > either const at end or not const > > Also, please document each function. This is especially important in an > interface declaration, as the concrete implementations often won't have > comments. Done.
jamescook, PTAL
Almost almost there! Hopefully you don't feel like this guy: https://memegen.googleplex.com/1065167 https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/80001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.cc:30: if (system_tray_notifier) On 2014/07/15 21:46:45, lisayin wrote: > On 2014/07/15 19:58:10, James Cook wrote: > > Why is this needed? Are you sure you are always removing the observer? > > The original code included this in its deconstructor. Should it belong in a > deconstructor in the root window instead? Hmm. OK. If it was in the destructor in the old code, you should keep it. Pity we don't know why it needs to be there. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_unittest.cc (right): https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:13: namespace test { I don't generally put the tests themselves in the "test" namespace, just test support code. I would remove this and just use namespace ash. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:15: class AshTouchExplorationManagerTest : public test::AshTestBase { optional: You can also do "typedef test::AshTestBase AshTouchExplorationManagerTest". https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:23: chromeos::CrasAudioHandler* audio_handler_(chromeos::CrasAudioHandler::Get()); "audio_handler" not "audio_handler_" since this is a local variable. We also typically use "=" instead of "()" for these sorts of things. Also, I would do this above the line that sets the output level, so you get into a nice pattern of: 1. set some data 2. test some data ... https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:24: EXPECT_EQ(audio_handler_->GetOutputVolumePercent(), 10); Expect not muted? https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:27: EXPECT_EQ(audio_handler_->GetOutputVolumePercent(), 100); Expect not muted? https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:35: } // namespace ash Hooray, we have a test! Now the next test will be easier. https://codereview.chromium.org/385073009/diff/100001/ash/root_window_control... File ash/root_window_controller.h (right): https://codereview.chromium.org/385073009/diff/100001/ash/root_window_control... ash/root_window_controller.h:301: scoped_ptr<AccessibilityObserver> touch_exploration_manager_; While this works, I would make this an AshTouchExplorationManagerChromeOS so the reader doesn't need to know that it inherits from AccessibilityObserver. https://codereview.chromium.org/385073009/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:38: // Takes a float from 1 to 100 that indicates the percent the volume should be nit: blank line above And nice comment, it explains that the range is 1-100 not 0.01 to 1.0. (Is 0.0 an acceptable value?)
https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_unittest.cc (right): https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:13: namespace test { On 2014/07/15 22:40:36, James Cook wrote: > I don't generally put the tests themselves in the "test" namespace, just test > support code. I would remove this and just use namespace ash. Done. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:15: class AshTouchExplorationManagerTest : public test::AshTestBase { On 2014/07/15 22:40:37, James Cook wrote: > optional: You can also do "typedef test::AshTestBase > AshTouchExplorationManagerTest". Done. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:23: chromeos::CrasAudioHandler* audio_handler_(chromeos::CrasAudioHandler::Get()); On 2014/07/15 22:40:37, James Cook wrote: > "audio_handler" not "audio_handler_" since this is a local variable. > > We also typically use "=" instead of "()" for these sorts of things. > > Also, I would do this above the line that sets the output level, so you get into > a nice pattern of: > 1. set some data > 2. test some data > ... Done. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:24: EXPECT_EQ(audio_handler_->GetOutputVolumePercent(), 10); On 2014/07/15 22:40:36, James Cook wrote: > Expect not muted? Done. https://codereview.chromium.org/385073009/diff/100001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:27: EXPECT_EQ(audio_handler_->GetOutputVolumePercent(), 100); On 2014/07/15 22:40:36, James Cook wrote: > Expect not muted? Done. https://codereview.chromium.org/385073009/diff/100001/ash/root_window_control... File ash/root_window_controller.h (right): https://codereview.chromium.org/385073009/diff/100001/ash/root_window_control... ash/root_window_controller.h:301: scoped_ptr<AccessibilityObserver> touch_exploration_manager_; On 2014/07/15 22:40:37, James Cook wrote: > While this works, I would make this an AshTouchExplorationManagerChromeOS so the > reader doesn't need to know that it inherits from AccessibilityObserver. Done. https://codereview.chromium.org/385073009/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:38: // Takes a float from 1 to 100 that indicates the percent the volume should be On 2014/07/15 22:40:37, James Cook wrote: > nit: blank line above > > And nice comment, it explains that the range is 1-100 not 0.01 to 1.0. (Is 0.0 > an acceptable value?) Done.
ash/* LGTM with one nit to fix before committing Nice patch! https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_unittest.cc (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:20: chromeos::CrasAudioHandler* audio_handler_ = no underscore after "audio_handler"
https://codereview.chromium.org/385073009/diff/120001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash.gyp#newcode926 ash/ash.gyp:926: 'ash_touch_exploration_manager_unittest.cc', Nit: should this be ash_touch_exploration_manager_chromeos_unittest? You should probably rename that if you get compile errors on Windows, since Windows compiles ash/ but doesn't compile files with "chromeos" in the name. If there's no compile error then this is fine as-is. :) https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.h:20: // feedback is on. I'd also say that this class implements the TouchExplorationControllerDelegate, allowing touch gestures to manipulate the system. https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.h:21: class ASH_EXPORT AshTouchExplorationManagerChromeOS Optional: I think it's fine to call this class AshTouchExplorationManager and just say "for Chrome OS only" in the comment above. We don't normally put "ChromeOS" in the name of classes unless there's a specific subclass for multiple operating systems. Normally the filename and class name should match exactly, but an operating-system prefix on the end of the filename is a common exception. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:26: const float kLeavingScreenEdge = 6; Need comment for this too. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:33: // bounds, the preset settings will still change. Note that this is in Dips too. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:263: if ((where | RIGHT_EDGE) == where) { This took me a second - the more typical way I'd see a test for a bit mask is: if (where & RIGHT_EDGE) { https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:587: event.touch_id() != initial_press_->touch_id()) { I'm not sure you should switch to the two-to-one passthrough in this second case, if you get a touch event with a different id. In general, we've been ignoring "surprise" events (like a move or release with a touch id we weren't tracking), not trying to handle them. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:604: VLOG(0) << "Location " << event.location().ToString(); Delete this log or add a bit more context so that 3 months from now you'll know the purpose of this log https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:608: if (((where | RIGHT_EDGE) != where) && (type != ui::ET_TOUCH_RELEASED)) { same here with bit testing https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:625: if (tap_timer_.IsRunning()) You shouldn't need to stop it every time. Can you stop it once when you enter the state and not again? https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:728: state_ = TOUCH_EXPLORATION; Can you explain this one? When do you go from the Side Slide state to Touch Exploration state? https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:761: (root_window_->bounds().bottom() - 2 * kMaxDistanceFromEdge) * 100; I think the root window bounds top may not be zero, so you should check height(), or subtract (bottom - top). Maybe break this up into a few lines? float volume_adjust_height = root_window_->bounds().bottom() - 2 * kMaxDistanceFromEdge; float ratio = (location.y - kMaxDistanceFromEdge) / volume_adjust_height; float volume = 100 - 100 * ratio; https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:801: // Since GetBoundsInScreen is in DIPs but p is not, then p needs to be What is "p"? https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:804: gfx::Rect window= root_window_->GetBoundsInScreen(); nit: spaces around = sign https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:817: result = result | LEFT_EDGE; nit: result |= LEFT_EDGE
https://codereview.chromium.org/385073009/diff/120001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash.gyp#newcode926 ash/ash.gyp:926: 'ash_touch_exploration_manager_unittest.cc', On 2014/07/16 16:54:39, dmazzoni wrote: > Nit: should this be ash_touch_exploration_manager_chromeos_unittest? > > You should probably rename that if you get compile errors on Windows, since > Windows compiles ash/ but doesn't compile files with "chromeos" in the name. If > there's no compile error then this is fine as-is. :) Done. https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.h:20: // feedback is on. On 2014/07/16 16:54:39, dmazzoni wrote: > I'd also say that this class implements the TouchExplorationControllerDelegate, > allowing touch gestures to manipulate the system. Done. https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.h:21: class ASH_EXPORT AshTouchExplorationManagerChromeOS On 2014/07/16 16:54:39, dmazzoni wrote: > Optional: I think it's fine to call this class AshTouchExplorationManager and > just say "for Chrome OS only" in the comment above. We don't normally put > "ChromeOS" in the name of classes unless there's a specific subclass for > multiple operating systems. > > Normally the filename and class name should match exactly, but an > operating-system prefix on the end of the filename is a common exception. Done. https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_unittest.cc (right): https://codereview.chromium.org/385073009/diff/120001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_unittest.cc:20: chromeos::CrasAudioHandler* audio_handler_ = On 2014/07/16 00:08:47, James Cook wrote: > no underscore after "audio_handler" Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:26: const float kLeavingScreenEdge = 6; On 2014/07/16 16:54:40, dmazzoni wrote: > Need comment for this too. Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:33: // bounds, the preset settings will still change. On 2014/07/16 16:54:40, dmazzoni wrote: > Note that this is in Dips too. Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:587: event.touch_id() != initial_press_->touch_id()) { On 2014/07/16 16:54:40, dmazzoni wrote: > I'm not sure you should switch to the two-to-one passthrough in this second > case, if you get a touch event with a different id. > > In general, we've been ignoring "surprise" events (like a move or release with a > touch id we weren't tracking), not trying to handle them. Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:604: VLOG(0) << "Location " << event.location().ToString(); On 2014/07/16 16:54:40, dmazzoni wrote: > Delete this log or add a bit more context so that 3 months from now you'll know > the purpose of this log Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:608: if (((where | RIGHT_EDGE) != where) && (type != ui::ET_TOUCH_RELEASED)) { On 2014/07/16 16:54:40, dmazzoni wrote: > same here with bit testing Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:625: if (tap_timer_.IsRunning()) On 2014/07/16 16:54:40, dmazzoni wrote: > You shouldn't need to stop it every time. Can you stop it once when you enter > the state and not again? Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:728: state_ = TOUCH_EXPLORATION; On 2014/07/16 16:54:39, dmazzoni wrote: > Can you explain this one? When do you go from the Side Slide state to Touch > Exploration state? Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:801: // Since GetBoundsInScreen is in DIPs but p is not, then p needs to be On 2014/07/16 16:54:40, dmazzoni wrote: > What is "p"? Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:804: gfx::Rect window= root_window_->GetBoundsInScreen(); On 2014/07/16 16:54:40, dmazzoni wrote: > nit: spaces around = sign Done.
Please upload the latest version after addressing these comments
Fixed touch exploration not turning on after turning off and on chromevox.
https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos_unittest.cc:18: AshTouchExplorationManager* touch_exploration_manager = This leaks. Either put it in a scoped_ptr<> or better yet just do: AshTouchExplorationManager touch_exploration_manager; touch_exploration_manager.DoFoo(); https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... ash/root_window_controller.cc:352: VLOG(0) << "Removed Touch Exploration"; Remove this line https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:324: // Volume control. Destruction is left up to the owner. The typical Chrome comment in these situations is "Not owned." So something like: // Handles volume control. Not owned. https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:106: delegate_ = new MockTouchExplorationControllerDelegate(); I think this causes a memory leak. delegate_ should probably be a normal member variable: MockTouchExplorationControllerDelegate delegate_; Then just pass around &delegate_ where you need the pointer. No tricky memory management then.
On 2014/07/21 21:05:36, James Cook wrote: > https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... > File ash/ash_touch_exploration_manager_chromeos_unittest.cc (right): > > https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... > ash/ash_touch_exploration_manager_chromeos_unittest.cc:18: > AshTouchExplorationManager* touch_exploration_manager = > This leaks. Either put it in a scoped_ptr<> or better yet just do: > > AshTouchExplorationManager touch_exploration_manager; > touch_exploration_manager.DoFoo(); > > https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... > File ash/root_window_controller.cc (right): > > https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... > ash/root_window_controller.cc:352: VLOG(0) << "Removed Touch Exploration"; > Remove this line > > https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... > File ui/chromeos/touch_exploration_controller.h (right): > > https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.h:324: // Volume control. Destruction > is left up to the owner. > The typical Chrome comment in these situations is "Not owned." So something > like: > > // Handles volume control. Not owned. > > https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... > File ui/chromeos/touch_exploration_controller_unittest.cc (right): > > https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller_unittest.cc:106: delegate_ = new > MockTouchExplorationControllerDelegate(); > I think this causes a memory leak. delegate_ should probably be a normal member > variable: > > MockTouchExplorationControllerDelegate delegate_; > > Then just pass around &delegate_ where you need the pointer. No tricky memory > management then. Also, running the linux_chromeos_valgrind trybot can sometimes help find memory leaks and double-deletes.
Dominic and James Cook, PTAL https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:263: if ((where | RIGHT_EDGE) == where) { On 2014/07/16 16:54:40, dmazzoni wrote: > This took me a second - the more typical way I'd see a test for a bit mask is: > > if (where & RIGHT_EDGE) { Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:761: (root_window_->bounds().bottom() - 2 * kMaxDistanceFromEdge) * 100; On 2014/07/16 16:54:40, dmazzoni wrote: > I think the root window bounds top may not be zero, so you should check > height(), or subtract (bottom - top). > > Maybe break this up into a few lines? > > float volume_adjust_height = root_window_->bounds().bottom() - 2 * > kMaxDistanceFromEdge; > float ratio = (location.y - kMaxDistanceFromEdge) / volume_adjust_height; > float volume = 100 - 100 * ratio; Done. https://codereview.chromium.org/385073009/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:817: result = result | LEFT_EDGE; On 2014/07/16 16:54:40, dmazzoni wrote: > nit: result |= LEFT_EDGE Done. https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/385073009/diff/200001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos_unittest.cc:18: AshTouchExplorationManager* touch_exploration_manager = On 2014/07/21 21:05:35, James Cook wrote: > This leaks. Either put it in a scoped_ptr<> or better yet just do: > > AshTouchExplorationManager touch_exploration_manager; > touch_exploration_manager.DoFoo(); Done. https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... File ash/root_window_controller.cc (right): https://codereview.chromium.org/385073009/diff/200001/ash/root_window_control... ash/root_window_controller.cc:352: VLOG(0) << "Removed Touch Exploration"; On 2014/07/21 21:05:35, James Cook wrote: > Remove this line Done. https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:324: // Volume control. Destruction is left up to the owner. On 2014/07/21 21:05:35, James Cook wrote: > The typical Chrome comment in these situations is "Not owned." So something > like: > > // Handles volume control. Not owned. Done. https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:106: delegate_ = new MockTouchExplorationControllerDelegate(); On 2014/07/21 21:05:35, James Cook wrote: > I think this causes a memory leak. delegate_ should probably be a normal member > variable: > > MockTouchExplorationControllerDelegate delegate_; > > Then just pass around &delegate_ where you need the pointer. No tricky memory > management then. > Done.
https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:26: // Within these bounds, the release event generated will reset the state to Nit: something like "within this many dips of the screen edge" https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:702: else if (where & TOP_EDGE) { nit: belongs on previous line after } https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1490: // DO SOMETHING TO MAKE THESE CONSTANTS ACCESSIBLE FROM ORIGINAL FILE Define the constants you need in the TouchExplorationController class (in touch_exploration_controller.h) and then use TouchExplorationControllerTestApi to access them from here.
lgtm Great tests! Just a few nits. https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:57: audio_handler_->SetOutputMute(true); Does SetOutputMute set the volume back to 0/DefaultMuteLevel ? https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos_unittest.cc:30: touch_exploration_manager.SetOutputLevel(0); A test here for negative |volume| would be good. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:32: const float kMaxDistanceFromEdge = 75; Where do these numbers come from? What are the units? Will they work on all devices? https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:224: int where = WithinBoundsOfEdge(event.location(), kMaxDistanceFromEdge); Similarly, I think 'where' here is a little misleading. How about 'edges'? https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:702: else if (where & TOP_EDGE) { Trivial, optional suggestion: move this and the below if blocks above the above if block; since you can't be on the top and bottom at once, you can early out and won't need any elses, then the above block becomes the default case and doesn't need to go in an if block at all (unless you want to assert that it's not in some impossible state). https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:293: RIGHT_EDGE = 0x1, Optional: these don't really need to be hex since they're all <10 :) You could make them regular decimal, or else go with something like https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.h&l=... - using bit shift operators to set each bit directly. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:297: SCREEN_CENTER = 0x0, Suggest renaming this to NONE or NO_EDGE and moving it to the top. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:304: int WithinBoundsOfEdge(gfx::Point point, float bounds); This method confused me; I thought it was going to return a bool. Perhaps something like FindEdgesWithinBounds? https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:315: // Handles volume control. Not owned. Nice comment :) https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:347: // A timer to fire a indicating sound when sliding to change volume. nit: an indicating https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:550: generator_->MoveTouch(gfx::Point(100, 200)); Perhaps move this above the preceding comment and explain why it's necessary. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1462: // elapsed, there should have two sounds that fired and two volume So the move within the slop region generates a volume change as well?
Dominic, Alice, and James, PTAL https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:57: audio_handler_->SetOutputMute(true); On 2014/07/22 17:20:41, aboxhall wrote: > Does SetOutputMute set the volume back to 0/DefaultMuteLevel ? Yes. https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/385073009/diff/240001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos_unittest.cc:30: touch_exploration_manager.SetOutputLevel(0); On 2014/07/22 17:20:41, aboxhall wrote: > A test here for negative |volume| would be good. Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:26: // Within these bounds, the release event generated will reset the state to On 2014/07/22 17:14:54, dmazzoni wrote: > Nit: something like "within this many dips of the screen edge" Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:32: const float kMaxDistanceFromEdge = 75; On 2014/07/22 17:20:41, aboxhall wrote: > Where do these numbers come from? What are the units? Will they work on all > devices? Dip stands for device independent pixels so they should work for all devices. The numbers are from letting Peter and David try it out and tweaking the numbers from there. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:224: int where = WithinBoundsOfEdge(event.location(), kMaxDistanceFromEdge); On 2014/07/22 17:20:41, aboxhall wrote: > Similarly, I think 'where' here is a little misleading. How about 'edges'? Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:702: else if (where & TOP_EDGE) { On 2014/07/22 17:20:41, aboxhall wrote: > Trivial, optional suggestion: move this and the below if blocks above the above > if block; since you can't be on the top and bottom at once, you can early out > and won't need any elses, then the above block becomes the default case and > doesn't need to go in an if block at all (unless you want to assert that it's > not in some impossible state). Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:293: RIGHT_EDGE = 0x1, On 2014/07/22 17:20:42, aboxhall wrote: > Optional: these don't really need to be hex since they're all <10 :) > You could make them regular decimal, or else go with something like > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.h&l=... > - using bit shift operators to set each bit directly. Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:297: SCREEN_CENTER = 0x0, On 2014/07/22 17:20:41, aboxhall wrote: > Suggest renaming this to NONE or NO_EDGE and moving it to the top. Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:304: int WithinBoundsOfEdge(gfx::Point point, float bounds); On 2014/07/22 17:20:41, aboxhall wrote: > This method confused me; I thought it was going to return a bool. Perhaps > something like FindEdgesWithinBounds? Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:347: // A timer to fire a indicating sound when sliding to change volume. On 2014/07/22 17:20:41, aboxhall wrote: > nit: an indicating Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:550: generator_->MoveTouch(gfx::Point(100, 200)); On 2014/07/22 17:20:42, aboxhall wrote: > Perhaps move this above the preceding comment and explain why it's necessary. Done. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1462: // elapsed, there should have two sounds that fired and two volume On 2014/07/22 17:20:42, aboxhall wrote: > So the move within the slop region generates a volume change as well? The slop region is an additional grace area so that the user can leave the edges and user a greater area to perform a volume change after they have started the volume change within the original bounds. https://codereview.chromium.org/385073009/diff/240001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1490: // DO SOMETHING TO MAKE THESE CONSTANTS ACCESSIBLE FROM ORIGINAL FILE On 2014/07/22 17:14:55, dmazzoni wrote: > Define the constants you need in the TouchExplorationController class (in > touch_exploration_controller.h) and then use TouchExplorationControllerTestApi > to access them from here. Done.
https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:223: // Swipe/scroll gestures within these bounds (in dips) will change preset Nit: newline before this. https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:146: float kMaxDistanceFromEdge() const { This is basically right but you can't name a function like a constant. I'd probably call it max_distance_from_edge(), or GetMaxDistanceFromEdge() would be okay too.
lgtm No more concerns from me once you address that last issue
lgtm https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:223: // Swipe/scroll gestures within these bounds (in dips) will change preset nit: s/dips/DIPs/ https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:146: float kMaxDistanceFromEdge() const { Is this used?
https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:223: // Swipe/scroll gestures within these bounds (in dips) will change preset On 2014/07/22 18:16:47, aboxhall wrote: > nit: s/dips/DIPs/ Done. https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:146: float kMaxDistanceFromEdge() const { On 2014/07/22 18:15:33, dmazzoni wrote: > This is basically right but you can't name a function like a constant. > > I'd probably call it max_distance_from_edge(), or GetMaxDistanceFromEdge() would > be okay too. Done. https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:146: float kMaxDistanceFromEdge() const { On 2014/07/22 18:16:47, aboxhall wrote: > Is this used? It is used in the slide functions to make sure we are within the correct boundaries.
Still lgtm https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/385073009/diff/260001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:146: float kMaxDistanceFromEdge() const { On 2014/07/22 18:26:51, lisayin wrote: > On 2014/07/22 18:16:47, aboxhall wrote: > > Is this used? > > It is used in the slide functions to make sure we are within the correct > boundaries. Ahhh I see it now. I was confused by the name :) Thanks.
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/385073009/300001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: 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...)
Message was sent while issue was closed.
Change committed as 284819
Message was sent while issue was closed.
On 2014/07/23 00:10:33, I haz the power (commit-bot) wrote: > Change committed as 284819 Hi. Looks like this is breaking the memory bots: http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... The TouchExplorationTest.TestingBoundaries and TouchExplorationTest.EnterSlideGestureState tests have leaks: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x448b2b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0x14703f7 in ui::GestureProviderAura::GetAndResetPendingGestures() ui/events/gestures/gesture_provider_aura.cc:112 #2 0x9f4986 in ui::TouchExplorationController::InSlideGesture(ui::TouchEvent const&, scoped_ptr\u003Cui::Event, base::DefaultDeleter\u003Cui::Event> >*) ui/chromeos/touch_exploration_controller.cc:578 #3 0x9ec04f in ui::TouchExplorationController::RewriteEvent(ui::Event const&, scoped_ptr\u003Cui::Event, base::DefaultDeleter\u003Cui::Event> >*) ui/chromeos/touch_exploration_controller.cc:140 #4 0x146d6d5 in ui::EventSource::SendEventToProcessor(ui::Event*) ui/events/event_source.cc:38 #5 0x9df1cf in ui::test::EventGenerator::DoDispatchEvent(ui::Event*, bool) ui/events/test/event_generator.cc:591 #6 0x9de94c in Dispatch ui/events/test/event_generator.cc:483 #7 0x9de94c in ReleaseTouchId ui/events/test/event_generator.cc:232 #8 0x9de94c in ui::test::EventGenerator::ReleaseTouch() ui/events/test/event_generator.cc:225 #9 0x6985c5 in ui::TouchExplorationTest_TestingBoundaries_Test::TestBody() ui/chromeos/touch_exploration_controller_unittest.cc:1611 #10 0x7b7158 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #11 0x7b7158 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #12 0x7b9558 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #13 0x7ba536 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #14 0x7cc7e5 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #15 0x7cbea0 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #16 0x7cbea0 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #17 0x772e2e in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #18 0x772e2e in base::TestSuite::Run() base/test/test_suite.cc:227 #19 0x7684e9 in Run base/callback.h:401 #20 0x7684e9 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #21 0x767c1c in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553 #22 0x5931da in main ui/base/test/run_all_unittests.cc:105 #23 0x7f85935a576c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 etc.
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/385073009/320001
Message was sent while issue was closed.
Change committed as 285149 |
