[Local NTP Voice] Substitute fragile string check for a state check.

Substitutes a string-based check in |startListeningMessageAnimation_|
for a proper check of the current state in the speech module.
Adjusts tests (and adds new ones) accordingly.
Removes last TODO from voice.js.

Bug: 583291
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ica08d32233eb69ae5b1930aeb00f34e879eadddf
Reviewed-on: https://chromium-review.googlesource.com/663731
Commit-Queue: Ondrej Škopek <oskopek@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501626}

chrome/browser/resources/local_ntp/voice.js

@@ -581,7 +581,7 @@ speech.isUserAgentMac_ = function() {
  * @param {KeyboardEvent} event The keydown event.
  */
 speech.onKeyDown = function(event) {
-  if (!speech.isRecognizing_()) {
+  if (!speech.isRecognizing()) {
     const ctrlKeyPressed =
         event.ctrlKey || (speech.isUserAgentMac_() && event.metaKey);
     if (speech.currentState_ == speech.State_.READY &&
@@ -765,12 +765,22 @@ speech.resetErrorTimer_ = function(duration) {
 };
 
 
+/**
+ * Check to see if the speech recognition interface is running, and has
+ * received any results.
+ * @return {boolean} True, if the speech recognition interface is running,
+ *     and has received any results.
+ */
+speech.hasReceivedResults = function() {
+  return speech.currentState_ == speech.State_.RESULT_RECEIVED;
+};
+
+
 /**
  * Check to see if the speech recognition interface is running.
  * @return {boolean} True, if the speech recognition interface is running.
- * @private
  */
-speech.isRecognizing_ = function() {
+speech.isRecognizing = function() {
   switch (speech.currentState_) {
     case speech.State_.STARTED:
     case speech.State_.AUDIO_RECEIVED:
@@ -785,7 +795,7 @@ speech.isRecognizing_ = function() {
 /**
  * Check if the controller is in a state where the UI is definitely hidden.
  * Since we show the UI for a few seconds after we receive an error from the
- * API, we need a separate definition to |speech.isRecognizing_()| to indicate
+ * API, we need a separate definition to |speech.isRecognizing()| to indicate
  * when the UI is hidden. <strong>Note:</strong> that if this function
  * returns false, it might not necessarily mean that the UI is visible.
  * @return {boolean} True if the UI is hidden.
@@ -1140,9 +1150,8 @@ text.getTextClassName_ = function() {
  */
 text.startListeningMessageAnimation_ = function() {
   const animateListeningText = function() {
-    // TODO(oskopek): Substitute the fragile string comparison with a correct
-    // state condition.
-    if (text.interim_.textContent == speech.messages.ready) {
+    // If speech is active with no results yet, show the message and animation.
+    if (speech.isRecognizing() && !speech.hasReceivedResults()) {
       text.updateTextArea(speech.messages.listening);
       text.interim_.classList.add(text.LISTENING_ANIMATION_CLASS_);
     }

chrome/test/data/local_ntp/voice_speech_browsertest.js

@@ -222,7 +222,7 @@ test.speech.testFakeboxClickStartsSpeechWithWorkingView = function() {
   assertEquals(speech.State_.STARTED, speech.currentState_);
   assertEquals(1, test.speech.recognitionActiveCount);
   assertEquals(1, test.speech.viewActiveCount);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertTrue(test.speech.clock.isTimeoutSet(speech.idleTimer_));
   assertFalse(test.speech.clock.isTimeoutSet(speech.errorTimer_));
 };
@@ -238,7 +238,7 @@ test.speech.testOmniboxFocusWithWorkingView = function() {
   assertEquals(speech.State_.STARTED, speech.currentState_);
   assertEquals(1, test.speech.recognitionActiveCount);
   assertEquals(1, test.speech.viewActiveCount);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertTrue(test.speech.clock.isTimeoutSet(speech.idleTimer_));
   assertFalse(test.speech.clock.isTimeoutSet(speech.errorTimer_));
 
@@ -275,7 +275,7 @@ test.speech.testHandleAudioStart = function() {
   speech.recognition_.onaudiostart(null);
 
   assertTrue('ready' in test.speech.viewState);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertEquals(1, test.speech.recognitionActiveCount);
   assertFalse(elementIsVisible($(test.speech.FAKEBOX_MICROPHONE_ID)));
 };
@@ -292,7 +292,7 @@ test.speech.testHandleSpeechStart = function() {
   speech.recognition_.onspeechstart(null);
 
   assertTrue('receiving' in test.speech.viewState);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertEquals(1, test.speech.recognitionActiveCount);
   assertFalse(elementIsVisible($(test.speech.FAKEBOX_MICROPHONE_ID)));
 };
@@ -315,7 +315,7 @@ test.speech.testHandleInterimSpeechResponse = function() {
   speech.recognition_.onspeechstart(null);
   speech.recognition_.onresult(responseEvent);
 
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertEquals(highConfidenceText, test.speech.viewState.final);
   assertEquals(viewText, test.speech.viewState.interim);
   assertEquals(highConfidenceText, speech.finalResult_);
@@ -374,7 +374,7 @@ test.speech.testInterruptSpeechInputAfterInterimResult = function() {
   // The user interrupts speech.
   speech.stop();
 
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals('', speech.interimResult_);
   assertEquals('', speech.finalResult_);
   assertEquals(0, test.speech.recognitionActiveCount);
@@ -383,7 +383,7 @@ test.speech.testInterruptSpeechInputAfterInterimResult = function() {
   test.speech.createInterimResponse('result should', 'be ignored');
   speech.recognition_.onresult(responseEvent);
 
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals('', speech.interimResult_);
   assertEquals('', speech.finalResult_);
   assertEquals(0, test.speech.recognitionActiveCount);
@@ -414,7 +414,7 @@ test.speech.testSpeechRecognitionErrorTimeout = function() {
   speech.start();
   speech.recognition_.onerror({error: 'some-error'});
 
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals(1, test.speech.recognitionActiveCount);
   assertEquals(1, test.speech.viewActiveCount);
   assertEquals(RecognitionError.OTHER, test.speech.viewState.error);
@@ -438,7 +438,7 @@ test.speech.testNoSpeechInput = function() {
   speech.recognition_.onaudiostart(null);
   speech.recognition_.onend(null);
 
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals(RecognitionError.NO_SPEECH, test.speech.viewState.error);
 
   test.speech.clock.advanceTime(7999);
@@ -490,7 +490,7 @@ test.speech.testRecognitionHandlersStayInitialized = function() {
   // Stop.
   speech.recognition_.onend(null);
   assertRecognitionHandlers(true);
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals(RecognitionError.NO_SPEECH, test.speech.viewState.error);
 
   test.speech.clock.advanceTime(7999);
@@ -519,11 +519,11 @@ test.speech.testStopStartErrorHandling = function() {
   test.speech.initSpeech();
   speech.start();
   assertEquals(speech.State_.STARTED, speech.currentState_);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.recognition_.onaudiostart(null);
   assertEquals(speech.State_.AUDIO_RECEIVED, speech.currentState_);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.stop();
   assertEquals(speech.State_.READY, speech.currentState_);
@@ -531,7 +531,7 @@ test.speech.testStopStartErrorHandling = function() {
 
   speech.start();
   assertEquals(speech.State_.STARTED, speech.currentState_);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.stop();
   assertEquals(speech.State_.READY, speech.currentState_);
@@ -554,14 +554,14 @@ test.speech.testStopStartKeyboardShortcutErrorHandling = function() {
   assertEquals(speech.State_.STARTED, speech.currentState_);
 
   speech.recognition_.onaudiostart(null);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.onKeyDown(stopShortcut);
   assertEquals(speech.State_.READY, speech.currentState_);
   test.speech.validateInactive();
 
   speech.onKeyDown(startShortcut);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertEquals(speech.State_.STARTED, speech.currentState_);
 
   speech.onKeyDown(stopShortcut);
@@ -635,10 +635,10 @@ test.speech.testKeyboardStartWithCtrl = function() {
   const ctrlShiftPeriod = new KeyboardEvent(
       'test', {ctrlKey: true, code: 'Period', shiftKey: true});
   speech.onKeyDown(ctrlShiftPeriod);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.onKeyDown(ctrlShiftPeriod);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 };
 
 
@@ -671,10 +671,10 @@ test.speech.testKeyboardStartWithCmd = function() {
   // Set a Mac user agent.
   isUserAgentMac = true;
   speech.onKeyDown(cmdShiftPeriod);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 
   speech.onKeyDown(cmdShiftPeriod);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
 };
 
 
@@ -707,7 +707,7 @@ test.speech.testClickToRetryWhenStopped = function() {
 
   speech.onClick_(
       /*submitQuery=*/false, /*shouldRetry=*/true, /*navigatingAway=*/false);
-  assertTrue(speech.isRecognizing_());
+  assertTrue(speech.isRecognizing());
   assertEquals(speech.State_.STARTED, speech.currentState_);
 };
 
@@ -739,7 +739,7 @@ test.speech.testNoSpeechInputMatched = function() {
   speech.recognition_.onspeechstart(null);
   speech.recognition_.onnomatch(null);
 
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals(1, test.speech.recognitionActiveCount);
   assertEquals(1, test.speech.viewActiveCount);
   assertEquals(RecognitionError.NO_MATCH, test.speech.viewState.error);
@@ -880,7 +880,7 @@ test.speech.unInitSpeech = function(fakeboxMicrophoneElem, searchboxApiHandle) {
  * Validates that speech is currently inactive and ready to start recognition.
  */
 test.speech.validateInactive = function() {
-  assertFalse(speech.isRecognizing_());
+  assertFalse(speech.isRecognizing());
   assertEquals(0, test.speech.recognitionActiveCount);
   assertEquals(0, test.speech.viewActiveCount);
   assertEquals('', speech.interimResult_);

chrome/test/data/local_ntp/voice_text_browsertest.js

@@ -176,10 +176,13 @@ test.text.testReadyMessage = function() {
 
 
 /**
- * Test showing the listening message when the ready message is shown.
+ * Test showing the listening message when the ready message is shown,
+ * and results have not yet been received.
  */
 test.text.testListeningMessageWhenReady = function() {
   text.interim_.textContent = 'Ready';
+  test.text.stubs.replace(speech, 'isRecognizing', () => true);
+  test.text.stubs.replace(speech, 'hasReceivedResults', () => false);
 
   test.text.clock.setTime(1);
   text.startListeningMessageAnimation_();
@@ -197,10 +200,38 @@ test.text.testListeningMessageWhenReady = function() {
 
 
 /**
- * Test not showing the listening message when the ready message is not shown.
+ * Test not showing the listening message when the ready message is shown,
+ * but results were already received.
+ */
+test.text.testListeningMessageWhenReadyButResultsAlreadyReceived = function() {
+  text.interim_.textContent = 'Ready';
+  test.text.stubs.replace(speech, 'isRecognizing', () => true);
+  test.text.stubs.replace(speech, 'hasReceivedResults', () => true);
+
+  test.text.clock.setTime(1);
+  text.startListeningMessageAnimation_();
+
+  assertEquals(1, test.text.clock.pendingTimeouts.length);
+  assertEquals(2001, test.text.clock.pendingTimeouts[0].activationTime);
+
+  test.text.clock.advanceTime(2000);
+  test.text.clock.pendingTimeouts.shift().callback();
+
+  // The message was *not* changed to "Listening".
+  assertEquals('Ready', text.interim_.textContent);
+  assertEquals('', text.final_.textContent);
+  assertEquals(0, test.text.clock.pendingTimeouts.length);
+};
+
+
+/**
+ * Test showing the listening message when the ready message is not shown,
+ * and results have not yet been received.
  */
 test.text.testListeningMessageWhenNotReady = function() {
   text.interim_.textContent = 'some text';
+  test.text.stubs.replace(speech, 'isRecognizing', () => true);
+  test.text.stubs.replace(speech, 'hasReceivedResults', () => false);
 
   test.text.clock.setTime(1);
   text.startListeningMessageAnimation_();
@@ -211,7 +242,7 @@ test.text.testListeningMessageWhenNotReady = function() {
   test.text.clock.advanceTime(2000);
   test.text.clock.pendingTimeouts.shift().callback();
 
-  assertEquals('some text', text.interim_.textContent);
+  assertEquals('Listening', text.interim_.textContent);
   assertEquals('', text.final_.textContent);
   assertEquals(0, test.text.clock.pendingTimeouts.length);
 };
@@ -219,7 +250,7 @@ test.text.testListeningMessageWhenNotReady = function() {
 /**
  * Test not showing the listening message when the ready message is spoken.
  */
-test.text.testListeningMessageWhenNotReady = function() {
+test.text.testListeningMessageWhenReadySpoken = function() {
   // Show the "Ready" message.
   text.interim_.textContent = 'Ready';
   assertEquals('', text.final_.textContent);