Hotword Audio Verification App: Adds to hotwordPrivate API

chrome/browser/search/hotword_service.cc

Index: chrome/browser/search/hotword_service.cc
diff --git a/chrome/browser/search/hotword_service.cc b/chrome/browser/search/hotword_service.cc
index f12ef059c4b7384b4f1a159e0a961b94ac319364..d16650139e1bf0f4aa7236bb47f6d097800aacb4 100644
--- a/chrome/browser/search/hotword_service.cc
+++ b/chrome/browser/search/hotword_service.cc
@@ -452,7 +452,7 @@ void HotwordService::DisableHotwordExtension(
 
 void HotwordService::OnHotwordAlwaysOnSearchEnabledChanged(
     const std::string& pref_name) {
-  DCHECK_EQ(pref_name, std::string(prefs::kHotwordAlwaysOnSearchEnabled));
+    DCHECK_EQ(pref_name, std::string(prefs::kHotwordAlwaysOnSearchEnabled));

[benwells, 2014/09/09 20:57:30]

The indenting was correct before.

[kcarattini, 2014/09/10 08:14:28]

Done.

 
   ExtensionService* extension_service = GetExtensionService(profile_);
   if (!extension_service)
@@ -464,11 +464,32 @@ void HotwordService::OnHotwordAlwaysOnSearchEnabledChanged(
     return;
 
   if (profile_->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled)) {
+    // TODO(kcarattini): Check if the app is already running. If so, bring it

[benwells, 2014/09/09 20:57:31]

This TODO is valid but should be in the app itself, as that's where you'd do
this. That is, you always send onLaunched. In the app's onLaunched handler it
can detect if it has a window visible and can focus the window if it does.

[kcarattini, 2014/09/10 08:14:28]

Done.

+    // to focus rather than launching it.
+    SetHotwordAudioVerificationLaunchState(pref_name);
     OpenApplication(AppLaunchParams(
         profile_, extension, extensions::LAUNCH_CONTAINER_WINDOW, NEW_WINDOW));
   }
 }
 
+void HotwordService::SetHotwordAudioVerificationLaunchState(
+    const std::string& pref_name) {
+  if (pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled)) {
+    if (profile_->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled)) {
+      hotword_audio_verification_launch_state_ = HOTWORD_ONLY;
+    } else {
+      hotword_audio_verification_launch_state_ = HOTWORD_AND_AUDIO_HISTORY;
+    }
+  } else if (pref_name == std::string(prefs::kHotwordAudioLoggingEnabled)){

[rpetterson, 2014/09/09 20:07:32]

Does this function (SetHotwordAudioVerificationLaunchState) get called
elsewhere? If not, then this condition will never be met since above we DCHECK
for pref_name == std::string(prefs::kHotwordAlwaysOnSearchEnabled.

[benwells, 2014/09/09 20:57:31]

I think it would be clearer if this function didn't exist, but the caller just
set hotword_audio_verification_launch_state to whatever it thought was
necessary. It is confusing setting this value based on a pref_name.

[kcarattini, 2014/09/10 08:14:28]

Yes, it will be called when the user retrains the model, and from anywhere we
might put a promo link.

The logic I had here doesn't make sense given the latest mocks, so I am removing
all of this and adding a TODO to follow up in a separate cl.

[kcarattini, 2014/09/10 08:14:28]

Refactored this. Please let me know if it makes more sense. The caller of the
launch function might be external to hotword_service.

+    hotword_audio_verification_launch_state_ = AUDIO_HISTORY_ONLY;
+  }
+}
+
+HotwordService::LaunchMode
+HotwordService::GetHotwordAudioVerificationLaunchState() {
+  return hotword_audio_verification_launch_state_;
+}
+
 void HotwordService::OnHotwordSearchEnabledChanged(
     const std::string& pref_name) {
   DCHECK_EQ(pref_name, std::string(prefs::kHotwordSearchEnabled));