Hotword Audio Verification App: UI polishes.

chrome/browser/resources/hotword_audio_verification/main.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 var appWindow = chrome.app.window.current();
 
 document.addEventListener('DOMContentLoaded', function() {
   var flow = new Flow();
   flow.startFlow();
 
   // Make the close buttons close the app window.
   var closeButtons = document.getElementsByClassName('close');
   for (var i = 0; i < closeButtons.length; ++i) {
     var closeButton = closeButtons[i];
     closeButton.addEventListener('click', function(e) {
       appWindow.close();
       e.preventDefault();
     });
   }
 
+  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();
+  };
+
+  $('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.

+
   $('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();
   });
 
   $('done-button').addEventListener('click', function(e) {
     appWindow.close();
     e.preventDefault();
   });
 
-  $('ho-cancel-button').addEventListener('click', function(e) {
-    appWindow.close();
-    e.preventDefault();
-  });
-
   $('hw-cancel-button').addEventListener('click', function(e) {
     appWindow.close();
     e.preventDefault();
   });
-
-  $('st-cancel-button').addEventListener('click', function(e) {
-    appWindow.close();
-    e.preventDefault();
-  });
 
   $('ah-agree-button').addEventListener('click', function(e) {
     // TODO(kcarattini): Set the Audio History setting.
     appWindow.close();
     e.preventDefault();
   });
 
   $('hw-agree-button').addEventListener('click', function(e) {
     flow.advanceStep();
     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.

 
-    if (chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled) {
-      chrome.hotwordPrivate.setHotwordAlwaysOnSearchEnabled(true,
-          flow.advanceStep.bind(flow));
+    var divElem = $(pagePrefix + '-train-' + divNum);
+    divElem.classList.remove('listening');
+    divElem.classList.add('recorded');
+
+    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);
 });