Hotword: Check for a speaker model before launching the opt-in flow

chrome/browser/search/hotword_service.h

 // Copyright 2013 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.
 
 #ifndef CHROME_BROWSER_SEARCH_HOTWORD_SERVICE_H_
 #define CHROME_BROWSER_SEARCH_HOTWORD_SERVICE_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 119 matching lines @@
   // Sets the pref value of the previous language.
   void SetPreviousLanguagePref();
 
   // Returns the current error message id. A value of 0 indicates
   // no error.
   int error_message() { return error_message_; }
 
   // These methods are for launching, and getting and setting the launch mode of
   // the Hotword Audio Verification App.
   //
-  // TODO(kcarattini): Remove this when
-  // https://code.google.com/p/chromium/issues/detail?id=165573 is fixed,
-  // at which time we can simply launch the app in the given mode instead of
-  // having to check for it here.
+  // MaybeLaunchHotwordAudioVerificationApp first determines if the app needs to
+  // be launched, and if so, launches the app (if Audio History is on and a
+  // speaker model exists, then we don't need to launch the app).
+  //
+  // LaunchHotwordAudioVerificationApp launches the app without the above
+  // check in the specified |launch_mode|.
   enum LaunchMode {
     HOTWORD_ONLY,
     HOTWORD_AND_AUDIO_HISTORY,
     RETRAIN
   };
+  void MaybeLaunchHotwordAudioVerificationApp(const LaunchMode& launch_mode);

[James Hawkins, 2015/03/19 22:57:07]

I highly suggest not naming methods MaybeBlah.  It's incredibly confusing, as
it's not clear under what circumstances the method will do Blah without reading
through the method implementation.

It's even more confusing given that there's already a method
LaunchHotwordAudioVerificationApp.

The solution to this issue is to move the logic higher up.

[Anand Mistry (off Chromium), 2015/03/20 01:37:55]

Another option might be to name the function after the action that it's supposed
to handle. Like UserTriesToTurnOnAlwaysOnHotwording(), but not in so many words.

[kcarattini, 2015/03/20 04:45:01]

Renamed to OptIntoHotwording

   void LaunchHotwordAudioVerificationApp(const LaunchMode& launch_mode);
   virtual LaunchMode GetHotwordAudioVerificationLaunchMode();
 
+  // Called when the SpeakerModelExists request is complete. Either
+  // sets the always-on hotword pref to true, or launches the Hotword
+  // Audio Verification App, depending on the value of |exists|.
+  void SpeakerModelExistsComplete(bool exists);
+
   // These methods control the speaker training communication between
   // the Hotword Audio Verification App and the Hotword Extension that
   // contains the NaCl module.
   void StartTraining();
   void FinalizeSpeakerModel();
   void StopTraining();
   void NotifyHotwordTriggered();
 
   // Returns true if speaker training is currently in progress.
   bool IsTraining();
@@ skipping 58 matching lines @@
 
   // The WeakPtrFactory should be the last member, so the weak pointer
   // gets invalidated before the destructors for other members run,
   // to avoid callbacks into a half-destroyed object.
   base::WeakPtrFactory<HotwordService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HotwordService);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_HOTWORD_SERVICE_H_