Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

components/metrics/metrics_state_manager_unittest.cc

Index: components/metrics/metrics_state_manager_unittest.cc
diff --git a/components/metrics/metrics_state_manager_unittest.cc b/components/metrics/metrics_state_manager_unittest.cc
index 2f7d85be2aa6055e133944d5e22ecfb52634dc24..dd354195377ecf28cb66feca9bf0b98930ae4b0b 100644
--- a/components/metrics/metrics_state_manager_unittest.cc
+++ b/components/metrics/metrics_state_manager_unittest.cc
@@ -10,7 +10,9 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/prefs/testing_pref_service.h"
+#include "components/metrics/client_info.h"
 #include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_service.h"
 #include "components/metrics/metrics_switches.h"
 #include "components/variations/caching_permuted_entropy_provider.h"
 #include "components/variations/pref_names.h"
@@ -21,13 +23,17 @@ namespace metrics {
 class MetricsStateManagerTest : public testing::Test {
  public:
   MetricsStateManagerTest() : is_metrics_reporting_enabled_(false) {
-    MetricsStateManager::RegisterPrefs(prefs_.registry());
+    MetricsService::RegisterPrefs(prefs_.registry());
   }
 
   scoped_ptr<MetricsStateManager> CreateStateManager() {
     return MetricsStateManager::Create(
         &prefs_,
         base::Bind(&MetricsStateManagerTest::is_metrics_reporting_enabled,
+                   base::Unretained(this)),
+        base::Bind(&MetricsStateManagerTest::MockStoreClientInfoBackup,
+                   base::Unretained(this)),
+        base::Bind(&MetricsStateManagerTest::LoadFakeClientInfoBackup,
                    base::Unretained(this))).Pass();
   }
 
@@ -36,14 +42,49 @@ class MetricsStateManagerTest : public testing::Test {
     is_metrics_reporting_enabled_ = true;
   }
 
+
+

[Ilya Sherman, 2014/07/11 23:25:00]

nit: Spurious blank lines.

[gab, 2014/07/14 19:17:15]

Done.

  protected:
   TestingPrefServiceSimple prefs_;
 
+  // Last ClientInfo stored by the MetricsStateManager via
+  // MockStoreClientInfoBackup.
+  scoped_ptr<ClientInfo> stored_client_info_backup_;
+
+  // If set, will be returned via LoadFakeClientInfoBackup if requested by the
+  // MetricsStateManager.
+  scoped_ptr<ClientInfo> fake_client_info_backup_;
+
  private:
   bool is_metrics_reporting_enabled() const {
     return is_metrics_reporting_enabled_;
   }
 
+  // Stores the |client_info| in |stored_client_info_backup_| for verification
+  // by the tests later.
+  void MockStoreClientInfoBackup(const ClientInfo& client_info) {
+    stored_client_info_backup_.reset(new ClientInfo);
+    stored_client_info_backup_->client_id = client_info.client_id;
+    stored_client_info_backup_->installation_date =
+        client_info.installation_date;
+    stored_client_info_backup_->reporting_enabled_date =
+        client_info.reporting_enabled_date;
+  }
+
+  // Hands out a copy of |fake_client_info_backup_| if it is set.
+  scoped_ptr<ClientInfo> LoadFakeClientInfoBackup() {
+    if (!fake_client_info_backup_)
+      return scoped_ptr<ClientInfo>();
+
+    scoped_ptr<ClientInfo> backup_copy(new ClientInfo);
+    backup_copy->client_id = fake_client_info_backup_->client_id;
+    backup_copy->installation_date =
+        fake_client_info_backup_->installation_date;
+    backup_copy->reporting_enabled_date =
+        fake_client_info_backup_->reporting_enabled_date;
+    return backup_copy.Pass();

[Ilya Sherman, 2014/07/11 23:25:00]

Wouldn't this be easier if copying the struct were allowed? ;)

[gab, 2014/07/14 19:17:14]

Yes, except that by [1] this means I would need an explicit copy constructor.

And I don't like adding extra complexity in the implementation only for the sake
of tests when it's not required :-)!

[1]
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

[grt (UTC plus 2), 2014/07/14 19:40:21]

Is this true for structs? I thought structs were permitted to use the implicit
copy ctor.

[gab, 2014/07/14 19:47:21]

I think they are, but I don't see a good reason to have it in this case.

(and I also don't see why it's okay for structs, not for classes... IMO the same
reasons that apply to classes directly apply to structs... the story will be
different with C++11 moves, but for now I'd rather move this by pointer than by
value)

[grt (UTC plus 2), 2014/07/15 16:04:27]

The reason we disallow copy and assign by default for classes (exceptions are
allowed) is that it can be hard to tell just by looking at a class whether or
not it's horribly inefficient to treat it as a value type. In the case of
something so simple as this struct (a string and a pair of 64-bit ints?), it's
easy to see by inspection that treating it as a value type is not horribly
inefficient. I don't think the rule should be a blind: okay to copy structs, not
okay to copy classes, since it's easy to make a struct that is inefficient to
copy. It's perfectly okay to create a struct and document it as being safe to
treat as a value, though. (Note: just continuing the discussion. I'm not asking
you to change anything here.)

[gab, 2014/07/15 20:50:05]

I see, makes sense, if you insist I'm happy to remove DISALLOW_COPY_AND_ASSIGN,
but then I'd make it explicitly passed by value everywhere (i.e. no scoped_ptr
passing), I don't think that allowing copies yet not using them is desired.

I'm willing to go either way, let me know if you feel it should be changed.

[grt (UTC plus 2), 2014/07/16 13:52:31]

I don't feel strongly that you must change it in this CL. I haven't looked at
how it's used, but if you think it simplifies the code, then go for it. Your
call.

+  }
+
   bool is_metrics_reporting_enabled_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsStateManagerTest);
@@ -171,4 +212,137 @@ TEST_F(MetricsStateManagerTest, ResetMetricsIDs) {
   EXPECT_NE(kInitialClientId, prefs_.GetString(prefs::kMetricsClientID));
 }
 
+TEST_F(MetricsStateManagerTest, ForceClientIdCreation) {
+  const int64 kFakeInstallationDate = 12345;
+  prefs_.SetInt64(prefs::kInstallDate, kFakeInstallationDate);
+
+  // Holds ClientInfo from previous scoped test for extra checks.
+  scoped_ptr<ClientInfo> tmp_client_info;

[Ilya Sherman, 2014/07/11 23:25:00]

nit: Perhaps s/tmp/previous/?

[gab, 2014/07/14 19:17:15]

Done.

+
+  {
+    scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+
+    // client_id shouldn't be auto-generated if metrics reporting is not
+    // enabled.
+    EXPECT_EQ(std::string(), state_manager->client_id());
+    EXPECT_EQ(0, prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
+
+    // Confirm that the initial ForceClientIdCreation call creates the client id
+    // and backs it up via MockStoreClientInfoBackup.
+    EXPECT_FALSE(stored_client_info_backup_);
+    state_manager->ForceClientIdCreation();
+    EXPECT_NE(std::string(), state_manager->client_id());
+    EXPECT_GT(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp), 0);
+
+    ASSERT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(state_manager->client_id(),
+              stored_client_info_backup_->client_id);
+    EXPECT_EQ(kFakeInstallationDate,
+              stored_client_info_backup_->installation_date);
+    EXPECT_EQ(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp),
+              stored_client_info_backup_->reporting_enabled_date);
+
+    tmp_client_info = stored_client_info_backup_.Pass();
+  }
+
+  EnableMetricsReporting();
+
+  {
+    EXPECT_FALSE(stored_client_info_backup_);
+
+    scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+
+    // client_id should be auto-obtained from the constructor when metrics
+    // reporting is enabled.
+    EXPECT_EQ(tmp_client_info->client_id, state_manager->client_id());
+
+    // The backup should also be refreshed when the client id re-initialized.
+    ASSERT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(tmp_client_info->client_id,
+              stored_client_info_backup_->client_id);
+    EXPECT_EQ(kFakeInstallationDate,
+              stored_client_info_backup_->installation_date);
+    EXPECT_EQ(tmp_client_info->reporting_enabled_date,
+              stored_client_info_backup_->reporting_enabled_date);
+
+    // Re-forcing client id creation shouldn't cause another backup and
+    // shouldn't affect the existing client id.
+    stored_client_info_backup_.reset();
+    state_manager->ForceClientIdCreation();
+    EXPECT_FALSE(stored_client_info_backup_);
+    EXPECT_EQ(tmp_client_info->client_id, state_manager->client_id());
+  }
+
+  const int64 kBackupInstallationDate = 1111;
+  const int64 kBackupReportingEnabledDate = 2222;
+  const char kBackupClientId[] = "AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE";
+  fake_client_info_backup_.reset(new ClientInfo);
+  fake_client_info_backup_->client_id = kBackupClientId;
+  fake_client_info_backup_->installation_date = kBackupInstallationDate;
+  fake_client_info_backup_->reporting_enabled_date =
+      kBackupReportingEnabledDate;
+
+  {
+    // The existence of a backup should result in the same behaviour as
+    // before if we already have a client id.
+
+    EXPECT_FALSE(stored_client_info_backup_);
+
+    scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+    EXPECT_EQ(tmp_client_info->client_id, state_manager->client_id());
+
+    // The backup should also be refreshed when the client id re-initialized.
+    ASSERT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(tmp_client_info->client_id,
+              stored_client_info_backup_->client_id);
+    EXPECT_EQ(kFakeInstallationDate,
+              stored_client_info_backup_->installation_date);
+    EXPECT_EQ(tmp_client_info->reporting_enabled_date,
+              stored_client_info_backup_->reporting_enabled_date);
+    stored_client_info_backup_.reset();
+  }
+
+  prefs_.ClearPref(prefs::kMetricsClientID);
+  prefs_.ClearPref(prefs::kMetricsReportingEnabledTimestamp);
+
+  {
+    // The backup should kick in if the client id has gone missing. It should
+    // replace remaining and missing dates as well.
+
+    EXPECT_FALSE(stored_client_info_backup_);
+
+    scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+    EXPECT_EQ(kBackupClientId, state_manager->client_id());
+    EXPECT_EQ(kBackupInstallationDate, prefs_.GetInt64(prefs::kInstallDate));
+    EXPECT_EQ(kBackupReportingEnabledDate,
+              prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
+
+    EXPECT_TRUE(stored_client_info_backup_);
+    stored_client_info_backup_.reset();
+  }
+
+  const char kNoDashesBackupClientId[] = "AAAAAAAABBBBCCCCDDDDEEEEEEEEEEEE";
+  fake_client_info_backup_.reset(new ClientInfo);
+  fake_client_info_backup_->client_id = kNoDashesBackupClientId;
+
+  prefs_.ClearPref(prefs::kInstallDate);
+  prefs_.ClearPref(prefs::kMetricsClientID);
+  prefs_.ClearPref(prefs::kMetricsReportingEnabledTimestamp);
+
+  {
+    // When running the backup from old-style client ids, dashes should be
+    // re-added. And missing dates in backup should be replaced by Time::Now().
+
+    EXPECT_FALSE(stored_client_info_backup_);
+
+    scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+    EXPECT_EQ(kBackupClientId, state_manager->client_id());
+    EXPECT_GT(prefs_.GetInt64(prefs::kInstallDate), 0);
+    EXPECT_GT(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp), 0);
+
+    EXPECT_TRUE(stored_client_info_backup_);
+    stored_client_info_backup_.reset();
+  }
+}
+
 }  // namespace metrics