Pass FilePath to Settings in Initialize()

Pulled out of jperaza's https://crrev.com/c/689745.

Future updates to the CrashReportDatabase would like to be decide on the
Settings location later than the constructor, but still keep the Settings
object embedded inline. To allow this, pass the location FilePath in
Initialize() rather than to the constructor.

Bug: crashpad:206
Change-Id: I8792188314541f6fd0bd04b168d22f8e445bc187
Reviewed-on: https://chromium-review.googlesource.com/916533
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/crash_report_database_mac.mm

@@ -243,7 +243,7 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
 CrashReportDatabaseMac::CrashReportDatabaseMac(const base::FilePath& path)
     : CrashReportDatabase(),
       base_dir_(path),
-      settings_(base_dir_.Append(kSettings)),
+      settings_(),
       xattr_new_names_(false),
       initialized_() {
 }
@@ -268,7 +268,7 @@ bool CrashReportDatabaseMac::Initialize(bool may_create) {
       return false;
   }
 
-  if (!settings_.Initialize())
+  if (!settings_.Initialize(base_dir_.Append(kSettings)))
     return false;
 
   // Do an xattr operation as the last step, to ensure the filesystem has

client/crash_report_database_win.cc

@@ -610,11 +610,7 @@ class CrashReportDatabaseWin : public CrashReportDatabase {
 };
 
 CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path)
-    : CrashReportDatabase(),
+    : CrashReportDatabase(), base_dir_(path), settings_(), initialized_() {}
-      base_dir_(path),
-      settings_(base_dir_.Append(kSettings)),
-      initialized_() {
-}
 
 CrashReportDatabaseWin::~CrashReportDatabaseWin() {
 }
@@ -634,7 +630,7 @@ bool CrashReportDatabaseWin::Initialize(bool may_create) {
   if (!CreateDirectoryIfNecessary(base_dir_.Append(kReportsDirectory)))
     return false;
 
-  if (!settings_.Initialize())
+  if (!settings_.Initialize(base_dir_.Append(kSettings)))
     return false;
 
   INITIALIZATION_STATE_SET_VALID(initialized_);

client/settings.cc

@@ -59,16 +59,14 @@ struct Settings::Data {
   UUID client_id;
 };
 
-Settings::Settings(const base::FilePath& file_path)
+Settings::Settings() = default;
-    : file_path_(file_path),
-      initialized_() {
-}
 
-Settings::~Settings() {
+Settings::~Settings() = default;
-}
 
-bool Settings::Initialize() {
+bool Settings::Initialize(const base::FilePath& file_path) {
+  DCHECK(initialized_.is_uninitialized());
   initialized_.set_invalid();
+  file_path_ = file_path;
 
   Data settings;
   if (!OpenForWritingAndReadSettings(&settings).is_valid())

client/settings.h

@@ -44,10 +44,18 @@ struct ScopedLockedFileHandleTraits {
 //! should be retrieved via CrashReportDatabase::GetSettings().
 class Settings {
  public:
-  explicit Settings(const base::FilePath& file_path);
+  Settings();
   ~Settings();
 
-  bool Initialize();
+  //! \brief Initializes the settings data store.
+  //!
+  //! This method must be called only once, and must be successfully called
+  //! before any other method in this class may be called.
+  //!
+  //! \param[in] path The location to store the settings data.
+  //! \return `true` if the data store was initialized successfully, otherwise
+  //!     `false` with an error logged.
+  bool Initialize(const base::FilePath& path);
 
   //! \brief Retrieves the immutable identifier for this client, which is used
   //!     on a server to locate all crash reports from a specific Crashpad

client/settings_test.cc

@@ -26,7 +26,7 @@ namespace {
 
 class SettingsTest : public testing::Test {
  public:
-  SettingsTest() : settings_(settings_path()) {}
+  SettingsTest() = default;
 
   base::FilePath settings_path() {
     return temp_dir_.path().Append(FILE_PATH_LITERAL("settings"));
@@ -49,7 +49,7 @@ class SettingsTest : public testing::Test {
  protected:
   // testing::Test:
   void SetUp() override {
-    ASSERT_TRUE(settings()->Initialize());
+    ASSERT_TRUE(settings()->Initialize(settings_path()));
   }
 
  private:
@@ -64,8 +64,8 @@ TEST_F(SettingsTest, ClientID) {
   EXPECT_TRUE(settings()->GetClientID(&client_id));
   EXPECT_NE(client_id, UUID());
 
-  Settings local_settings(settings_path());
+  Settings local_settings;
-  EXPECT_TRUE(local_settings.Initialize());
+  EXPECT_TRUE(local_settings.Initialize(settings_path()));
   UUID actual;
   EXPECT_TRUE(local_settings.GetClientID(&actual));
   EXPECT_EQ(actual, client_id);
@@ -81,8 +81,8 @@ TEST_F(SettingsTest, UploadsEnabled) {
   EXPECT_TRUE(settings()->GetUploadsEnabled(&enabled));
   EXPECT_TRUE(enabled);
 
-  Settings local_settings(settings_path());
+  Settings local_settings;
-  EXPECT_TRUE(local_settings.Initialize());
+  EXPECT_TRUE(local_settings.Initialize(settings_path()));
   enabled = false;
   EXPECT_TRUE(local_settings.GetUploadsEnabled(&enabled));
   EXPECT_TRUE(enabled);
@@ -107,8 +107,8 @@ TEST_F(SettingsTest, LastUploadAttemptTime) {
   EXPECT_TRUE(settings()->GetLastUploadAttemptTime(&actual));
   EXPECT_EQ(actual, expected);
 
-  Settings local_settings(settings_path());
+  Settings local_settings;
-  EXPECT_TRUE(local_settings.Initialize());
+  EXPECT_TRUE(local_settings.Initialize(settings_path()));
   actual = -1;
   EXPECT_TRUE(local_settings.GetLastUploadAttemptTime(&actual));
   EXPECT_EQ(actual, expected);
@@ -120,8 +120,8 @@ TEST_F(SettingsTest, LastUploadAttemptTime) {
 TEST_F(SettingsTest, BadFileOnInitialize) {
   InitializeBadFile();
 
-  Settings settings(settings_path());
+  Settings settings;
-  EXPECT_TRUE(settings.Initialize());
+  EXPECT_TRUE(settings.Initialize(settings_path()));
 }
 
 TEST_F(SettingsTest, BadFileOnGet) {
@@ -131,8 +131,8 @@ TEST_F(SettingsTest, BadFileOnGet) {
   EXPECT_TRUE(settings()->GetClientID(&client_id));
   EXPECT_NE(client_id, UUID());
 
-  Settings local_settings(settings_path());
+  Settings local_settings;
-  EXPECT_TRUE(local_settings.Initialize());
+  EXPECT_TRUE(local_settings.Initialize(settings_path()));
   UUID actual;
   EXPECT_TRUE(local_settings.GetClientID(&actual));
   EXPECT_EQ(actual, client_id);
@@ -161,8 +161,8 @@ TEST_F(SettingsTest, UnlinkFile) {
       << ErrnoMessage("unlink");
 #endif
 
-  Settings local_settings(settings_path());
+  Settings local_settings;
-  EXPECT_TRUE(local_settings.Initialize());
+  EXPECT_TRUE(local_settings.Initialize(settings_path()));
   UUID new_client_id;
   EXPECT_TRUE(local_settings.GetClientID(&new_client_id));
   EXPECT_NE(new_client_id, client_id);