Record ChromeVox metrics

Record ChromeVox metrics

BUG=617790, 661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/db1fd8112d8835f1173dd02397275c3052b8475d
Cr-Commit-Position: refs/heads/master@{#432729}

[dmazzoni, 2016-11-15 00:30:31]

Description was changed from

==========
Record ChromeVox metrics

BUG=
==========

to

==========
Record ChromeVox metrics

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-11-15 00:31:10]

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

[dmazzoni, 2016-11-15 00:31:11]

David, could you look at the ChromeVox code first?

[David Tseng, 2016-11-15 06:20:39]

https://codereview.chromium.org/2495373005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2495373005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:234:
useNext ? 1 : 0);
Do we really want this called every time getMode gets called?

https://codereview.chromium.org/2495373005/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2495373005/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:2671: navigation action, like navigating to
the next object on the screen.
Not really; it's just next and previous line

[dmazzoni, 2016-11-15 17:42:00]

Description was changed from

==========
Record ChromeVox metrics

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Record ChromeVox metrics

BUG=661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-11-15 17:48:24]

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

[dmazzoni, 2016-11-15 17:48:47]

Addressed feedback and updated externs

https://codereview.chromium.org/2495373005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2495373005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:234:
useNext ? 1 : 0);
On 2016/11/15 06:20:39, David Tseng wrote:
> Do we really want this called every time getMode gets called?

Oops, no. Moved to the Background constructor.

https://codereview.chromium.org/2495373005/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2495373005/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:2671: navigation action, like navigating to
the next object on the screen.
On 2016/11/15 06:20:39, David Tseng wrote:
> Not really; it's just next and previous line

I changed the code in command_handler to record all navigations.

[commit-bot: I haz the power, 2016-11-15 17:49:08]

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

[David Tseng, 2016-11-15 18:11:17]

lgtm

[dmazzoni, 2016-11-15 18:20:31]

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

[dmazzoni, 2016-11-15 18:20:32]

+isherman for tools/metrics

[commit-bot: I haz the power, 2016-11-15 19:08:41]

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

[commit-bot: I haz the power, 2016-11-15 19:08:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2016-11-15 22:40:32]

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:2651: <action name="ChromeVox.Jump">
nit: It might be good to use the same "Accessibility" prefix here as with the
histogram metrics.

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:120: +    version of ChromeVox.
Hmm, I'm confused: What is the "next" version?  Next relative to what?  Could
you please expand this description a bit?  Also, please document when precisely
this metric is logged -- is it at browser startup?  When interacting with the
Chrome Vox system?  Something else?

[dmazzoni, 2016-11-16 05:11:52]

Description was changed from

==========
Record ChromeVox metrics

BUG=661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Record ChromeVox metrics

BUG=617790,661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-11-16 18:21:25]

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:2651: <action name="ChromeVox.Jump">
On 2016/11/15 22:40:32, Ilya Sherman wrote:
> nit: It might be good to use the same "Accessibility" prefix here as with the
> histogram metrics.

Done.

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2495373005/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:120: +    version of ChromeVox.
On 2016/11/15 22:40:32, Ilya Sherman wrote:
> Hmm, I'm confused: What is the "next" version?  Next relative to what?  Could
> you please expand this description a bit?  Also, please document when
precisely
> this metric is logged -- is it at browser startup?  When interacting with the
> Chrome Vox system?  Something else?

"ChromeVox Next" is the name we gave to a new ChromeVox experience
during this period when users can toggle between the old and new
versions. I clarified that in the comment.

[dmazzoni, 2016-11-16 18:21:28]

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

[commit-bot: I haz the power, 2016-11-16 18:22:12]

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, 2016-11-16 20:24:25]

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

[commit-bot: I haz the power, 2016-11-16 20:24:26]

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_...)

[Ilya Sherman, 2016-11-16 23:14:47]

Thanks!  Metrics LGTM % a final nit:

https://codereview.chromium.org/2495373005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2495373005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:122: +    once each time ChromeVox
starts up.
Thanks, this is much clearer to me!  I still have one point of confusion: there
are two conditions listed in the first sentence: (1) spoken feedback must be
enabled, and (2) the user must be using ChromeVox Next.  I *think* that this
histogram is only emitted if (1) is true, and records whether (2) is true.  But,
I could also interpret this as meaning that the histogram is recorded even when
(1) is false, and the output value records whether either (1) or (2) (or both)
were false.  Could you please clarify the phrasing a bit to make this
distinction clearer?

[dmazzoni, 2016-11-16 23:39:08]

https://codereview.chromium.org/2495373005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2495373005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:122: +    once each time ChromeVox
starts up.
On 2016/11/16 23:14:47, Ilya Sherman wrote:
> Thanks, this is much clearer to me!  I still have one point of confusion:
there
> are two conditions listed in the first sentence: (1) spoken feedback must be
> enabled, and (2) the user must be using ChromeVox Next.  I *think* that this
> histogram is only emitted if (1) is true, and records whether (2) is true. 
But,
> I could also interpret this as meaning that the histogram is recorded even
when
> (1) is false, and the output value records whether either (1) or (2) (or both)
> were false.  Could you please clarify the phrasing a bit to make this
> distinction clearer?

Ah, that makes sense.

Clarified.

[dmazzoni, 2016-11-16 23:39:54]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-11-16 23:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2495373005/#ps60001
(title: "Reword Accessibility.CrosChromeVoxNext description")

[commit-bot: I haz the power, 2016-11-16 23:41:05]

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, 2016-11-17 03:45:41]

Description was changed from

==========
Record ChromeVox metrics

BUG=617790,661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Record ChromeVox metrics

BUG=617790,661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-17 03:45:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 03:47:28]

Description was changed from

==========
Record ChromeVox metrics

BUG=617790,661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Record ChromeVox metrics

BUG=617790,661056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/db1fd8112d8835f1173dd02397275c3052b8475d
Cr-Commit-Position: refs/heads/master@{#432729}
==========

[commit-bot: I haz the power, 2016-11-17 03:47:30]

Patchset 4 (id:??) landed as
https://crrev.com/db1fd8112d8835f1173dd02397275c3052b8475d
Cr-Commit-Position: refs/heads/master@{#432729}