[GCM] Removing dependency between GServicesSettings and GCMStore

* Removing GCMStore from GServicesSettings
* Moving the settings writing to the store to the GCMClientImpl
* Adding capability to get settings in a string->string map
* Adding tests for the write case, and pulling the settings in a map

R=zea@chromium.org,jianli@chromium.org
BUG=359256

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@268400 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 98 insertions, 127 deletions

diff --git a/google_apis/gcm/engine/gservices_settings.cc b/google_apis/gcm/engine/gservices_settings.cc
index 340d1a7..8e228ac 100644
--- a/google_apis/gcm/engine/gservices_settings.cc
+++ b/google_apis/gcm/engine/gservices_settings.cc
@@ -36,9 +36,13 @@ const base::TimeDelta GServicesSettings::MinimumCheckinInterval() {
   return base::TimeDelta::FromSeconds(kMinimumCheckinInterval);
 }
 
-GServicesSettings::GServicesSettings(GCMStore* gcm_store)
-    : gcm_store_(gcm_store),
-      checkin_interval_(base::TimeDelta::FromSeconds(kDefaultCheckinInterval)),
+// static
+const GURL GServicesSettings::DefaultCheckinURL() {
+  return GURL(kDefaultCheckinURL);
+}
+
+GServicesSettings::GServicesSettings()
+    : checkin_interval_(base::TimeDelta::FromSeconds(kDefaultCheckinInterval)),
       checkin_url_(kDefaultCheckinURL),
       mcs_hostname_(kDefaultMCSHostname),
       mcs_secure_port_(kDefaultMCSSecurePort),
@@ -48,12 +52,12 @@ GServicesSettings::GServicesSettings(GCMStore* gcm_store)
 
 GServicesSettings::~GServicesSettings() {}
 
-void GServicesSettings::UpdateFromCheckinResponse(
+bool GServicesSettings::UpdateFromCheckinResponse(
   const checkin_proto::AndroidCheckinResponse& checkin_response) {
   if (!checkin_response.has_digest() ||
       checkin_response.digest() == digest_) {
     // There are no changes as digest is the same or no settings provided.
-    return;
+    return false;
   }
 
   std::map<std::string, std::string> settings;
@@ -67,12 +71,10 @@ void GServicesSettings::UpdateFromCheckinResponse(
   // passed the verificaiton in update settings.
   if (UpdateSettings(settings)) {
     digest_ = checkin_response.digest();
-    gcm_store_->SetGServicesSettings(
-        settings,
-        digest_,
-        base::Bind(&GServicesSettings::SetGServicesSettingsCallback,
-                   weak_ptr_factory_.GetWeakPtr()));
+    return true;
   }
+
+  return false;
 }
 
 void GServicesSettings::UpdateFromLoadResult(
@@ -81,6 +83,17 @@ void GServicesSettings::UpdateFromLoadResult(
     digest_ = load_result.gservices_digest;
 }
 
+std::map<std::string, std::string> GServicesSettings::GetSettingsMap() const {
+  std::map<std::string, std::string> settings;
+  settings[kCheckinIntervalKey] =
+      base::Int64ToString(checkin_interval_.InSeconds());
+  settings[kCheckinURLKey] = checkin_url_.spec();
+  settings[kMCSHostnameKey] = mcs_hostname_;
+  settings[kMCSSecurePortKey] = base::IntToString(mcs_secure_port_);
+  settings[kRegistrationURLKey] = registration_url_.spec();
+  return settings;
+}
+
 bool GServicesSettings::UpdateSettings(
     const std::map<std::string, std::string>& settings) {
   int64 new_checkin_interval = kMinimumCheckinInterval;
@@ -166,8 +179,4 @@ bool GServicesSettings::UpdateSettings(
   return true;
 }
 
-void GServicesSettings::SetGServicesSettingsCallback(bool success) {
-  DCHECK(success);
-}
-
 }  // namespace gcm
diff --git a/google_apis/gcm/engine/gservices_settings.h b/google_apis/gcm/engine/gservices_settings.h
index 9cc6727..92e0e23 100644
--- a/google_apis/gcm/engine/gservices_settings.h
+++ b/google_apis/gcm/engine/gservices_settings.h
@@ -24,18 +24,23 @@ class GCM_EXPORT GServicesSettings {
   // Minimum periodic checkin interval in seconds.
   static const base::TimeDelta MinimumCheckinInterval();
 
-  // Create an instance of GServicesSettings class. |gcm_store| is used to store
-  // the settings after they are extracted from checkin response.
-  explicit GServicesSettings(GCMStore* gcm_store);
+  // Default checkin URL.
+  static const GURL DefaultCheckinURL();
+
+  GServicesSettings();
   ~GServicesSettings();
 
-  // Udpates the settings based on |checkin_response|.
-  void UpdateFromCheckinResponse(
+  // Updates the settings based on |checkin_response|.
+  bool UpdateFromCheckinResponse(
       const checkin_proto::AndroidCheckinResponse& checkin_response);
 
-  // Updates the settings based on |load_result|.
+  // Updates the settings based on |load_result|. Returns true if update was
+  // successful, false otherwise.
   void UpdateFromLoadResult(const GCMStore::LoadResult& load_result);
 
+  // Gets the settings as a map of string to string for storing.
+  std::map<std::string, std::string> GetSettingsMap() const;
+
   std::string digest() const { return digest_; }
 
   base::TimeDelta checkin_interval() const { return checkin_interval_; }
@@ -54,12 +59,6 @@ class GCM_EXPORT GServicesSettings {
   // TODO(fgorski): Change to a status variable that can be logged to UMA.
   bool UpdateSettings(const std::map<std::string, std::string>& settings);
 
-  // Callback passed to GCMStore::SetGServicesSettings.
-  void SetGServicesSettingsCallback(bool success);
-
-  // GCM store to persist the settings. Not owned.
-  GCMStore* gcm_store_;
-
   // Digest (hash) of the settings, used to check whether settings need update.
   // It is meant to be sent with checkin request, instead of sending the whole
   // settings table.
diff --git a/google_apis/gcm/engine/gservices_settings_unittest.cc b/google_apis/gcm/engine/gservices_settings_unittest.cc
index 9ed4d9f..886bc6d 100644
--- a/google_apis/gcm/engine/gservices_settings_unittest.cc
+++ b/google_apis/gcm/engine/gservices_settings_unittest.cc
@@ -26,77 +26,6 @@ const int kDefaultMCSSecurePort = 5228;
 const char kDefaultRegistrationURL[] =
     "https://android.clients.google.com/c2dm/register3";
 
-class FakeGCMStore : public GCMStore {
- public:
-  FakeGCMStore();
-  virtual ~FakeGCMStore();
-
-  virtual void Load(const gcm::GCMStore::LoadCallback& callback) OVERRIDE {}
-  virtual void Close() OVERRIDE {}
-  virtual void Destroy(const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {
-  }
-  virtual void SetDeviceCredentials(
-      uint64 device_android_id,
-      uint64 device_security_token,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void AddRegistration(
-      const std::string& app_id,
-      const linked_ptr<gcm::RegistrationInfo>& registration,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void RemoveRegistration(
-      const std::string& app_id,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void AddIncomingMessage(
-      const std::string& persistent_id,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void RemoveIncomingMessage(
-      const std::string& persistent_id,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void RemoveIncomingMessages(
-      const PersistentIdList& persistent_ids,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual bool AddOutgoingMessage(
-      const std::string& persistent_id,
-      const gcm::MCSMessage& message,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {
-    return true;
-  }
-  virtual void OverwriteOutgoingMessage(
-      const std::string& persistent_id,
-      const gcm::MCSMessage& message,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void RemoveOutgoingMessage(
-      const std::string& persistent_id,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void RemoveOutgoingMessages(
-      const PersistentIdList& persistent_ids,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-  virtual void SetLastCheckinTime(
-      const base::Time& last_checkin_time,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {}
-
-  // G-service settings handling.
-  virtual void SetGServicesSettings(
-      const std::map<std::string, std::string>& settings,
-      const std::string& settings_digest,
-      const gcm::GCMStore::UpdateCallback& callback) OVERRIDE {
-    settings_saved_ = true;
-  }
-
-  void Reset() {
-    settings_saved_ = false;
-  }
-
-  bool settings_saved() const { return settings_saved_; }
-
- private:
-  bool settings_saved_;
-};
-
-FakeGCMStore::FakeGCMStore() : settings_saved_(false) {}
-
-FakeGCMStore::~FakeGCMStore() {}
-
 }  // namespace
 
 class GServicesSettingsTest : public testing::Test {
@@ -119,17 +48,14 @@ class GServicesSettingsTest : public testing::Test {
     return alternative_settings_;
   }
 
-  FakeGCMStore& gcm_store() { return gcm_store_; }
-
   std::map<std::string, std::string> alternative_settings_;
 
  private:
-  FakeGCMStore gcm_store_;
   GServicesSettings gserivces_settings_;
 };
 
 GServicesSettingsTest::GServicesSettingsTest()
-    : gserivces_settings_(&gcm_store_) {
+    : gserivces_settings_() {
 }
 
 GServicesSettingsTest::~GServicesSettingsTest() {}
@@ -220,10 +146,10 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponse) {
   checkin_response.set_digest("digest_value");
   SetWithAlternativeSettings(checkin_response);
 
-  settings().UpdateFromCheckinResponse(checkin_response);
-  EXPECT_TRUE(gcm_store().settings_saved());
+  EXPECT_TRUE(settings().UpdateFromCheckinResponse(checkin_response));
 
   CheckAllSetToAlternative();
+  EXPECT_EQ(alternative_settings_, settings().GetSettingsMap());
   EXPECT_EQ("digest_value", settings().digest());
 }
 
@@ -237,8 +163,7 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseMinimumCheckinInterval) {
   alternative_settings_["checkin_interval"] = base::IntToString(3600);
   SetWithAlternativeSettings(checkin_response);
 
-  settings().UpdateFromCheckinResponse(checkin_response);
-  EXPECT_TRUE(gcm_store().settings_saved());
+  EXPECT_TRUE(settings().UpdateFromCheckinResponse(checkin_response));
 
   EXPECT_EQ(GServicesSettings::MinimumCheckinInterval(),
             settings().checkin_interval());
@@ -253,8 +178,7 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseWithSettingMissing) {
   alternative_settings_.erase("gcm_hostname");
   SetWithAlternativeSettings(checkin_response);
 
-  settings().UpdateFromCheckinResponse(checkin_response);
-  EXPECT_FALSE(gcm_store().settings_saved());
+  EXPECT_FALSE(settings().UpdateFromCheckinResponse(checkin_response));
 
   CheckAllSetToDefault();
   EXPECT_EQ(std::string(), settings().digest());
@@ -265,8 +189,7 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseNoDigest) {
   checkin_proto::AndroidCheckinResponse checkin_response;
 
   SetWithAlternativeSettings(checkin_response);
-  settings().UpdateFromCheckinResponse(checkin_response);
-  EXPECT_FALSE(gcm_store().settings_saved());
+  EXPECT_FALSE(settings().UpdateFromCheckinResponse(checkin_response));
 
   CheckAllSetToDefault();
   EXPECT_EQ(std::string(), settings().digest());
@@ -282,8 +205,7 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseSameDigest) {
   checkin_proto::AndroidCheckinResponse checkin_response;
   checkin_response.set_digest("old_digest");
   SetWithAlternativeSettings(checkin_response);
-  settings().UpdateFromCheckinResponse(checkin_response);
-  EXPECT_FALSE(gcm_store().settings_saved());
+  EXPECT_FALSE(settings().UpdateFromCheckinResponse(checkin_response));
 
   CheckAllSetToAlternative();
   EXPECT_EQ("old_digest", settings().digest());
diff --git a/google_apis/gcm/gcm_client_impl.cc b/google_apis/gcm/gcm_client_impl.cc
index 5b02796..e5c13da 100644
--- a/google_apis/gcm/gcm_client_impl.cc
+++ b/google_apis/gcm/gcm_client_impl.cc
@@ -19,8 +19,6 @@
 #include "google_apis/gcm/engine/checkin_request.h"
 #include "google_apis/gcm/engine/connection_factory_impl.h"
 #include "google_apis/gcm/engine/gcm_store_impl.h"
-#include "google_apis/gcm/engine/gservices_settings.h"
-#include "google_apis/gcm/engine/mcs_client.h"
 #include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/http/http_network_session.h"
@@ -194,7 +192,6 @@ void GCMClientImpl::Initialize(
   account_ids_ = account_ids;
 
   gcm_store_.reset(new GCMStoreImpl(path, blocking_task_runner));
-  gservices_settings_.reset(new GServicesSettings(gcm_store_.get()));
 
   delegate_ = delegate;
 
@@ -222,7 +219,7 @@ void GCMClientImpl::OnLoadCompleted(scoped_ptr<GCMStore::LoadResult> result) {
   device_checkin_info_.android_id = result->device_android_id;
   device_checkin_info_.secret = result->device_security_token;
   last_checkin_time_ = result->last_checkin_time;
-  gservices_settings_->UpdateFromLoadResult(*result);
+  gservices_settings_.UpdateFromLoadResult(*result);
   InitializeMCSClient(result.Pass());
 
   if (device_checkin_info_.IsValid()) {
@@ -303,11 +300,11 @@ void GCMClientImpl::StartCheckin() {
 
   CheckinRequest::RequestInfo request_info(device_checkin_info_.android_id,
                                            device_checkin_info_.secret,
-                                           gservices_settings_->digest(),
+                                           gservices_settings_.digest(),
                                            account_ids_,
                                            chrome_build_proto_);
   checkin_request_.reset(
-      new CheckinRequest(gservices_settings_->checkin_url(),
+      new CheckinRequest(gservices_settings_.checkin_url(),
                          request_info,
                          kDefaultBackoffPolicy,
                          base::Bind(&GCMClientImpl::OnCheckinCompleted,
@@ -344,7 +341,14 @@ void GCMClientImpl::OnCheckinCompleted(
 
   if (device_checkin_info_.IsValid()) {
     // First update G-services settings, as something might have changed.
-    gservices_settings_->UpdateFromCheckinResponse(checkin_response);
+    if (gservices_settings_.UpdateFromCheckinResponse(checkin_response)) {
+      gcm_store_->SetGServicesSettings(
+          gservices_settings_.GetSettingsMap(),
+          gservices_settings_.digest(),
+          base::Bind(&GCMClientImpl::SetGServicesSettingsCallback,
+                     weak_ptr_factory_.GetWeakPtr()));
+    }
+
     last_checkin_time_ = clock_->Now();
     gcm_store_->SetLastCheckinTime(
         last_checkin_time_,
@@ -354,6 +358,10 @@ void GCMClientImpl::OnCheckinCompleted(
   }
 }
 
+void GCMClientImpl::SetGServicesSettingsCallback(bool success) {
+  DCHECK(success);
+}
+
 void GCMClientImpl::SchedulePeriodicCheckin() {
   // Make sure no checkin is in progress.
   if (checkin_request_.get())
@@ -375,7 +383,7 @@ void GCMClientImpl::SchedulePeriodicCheckin() {
 }
 
 base::TimeDelta GCMClientImpl::GetTimeToNextCheckin() const {
-  return last_checkin_time_ + gservices_settings_->checkin_interval() -
+  return last_checkin_time_ + gservices_settings_.checkin_interval() -
          clock_->Now();
 }
 
@@ -433,7 +441,7 @@ void GCMClientImpl::Register(const std::string& app_id,
   DCHECK_EQ(0u, pending_registration_requests_.count(app_id));
 
   RegistrationRequest* registration_request =
-      new RegistrationRequest(gservices_settings_->registration_url(),
+      new RegistrationRequest(gservices_settings_.registration_url(),
                               request_info,
                               kDefaultBackoffPolicy,
                               base::Bind(&GCMClientImpl::OnRegisterCompleted,
@@ -509,7 +517,7 @@ void GCMClientImpl::Unregister(const std::string& app_id) {
 
   UnregistrationRequest* unregistration_request =
       new UnregistrationRequest(
-          gservices_settings_->registration_url(),
+          gservices_settings_.registration_url(),
           request_info,
           kDefaultBackoffPolicy,
           base::Bind(&GCMClientImpl::OnUnregisterCompleted,
diff --git a/google_apis/gcm/gcm_client_impl.h b/google_apis/gcm/gcm_client_impl.h
index f38a08d..85f206b 100644
--- a/google_apis/gcm/gcm_client_impl.h
+++ b/google_apis/gcm/gcm_client_impl.h
@@ -15,6 +15,7 @@
 #include "base/stl_util.h"
 #include "google_apis/gcm/base/mcs_message.h"
 #include "google_apis/gcm/engine/gcm_store.h"
+#include "google_apis/gcm/engine/gservices_settings.h"
 #include "google_apis/gcm/engine/mcs_client.h"
 #include "google_apis/gcm/engine/registration_request.h"
 #include "google_apis/gcm/engine/unregistration_request.h"
@@ -45,7 +46,6 @@ namespace gcm {
 class CheckinRequest;
 class ConnectionFactory;
 class GCMClientImplTest;
-class GServicesSettings;
 
 // Helper class for building GCM internals. Allows tests to inject fake versions
 // as necessary.
@@ -179,6 +179,10 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
   // Function also cleans up the pending checkin.
   void OnCheckinCompleted(
       const checkin_proto::AndroidCheckinResponse& checkin_response);
+
+  // Callback passed to GCMStore::SetGServicesSettings.
+  void SetGServicesSettingsCallback(bool success);
+
   // Schedules next periodic device checkin and makes sure there is at most one
   // pending checkin at a time. This function is meant to be called after a
   // successful checkin.
@@ -275,7 +279,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
       pending_unregistration_requests_deleter_;
 
   // G-services settings that were provided by MCS.
-  scoped_ptr<GServicesSettings> gservices_settings_;
+  GServicesSettings gservices_settings_;
 
   // Time of the last successful checkin.
   base::Time last_checkin_time_;
diff --git a/google_apis/gcm/gcm_client_impl_unittest.cc b/google_apis/gcm/gcm_client_impl_unittest.cc
index 451c2c4..ba49590 100644
--- a/google_apis/gcm/gcm_client_impl_unittest.cc
+++ b/google_apis/gcm/gcm_client_impl_unittest.cc
@@ -281,8 +281,8 @@ class GCMClientImplTest : public testing::Test,
     return last_error_details_;
   }
 
-  const GServicesSettings* gservices_settings() const {
-    return gcm_client_->gservices_settings_.get();
+  const GServicesSettings& gservices_settings() const {
+    return gcm_client_->gservices_settings_;
   }
 
   int64 CurrentTime();
@@ -724,7 +724,13 @@ TEST_F(GCMClientImplCheckinTest, GServicesSettingsAfterInitialCheckin) {
   CompleteCheckin(
       kDeviceAndroidId, kDeviceSecurityToken, kSettingsDefaultDigest, settings);
   EXPECT_EQ(base::TimeDelta::FromSeconds(kSettingsCheckinInterval),
-            gservices_settings()->checkin_interval());
+            gservices_settings().checkin_interval());
+  EXPECT_EQ(GURL("http://alternative.url/checkin"),
+            gservices_settings().checkin_url());
+  EXPECT_EQ(GURL("http://alternative.url/registration"),
+            gservices_settings().registration_url());
+  EXPECT_EQ("http://alternative.gcm.host", gservices_settings().mcs_hostname());
+  EXPECT_EQ(443, gservices_settings().mcs_secure_port());
 }
 
 // This test only checks that periodic checkin happens.
@@ -744,4 +750,27 @@ TEST_F(GCMClientImplCheckinTest, PeriodicCheckin) {
       kDeviceAndroidId, kDeviceSecurityToken, kSettingsDefaultDigest, settings);
 }
 
+TEST_F(GCMClientImplCheckinTest, LoadGSettingsFromStore) {
+  std::map<std::string, std::string> settings;
+  settings["checkin_interval"] = base::IntToString(kSettingsCheckinInterval);
+  settings["checkin_url"] = "http://alternative.url/checkin";
+  settings["gcm_hostname"] = "http://alternative.gcm.host";
+  settings["gcm_secure_port"] = "443";
+  settings["gcm_registration_url"] = "http://alternative.url/registration";
+  CompleteCheckin(
+      kDeviceAndroidId, kDeviceSecurityToken, kSettingsDefaultDigest, settings);
+
+  BuildGCMClient(base::TimeDelta());
+  InitializeGCMClient();
+
+  EXPECT_EQ(base::TimeDelta::FromSeconds(kSettingsCheckinInterval),
+            gservices_settings().checkin_interval());
+  EXPECT_EQ(GURL("http://alternative.url/checkin"),
+            gservices_settings().checkin_url());
+  EXPECT_EQ(GURL("http://alternative.url/registration"),
+            gservices_settings().registration_url());
+  EXPECT_EQ("http://alternative.gcm.host", gservices_settings().mcs_hostname());
+  EXPECT_EQ(443, gservices_settings().mcs_secure_port());
+}
+
 }  // namespace gcm
diff --git a/google_apis/gcm/tools/mcs_probe.cc b/google_apis/gcm/tools/mcs_probe.cc
index 74d7d2c..b7f984d 100644
--- a/google_apis/gcm/tools/mcs_probe.cc
+++ b/google_apis/gcm/tools/mcs_probe.cc
@@ -434,7 +434,7 @@ void MCSProbe::CheckIn() {
       0, 0, std::string(), std::vector<std::string>(), chrome_build_proto);
 
   checkin_request_.reset(new CheckinRequest(
-      GServicesSettings(gcm_store_.get()).checkin_url(),
+      GServicesSettings::DefaultCheckinURL(),
       request_info,
       kDefaultBackoffPolicy,
       base::Bind(&MCSProbe::OnCheckInCompleted, base::Unretained(this)),