[GCM] fixing G-settings initialization from an empty store

Adding a test for the problem
Fixing the problem
Adding a minimum checkin interval

BUG=359254

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

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

google_apis/gcm/engine/gservices_settings.cc

@@ -30,6 +30,8 @@ const char kDefaultRegistrationURL[] =
 
 namespace gcm {
 
+const int64 GServicesSettings::kMinimumCheckinInterval = 12 * 60 * 60;
+
 GServicesSettings::GServicesSettings(GCMStore* gcm_store)
     : gcm_store_(gcm_store),
       checkin_interval_(kDefaultCheckinInterval),
@@ -77,32 +79,41 @@ void GServicesSettings::UpdateFromLoadResult(
 
 bool GServicesSettings::UpdateSettings(
     const std::map<std::string, std::string>& settings) {
-  int64 new_checkin_interval = 0LL;
+  int64 new_checkin_interval = kMinimumCheckinInterval;
   std::map<std::string, std::string>::const_iterator iter =
       settings.find(kCheckinIntervalKey);
-  if (iter != settings.end()) {
-    if (!base::StringToInt64(iter->second, &new_checkin_interval)) {
-      LOG(ERROR) << "Failed to parse checkin interval: " << iter->second;
+  if (iter == settings.end()) {
+    LOG(ERROR) << "Setting not found: " << kCheckinIntervalKey;
     return false;
   }
-    if (new_checkin_interval <= 0LL) {
-      LOG(ERROR) << "Checkin interval not positive: " << new_checkin_interval;
+  if (!base::StringToInt64(iter->second, &new_checkin_interval)) {
+    LOG(ERROR) << "Failed to parse checkin interval: " << iter->second;
     return false;
   }
+  if (new_checkin_interval < kMinimumCheckinInterval) {
+    LOG(ERROR) << "Checkin interval: " << new_checkin_interval
+               << " is less than allowed minimum: " << kMinimumCheckinInterval;
+    new_checkin_interval = kMinimumCheckinInterval;
   }
 
   std::string new_mcs_hostname;
-  int new_mcs_secure_port = -1;
   iter = settings.find(kMCSHostnameKey);
-  if (iter != settings.end()) {
+  if (iter == settings.end()) {
+    LOG(ERROR) << "Setting not found: " << kMCSHostnameKey;
+    return false;
+  }
   new_mcs_hostname = iter->second;
   if (new_mcs_hostname.empty()) {
     LOG(ERROR) << "Empty MCS hostname provided.";
     return false;
   }
 
+  int new_mcs_secure_port = -1;
   iter = settings.find(kMCSSecurePortKey);
-    if (iter != settings.end()) {
+  if (iter == settings.end()) {
+    LOG(ERROR) << "Setting not found: " << kMCSSecurePortKey;
+    return false;
+  }
   if (!base::StringToInt(iter->second, &new_mcs_secure_port)) {
     LOG(ERROR) << "Failed to parse MCS secure port: " << iter->second;
     return false;
@@ -111,28 +122,30 @@ bool GServicesSettings::UpdateSettings(
     LOG(ERROR) << "Incorrect port value: " << new_mcs_secure_port;
     return false;
   }
-    }
-  }
 
   std::string new_checkin_url;
   iter = settings.find(kCheckinURLKey);
-  if (iter != settings.end()) {
+  if (iter == settings.end()) {
+    LOG(ERROR) << "Setting not found: " << kCheckinURLKey;
+    return false;
+  }
   new_checkin_url = iter->second;
   if (new_checkin_url.empty()) {
     LOG(ERROR) << "Empty checkin URL provided.";
     return false;
   }
-  }
 
   std::string new_registration_url;
   iter = settings.find(kRegistrationURLKey);
-  if (iter != settings.end()) {
+  if (iter == settings.end()) {
+    LOG(ERROR) << "Setting not found: " << kRegistrationURLKey;
+    return false;
+  }
   new_registration_url = iter->second;
   if (new_registration_url.empty()) {
     LOG(ERROR) << "Empty registration URL provided.";
     return false;
   }
-  }
 
   // We only update the settings once all of them are correct.
   checkin_interval_ = new_checkin_interval;

google_apis/gcm/engine/gservices_settings.h

@@ -19,6 +19,9 @@ namespace gcm {
 // extracting them from checkin response and storing in GCMStore.
 class GCM_EXPORT GServicesSettings {
  public:
+  // Minimum periodic checkin interval in seconds.
+  static const int64 kMinimumCheckinInterval;
+
   // 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);

google_apis/gcm/engine/gservices_settings_unittest.cc

@@ -12,7 +12,7 @@ namespace gcm {
 
 namespace {
 
-const int64 kAlternativeCheckinInterval = 2000LL;
+const int64 kAlternativeCheckinInterval = 16 * 60 * 60;
 const char kAlternativeCheckinURL[] = "http://alternative.url/checkin";
 const char kAlternativeMCSHostname[] = "http://alternative.gcm.host";
 const int kAlternativeMCSSecurePort = 443;
@@ -121,10 +121,11 @@ class GServicesSettingsTest : public testing::Test {
 
   FakeGCMStore& gcm_store() { return gcm_store_; }
 
+  std::map<std::string, std::string> alternative_settings_;
+
  private:
   FakeGCMStore gcm_store_;
   GServicesSettings gserivces_settings_;
-  std::map<std::string, std::string> alternative_settings_;
 };
 
 GServicesSettingsTest::GServicesSettingsTest()
@@ -176,6 +177,28 @@ TEST_F(GServicesSettingsTest, DefaultSettingsAndDigest) {
   EXPECT_EQ(std::string(), settings().digest());
 }
 
+// Verifies that settings are not updated when load result is empty.
+TEST_F(GServicesSettingsTest, UpdateFromEmptyLoadResult) {
+  GCMStore::LoadResult result;
+  result.gservices_digest = "digest_value";
+  settings().UpdateFromLoadResult(result);
+
+  CheckAllSetToDefault();
+  EXPECT_EQ(std::string(), settings().digest());
+}
+
+// Verifies that settings are not updated when one of them is missing.
+TEST_F(GServicesSettingsTest, UpdateFromLoadResultWithSettingMissing) {
+  GCMStore::LoadResult result;
+  result.gservices_settings = alternative_settings();
+  result.gservices_digest = "digest_value";
+  result.gservices_settings.erase("gcm_hostname");
+  settings().UpdateFromLoadResult(result);
+
+  CheckAllSetToDefault();
+  EXPECT_EQ(std::string(), settings().digest());
+}
+
 // Verifies that the settings are set correctly based on the load result.
 TEST_F(GServicesSettingsTest, UpdateFromLoadResult) {
   GCMStore::LoadResult result;
@@ -202,6 +225,39 @@ TEST_F(GServicesSettingsTest, UpdateFromCheckinResponse) {
   EXPECT_EQ("digest_value", settings().digest());
 }
 
+// Verifies that the checkin interval is updated to minimum if the original
+// value is less than minimum.
+TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseMinimumCheckinInterval) {
+  checkin_proto::AndroidCheckinResponse checkin_response;
+
+  checkin_response.set_digest("digest_value");
+  // Setting the checkin interval to less than minimum.
+  alternative_settings_["checkin_interval"] = base::IntToString(3600);
+  SetWithAlternativeSettings(checkin_response);
+
+  settings().UpdateFromCheckinResponse(checkin_response);
+  EXPECT_TRUE(gcm_store().settings_saved());
+
+  EXPECT_EQ(GServicesSettings::kMinimumCheckinInterval,
+            settings().checkin_interval());
+  EXPECT_EQ("digest_value", settings().digest());
+}
+
+// Verifies that settings are not updated when one of them is missing.
+TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseWithSettingMissing) {
+  checkin_proto::AndroidCheckinResponse checkin_response;
+
+  checkin_response.set_digest("digest_value");
+  alternative_settings_.erase("gcm_hostname");
+  SetWithAlternativeSettings(checkin_response);
+
+  settings().UpdateFromCheckinResponse(checkin_response);
+  EXPECT_FALSE(gcm_store().settings_saved());
+
+  CheckAllSetToDefault();
+  EXPECT_EQ(std::string(), settings().digest());
+}
+
 // Verifies that no update is done, when a checkin response misses digest.
 TEST_F(GServicesSettingsTest, UpdateFromCheckinResponseNoDigest) {
   checkin_proto::AndroidCheckinResponse checkin_response;