From e5bbdaff87a9dc1ffba88ab017c98d348f5560c3 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Tue, 13 Feb 2018 14:25:14 -0800 Subject: [PATCH] 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 Reviewed-by: Mark Mentovai --- client/crash_report_database_mac.mm | 4 ++-- client/crash_report_database_win.cc | 8 ++------ client/settings.cc | 12 +++++------- client/settings.h | 12 ++++++++++-- client/settings_test.cc | 28 ++++++++++++++-------------- 5 files changed, 33 insertions(+), 31 deletions(-) diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 7a9154b8..3bba2b63 100644 --- a/client/crash_report_database_mac.mm +++ b/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 diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 538eff5f..d6699e1b 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -610,11 +610,7 @@ class CrashReportDatabaseWin : public CrashReportDatabase { }; CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path) - : CrashReportDatabase(), - base_dir_(path), - settings_(base_dir_.Append(kSettings)), - initialized_() { -} + : CrashReportDatabase(), base_dir_(path), settings_(), 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_); diff --git a/client/settings.cc b/client/settings.cc index 15d16f2e..8d4dbec8 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -59,16 +59,14 @@ struct Settings::Data { UUID client_id; }; -Settings::Settings(const base::FilePath& file_path) - : file_path_(file_path), - initialized_() { -} +Settings::Settings() = default; -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()) diff --git a/client/settings.h b/client/settings.h index b64f74fb..488080c1 100644 --- a/client/settings.h +++ b/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 diff --git a/client/settings_test.cc b/client/settings_test.cc index ca961a23..3a5730bd 100644 --- a/client/settings_test.cc +++ b/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()); - EXPECT_TRUE(local_settings.Initialize()); + Settings local_settings; + 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()); - EXPECT_TRUE(local_settings.Initialize()); + Settings local_settings; + 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()); - EXPECT_TRUE(local_settings.Initialize()); + Settings local_settings; + 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()); - EXPECT_TRUE(settings.Initialize()); + Settings settings; + 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()); - EXPECT_TRUE(local_settings.Initialize()); + Settings local_settings; + 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()); - EXPECT_TRUE(local_settings.Initialize()); + Settings local_settings; + 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);