Corner Passthrough for Accessibility

Implementated corner passthrough when touch exploration
 controller is on. 

If the user presses and holds the bottom right or left
 corner of the screen past a time delay, an earcon will
 sound, and then any subsequent fingers added onto the
 screen will send touch events as normal (passed through)
 as long as one finger is at a corner of the screen. Once
 the finger on the corner of the screen has lifted, then
 nothing will happen until all the fingers on the screen
 have been lifted.

UPDATE: Added earcons (short sound notifications) to
 indicate to the user if they have moved their finger off
 the screen or onto the screen or if they are in passthrough.
BUG=396310
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289468

[lisayin, 2014-07-24 18:10:53]

Implementation of Corner Passthrough

[lisayin, 2014-07-24 18:10:54]

Implementation of Corner Passthrough

[aboxhall, 2014-07-24 18:26:34]

Haven't looked at test yet.

https://codereview.chromium.org/410783002/diff/20001/ash/ash_touch_exploratio...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ash/ash_touch_exploratio...
ash/ash_touch_exploration_manager_chromeos.cc:63:
PlaySystemSoundIfSpokenFeedback(chromeos::SOUND_VOLUME_ADJUST);
nit: indentation

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:203: if (event.touch_id() ==
initial_press_->touch_id() && in_a_bottom_corner) {
I think |in_a_bottom_corner| should be negated...

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:491: if (event.touch_id() ==
initial_press_->touch_id()) {
Some comments explaining this block would be helpful. I gather it's detecting
where you've slightly moved the original finger within the corner and discarding
that event, and also discarding any events on that finger which follow from
moving it out of the corner?

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:492: int edge =
FindEdgesWithinBounds(event.location(), kSlopDistanceFromEdge);
suggestion: s/edge/edges/

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:324: BOTTOM_LEFT_CORNER = LEFT_EDGE |
BOTTOM_EDGE,
Very nice!

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:328: 
nit: rogue newline

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:374: // the allocaated time.
nit: allocaated (Dutch?)

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:375:
base::OneShotTimer<TouchExplorationController> tap_timer_;
This may need to be renamed, based on the comment change.
action_timer_?

[James Cook, 2014-07-24 18:29:17]

Ping me separately for ash OWNERS after dmazzoni/aboxhall are happy with the
a11y bits.

[aboxhall, 2014-07-24 18:34:17]

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1644: gfx::Point
left_corner_press(10,
nit: left_corner rather than left_corner_press.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1646: ui::TouchEvent
first_press(
Everything below this line is common to this and the below test, right? Perhaps
extract it out into a helper method, e.g. "assertCornerPassthroughWorking"?

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1654:
EXPECT_FALSE(IsInTouchToMouseMode());
Explicitly test for CORNER_PASSTHROUGH mode here?

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1656:
AdvanceSimulatedTimePastTapDelay();
Should this be the corner passthrough delay instead?

[lisayin, 2014-07-25 20:11:58]

Added Earcons, fixed tests, and addressed Alice's comments

https://codereview.chromium.org/410783002/diff/20001/ash/ash_touch_exploratio...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ash/ash_touch_exploratio...
ash/ash_touch_exploration_manager_chromeos.cc:63:
PlaySystemSoundIfSpokenFeedback(chromeos::SOUND_VOLUME_ADJUST);
On 2014/07/24 18:26:34, aboxhall wrote:
> nit: indentation

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:203: if (event.touch_id() ==
initial_press_->touch_id() && in_a_bottom_corner) {
On 2014/07/24 18:26:34, aboxhall wrote:
> I think |in_a_bottom_corner| should be negated...

Opps...

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:491: if (event.touch_id() ==
initial_press_->touch_id()) {
On 2014/07/24 18:26:34, aboxhall wrote:
> Some comments explaining this block would be helpful. I gather it's detecting
> where you've slightly moved the original finger within the corner and
discarding
> that event, and also discarding any events on that finger which follow from
> moving it out of the corner?

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:492: int edge =
FindEdgesWithinBounds(event.location(), kSlopDistanceFromEdge);
On 2014/07/24 18:26:34, aboxhall wrote:
> suggestion: s/edge/edges/

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:328: 
On 2014/07/24 18:26:34, aboxhall wrote:
> nit: rogue newline

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:374: // the allocaated time.
On 2014/07/24 18:26:34, aboxhall wrote:
> nit: allocaated (Dutch?)

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1644: gfx::Point
left_corner_press(10,
On 2014/07/24 18:34:16, aboxhall wrote:
> nit: left_corner rather than left_corner_press.

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1646: ui::TouchEvent
first_press(
On 2014/07/24 18:34:16, aboxhall wrote:
> Everything below this line is common to this and the below test, right?
Perhaps
> extract it out into a helper method, e.g. "assertCornerPassthroughWorking"? 

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1654:
EXPECT_FALSE(IsInTouchToMouseMode());
On 2014/07/24 18:34:16, aboxhall wrote:
> Explicitly test for CORNER_PASSTHROUGH mode here?

Done.

https://codereview.chromium.org/410783002/diff/20001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:1656:
AdvanceSimulatedTimePastTapDelay();
On 2014/07/24 18:34:16, aboxhall wrote:
> Should this be the corner passthrough delay instead?

Done.

[dmazzoni, 2014-07-25 20:28:15]

https://codereview.chromium.org/410783002/diff/60001/ash/ash_touch_exploratio...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/410783002/diff/60001/ash/ash_touch_exploratio...
ash/ash_touch_exploration_manager_chromeos.h:38: virtual void PlayEarCon()
OVERRIDE;
This should specify a particular sound to play, like PlayCornerPassthroughSound.
"Earcon" (no capitalization on "con") is generic lingo.

https://codereview.chromium.org/410783002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/410783002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:377:
manager->Initialize(SOUND_PASSTHROUGH,
This isn't ideal since you're initializing the sound here but playing it from
ash.

AccessibilityManager is already implementing an interface defined in ash. Maybe
AccessibilityManager should actually be the one to play the sound, and ash
should send it a message somehow?

[lisayin, 2014-07-25 23:09:30]

https://codereview.chromium.org/410783002/diff/60001/ash/ash_touch_exploratio...
File ash/ash_touch_exploration_manager_chromeos.h (right):

https://codereview.chromium.org/410783002/diff/60001/ash/ash_touch_exploratio...
ash/ash_touch_exploration_manager_chromeos.h:38: virtual void PlayEarCon()
OVERRIDE;
On 2014/07/25 20:28:14, dmazzoni wrote:
> This should specify a particular sound to play, like
PlayCornerPassthroughSound.
> "Earcon" (no capitalization on "con") is generic lingo.

Done.

https://codereview.chromium.org/410783002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/410783002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:377:
manager->Initialize(SOUND_PASSTHROUGH,
On 2014/07/25 20:28:14, dmazzoni wrote:
> This isn't ideal since you're initializing the sound here but playing it from
> ash.
> 
> AccessibilityManager is already implementing an interface defined in ash.
Maybe
> AccessibilityManager should actually be the one to play the sound, and ash
> should send it a message somehow?
> 

In the latest CL, I used the manager and delegate to play a sound through this
file.

[dmazzoni, 2014-07-28 05:32:44]

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/browser_r...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/browser_r...
chrome/browser/browser_resources.grd:414: <include
name="IDR_SOUND_PASSTHROUGH_WAV"
file="resources\chromeos\sounds\earcons\alert_modal.wav" type="BINDATA" />
I don't see this file as part of the changelist. Did you need to git add it?

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:702:
ash::PlaySystemSoundIfSpokenFeedback(SOUND_PASSTHROUGH);
Do you need the switch? Why not just:

ash::PlaySystemSoundIfSpokenFeedback(sound_key);

https://codereview.chromium.org/410783002/diff/80001/chromeos/audio/chromeos_...
File chromeos/audio/chromeos_sounds.h (right):

https://codereview.chromium.org/410783002/diff/80001/chromeos/audio/chromeos_...
chromeos/audio/chromeos_sounds.h:22: SOUND_COUNT,
SOUND_COUNT is supposed to be the last element in the enum - it's not a sound,
it's a number that always represents the total number of sounds.

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:43: // This function should be called
whenever the delegate should play an earcon.
Update this comment; now the function is just for one specific earcon.

[aboxhall, 2014-07-28 16:00:38]

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:297: // bottom
nit: comment formatting

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:300: gfx::Point
passthrough) {
|passthrough| seems to be identical both times this method is called. Any reason
it can't be initialized inside the method rather than being passed in?

[lisayin, 2014-07-28 16:28:54]

Dominic and Alice, PTAL

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/browser_r...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/browser_r...
chrome/browser/browser_resources.grd:414: <include
name="IDR_SOUND_PASSTHROUGH_WAV"
file="resources\chromeos\sounds\earcons\alert_modal.wav" type="BINDATA" />
On 2014/07/28 05:32:44, dmazzoni wrote:
> I don't see this file as part of the changelist. Did you need to git add it?

I'm waiting until after we demo it to add them in.

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/410783002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:702:
ash::PlaySystemSoundIfSpokenFeedback(SOUND_PASSTHROUGH);
On 2014/07/28 05:32:44, dmazzoni wrote:
> Do you need the switch? Why not just:
> 
> ash::PlaySystemSoundIfSpokenFeedback(sound_key);

Done.

https://codereview.chromium.org/410783002/diff/80001/chromeos/audio/chromeos_...
File chromeos/audio/chromeos_sounds.h (right):

https://codereview.chromium.org/410783002/diff/80001/chromeos/audio/chromeos_...
chromeos/audio/chromeos_sounds.h:22: SOUND_COUNT,
On 2014/07/28 05:32:44, dmazzoni wrote:
> SOUND_COUNT is supposed to be the last element in the enum - it's not a sound,
> it's a number that always represents the total number of sounds.

Done.

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.h:43: // This function should be called
whenever the delegate should play an earcon.
On 2014/07/28 05:32:44, dmazzoni wrote:
> Update this comment; now the function is just for one specific earcon.

Done.

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:297: // bottom
On 2014/07/28 16:00:38, aboxhall wrote:
> nit: comment formatting

Done.

https://codereview.chromium.org/410783002/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller_unittest.cc:300: gfx::Point
passthrough) {
On 2014/07/28 16:00:38, aboxhall wrote:
> |passthrough| seems to be identical both times this method is called. Any
reason
> it can't be initialized inside the method rather than being passed in?

Done.

[lisayin, 2014-07-28 17:33:51]

+sky for owners review for browsers_resources_.grd
+jennyz for owners review for chromeos/audio/chromeos_sounds.h

[sky, 2014-07-28 19:04:59]

https://codereview.chromium.org/410783002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/410783002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:414: <include
name="IDR_SOUND_PASSTHROUGH_WAV"
file="resources\chromeos\sounds\earcons\alert_modal.wav" type="BINDATA" />
Idr and file names should match (as the three previous lines do).

[James Cook, 2014-07-28 19:10:42]

ash/* comments

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
ash/accessibility_delegate.h:95: // Plays an earcon.
Can you provide a one-sentence description of what an "earcon" is in this
comment for code readers who may not know?

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:55: if
((!audio_handler_->IsOutputMuted()) ||
Too many parens here:

if (!audio_handler_->IsOutputMuted() || audio_handler_->GetOutputVolumePercent()
!= 100) ...

but is that what you really want?  It seems like this should be !muted &&
!full_volume

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:64: void
AshTouchExplorationManager::PlayExitScreenEarcon() {
nit: blank line above

[lisayin, 2014-07-28 20:31:28]

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
ash/accessibility_delegate.h:95: // Plays an earcon.
On 2014/07/28 19:10:41, James Cook wrote:
> Can you provide a one-sentence description of what an "earcon" is in this
> comment for code readers who may not know?

Done.

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:55: if
((!audio_handler_->IsOutputMuted()) ||
On 2014/07/28 19:10:42, James Cook wrote:
> Too many parens here:
> 
> if (!audio_handler_->IsOutputMuted() ||
audio_handler_->GetOutputVolumePercent()
> != 100) ...
> 
> but is that what you really want?  It seems like this should be !muted &&
> !full_volume

Done.

https://codereview.chromium.org/410783002/diff/140001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:64: void
AshTouchExplorationManager::PlayExitScreenEarcon() {
On 2014/07/28 19:10:42, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/410783002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/410783002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:414: <include
name="IDR_SOUND_PASSTHROUGH_WAV"
file="resources\chromeos\sounds\earcons\alert_modal.wav" type="BINDATA" />
On 2014/07/28 19:04:59, sky wrote:
> Idr and file names should match (as the three previous lines do).

Done.

[lisayin, 2014-07-28 20:35:51]

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
ash/accessibility_delegate.h:95: // Plays an earcon.
On 2014/07/28 20:31:27, lisayin wrote:
> On 2014/07/28 19:10:41, James Cook wrote:
> > Can you provide a one-sentence description of what an "earcon" is in this
> > comment for code readers who may not know?
> 
> Done.
 Sorry I added it to the wrong file. I will add it to this file as well.

[dmazzoni, 2014-07-28 20:39:05]

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/140001/ash/accessibility_deleg...
ash/accessibility_delegate.h:95: // Plays an earcon.
On 2014/07/28 20:35:50, lisayin wrote:
> On 2014/07/28 20:31:27, lisayin wrote:
> > On 2014/07/28 19:10:41, James Cook wrote:
> > > Can you provide a one-sentence description of what an "earcon" is in this
> > > comment for code readers who may not know?
> > 
> > Done.
>  Sorry I added it to the wrong file. I will add it to this file as well.

I'd also be fine with just calling it "sound" here. Earcon is a term we use only
within screen reader code and documentation. (It's a play on the word "icon")

[aboxhall, 2014-07-28 22:41:45]

https://codereview.chromium.org/410783002/diff/180001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/410783002/diff/180001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:1756: ui::TouchEvent
upper_left_corner(
Suggestion (similarly for ExitEarconPlays test): pull these points into a vector
of points, then iterate over them below. 

I'd also suggest (again, similarly for ExitEarconPlays) adding some points which
are on an edge, but not in a corner. (This will be less work if you do the above
first, obviously.)

[James Cook, 2014-07-29 04:27:24]

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
ash/accessibility_delegate.h:98: virtual void
PlayEarcon(media::SoundsManager::SoundKey sound_key) = 0;
I would prefer you keep this an "int". I prefer interface definitions to use
plain-old-data types as much as possible.  In particular, interfaces are often
widely #included. Including sounds_manager.h means that every class that uses
this interface recursively includes hash_table.h, string_piece.h, etc.

Let me know if one of your other reviewers disagrees.

https://codereview.chromium.org/410783002/diff/200001/ash/default_accessibili...
File ash/default_accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/200001/ash/default_accessibili...
ash/default_accessibility_delegate.h:12: #include
"media/audio/sounds/sounds_manager.h"
Not needed. In general, if a superclass definition will define a type you don't
have to include the header for that type.

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:26: 
nit: no blank line

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:204:
DCHECK(chromeos::AccessibilityManager::Get());
We don't generally DCHECK something just before dereferencing it. If it's null,
the next line will just crash and it should be obvious why.

[dmazzoni, 2014-07-29 05:47:50]

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
ash/accessibility_delegate.h:98: virtual void
PlayEarcon(media::SoundsManager::SoundKey sound_key) = 0;
On 2014/07/29 04:27:24, James Cook wrote:
> I would prefer you keep this an "int". I prefer interface definitions to use
> plain-old-data types as much as possible.  In particular, interfaces are often
> widely #included. Including sounds_manager.h means that every class that uses
> this interface recursively includes hash_table.h, string_piece.h, etc.
> 
> Let me know if one of your other reviewers disagrees.

That's a good reason. How about a comment here specifying the file where the
enums you're supposed to use are defined, then?

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:204:
DCHECK(chromeos::AccessibilityManager::Get());
On 2014/07/29 04:27:24, James Cook wrote:
> We don't generally DCHECK something just before dereferencing it. If it's
null,
> the next line will just crash and it should be obvious why.

I see that pattern a fair amount. Must be different conventions in different
parts of the code.

[James Cook, 2014-07-29 15:45:51]

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:204:
DCHECK(chromeos::AccessibilityManager::Get());
On 2014/07/29 05:47:50, dmazzoni wrote:
> On 2014/07/29 04:27:24, James Cook wrote:
> > We don't generally DCHECK something just before dereferencing it. If it's
> null,
> > the next line will just crash and it should be obvious why.
> 
> I see that pattern a fair amount. Must be different conventions in different
> parts of the code.

Yeah. I use it as a way to establish preconditions for input to a function,
especially if that function is long:

void Foo(A* a, B* b) {
  DCHECK(a);  // precondition
  // No DCHECK(b) so it's probably OK to be NULL
  ...lots of code...
  a->DoSomething();
}

But I generally don't for things like:

void Foo(A* a, B* b) {
  a->DoSomething();  // Should be immediately obvious it can't be null
}

or

void Foo() {
  SomeAnnoyingSingleton::Get()->DoSomething();
}

That said, the rest of this file DCHECK's it, so in the interest of consistency,
keep the DCHECK.

[lisayin, 2014-07-30 15:32:51]

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/200001/ash/accessibility_deleg...
ash/accessibility_delegate.h:98: virtual void
PlayEarcon(media::SoundsManager::SoundKey sound_key) = 0;
On 2014/07/29 05:47:49, dmazzoni wrote:
> On 2014/07/29 04:27:24, James Cook wrote:
> > I would prefer you keep this an "int". I prefer interface definitions to use
> > plain-old-data types as much as possible.  In particular, interfaces are
often
> > widely #included. Including sounds_manager.h means that every class that
uses
> > this interface recursively includes hash_table.h, string_piece.h, etc.
> > 
> > Let me know if one of your other reviewers disagrees.
> 
> That's a good reason. How about a comment here specifying the file where the
> enums you're supposed to use are defined, then?

Done.

https://codereview.chromium.org/410783002/diff/200001/ash/default_accessibili...
File ash/default_accessibility_delegate.h (right):

https://codereview.chromium.org/410783002/diff/200001/ash/default_accessibili...
ash/default_accessibility_delegate.h:12: #include
"media/audio/sounds/sounds_manager.h"
On 2014/07/29 04:27:24, James Cook wrote:
> Not needed. In general, if a superclass definition will define a type you
don't
> have to include the header for that type.

Done.

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:26: 
On 2014/07/29 04:27:24, James Cook wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/410783002/diff/200001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:204:
DCHECK(chromeos::AccessibilityManager::Get());
On 2014/07/29 15:45:51, James Cook wrote:
> On 2014/07/29 05:47:50, dmazzoni wrote:
> > On 2014/07/29 04:27:24, James Cook wrote:
> > > We don't generally DCHECK something just before dereferencing it. If it's
> > null,
> > > the next line will just crash and it should be obvious why.
> > 
> > I see that pattern a fair amount. Must be different conventions in different
> > parts of the code.
> 
> Yeah. I use it as a way to establish preconditions for input to a function,
> especially if that function is long:
> 
> void Foo(A* a, B* b) {
>   DCHECK(a);  // precondition
>   // No DCHECK(b) so it's probably OK to be NULL
>   ...lots of code...
>   a->DoSomething();
> }
> 
> But I generally don't for things like:
> 
> void Foo(A* a, B* b) {
>   a->DoSomething();  // Should be immediately obvious it can't be null
> }
> 
> or
> 
> void Foo() {
>   SomeAnnoyingSingleton::Get()->DoSomething();
> }
> 
> That said, the rest of this file DCHECK's it, so in the interest of
consistency,
> keep the DCHECK.

Acknowledged.

[James Cook, 2014-07-30 15:36:35]

*/ash/* LGTM

[aboxhall, 2014-08-06 18:10:45]

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:92: if (FindEdgesWithinBounds(edges,
kLeavingScreenEdge) != NO_EDGE)
Do we want to not play this if a user already has a finger down? I'm wondering
whether this logic belongs in InNoFingersDown instead.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:101: if
(FindEdgesWithinBounds(edges, kLeavingScreenEdge) != NO_EDGE) {
Perhaps this needs to check for state as well: I imagine there are several
states for which it doesn't make sense (e.g. mid-swipe gesture? Do we want
someone to know they swiped off the screen if a swipe has already been
successfully registered?)

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:188: waiting_for_corner_passthrough_
= true;
Why not just use the long_press_timer_ (or corner_passthrough_timer_) to track
whether we're waiting for corner passthrough? When does it make sense to set
waiting_for_corner_passthrough_ to false but not stop the timer?

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:217: if
(long_press_timer_.IsRunning()) {
If we're not going to use this for anything other than corner passthrough,
perhaps it would be better named 'corner_passthrough_timer_'.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:221: OnTapTimerFired();
Why do this? Could you add a comment?

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:702: if
(waiting_for_corner_passthrough_) {
Invert this logic:
if (!waiting_for_corner_passthrough_)
  return;
...

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.h:213: // It fires after the long_press
timeout elapses (500 ms by default). If the
Looks like it's 700ms now. Maybe just refer to the variable name instead, to
stop this comment getting out of date?

[lisayin, 2014-08-06 20:37:36]

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:92: if (FindEdgesWithinBounds(edges,
kLeavingScreenEdge) != NO_EDGE)
On 2014/08/06 18:10:45, aboxhall wrote:
> Do we want to not play this if a user already has a finger down? I'm wondering
> whether this logic belongs in InNoFingersDown instead.

Hmmm. Good point. I'll move it to InNoFingersDown.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:101: if
(FindEdgesWithinBounds(edges, kLeavingScreenEdge) != NO_EDGE) {
I actually think it makes sense to notify the user that their swipe has left the
screen since it'll still process the gesture even if they've left the screen.
Swiping off the screen in the middle of a gesture will cause it to process since
we're only checking the type of the event and not where it's located.

On 2014/08/06 18:10:45, aboxhall wrote:
> Perhaps this needs to check for state as well: I imagine there are several
> states for which it doesn't make sense (e.g. mid-swipe gesture? Do we want
> someone to know they swiped off the screen if a swipe has already been
> successfully registered?)

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:188: waiting_for_corner_passthrough_
= true;
On 2014/08/06 18:10:45, aboxhall wrote:
> Why not just use the long_press_timer_ (or corner_passthrough_timer_) to track
> whether we're waiting for corner passthrough? When does it make sense to set
> waiting_for_corner_passthrough_ to false but not stop the timer?

I'm hoping to use this timer for evy's passthrough implementation as well so
that they both will fire an earcon if activated, but I have removed
waiting_for_corner_passthrough_ and renamed the timer to passthrough_timer_.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:217: if
(long_press_timer_.IsRunning()) {
On 2014/08/06 18:10:44, aboxhall wrote:
> If we're not going to use this for anything other than corner passthrough,
> perhaps it would be better named 'corner_passthrough_timer_'.

Done.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:221: OnTapTimerFired();
On 2014/08/06 18:10:44, aboxhall wrote:
> Why do this? Could you add a comment?


This is in case the finger moves out of the corner before the long press timer
has fired but after the tap timer has fired to make sure you can still go into
touch exploration.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:702: if
(waiting_for_corner_passthrough_) {
On 2014/08/06 18:10:44, aboxhall wrote:
> Invert this logic:
> if (!waiting_for_corner_passthrough_)
>   return;
> ...

Done.

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.h (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.h:213: // It fires after the long_press
timeout elapses (500 ms by default). If the
On 2014/08/06 18:10:45, aboxhall wrote:
> Looks like it's 700ms now. Maybe just refer to the variable name instead, to
> stop this comment getting out of date?

Done.

[aboxhall, 2014-08-06 21:03:12]

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:101: if
(FindEdgesWithinBounds(edges, kLeavingScreenEdge) != NO_EDGE) {
On 2014/08/06 20:37:35, lisayin wrote:
> I actually think it makes sense to notify the user that their swipe has left
the
> screen since it'll still process the gesture even if they've left the screen.
> Swiping off the screen in the middle of a gesture will cause it to process
since
> we're only checking the type of the event and not where it's located.

I guess it could be handy knowing that you'll consistently hear a beep if you
swipe off the edge, but I could imagine that if you're simply performing a swipe
gesture you don't care whether you've swiped off the edge: chances are you'll
lift your finger very shortly afterwards anyway (or else swipe off the screen
altogether) so it'll just be noise. Is that not the case?

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:221: OnTapTimerFired();
On 2014/08/06 20:37:35, lisayin wrote:
> On 2014/08/06 18:10:44, aboxhall wrote:
> > Why do this? Could you add a comment?
> 
> 
> This is in case the finger moves out of the corner before the long press timer
> has fired but after the tap timer has fired to make sure you can still go into
> touch exploration.

Ah, I see now. But won't the tap timer have been running (due to line 190) and
caused OnTapTimerFired() to be executed already?

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:700: // The long press timer should
only be running if one finger is in the corner
I see why this comment was added, but I think it's a little confusing out of
context since you're not checking for the stated condition - perhaps reword as
"The long press timer will only fire if the user has held a finger in one of the
passthrough corners for the duration of the passthrough timeout" or similar.

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:708: return;
Is this an error/shouldn't happen condition? Also, nit: use {} since your
condition goes over more than one line.

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:919: if
(passthrough_timer_.IsRunning())
Note that in each of these cases, the IsRunning() check is redundant (I should
have picked that up much earlier, sorry).

[lisayin, 2014-08-06 21:48:23]

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:101: if
(FindEdgesWithinBounds(edges, kLeavingScreenEdge) != NO_EDGE) {
So another thing that kind of keeps the sound from playing is the fact that it
only plays if you're within a very small number of pixels. I played around with
gestures on the pixel and even when I leave the screen, the official "release"
location is actually a lot farther away from the edge. Should I compensate for
this discrepancy?

On 2014/08/06 21:03:12, aboxhall wrote:
> On 2014/08/06 20:37:35, lisayin wrote:
> > I actually think it makes sense to notify the user that their swipe has left
> the
> > screen since it'll still process the gesture even if they've left the
screen.
> > Swiping off the screen in the middle of a gesture will cause it to process
> since
> > we're only checking the type of the event and not where it's located.
> 
> I guess it could be handy knowing that you'll consistently hear a beep if you
> swipe off the edge, but I could imagine that if you're simply performing a
swipe
> gesture you don't care whether you've swiped off the edge: chances are you'll
> lift your finger very shortly afterwards anyway (or else swipe off the screen
> altogether) so it'll just be noise. Is that not the case?

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:221: OnTapTimerFired();
In TapTimerFired, if the long press timer is running, I don't change the state.
So now that the long press timer has been stopped, I need to check to see if the
tap timer should have been fired or not.

On 2014/08/06 21:03:12, aboxhall wrote:
> On 2014/08/06 20:37:35, lisayin wrote:
> > On 2014/08/06 18:10:44, aboxhall wrote:
> > > Why do this? Could you add a comment?
> > 
> > 
> > This is in case the finger moves out of the corner before the long press
timer
> > has fired but after the tap timer has fired to make sure you can still go
into
> > touch exploration.
> 
> Ah, I see now. But won't the tap timer have been running (due to line 190) and
> caused OnTapTimerFired() to be executed already?

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:700: // The long press timer should
only be running if one finger is in the corner
On 2014/08/06 21:03:12, aboxhall wrote:
> I see why this comment was added, but I think it's a little confusing out of
> context since you're not checking for the stated condition - perhaps reword as
> "The long press timer will only fire if the user has held a finger in one of
the
> passthrough corners for the duration of the passthrough timeout" or similar.

Done.

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:708: return;
On 2014/08/06 21:03:12, aboxhall wrote:
> Is this an error/shouldn't happen condition? Also, nit: use {} since your
> condition goes over more than one line.

Done.

https://codereview.chromium.org/410783002/diff/380001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:919: if
(passthrough_timer_.IsRunning())
On 2014/08/06 21:03:12, aboxhall wrote:
> Note that in each of these cases, the IsRunning() check is redundant (I should
> have picked that up much earlier, sorry).

Done.

[aboxhall, 2014-08-06 22:01:25]

lgtm

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/360001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:221: OnTapTimerFired();
On 2014/08/06 21:48:22, lisayin wrote:
> In TapTimerFired, if the long press timer is running, I don't change the
state.
> So now that the long press timer has been stopped, I need to check to see if
the
> tap timer should have been fired or not.

Oh, duh! Makes sense, thanks for the explanation :)

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:712: DCHECK(touch_locations_.end()
!=
- Maybe combine this with the check above? (I think NOTREACHED is just a
DCHECK(false))
- I think the "Check that" in the comment is unnecessary
- This is a bit of a 'yoda
condition'(http://blog.codinghorror.com/new-programming-jargon/) - I'd swap the
two sides of the inequation

Also, I know I suggested that this might be a "shouldn't happen", but maybe
consider logging a warning rather than crashing if there's any chance it might
happen in practice, unless there's some chance it could be in an inconsistent
state.

[lisayin, 2014-08-06 22:18:20]

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:712: DCHECK(touch_locations_.end()
!=
Would the change in this next patch be appropriate?

On 2014/08/06 22:01:25, aboxhall wrote:
> - Maybe combine this with the check above? (I think NOTREACHED is just a
> DCHECK(false))
> - I think the "Check that" in the comment is unnecessary
> - This is a bit of a 'yoda
> condition'(http://blog.codinghorror.com/new-programming-jargon/) - I'd swap
the
> two sides of the inequation
> 
> Also, I know I suggested that this might be a "shouldn't happen", but maybe
> consider logging a warning rather than crashing if there's any chance it might
> happen in practice, unless there's some chance it could be in an inconsistent
> state.

[lisayin, 2014-08-06 22:41:08]

sky, jennyz PTAL at chrome/ and chromeos/ files. Thank you!

[aboxhall, 2014-08-06 23:35:19]

lgtm

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/410783002/diff/420001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:712: DCHECK(touch_locations_.end()
!=
On 2014/08/06 22:18:19, lisayin wrote:
> Would the change in this next patch be appropriate?

Looks good; I'd only add a hint in the log message that the passthrough timer
fired, to make it easier to figure out the issue if we see this log start
showing up :)

[jennyz, 2014-08-06 23:44:42]

lgtm

[sky, 2014-08-07 21:05:28]

LGTM

[dmazzoni, 2014-08-11 08:29:43]

lgtm

[lisayin, 2014-08-13 21:28:33]

The CQ bit was checked by lisayin@chromium.org

[commit-bot: I haz the power, 2014-08-13 21:29:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lisayin@chromium.org/410783002/510001

[commit-bot: I haz the power, 2014-08-14 05:24:00]

Committed patchset #25 (510001) as 289468