[Hotword] Implement audio history pref accessing and setting.

This CL adds new functionality to the web history service to access audio history as well since accessing audio history uses the same underlying mechanism as standard web history.

This CL also removes the onChange observer for the pref so that it is entirely handled by the handler. There is no longer a need for it since the only access to the pref is through this handler (i.e., there is no more settings checkbox).

Note: The completion callbacks in the hotword_audio_history_handler will probably be updated once some additional extension apis have been added.

We may eventually shift to a newer technology but it is appearing more likely it might not be ready when we need it. However, we will continue to use this method for the accessing of the audio history pref for quite some time (it's one of the approved methods) even once the setting of the pref switches to the newer model.

BUG=426640

Review URL: https://codereview.chromium.org/687803004

Cr-Commit-Position: refs/heads/master@{#304994}

chrome/browser/extensions/component_loader_unittest.cc

@@ -6,11 +6,13 @@
 
 #include <string>
 
+#include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "chrome/browser/extensions/test_extension_service.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
@@ -222,6 +224,10 @@ TEST_F(ComponentLoaderTest, Remove) {
 }
 
 TEST_F(ComponentLoaderTest, LoadAll) {
+  // This loads the hotword component extension which has a dependency on sync.
+  // However, this doesn't work with unittests so disable sync.
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableSync);
+
   extension_service_.set_ready(false);
   ExtensionRegistry* registry = ExtensionRegistry::Get(&profile_);
 
@@ -243,6 +249,10 @@ TEST_F(ComponentLoaderTest, LoadAll) {
 }
 
 TEST_F(ComponentLoaderTest, AddOrReplace) {
+  // This loads the hotword component extension which has a dependency on sync.
+  // However, this doesn't work with unittests so disable sync.
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableSync);
+
   EXPECT_EQ(0u, component_loader_.registered_extensions_count());
   component_loader_.AddDefaultComponentExtensions(false);
   size_t const default_count = component_loader_.registered_extensions_count();

chrome/browser/history/DEPS

@@ -41,6 +41,7 @@ include_rules = [
   "!chrome/browser/signin/token_service_factory.h",
   "!chrome/browser/sync/profile_sync_service.h",
   "!chrome/browser/sync/profile_sync_service_factory.h",
+  "!chrome/browser/sync/profile_sync_service_mock.h",
   "!chrome/browser/ui/browser.h",
   "!chrome/browser/ui/browser_finder.h",
   "!chrome/browser/ui/profile_error_dialog.h",

chrome/browser/history/web_history_service.cc

@@ -40,6 +40,12 @@ const char kHistoryQueryHistoryUrl[] =
 const char kHistoryDeleteHistoryUrl[] =
     "https://history.google.com/history/api/delete?client=chrome";
 
+const char kHistoryAudioHistoryUrl[] =
+    "https://history.google.com/history/api/lookup?client=audio";
+
+const char kHistoryAudioHistoryChangeUrl[] =
+    "https://history.google.com/history/api/change";
+
 const char kPostDataMimeType[] = "text/plain";
 
 // The maximum number of retries for the URLFetcher requests.
@@ -53,21 +59,19 @@ class RequestImpl : public WebHistoryService::Request,
 
   // Returns the response code received from the server, which will only be
   // valid if the request succeeded.
-  int response_code() { return response_code_; }
+  int GetResponseCode() override { return response_code_; }
 
   // Returns the contents of the response body received from the server.
-  const std::string& response_body() { return response_body_; }
+  const std::string& GetResponseBody() override { return response_body_; }
 
-  bool is_pending() override { return is_pending_; }
+  bool IsPending() override { return is_pending_; }
 
  private:
   friend class history::WebHistoryService;
 
-  typedef base::Callback<void(Request*, bool)> CompletionCallback;
-
   RequestImpl(Profile* profile,
               const GURL& url,
-              const CompletionCallback& callback)
+              const WebHistoryService::CompletionCallback& callback)
       : OAuth2TokenService::Consumer("web_history"),
         profile_(profile),
         url_(url),
@@ -78,7 +82,7 @@ class RequestImpl : public WebHistoryService::Request,
   }
 
   // Tells the request to do its thang.
-  void Start() {
+  void Start() override {
     OAuth2TokenService::ScopeSet oauth_scopes;
     oauth_scopes.insert(kHistoryOAuthScope);
 
@@ -171,7 +175,7 @@ class RequestImpl : public WebHistoryService::Request,
     return fetcher;
   }
 
-  void set_post_data(const std::string& post_data) {
+  void SetPostData(const std::string& post_data) override {
     post_data_ = post_data;
   }
 
@@ -203,27 +207,12 @@ class RequestImpl : public WebHistoryService::Request,
   int auth_retry_count_;
 
   // The callback to execute when the query is complete.
-  CompletionCallback callback_;
+  WebHistoryService::CompletionCallback callback_;
 
   // True if the request was started and has not yet completed, otherwise false.
   bool is_pending_;
 };
 
-// Extracts a JSON-encoded HTTP response into a DictionaryValue.
-// If |request|'s HTTP response code indicates failure, or if the response
-// body is not JSON, a null pointer is returned.
-scoped_ptr<base::DictionaryValue> ReadResponse(RequestImpl* request) {
-  scoped_ptr<base::DictionaryValue> result;
-  if (request->response_code() == net::HTTP_OK) {
-    base::Value* value = base::JSONReader::Read(request->response_body());
-    if (value && value->IsType(base::Value::TYPE_DICTIONARY))
-      result.reset(static_cast<base::DictionaryValue*>(value));
-    else
-      DLOG(WARNING) << "Non-JSON response received from history server.";
-  }
-  return result.Pass();
-}
-
 // Converts a time into a string for use as a parameter in a request to the
 // history server.
 std::string ServerTimeString(base::Time time) {
@@ -303,6 +292,27 @@ WebHistoryService::WebHistoryService(Profile* profile)
 
 WebHistoryService::~WebHistoryService() {
   STLDeleteElements(&pending_expire_requests_);
+  STLDeleteElements(&pending_audio_history_requests_);
+}
+
+WebHistoryService::Request* WebHistoryService::CreateRequest(
+    const GURL& url,
+    const CompletionCallback& callback) {
+  return new RequestImpl(profile_, url, callback);
+}
+
+// static
+scoped_ptr<base::DictionaryValue> WebHistoryService::ReadResponse(
+  WebHistoryService::Request* request) {
+  scoped_ptr<base::DictionaryValue> result;
+  if (request->GetResponseCode() == net::HTTP_OK) {
+    base::Value* value = base::JSONReader::Read(request->GetResponseBody());
+    if (value && value->IsType(base::Value::TYPE_DICTIONARY))
+      result.reset(static_cast<base::DictionaryValue*>(value));
+    else
+      DLOG(WARNING) << "Non-JSON response received from history server.";
+  }
+  return result.Pass();
 }
 
 scoped_ptr<WebHistoryService::Request> WebHistoryService::QueryHistory(
@@ -310,12 +320,11 @@ scoped_ptr<WebHistoryService::Request> WebHistoryService::QueryHistory(
     const QueryOptions& options,
     const WebHistoryService::QueryWebHistoryCallback& callback) {
   // Wrap the original callback into a generic completion callback.
-  RequestImpl::CompletionCallback completion_callback = base::Bind(
+  CompletionCallback completion_callback = base::Bind(
       &WebHistoryService::QueryHistoryCompletionCallback, callback);
 
   GURL url = GetQueryUrl(text_query, options, server_version_info_);
-  scoped_ptr<RequestImpl> request(
-      new RequestImpl(profile_, url, completion_callback));
+  scoped_ptr<Request> request(CreateRequest(url, completion_callback));
   request->Start();
   return request.Pass();
 }
@@ -358,14 +367,13 @@ void WebHistoryService::ExpireHistory(
     url = net::AppendQueryParameter(url, "kvi", server_version_info_);
 
   // Wrap the original callback into a generic completion callback.
-  RequestImpl::CompletionCallback completion_callback =
+  CompletionCallback completion_callback =
       base::Bind(&WebHistoryService::ExpireHistoryCompletionCallback,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback);
 
-  scoped_ptr<RequestImpl> request(
-      new RequestImpl(profile_, url, completion_callback));
-  request->set_post_data(post_data);
+  scoped_ptr<Request> request(CreateRequest(url, completion_callback));
+  request->SetPostData(post_data);
   request->Start();
   pending_expire_requests_.insert(request.release());
 }
@@ -382,6 +390,48 @@ void WebHistoryService::ExpireHistoryBetween(
   ExpireHistory(expire_list, callback);
 }
 
+void WebHistoryService::GetAudioHistoryEnabled(
+    const AudioWebHistoryCallback& callback) {
+  // Wrap the original callback into a generic completion callback.
+  CompletionCallback completion_callback =
+    base::Bind(&WebHistoryService::AudioHistoryCompletionCallback,
+    weak_ptr_factory_.GetWeakPtr(),
+    callback);
+
+  GURL url(kHistoryAudioHistoryUrl);
+  scoped_ptr<Request> request(CreateRequest(url, completion_callback));
+  request->Start();
+  pending_audio_history_requests_.insert(request.release());
+}
+
+void WebHistoryService::SetAudioHistoryEnabled(
+  bool new_enabled_value,
+  const AudioWebHistoryCallback& callback) {
+  // Wrap the original callback into a generic completion callback.
+  CompletionCallback completion_callback =
+      base::Bind(&WebHistoryService::AudioHistoryCompletionCallback,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback);
+
+  GURL url(kHistoryAudioHistoryChangeUrl);
+  scoped_ptr<Request> request(CreateRequest(url, completion_callback));
+
+  base::DictionaryValue enable_audio_history;
+  enable_audio_history.SetBoolean("enable_history_recording",
+                                  new_enabled_value);
+  enable_audio_history.SetString("client", "audio");
+  std::string post_data;
+  base::JSONWriter::Write(&enable_audio_history, &post_data);
+  request->SetPostData(post_data);
+
+  request->Start();
+  pending_audio_history_requests_.insert(request.release());
+}
+
+size_t WebHistoryService::GetNumberOfPendingAudioHistoryRequests() {
+  return pending_audio_history_requests_.size();
+}
+
 // static
 void WebHistoryService::QueryHistoryCompletionCallback(
     const WebHistoryService::QueryWebHistoryCallback& callback,
@@ -389,7 +439,7 @@ void WebHistoryService::QueryHistoryCompletionCallback(
     bool success) {
   scoped_ptr<base::DictionaryValue> response_value;
   if (success)
-    response_value = ReadResponse(static_cast<RequestImpl*>(request));
+    response_value = ReadResponse(request);
   callback.Run(request, response_value.get());
 }
 
@@ -399,7 +449,7 @@ void WebHistoryService::ExpireHistoryCompletionCallback(
     bool success) {
   scoped_ptr<base::DictionaryValue> response_value;
   if (success) {
-    response_value = ReadResponse(static_cast<RequestImpl*>(request));
+    response_value = ReadResponse(request);
     if (response_value)
       response_value->GetString("version_info", &server_version_info_);
   }
@@ -409,4 +459,21 @@ void WebHistoryService::ExpireHistoryCompletionCallback(
   delete request;
 }
 
+void WebHistoryService::AudioHistoryCompletionCallback(
+    const WebHistoryService::AudioWebHistoryCallback& callback,
+    WebHistoryService::Request* request,
+    bool success) {
+  pending_audio_history_requests_.erase(request);
+  scoped_ptr<WebHistoryService::Request> request_ptr(request);
+
+  scoped_ptr<base::DictionaryValue> response_value;
+  bool enabled_value = false;
+  if (success) {
+    response_value = ReadResponse(request_ptr.get());
+    if (response_value)
+      response_value->GetBoolean("history_recording_enabled", &enabled_value);
+  }
+  callback.Run(success, enabled_value);
+}
+
 }  // namespace history

chrome/browser/history/web_history_service.h

@@ -36,7 +36,19 @@ class WebHistoryService : public KeyedService {
 
     // Returns true if the request is "pending" (i.e., it has been started, but
     // is not yet been complete).
-    virtual bool is_pending() = 0;
+    virtual bool IsPending() = 0;
+
+    // Returns the response code received from the server, which will only be
+    // valid if the request succeeded.
+    virtual int GetResponseCode() = 0;
+
+    // Returns the contents of the response body received from the server.
+    virtual const std::string& GetResponseBody() = 0;
+
+    virtual void SetPostData(const std::string& post_data) = 0;
+
+    // Tells the request to begin.
+    virtual void Start() = 0;
 
    protected:
     Request();
@@ -50,6 +62,11 @@ class WebHistoryService : public KeyedService {
 
   typedef base::Callback<void(bool success)> ExpireWebHistoryCallback;
 
+  typedef base::Callback<void(bool success, bool new_enabled_value)>
+      AudioWebHistoryCallback;
+
+  typedef base::Callback<void(Request*, bool success)> CompletionCallback;
+
   explicit WebHistoryService(Profile* profile);
   ~WebHistoryService() override;
 
@@ -76,7 +93,27 @@ class WebHistoryService : public KeyedService {
                             base::Time end_time,
                             const ExpireWebHistoryCallback& callback);
 
- private:
+  // Requests whether audio history recording is enabled.
+  void GetAudioHistoryEnabled(const AudioWebHistoryCallback& callback);
+
+  // Sets the state of audio history recording to |new_enabled_value|.
+  void SetAudioHistoryEnabled(bool new_enabled_value,
+                              const AudioWebHistoryCallback& callback);
+
+  // Used for tests.
+  size_t GetNumberOfPendingAudioHistoryRequests();
+
+ protected:
+  // This function is pulled out for testing purposes. Caller takes ownership of
+  // the new Request.
+  virtual Request* CreateRequest(const GURL& url,
+                                 const CompletionCallback& callback);
+
+  // Extracts a JSON-encoded HTTP response into a DictionaryValue.
+  // If |request|'s HTTP response code indicates failure, or if the response
+  // body is not JSON, a null pointer is returned.
+  static scoped_ptr<base::DictionaryValue> ReadResponse(Request* request);
+
   // Called by |request| when a web history query has completed. Unpacks the
   // response and calls |callback|, which is the original callback that was
   // passed to QueryHistory().
@@ -93,6 +130,17 @@ class WebHistoryService : public KeyedService {
       WebHistoryService::Request* request,
       bool success);
 
+  // Called by |request| when a request to get or set audio history from the
+  // server has completed. Unpacks the response and calls |callback|, which is
+  // the original callback that was passed to AudioHistory().
+  void AudioHistoryCompletionCallback(
+    const WebHistoryService::AudioWebHistoryCallback& callback,
+    WebHistoryService::Request* request,
+    bool success);
+
+ private:
+  friend class WebHistoryServiceTest;
+
   Profile* profile_;
 
   // Stores the version_info token received from the server in response to
@@ -104,6 +152,9 @@ class WebHistoryService : public KeyedService {
   // shutdown.
   std::set<Request*> pending_expire_requests_;
 
+  // Pending requests to be canceled if not complete by profile shutdown.
+  std::set<Request*> pending_audio_history_requests_;
+
   base::WeakPtrFactory<WebHistoryService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(WebHistoryService);

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,36 @@ HotwordAudioHistoryHandler::HotwordAudioHistoryHandler(
 HotwordAudioHistoryHandler::~HotwordAudioHistoryHandler() {
 }
 
-bool HotwordAudioHistoryHandler::GetAudioHistoryEnabled() {
-  // TODO(rlp): fill in.
-  return false;
+void HotwordAudioHistoryHandler::GetAudioHistoryEnabled() {
+  history::WebHistoryService* web_history =
+    WebHistoryServiceFactory::GetForProfile(profile_);
+  if (web_history) {
+    web_history->GetAudioHistoryEnabled(
+        base::Bind(&HotwordAudioHistoryHandler::AudioHistoryComplete,
+                   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(
+        enabled,
+        base::Bind(&HotwordAudioHistoryHandler::AudioHistoryComplete,
+                   weak_factory_.GetWeakPtr()));
+  }
 }
 
-void HotwordAudioHistoryHandler::OnAudioHistoryEnabledChanged(
-    const std::string& pref_name) {
-  DCHECK(pref_name == std::string(prefs::kHotwordAudioHistoryEnabled));
-
+void HotwordAudioHistoryHandler::AudioHistoryComplete(
+  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);
 }

chrome/browser/search/hotword_audio_history_handler.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_SEARCH_HOTWORD_AUDIO_HISTORY_HANDLER_H_
 #define CHROME_BROWSER_SEARCH_HOTWORD_AUDIO_HISTORY_HANDLER_H_
 
+#include "base/memory/weak_ptr.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "content/public/browser/browser_context.h"
 
@@ -18,19 +19,20 @@ class HotwordAudioHistoryHandler {
   explicit HotwordAudioHistoryHandler(content::BrowserContext* context);
   ~HotwordAudioHistoryHandler();
 
-  // Returns the current preference value based on the user's account info
+  // Updates the current preference value based on the user's account info
   // or false if the user is not signed in.
-  // TODO(rlp): Determine return value -- pref value or success?
-  bool GetAudioHistoryEnabled();
+  void GetAudioHistoryEnabled();
 
- private:
   // Sets the user's global pref value for enabling audio history.
   void SetAudioHistoryEnabled(const bool enabled);
 
-  void OnAudioHistoryEnabledChanged(const std::string& pref_name);
+ private:
+  // Callback called upon completion of the web history request.
+  void AudioHistoryComplete(bool success, bool new_enabled_value);
 
   Profile* profile_;
-  PrefChangeRegistrar pref_change_registrar_;
+
+  base::WeakPtrFactory<HotwordAudioHistoryHandler> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HotwordAudioHistoryHandler);
 };

chrome/chrome_tests_unit.gypi

@@ -476,6 +476,7 @@
       'browser/history/visit_database_unittest.cc',
       'browser/history/visit_filter_unittest.cc',
       'browser/history/visit_tracker_unittest.cc',
+      'browser/history/web_history_service_unittest.cc',
       'browser/image_holder_unittest.cc',
       'browser/importer/firefox_profile_lock_unittest.cc',
       'browser/importer/profile_writer_unittest.cc',