Hotword Audio Verification App: UI polishes.

Hotword Audio Verification App: UI polishes.

Adds urls to the settings and 'learn more' links.
Adds CSS animation to the speech training pages.
Adds steps through speech training UI with clicks.

BUG=390086
Committed: https://crrev.com/d21850201a4ab626fdc703d9e534c04b683895d3
Cr-Commit-Position: refs/heads/master@{#301542}

[kcarattini, 2014-10-23 00:42:56]

kcarattini@chromium.org changed reviewers:
+ dbeam@chromium.org, rlp@chromium.org

[kcarattini, 2014-10-23 00:42:57]

rlp: please review overall
dbeam: please review for OWNERS approval in chrome/browser/resources/...

[rpetterson, 2014-10-23 22:24:46]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:91: $(pagePrefix +
'-train-' + numDivs).addEventListener('click', function(e) {
why isn't this handled above?

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html:19:
<a href="https://support.google.com/websearch/answer/6030020"
Is there a way to use a constant here? Or I think last time we had a link that
the ConOps folks could change . . . maybe take a look at how we did it for the
other button?

[rpetterson, 2014-10-23 22:35:22]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:91: $(pagePrefix +
'-train-' + numDivs).addEventListener('click', function(e) {
On 2014/10/23 22:24:46, rpetterson wrote:
> why isn't this handled above?

Nevermind. Missed the rest of the function below.

[rpetterson, 2014-10-23 22:42:12]

LGTM for overall flow (although my JS is weak)

discussed remaining issues offline.

[Dan Beam, 2014-10-23 23:56:02]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:21: var
cancelTrainingHandler = function(e) {
nit: closeAppWindow

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:22:
//TODO(kcarattini): Cancel training in NaCl module.
// TODO

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:30:
$('st-close-button').addEventListener('click', cancelTrainingHandler);
event delegation?
http://davidwalsh.name/event-delegate

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:32:
$('ah-cancel-button').addEventListener('click', function(e) {
change all of these to use closeAppWindow (that you can)

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:64: // speech
training is implemented.
/**
 * @param {string} pagePrefix Doc comment goes here.
 * @param {number} divNum What the heck is a divNum?
 */

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:65: function
doTraining(pagePrefix, divNum) {
nit: s/divNum/index or trainingStep

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:65: function
doTraining(pagePrefix, divNum) {
can't both pagePrefix and divNum be extracted via

  var parts = e.currentTarget.id.split('-train-');
  var prefix = parts[0];
  var index = parseInt(parts[0], 10);

where |e| is the passed in click event?

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:66: $(pagePrefix +
'-train-text-' + divNum).textContent = 'Recorded';
l10n?

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:80: for (var i = 1;
i < numDivs; ++i) {
please document that this is i < numDivs instead of i <= numDivs because the
last div is special

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:85:
doTraining(pagePrefix, j);
why not just extract the index from the ID?

  var train = function() {
    doTraining(pagePrefix, parseInt(e.currentTarget.id.slice(-1), 10));
  };

  $(pagePrefix + '-train-' + j).addEventListener('click', train);

remove (function() {...})()

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:88: })();
if you really want to keep this self-executing anonymous function, I think this
is simpler

  (function(j) {
    ...
  }(i));

instead of var j = i;

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:5:
<span id="ho-close-button" class="speech-close"></span>
s/ho/hotword-only/g

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:30:
<div id="ho-train-3" class="train not-started">
can't these just be querySelected, e.g.

  $('ho-training').querySelectorAll('.train')

?

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:5:
<span id="st-close-button" class="speech-close"></span>
s/st/speech-training/g

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/style.css (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/style.css:88:
text-transform: initial;
nit: i guess this is OK but I'd say "none" is more common

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/style.css:192: from {
-webkit-transform: rotate(0);
from { ... }
to { ... }

OR

 from {
   ...
 }
 to {
   ...
 }

take your pick.

if this was a presubmit thing, I fixed that really recently:
http://crbug.com/426286

[kcarattini, 2014-10-24 07:02:42]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:21: var
cancelTrainingHandler = function(e) {
On 2014/10/23 23:56:00, Dan Beam wrote:
> nit: closeAppWindow

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:22:
//TODO(kcarattini): Cancel training in NaCl module.
On 2014/10/23 23:56:01, Dan Beam wrote:
> // TODO

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:30:
$('st-close-button').addEventListener('click', cancelTrainingHandler);
On 2014/10/23 23:56:00, Dan Beam wrote:
> event delegation?
> http://davidwalsh.name/event-delegate

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:32:
$('ah-cancel-button').addEventListener('click', function(e) {
On 2014/10/23 23:56:01, Dan Beam wrote:
> change all of these to use closeAppWindow (that you can)

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:64: // speech
training is implemented.
On 2014/10/23 23:56:00, Dan Beam wrote:
> /**
>  * @param {string} pagePrefix Doc comment goes here.
>  * @param {number} divNum What the heck is a divNum?
>  */

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:65: function
doTraining(pagePrefix, divNum) {
On 2014/10/23 23:56:01, Dan Beam wrote:
> nit: s/divNum/index or trainingStep

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:65: function
doTraining(pagePrefix, divNum) {
On 2014/10/23 23:56:01, Dan Beam wrote:
> can't both pagePrefix and divNum be extracted via
> 
>   var parts = e.currentTarget.id.split('-train-');
>   var prefix = parts[0];
>   var index = parseInt(parts[0], 10);
> 
> where |e| is the passed in click event?

Updated to use querySelectorAll.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:66: $(pagePrefix +
'-train-text-' + divNum).textContent = 'Recorded';
On 2014/10/23 23:56:01, Dan Beam wrote:
> l10n?

Added TODO.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:80: for (var i = 1;
i < numDivs; ++i) {
On 2014/10/23 23:56:01, Dan Beam wrote:
> please document that this is i < numDivs instead of i <= numDivs because the
> last div is special

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:85:
doTraining(pagePrefix, j);
On 2014/10/23 23:56:01, Dan Beam wrote:
> why not just extract the index from the ID?
> 
>   var train = function() {
>     doTraining(pagePrefix, parseInt(e.currentTarget.id.slice(-1), 10));
>   };
> 
>   $(pagePrefix + '-train-' + j).addEventListener('click', train);
> 
> remove (function() {...})()

Updated to use querySelectorAll

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:5:
<span id="ho-close-button" class="speech-close"></span>
On 2014/10/23 23:56:01, Dan Beam wrote:
> s/ho/hotword-only/g

I've been consistently using the two-letter namespace in this app in all of the
other step html files. I think it would be best to remain consistent here.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:30:
<div id="ho-train-3" class="train not-started">
On 2014/10/23 23:56:01, Dan Beam wrote:
> can't these just be querySelected, e.g.
> 
>   $('ho-training').querySelectorAll('.train')
> 
> ?

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:5:
<span id="st-close-button" class="speech-close"></span>
On 2014/10/23 23:56:01, Dan Beam wrote:
> s/st/speech-training/g

see above.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/style.css (right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/style.css:88:
text-transform: initial;
On 2014/10/23 23:56:01, Dan Beam wrote:
> nit: i guess this is OK but I'd say "none" is more common

Done.

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/style.css:192: from {
-webkit-transform: rotate(0);
On 2014/10/23 23:56:01, Dan Beam wrote:
> from { ... }
> to { ... }
> 
> OR
> 
>  from {
>    ...
>  }
>  to {
>    ...
>  }
> 
> take your pick.
> 
> if this was a presubmit thing, I fixed that really recently:
> http://crbug.com/426286

Done.

[Dan Beam, 2014-10-24 19:07:04]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:5:
<span id="ho-close-button" class="speech-close"></span>
On 2014/10/24 07:02:42, kcarattini wrote:
> On 2014/10/23 23:56:01, Dan Beam wrote:
> > s/ho/hotword-only/g
> 
> I've been consistently using the two-letter namespace in this app in all of
the
> other step html files. I think it would be best to remain consistent here.

everywhere else in chrome disagrees with you

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:15: if (classes[i]
== 'close' || classes[i] == 'finish-button') {
var classes = e.target.classList;
if (classes.contains('close') || classes.contains('finish-button'))

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:46: var index = 0;
var index = Math.min(0, Array.prototype.indexOf.call(steps, e.currentTarget));

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:57: if (index !=
steps.length - 1) {
nit: why not if (steps[index + 1]) ?

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:69: // has different
behavior.
indent off

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:81: // Update the
'Cancel' button.
nit: just use the same event listener and detect the last time, e.g.,

for (var i = 0; i < steps.length; ++i) {
  steps[i].addEventListener('click', function(e) {
    doTraining(pagePrefix, e);
    e.preventDefault();

    if (e.currentTarget != steps[steps.length])
      return;

    ...last step code here...
  });
}

[kcarattini, 2014-10-25 03:47:57]

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html
(right):

https://codereview.chromium.org/666073005/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/steps/hotword_only_step.html:5:
<span id="ho-close-button" class="speech-close"></span>
On 2014/10/24 19:07:03, Dan Beam wrote:
> On 2014/10/24 07:02:42, kcarattini wrote:
> > On 2014/10/23 23:56:01, Dan Beam wrote:
> > > s/ho/hotword-only/g
> > 
> > I've been consistently using the two-letter namespace in this app in all of
> the
> > other step html files. I think it would be best to remain consistent here.
> 
> everywhere else in chrome disagrees with you

Done.

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:15: if (classes[i]
== 'close' || classes[i] == 'finish-button') {
On 2014/10/24 19:07:03, Dan Beam wrote:
> var classes = e.target.classList;
> if (classes.contains('close') || classes.contains('finish-button'))

Done.

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:46: var index = 0;
On 2014/10/24 19:07:03, Dan Beam wrote:
> var index = Math.min(0, Array.prototype.indexOf.call(steps, e.currentTarget));

Done.

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:57: if (index !=
steps.length - 1) {
On 2014/10/24 19:07:03, Dan Beam wrote:
> nit: why not if (steps[index + 1]) ?

Done.

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:69: // has different
behavior.
On 2014/10/24 19:07:03, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/666073005/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_audio_verification/main.js:81: // Update the
'Cancel' button.
On 2014/10/24 19:07:04, Dan Beam wrote:
> nit: just use the same event listener and detect the last time, e.g.,
> 
> for (var i = 0; i < steps.length; ++i) {
>   steps[i].addEventListener('click', function(e) {
>     doTraining(pagePrefix, e);
>     e.preventDefault();
> 
>     if (e.currentTarget != steps[steps.length])
>       return;
> 
>     ...last step code here...
>   });
> }
>   

Done.

[Dan Beam, 2014-10-27 16:10:33]

lgtm but please address nits first (either by fixing or justifying)

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.js:42: var index =
Array.prototype.indexOf.call(steps, e.currentTarget);
note: this can return -1

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html
(right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html:21:
</div>
if this is a normal link (i.e. has an href, navigates on click) remove the
is=action-link and class=link-button.

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html
(right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:5:
<span id="speech-training-close-button" class="close"></span>
nit: try not to imply tag names or control types in IDs. these often change or
can be misleading (e.g. <span> with an ID of *-button).

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:11:
<div id="speech-training-training" class="content">
awkward but consistent I guess (-training-training)

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/style.css (right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/style.css:193:
-webkit-animation-timing-function: linear;
nit: -webkit-animation: rotate 2s linear infinite;

[kcarattini, 2014-10-27 23:20:11]

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.js (right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.js:42: var index =
Array.prototype.indexOf.call(steps, e.currentTarget);
On 2014/10/27 16:10:33, Dan Beam wrote:
> note: this can return -1

Done.

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html
(right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/hotword_audio_history_step.html:21:
</div>
On 2014/10/27 16:10:33, Dan Beam wrote:
> if this is a normal link (i.e. has an href, navigates on click) remove the
> is=action-link and class=link-button.

Done.

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html
(right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:5:
<span id="speech-training-close-button" class="close"></span>
On 2014/10/27 16:10:33, Dan Beam wrote:
> nit: try not to imply tag names or control types in IDs. these often change or
> can be misleading (e.g. <span> with an ID of *-button).

Done.

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/steps/speech_training_step.html:11:
<div id="speech-training-training" class="content">
On 2014/10/27 16:10:33, Dan Beam wrote:
> awkward but consistent I guess (-training-training)

Yes. Going for consistency here.

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/style.css (right):

https://codereview.chromium.org/666073005/diff/140001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/style.css:193:
-webkit-animation-timing-function: linear;
On 2014/10/27 16:10:33, Dan Beam wrote:
> nit: -webkit-animation: rotate 2s linear infinite;

Done.

[kcarattini, 2014-10-28 00:02:01]

The CQ bit was checked by kcarattini@chromium.org

[commit-bot: I haz the power, 2014-10-28 00:07:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666073005/200001

[commit-bot: I haz the power, 2014-10-28 01:28:30]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-10-28 01:29:19]

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