Hotword Audio Verification App: UI polishes.

chrome/browser/resources/hotword_audio_verification/main.js

Index: chrome/browser/resources/hotword_audio_verification/main.js
diff --git a/chrome/browser/resources/hotword_audio_verification/main.js b/chrome/browser/resources/hotword_audio_verification/main.js
index 8c78ef4a4f05b56f796e6e51e81022f7d3b88722..63282b7bf68b9327fa4be670a9d051cdc1c21e2c 100644
--- a/chrome/browser/resources/hotword_audio_verification/main.js
+++ b/chrome/browser/resources/hotword_audio_verification/main.js
@@ -18,27 +18,28 @@ document.addEventListener('DOMContentLoaded', function() {
     });
   }
 
-  $('ah-cancel-button').addEventListener('click', function(e) {
+  var cancelTrainingHandler = function(e) {

[Dan Beam, 2014/10/23 23:56:00]

nit: closeAppWindow

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

Done.

+    //TODO(kcarattini): Cancel training in NaCl module.

[Dan Beam, 2014/10/23 23:56:01]

// TODO

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

Done.

     appWindow.close();
     e.preventDefault();
-  });
+  };
 
-  $('done-button').addEventListener('click', function(e) {
-    appWindow.close();
-    e.preventDefault();
-  });
+  $('ho-cancel-button').addEventListener('click', cancelTrainingHandler);
+  $('st-cancel-button').addEventListener('click', cancelTrainingHandler);
+  $('ho-close-button').addEventListener('click', cancelTrainingHandler);
+  $('st-close-button').addEventListener('click', cancelTrainingHandler);

[Dan Beam, 2014/10/23 23:56:00]

event delegation?
http://davidwalsh.name/event-delegate

[kcarattini, 2014/10/24 07:02:41]

Done.

 
-  $('ho-cancel-button').addEventListener('click', function(e) {
+  $('ah-cancel-button').addEventListener('click', function(e) {

[Dan Beam, 2014/10/23 23:56:01]

change all of these to use closeAppWindow (that you can)

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

Done.

     appWindow.close();
     e.preventDefault();
   });
 
-  $('hw-cancel-button').addEventListener('click', function(e) {
+  $('done-button').addEventListener('click', function(e) {
     appWindow.close();
     e.preventDefault();
   });
 
-  $('st-cancel-button').addEventListener('click', function(e) {
+  $('hw-cancel-button').addEventListener('click', function(e) {
     appWindow.close();
     e.preventDefault();
   });
@@ -54,29 +55,62 @@ document.addEventListener('DOMContentLoaded', function() {
     e.preventDefault();
   });
 
-  // TODO(kcarattini): Remove this once speech training is implemented. The
-  // way to get to the next page will be to complete the speech training.
-  $('st-training').addEventListener('click', function(e) {
-    if (chrome.hotwordPrivate.setAudioLoggingEnabled)
-      chrome.hotwordPrivate.setAudioLoggingEnabled(true, function() {});
-
-    if (chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled) {
-      chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled(true,
-          flow.advanceStep.bind(flow));
-    }
+  $('settings-link').addEventListener('click', function(e) {
+    chrome.browser.openTab({'url': 'chrome://settings'}, function() {});
     e.preventDefault();
   });
 
-  // TODO(kcarattini): Remove this once speech training is implemented. The
-  // way to get to the next page will be to complete the speech training.
-  $('ho-training').addEventListener('click', function(e) {
-    if (chrome.hotwordPrivate.setAudioLoggingEnabled)
-      chrome.hotwordPrivate.setAudioLoggingEnabled(true, function() {});
+  // TODO(kcarattini): Update this to respond to a hotword trigger once
+  // speech training is implemented.

[Dan Beam, 2014/10/23 23:56:00]

/**
 * @param {string} pagePrefix Doc comment goes here.
 * @param {number} divNum What the heck is a divNum?
 */

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

Done.

+  function doTraining(pagePrefix, divNum) {

[Dan Beam, 2014/10/23 23:56:01]

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?

[Dan Beam, 2014/10/23 23:56:01]

nit: s/divNum/index or trainingStep

[kcarattini, 2014/10/24 07:02:41]

Updated to use querySelectorAll.

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

Done.

+    $(pagePrefix + '-train-text-' + divNum).textContent = 'Recorded';

[Dan Beam, 2014/10/23 23:56:01]

l10n?

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

Added TODO.

+
+    var divElem = $(pagePrefix + '-train-' + divNum);
+    divElem.classList.remove('listening');
+    divElem.classList.add('recorded');
 
-    if (chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled) {
-      chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled(true,
-          flow.advanceStep.bind(flow));
+    if (divNum != 3) {
+      var nextDivElem = $(pagePrefix + '-train-' + (divNum + 1));
+      nextDivElem.classList.remove('not-started');
+      nextDivElem.classList.add('listening');
     }
-    e.preventDefault();
-  });
+  }
+
+  function addTrainingEventListeners(pagePrefix, numDivs) {
+    for (var i = 1; i < numDivs; ++i) {

[Dan Beam, 2014/10/23 23:56:01]

please document that this is i < numDivs instead of i <= numDivs because the
last div is special

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

Done.

+      (function() {
+        var j = i;
+
+        $(pagePrefix + '-train-' + j).addEventListener('click', function(e) {
+          doTraining(pagePrefix, j);

[Dan Beam, 2014/10/23 23:56:01]

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() {...})()

[kcarattini, 2014/10/24 07:02:41]

Updated to use querySelectorAll

+          e.preventDefault();
+        });
+      })();

[Dan Beam, 2014/10/23 23:56:01]

if you really want to keep this self-executing anonymous function, I think this
is simpler

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

instead of var j = i;

+    }
+
+    $(pagePrefix + '-train-' + numDivs).addEventListener('click', function(e) {

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

why isn't this handled above?

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

Nevermind. Missed the rest of the function below.

+      doTraining(pagePrefix, numDivs);
+
+      // Update the 'Cancel' button.
+      // TODO(kcarattini): Localize this string.
+      var buttonElem = $(pagePrefix + '-cancel-button');
+      buttonElem.textContent = 'Please wait ...';
+      buttonElem.classList.add('grayed-out');
+      buttonElem.removeEventListener('click', cancelTrainingHandler);
+
+      setTimeout(function() {
+        if (chrome.hotwordPrivate.setAudioLoggingEnabled)
+          chrome.hotwordPrivate.setAudioLoggingEnabled(true, function() {});
+
+        if (chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled) {
+          chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled(true,
+              flow.advanceStep.bind(flow));
+        }
+        e.preventDefault();
+      }, 2500);
+    });
+  }
+
+  addTrainingEventListeners('st', 3);
+  addTrainingEventListeners('ho', 3);
 });