[Hotword] Implement audio history pref accessing and setting.

chrome/browser/search/hotword_audio_history_handler.cc

Index: chrome/browser/search/hotword_audio_history_handler.cc
diff --git a/chrome/browser/search/hotword_audio_history_handler.cc b/chrome/browser/search/hotword_audio_history_handler.cc
index 6bf15b0c5f4aaf882aaf509ebd9a3bb72d7542ec..b1a67ad682716e2c95b142e7ef1d041b521b0e4a 100644
--- a/chrome/browser/search/hotword_audio_history_handler.cc
+++ b/chrome/browser/search/hotword_audio_history_handler.cc
@@ -5,17 +5,15 @@
 #include "chrome/browser/search/hotword_audio_history_handler.h"
 
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/history/web_history_service.h"
+#include "chrome/browser/history/web_history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 
 HotwordAudioHistoryHandler::HotwordAudioHistoryHandler(
     content::BrowserContext* context)
-    : profile_(Profile::FromBrowserContext(context)) {
-  pref_change_registrar_.Init(profile_->GetPrefs());
-  pref_change_registrar_.Add(
-    prefs::kHotwordAudioHistoryEnabled,
-    base::Bind(&HotwordAudioHistoryHandler::OnAudioHistoryEnabledChanged,
-    base::Unretained(this)));
+    : profile_(Profile::FromBrowserContext(context)),
+      weak_factory_(this) {
 
   // Poll for current value on init which should happen on first use by
   // way of the hotword service init.
@@ -25,19 +23,41 @@ HotwordAudioHistoryHandler::HotwordAudioHistoryHandler(
 HotwordAudioHistoryHandler::~HotwordAudioHistoryHandler() {
 }
 
-bool HotwordAudioHistoryHandler::GetAudioHistoryEnabled() {
-  // TODO(rlp): fill in.
-  return false;
+void HotwordAudioHistoryHandler::GetAudioHistoryEnabled() {
+  bool success = false;
+  history::WebHistoryService* web_history =
+    WebHistoryServiceFactory::GetForProfile(profile_);
+  if (web_history) {
+    web_history->GetAudioHistoryEnabled(
+        base::Bind(&HotwordAudioHistoryHandler::GetAudioHistoryComplete,
+                   weak_factory_.GetWeakPtr()));
+  } else {
+    // If web_history is null, the user is not signed in.
+    PrefService* prefs = profile_->GetPrefs();
+    prefs->SetBoolean(prefs::kHotwordAudioHistoryEnabled, false);
+  }
 }
 
 void HotwordAudioHistoryHandler::SetAudioHistoryEnabled(const bool enabled) {
-  // TODO(rlp): fill in.
+  history::WebHistoryService* web_history =
+  WebHistoryServiceFactory::GetForProfile(profile_);
+  if (web_history) {
+    web_history->SetAudioHistoryEnabled(

[Anand Mistry (off Chromium), 2014/11/12 00:00:10]

How is the local PrefService setting supposed to be updated when the user calls
this to update the setting?

[rpetterson, 2014/11/12 00:36:04]

Good point. I suppose we should not leave it up to the caller.

Either it can be done in the callback SetAudioHistoryComplete (which makes that
callback essentially identical to GetAudioHistoryComplete) or we can do it
before we make the call.

In reality, with respect to the logging, it should not make a difference since
we will check before logging if it's legit to do so. So as of right now, the
only place the pref will be used is in chrome://settings.

The former is preferable since we don't update unless we know the set call was
successful. The latter is cleaner IMO.

Do you have a preference?

[Anand Mistry (off Chromium), 2014/11/12 00:56:55]

I think the former (on success), since we should aim to be as close to the
current web pref as possible and a successful response indicates what the true
value is.

Speaking of success, I think something should happen on failure. If the request
to "enable" failed, maybe it should be disabled. If a request to "disable"
failed, then... um... dunno.

[rpetterson, 2014/11/12 01:15:42]

I've combined the two callbacks since they would essentially do the same thing:
set the pref, but make it false if it was unsuccessful.

I think this is the correct behavior because we don't want to be logging unless
we're sure they wanted it enabled. In practice, this will only be set from the
hotword verification, so a cancel (don't enable) and a failure should have the
same result of disabling.

+        enabled,
+        base::Bind(&HotwordAudioHistoryHandler::SetAudioHistoryComplete,
+                   weak_factory_.GetWeakPtr()));
+  }
 }
 
-void HotwordAudioHistoryHandler::OnAudioHistoryEnabledChanged(
-    const std::string& pref_name) {
-  DCHECK(pref_name == std::string(prefs::kHotwordAudioHistoryEnabled));
-
+void HotwordAudioHistoryHandler::GetAudioHistoryComplete(
+  bool success, bool new_enabled_value) {
   PrefService* prefs = profile_->GetPrefs();
-  SetAudioHistoryEnabled(prefs->GetBoolean(prefs::kHotwordAudioHistoryEnabled));
+  // Set preference to false if the call was not successful to err on the safe
+  // side.
+  prefs->SetBoolean(prefs::kHotwordAudioHistoryEnabled,
+                    success && new_enabled_value);
+}
+
+void HotwordAudioHistoryHandler::SetAudioHistoryComplete(
+    bool success, bool new_enabled_value) {
 }