Fire accessibilityPrivate events on two-finger hold gesture.

Fire accessibilityPrivate events on two-finger hold gesture.

ChromeBox for Meetings devices want a way to toggle spoken feedback when
the user holds down two fingers. Use existing code that detects this gesture,
but instead of toggling directly, fire private events on accessibilityPrivate
that they can intercept and then trigger with their own UI and timing.

BUG=662501
Review-Url: https://codereview.chromium.org/2808053004
Cr-Commit-Position: refs/heads/master@{#464306}
Committed: https://chromium.googlesource.com/chromium/src/+/df9a1e9283950cdbeaccfbd83587d424f4dc2436

[dmazzoni, 2017-04-10 17:31:42]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 17:32:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-10 17:33:57]

dmazzoni@chromium.org changed reviewers:
+ oshima@chromium.org, rdevlin.cronin@chromium.org

[dmazzoni, 2017-04-10 17:33:57]

oshima: primary review
asvitkine: metrics
devlin: extensions api

[commit-bot: I haz the power, 2017-04-10 17:35:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 17:35:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dmazzoni, 2017-04-10 17:50:06]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 17:50:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 18:16:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 18:16:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2017-04-10 18:32:08]

can you take a look at test failures?

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

https://codereview.chromium.org/2808053004/diff/20001/ash/default_accessibili...
ash/default_accessibility_delegate.h:60: void OnTwoFingerTouchStop() override;
do you need these?

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

https://codereview.chromium.org/2808053004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:605:
std::unique_ptr<base::ListValue> event_args(new base::ListValue());
nit: MakeUnique

same for the rest.

https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
File ui/chromeos/touch_accessibility_enabler.h (right):

https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
ui/chromeos/touch_accessibility_enabler.h:39: 
one suggestion. ash can use ui/chromeos, and chrome/browser/chromeos can
dependds on ash/ui/chromeos, so you could simply let the chrome/browser/chromeos
provides the implementation instead of invoking thorough ash,
AccessibilityDelegate creates the browser impl. (which can still be a single
instance)

[dmazzoni, 2017-04-10 18:54:50]

https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
File ui/chromeos/touch_accessibility_enabler.h (right):

https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
ui/chromeos/touch_accessibility_enabler.h:39: 
On 2017/04/10 18:32:08, oshima wrote:
> one suggestion. ash can use ui/chromeos, and chrome/browser/chromeos can
> dependds on ash/ui/chromeos, so you could simply let the
chrome/browser/chromeos
> provides the implementation instead of invoking thorough ash,
> AccessibilityDelegate creates the browser impl. (which can still be a single
> instance)

To clarify, you're saying AccessibiliiyManager (in
chrome/browser/chromeos/accessibility/) could implement
TouchAccessibilityEnablerDelegate and bypass ash entirely?

[oshima, 2017-04-10 20:19:59]

On 2017/04/10 18:54:50, dmazzoni wrote:
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> File ui/chromeos/touch_accessibility_enabler.h (right):
> 
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> ui/chromeos/touch_accessibility_enabler.h:39: 
> On 2017/04/10 18:32:08, oshima wrote:
> > one suggestion. ash can use ui/chromeos, and chrome/browser/chromeos can
> > dependds on ash/ui/chromeos, so you could simply let the
> chrome/browser/chromeos
> > provides the implementation instead of invoking thorough ash,
> > AccessibilityDelegate creates the browser impl. (which can still be a single
> > instance)
> 
> To clarify, you're saying AccessibiliiyManager (in
> chrome/browser/chromeos/accessibility/) could implement
> TouchAccessibilityEnablerDelegate and bypass ash entirely?

Yes. Do you think there will be a problem? (btw, i'm not asking you to do this
now. One possible simplification that you can do when you have a time)

[Devlin, 2017-04-10 21:12:07]

extensions lgtm.  It's a rather specific API, but, since it's private, that's
probably okay.

[dmazzoni, 2017-04-11 05:48:37]

On 2017/04/10 20:19:59, oshima wrote:
> On 2017/04/10 18:54:50, dmazzoni wrote:
> >
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> > File ui/chromeos/touch_accessibility_enabler.h (right):
> > 
> >
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> > ui/chromeos/touch_accessibility_enabler.h:39: 
> > On 2017/04/10 18:32:08, oshima wrote:
> > > one suggestion. ash can use ui/chromeos, and chrome/browser/chromeos can
> > > dependds on ash/ui/chromeos, so you could simply let the
> > chrome/browser/chromeos
> > > provides the implementation instead of invoking thorough ash,
> > > AccessibilityDelegate creates the browser impl. (which can still be a
single
> > > instance)
> > 
> > To clarify, you're saying AccessibiliiyManager (in
> > chrome/browser/chromeos/accessibility/) could implement
> > TouchAccessibilityEnablerDelegate and bypass ash entirely?
> 
> Yes. Do you think there will be a problem? (btw, i'm not asking you to do this
> now. One possible simplification that you can do when you have a time)

I'm not sure that's going to work under mus+ash, because event filtering in
ui/chromeos
will be in a different process than accessibility_manager.

[dmazzoni, 2017-04-11 05:49:40]

Ready for another look.

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

https://codereview.chromium.org/2808053004/diff/20001/ash/default_accessibili...
ash/default_accessibility_delegate.h:60: void OnTwoFingerTouchStop() override;
On 2017/04/10 18:32:08, oshima wrote:
> do you need these?

No. Looks like an opportunity for cleanup, as most of these
aren't needed.

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

https://codereview.chromium.org/2808053004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:605:
std::unique_ptr<base::ListValue> event_args(new base::ListValue());
On 2017/04/10 18:32:08, oshima wrote:
> nit: MakeUnique
> 
> same for the rest.

Done.

[dmazzoni, 2017-04-11 05:50:03]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 05:50:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 06:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 06:59:20]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-12 21:01:03]

Oshima, any chance you could look today since I'm
OOO the rest of the week? Thanks!

[oshima, 2017-04-12 22:21:16]

On 2017/04/11 05:48:37, dmazzoni wrote:
> On 2017/04/10 20:19:59, oshima wrote:
> > On 2017/04/10 18:54:50, dmazzoni wrote:
> > >
> >
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> > > File ui/chromeos/touch_accessibility_enabler.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2808053004/diff/20001/ui/chromeos/touch_acces...
> > > ui/chromeos/touch_accessibility_enabler.h:39: 
> > > On 2017/04/10 18:32:08, oshima wrote:
> > > > one suggestion. ash can use ui/chromeos, and chrome/browser/chromeos can
> > > > dependds on ash/ui/chromeos, so you could simply let the
> > > chrome/browser/chromeos
> > > > provides the implementation instead of invoking thorough ash,
> > > > AccessibilityDelegate creates the browser impl. (which can still be a
> single
> > > > instance)
> > > 
> > > To clarify, you're saying AccessibiliiyManager (in
> > > chrome/browser/chromeos/accessibility/) could implement
> > > TouchAccessibilityEnablerDelegate and bypass ash entirely?
> > 
> > Yes. Do you think there will be a problem? (btw, i'm not asking you to do
this
> > now. One possible simplification that you can do when you have a time)
> 
> I'm not sure that's going to work under mus+ash, because event filtering in
> ui/chromeos
> will be in a different process than accessibility_manager.

ack

[oshima, 2017-04-12 22:21:48]

lgtm

[dmazzoni, 2017-04-12 22:22:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-04-12 22:22:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2808053004/#ps60001
(title: "Address feedback")

[commit-bot: I haz the power, 2017-04-12 22:23:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 22:39:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 22:39:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2017-04-13 04:28:02]

dmazzoni@chromium.org changed reviewers:
+ mpearson@chromium.org

[dmazzoni, 2017-04-13 04:28:03]

+mpearson for metrics

[Mark P, 2017-04-13 04:36:23]

histograms.xml lgtm

[dmazzoni, 2017-04-13 04:38:59]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2017-04-13 04:39:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 05:10:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492058339326900,
"parent_rev": "caa0b215d5d6ce180c4ee1ec8879630e575dba8f", "commit_rev":
"df9a1e9283950cdbeaccfbd83587d424f4dc2436"}

[commit-bot: I haz the power, 2017-04-13 05:15:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492058339326900,
"parent_rev": "caa0b215d5d6ce180c4ee1ec8879630e575dba8f", "commit_rev":
"df9a1e9283950cdbeaccfbd83587d424f4dc2436"}

[commit-bot: I haz the power, 2017-04-13 05:16:05]

Description was changed from

==========
Fire accessibilityPrivate events on two-finger hold gesture.

ChromeBox for Meetings devices want a way to toggle spoken feedback when
the user holds down two fingers. Use existing code that detects this gesture,
but instead of toggling directly, fire private events on accessibilityPrivate
that they can intercept and then trigger with their own UI and timing.

BUG=662501
==========

to

==========
Fire accessibilityPrivate events on two-finger hold gesture.

ChromeBox for Meetings devices want a way to toggle spoken feedback when
the user holds down two fingers. Use existing code that detects this gesture,
but instead of toggling directly, fire private events on accessibilityPrivate
that they can intercept and then trigger with their own UI and timing.

BUG=662501

Review-Url: https://codereview.chromium.org/2808053004
Cr-Commit-Position: refs/heads/master@{#464306}
Committed:
https://chromium.googlesource.com/chromium/src/+/df9a1e9283950cdbeaccfbd83587...
==========

[commit-bot: I haz the power, 2017-04-13 05:16:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/df9a1e9283950cdbeaccfbd83587...