update_manager: Make Prefs Variable async.
The update_engine prefs, while stored in disk, are private daemon
values changed by the daemon only. There was a 5 minutes delay between
changing this value and the update policy checking it again, and there
was a log spam every 5 minutes due to policy re-evaluations.
This patch makes these Prefs-based variables async by implementing an
observer pattern in the PrefsInterface and makes these variables async.
Bug: chromium:367333
Test: Added uniittest. No more log spam every 5 minutes.
Change-Id: I8b3f7072cc87515972c9f5b1ddcc54b865ffe238
diff --git a/fake_prefs.cc b/fake_prefs.cc
index 66b9152..c849388 100644
--- a/fake_prefs.cc
+++ b/fake_prefs.cc
@@ -16,6 +16,8 @@
#include "update_engine/fake_prefs.h"
+#include <algorithm>
+
#include <gtest/gtest.h>
using std::string;
@@ -33,6 +35,10 @@
namespace chromeos_update_engine {
+FakePrefs::~FakePrefs() {
+ EXPECT_TRUE(observers_.empty());
+}
+
// Compile-time type-dependent constants definitions.
template<>
FakePrefs::PrefType const FakePrefs::PrefConsts<string>::type =
@@ -55,8 +61,7 @@
bool FakePrefs::PrefValue::* const FakePrefs::PrefConsts<bool>::member =
&FakePrefs::PrefValue::as_bool;
-
-bool FakePrefs::GetString(const string& key, string* value) {
+bool FakePrefs::GetString(const string& key, string* value) const {
return GetValue(key, value);
}
@@ -65,7 +70,7 @@
return true;
}
-bool FakePrefs::GetInt64(const string& key, int64_t* value) {
+bool FakePrefs::GetInt64(const string& key, int64_t* value) const {
return GetValue(key, value);
}
@@ -74,7 +79,7 @@
return true;
}
-bool FakePrefs::GetBoolean(const string& key, bool* value) {
+bool FakePrefs::GetBoolean(const string& key, bool* value) const {
return GetValue(key, value);
}
@@ -83,7 +88,7 @@
return true;
}
-bool FakePrefs::Exists(const string& key) {
+bool FakePrefs::Exists(const string& key) const {
return values_.find(key) != values_.end();
}
@@ -91,6 +96,12 @@
if (values_.find(key) == values_.end())
return false;
values_.erase(key);
+ const auto observers_for_key = observers_.find(key);
+ if (observers_for_key != observers_.end()) {
+ std::vector<ObserverInterface*> copy_observers(observers_for_key->second);
+ for (ObserverInterface* observer : copy_observers)
+ observer->OnPrefDeleted(key);
+ }
return true;
}
@@ -118,6 +129,12 @@
CheckKeyType(key, PrefConsts<T>::type);
values_[key].type = PrefConsts<T>::type;
values_[key].value.*(PrefConsts<T>::member) = value;
+ const auto observers_for_key = observers_.find(key);
+ if (observers_for_key != observers_.end()) {
+ std::vector<ObserverInterface*> copy_observers(observers_for_key->second);
+ for (ObserverInterface* observer : copy_observers)
+ observer->OnPrefSet(key);
+ }
}
template<typename T>
@@ -131,4 +148,21 @@
return true;
}
+void FakePrefs::AddObserver(const string& key, ObserverInterface* observer) {
+ observers_[key].push_back(observer);
+}
+
+void FakePrefs::RemoveObserver(const string& key, ObserverInterface* observer) {
+ std::vector<ObserverInterface*>& observers_for_key = observers_[key];
+ auto observer_it =
+ std::find(observers_for_key.begin(), observers_for_key.end(), observer);
+ EXPECT_NE(observer_it, observers_for_key.end())
+ << "Trying to remove an observer instance not watching the key "
+ << key;
+ if (observer_it != observers_for_key.end())
+ observers_for_key.erase(observer_it);
+ if (observers_for_key.empty())
+ observers_.erase(key);
+}
+
} // namespace chromeos_update_engine
diff --git a/fake_prefs.h b/fake_prefs.h
index fbc8c67..2777ad6 100644
--- a/fake_prefs.h
+++ b/fake_prefs.h
@@ -19,6 +19,7 @@
#include <map>
#include <string>
+#include <vector>
#include <base/macros.h>
@@ -34,19 +35,25 @@
class FakePrefs : public PrefsInterface {
public:
- FakePrefs() {}
+ FakePrefs() = default;
+ ~FakePrefs();
// PrefsInterface methods.
- bool GetString(const std::string& key, std::string* value) override;
+ bool GetString(const std::string& key, std::string* value) const override;
bool SetString(const std::string& key, const std::string& value) override;
- bool GetInt64(const std::string& key, int64_t* value) override;
+ bool GetInt64(const std::string& key, int64_t* value) const override;
bool SetInt64(const std::string& key, const int64_t value) override;
- bool GetBoolean(const std::string& key, bool* value) override;
+ bool GetBoolean(const std::string& key, bool* value) const override;
bool SetBoolean(const std::string& key, const bool value) override;
- bool Exists(const std::string& key) override;
+ bool Exists(const std::string& key) const override;
bool Delete(const std::string& key) override;
+ void AddObserver(const std::string& key,
+ ObserverInterface* observer) override;
+ void RemoveObserver(const std::string& key,
+ ObserverInterface* observer) override;
+
private:
enum class PrefType {
kString,
@@ -95,6 +102,9 @@
// Container for all the key/value pairs.
std::map<std::string, PrefTypeValue> values_;
+ // The registered observers watching for changes.
+ std::map<std::string, std::vector<ObserverInterface*>> observers_;
+
DISALLOW_COPY_AND_ASSIGN(FakePrefs);
};
diff --git a/mock_prefs.h b/mock_prefs.h
index fafe39a..d1a97e8 100644
--- a/mock_prefs.h
+++ b/mock_prefs.h
@@ -28,17 +28,22 @@
class MockPrefs : public PrefsInterface {
public:
- MOCK_METHOD2(GetString, bool(const std::string& key, std::string* value));
+ MOCK_CONST_METHOD2(GetString,
+ bool(const std::string& key, std::string* value));
MOCK_METHOD2(SetString, bool(const std::string& key,
const std::string& value));
- MOCK_METHOD2(GetInt64, bool(const std::string& key, int64_t* value));
+ MOCK_CONST_METHOD2(GetInt64, bool(const std::string& key, int64_t* value));
MOCK_METHOD2(SetInt64, bool(const std::string& key, const int64_t value));
- MOCK_METHOD2(GetBoolean, bool(const std::string& key, bool* value));
+ MOCK_CONST_METHOD2(GetBoolean, bool(const std::string& key, bool* value));
MOCK_METHOD2(SetBoolean, bool(const std::string& key, const bool value));
- MOCK_METHOD1(Exists, bool(const std::string& key));
+ MOCK_CONST_METHOD1(Exists, bool(const std::string& key));
MOCK_METHOD1(Delete, bool(const std::string& key));
+
+ MOCK_METHOD2(AddObserver, void(const std::string& key, ObserverInterface*));
+ MOCK_METHOD2(RemoveObserver,
+ void(const std::string& key, ObserverInterface*));
};
} // namespace chromeos_update_engine
diff --git a/prefs.cc b/prefs.cc
index dea21e2..89da8bf 100644
--- a/prefs.cc
+++ b/prefs.cc
@@ -16,6 +16,8 @@
#include "update_engine/prefs.h"
+#include <algorithm>
+
#include <base/files/file_util.h>
#include <base/logging.h>
#include <base/strings/string_number_conversions.h>
@@ -32,7 +34,7 @@
return true;
}
-bool Prefs::GetString(const string& key, string* value) {
+bool Prefs::GetString(const string& key, string* value) const {
base::FilePath filename;
TEST_AND_RETURN_FALSE(GetFileNameForKey(key, &filename));
if (!base::ReadFileToString(filename, value)) {
@@ -48,10 +50,16 @@
TEST_AND_RETURN_FALSE(base::CreateDirectory(filename.DirName()));
TEST_AND_RETURN_FALSE(base::WriteFile(filename, value.data(), value.size()) ==
static_cast<int>(value.size()));
+ const auto observers_for_key = observers_.find(key);
+ if (observers_for_key != observers_.end()) {
+ std::vector<ObserverInterface*> copy_observers(observers_for_key->second);
+ for (ObserverInterface* observer : copy_observers)
+ observer->OnPrefSet(key);
+ }
return true;
}
-bool Prefs::GetInt64(const string& key, int64_t* value) {
+bool Prefs::GetInt64(const string& key, int64_t* value) const {
string str_value;
if (!GetString(key, &str_value))
return false;
@@ -64,7 +72,7 @@
return SetString(key, base::Int64ToString(value));
}
-bool Prefs::GetBoolean(const string& key, bool* value) {
+bool Prefs::GetBoolean(const string& key, bool* value) const {
string str_value;
if (!GetString(key, &str_value))
return false;
@@ -84,7 +92,7 @@
return SetString(key, value ? "true" : "false");
}
-bool Prefs::Exists(const string& key) {
+bool Prefs::Exists(const string& key) const {
base::FilePath filename;
TEST_AND_RETURN_FALSE(GetFileNameForKey(key, &filename));
return base::PathExists(filename);
@@ -93,11 +101,30 @@
bool Prefs::Delete(const string& key) {
base::FilePath filename;
TEST_AND_RETURN_FALSE(GetFileNameForKey(key, &filename));
- return base::DeleteFile(filename, false);
+ TEST_AND_RETURN_FALSE(base::DeleteFile(filename, false));
+ const auto observers_for_key = observers_.find(key);
+ if (observers_for_key != observers_.end()) {
+ std::vector<ObserverInterface*> copy_observers(observers_for_key->second);
+ for (ObserverInterface* observer : copy_observers)
+ observer->OnPrefDeleted(key);
+ }
+ return true;
+}
+
+void Prefs::AddObserver(const string& key, ObserverInterface* observer) {
+ observers_[key].push_back(observer);
+}
+
+void Prefs::RemoveObserver(const string& key, ObserverInterface* observer) {
+ std::vector<ObserverInterface*>& observers_for_key = observers_[key];
+ auto observer_it =
+ std::find(observers_for_key.begin(), observers_for_key.end(), observer);
+ if (observer_it != observers_for_key.end())
+ observers_for_key.erase(observer_it);
}
bool Prefs::GetFileNameForKey(const string& key,
- base::FilePath* filename) {
+ base::FilePath* filename) const {
// Allows only non-empty keys containing [A-Za-z0-9_-].
TEST_AND_RETURN_FALSE(!key.empty());
for (size_t i = 0; i < key.size(); ++i) {
diff --git a/prefs.h b/prefs.h
index 5ed56c6..1bcfef8 100644
--- a/prefs.h
+++ b/prefs.h
@@ -17,9 +17,12 @@
#ifndef UPDATE_ENGINE_PREFS_H_
#define UPDATE_ENGINE_PREFS_H_
+#include <map>
#include <string>
+#include <vector>
#include <base/files/file_path.h>
+
#include "gtest/gtest_prod.h" // for FRIEND_TEST
#include "update_engine/prefs_interface.h"
@@ -31,7 +34,7 @@
class Prefs : public PrefsInterface {
public:
- Prefs() {}
+ Prefs() = default;
// Initializes the store by associating this object with |prefs_dir|
// as the preference store directory. Returns true on success, false
@@ -39,16 +42,21 @@
bool Init(const base::FilePath& prefs_dir);
// PrefsInterface methods.
- bool GetString(const std::string& key, std::string* value) override;
+ bool GetString(const std::string& key, std::string* value) const override;
bool SetString(const std::string& key, const std::string& value) override;
- bool GetInt64(const std::string& key, int64_t* value) override;
+ bool GetInt64(const std::string& key, int64_t* value) const override;
bool SetInt64(const std::string& key, const int64_t value) override;
- bool GetBoolean(const std::string& key, bool* value) override;
+ bool GetBoolean(const std::string& key, bool* value) const override;
bool SetBoolean(const std::string& key, const bool value) override;
- bool Exists(const std::string& key) override;
+ bool Exists(const std::string& key) const override;
bool Delete(const std::string& key) override;
+ void AddObserver(const std::string& key,
+ ObserverInterface* observer) override;
+ void RemoveObserver(const std::string& key,
+ ObserverInterface* observer) override;
+
private:
FRIEND_TEST(PrefsTest, GetFileNameForKey);
FRIEND_TEST(PrefsTest, GetFileNameForKeyBadCharacter);
@@ -56,11 +64,15 @@
// Sets |filename| to the full path to the file containing the data
// associated with |key|. Returns true on success, false otherwise.
- bool GetFileNameForKey(const std::string& key, base::FilePath* filename);
+ bool GetFileNameForKey(const std::string& key,
+ base::FilePath* filename) const;
// Preference store directory.
base::FilePath prefs_dir_;
+ // The registered observers watching for changes.
+ std::map<std::string, std::vector<ObserverInterface*>> observers_;
+
DISALLOW_COPY_AND_ASSIGN(Prefs);
};
diff --git a/prefs_interface.h b/prefs_interface.h
index eedba4b..d409a1f 100644
--- a/prefs_interface.h
+++ b/prefs_interface.h
@@ -30,12 +30,24 @@
class PrefsInterface {
public:
- virtual ~PrefsInterface() {}
+ // Observer class to be notified about key value changes.
+ class ObserverInterface {
+ public:
+ virtual ~ObserverInterface() = default;
+
+ // Called when the value is set for the observed |key|.
+ virtual void OnPrefSet(const std::string& key) = 0;
+
+ // Called when the observed |key| is deleted.
+ virtual void OnPrefDeleted(const std::string& key) = 0;
+ };
+
+ virtual ~PrefsInterface() = default;
// Gets a string |value| associated with |key|. Returns true on
// success, false on failure (including when the |key| is not
// present in the store).
- virtual bool GetString(const std::string& key, std::string* value) = 0;
+ virtual bool GetString(const std::string& key, std::string* value) const = 0;
// Associates |key| with a string |value|. Returns true on success,
// false otherwise.
@@ -44,7 +56,7 @@
// Gets an int64_t |value| associated with |key|. Returns true on
// success, false on failure (including when the |key| is not
// present in the store).
- virtual bool GetInt64(const std::string& key, int64_t* value) = 0;
+ virtual bool GetInt64(const std::string& key, int64_t* value) const = 0;
// Associates |key| with an int64_t |value|. Returns true on success,
// false otherwise.
@@ -53,7 +65,7 @@
// Gets a boolean |value| associated with |key|. Returns true on
// success, false on failure (including when the |key| is not
// present in the store).
- virtual bool GetBoolean(const std::string& key, bool* value) = 0;
+ virtual bool GetBoolean(const std::string& key, bool* value) const = 0;
// Associates |key| with a boolean |value|. Returns true on success,
// false otherwise.
@@ -61,11 +73,23 @@
// Returns true if the setting exists (i.e. a file with the given key
// exists in the prefs directory)
- virtual bool Exists(const std::string& key) = 0;
+ virtual bool Exists(const std::string& key) const = 0;
// Returns true if successfully deleted the file corresponding to
// this key. Calling with non-existent keys does nothing.
virtual bool Delete(const std::string& key) = 0;
+
+ // Add an observer to watch whenever the given |key| is modified. The
+ // OnPrefSet() and OnPrefDelete() methods will be called whenever any of the
+ // Set*() methods or the Delete() method are called on the given key,
+ // respectively.
+ virtual void AddObserver(const std::string& key,
+ ObserverInterface* observer) = 0;
+
+ // Remove an observer added with AddObserver(). The observer won't be called
+ // anymore for future Set*() and Delete() method calls.
+ virtual void RemoveObserver(const std::string& key,
+ ObserverInterface* observer) = 0;
};
} // namespace chromeos_update_engine
diff --git a/prefs_unittest.cc b/prefs_unittest.cc
index 0053fd0..df356b1 100644
--- a/prefs_unittest.cc
+++ b/prefs_unittest.cc
@@ -24,9 +24,17 @@
#include <base/macros.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
using std::string;
+using testing::Eq;
+using testing::_;
+
+namespace {
+// Test key used along the tests.
+const char kKey[] = "test-key";
+}
namespace chromeos_update_engine {
@@ -51,10 +59,11 @@
};
TEST_F(PrefsTest, GetFileNameForKey) {
- const char kKey[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-";
+ const char kAllvalidCharsKey[] =
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-";
base::FilePath path;
- EXPECT_TRUE(prefs_.GetFileNameForKey(kKey, &path));
- EXPECT_EQ(prefs_dir_.Append(kKey).value(), path.value());
+ EXPECT_TRUE(prefs_.GetFileNameForKey(kAllvalidCharsKey, &path));
+ EXPECT_EQ(prefs_dir_.Append(kAllvalidCharsKey).value(), path.value());
}
TEST_F(PrefsTest, GetFileNameForKeyBadCharacter) {
@@ -68,7 +77,6 @@
}
TEST_F(PrefsTest, GetString) {
- const char kKey[] = "test-key";
const string test_data = "test data";
ASSERT_TRUE(SetValue(kKey, test_data));
string value;
@@ -87,7 +95,6 @@
}
TEST_F(PrefsTest, SetString) {
- const char kKey[] = "my_test_key";
const char kValue[] = "some test value\non 2 lines";
EXPECT_TRUE(prefs_.SetString(kKey, kValue));
string value;
@@ -96,13 +103,12 @@
}
TEST_F(PrefsTest, SetStringBadKey) {
- const char kKey[] = ".no-dots";
- EXPECT_FALSE(prefs_.SetString(kKey, "some value"));
- EXPECT_FALSE(base::PathExists(prefs_dir_.Append(kKey)));
+ const char kKeyWithDots[] = ".no-dots";
+ EXPECT_FALSE(prefs_.SetString(kKeyWithDots, "some value"));
+ EXPECT_FALSE(base::PathExists(prefs_dir_.Append(kKeyWithDots)));
}
TEST_F(PrefsTest, SetStringCreateDir) {
- const char kKey[] = "a-test-key";
const char kValue[] = "test value";
base::FilePath subdir = prefs_dir_.Append("subdir1").Append("subdir2");
EXPECT_TRUE(prefs_.Init(subdir));
@@ -114,19 +120,16 @@
TEST_F(PrefsTest, SetStringDirCreationFailure) {
EXPECT_TRUE(prefs_.Init(base::FilePath("/dev/null")));
- const char kKey[] = "test-key";
EXPECT_FALSE(prefs_.SetString(kKey, "test value"));
}
TEST_F(PrefsTest, SetStringFileCreationFailure) {
- const char kKey[] = "a-test-key";
base::CreateDirectory(prefs_dir_.Append(kKey));
EXPECT_FALSE(prefs_.SetString(kKey, "test value"));
EXPECT_TRUE(base::DirectoryExists(prefs_dir_.Append(kKey)));
}
TEST_F(PrefsTest, GetInt64) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, " \n 25 \t "));
int64_t value;
EXPECT_TRUE(prefs_.GetInt64(kKey, &value));
@@ -134,14 +137,12 @@
}
TEST_F(PrefsTest, GetInt64BadValue) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, "30a"));
int64_t value;
EXPECT_FALSE(prefs_.GetInt64(kKey, &value));
}
TEST_F(PrefsTest, GetInt64Max) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, base::StringPrintf("%" PRIi64, kint64max)));
int64_t value;
EXPECT_TRUE(prefs_.GetInt64(kKey, &value));
@@ -149,7 +150,6 @@
}
TEST_F(PrefsTest, GetInt64Min) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, base::StringPrintf("%" PRIi64, kint64min)));
int64_t value;
EXPECT_TRUE(prefs_.GetInt64(kKey, &value));
@@ -157,7 +157,6 @@
}
TEST_F(PrefsTest, GetInt64Negative) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, " \t -100 \n "));
int64_t value;
EXPECT_TRUE(prefs_.GetInt64(kKey, &value));
@@ -170,7 +169,6 @@
}
TEST_F(PrefsTest, SetInt64) {
- const char kKey[] = "test_int";
EXPECT_TRUE(prefs_.SetInt64(kKey, -123));
string value;
EXPECT_TRUE(base::ReadFileToString(prefs_dir_.Append(kKey), &value));
@@ -178,13 +176,12 @@
}
TEST_F(PrefsTest, SetInt64BadKey) {
- const char kKey[] = "s p a c e s";
- EXPECT_FALSE(prefs_.SetInt64(kKey, 20));
- EXPECT_FALSE(base::PathExists(prefs_dir_.Append(kKey)));
+ const char kKeyWithSpaces[] = "s p a c e s";
+ EXPECT_FALSE(prefs_.SetInt64(kKeyWithSpaces, 20));
+ EXPECT_FALSE(base::PathExists(prefs_dir_.Append(kKeyWithSpaces)));
}
TEST_F(PrefsTest, SetInt64Max) {
- const char kKey[] = "test-max-int";
EXPECT_TRUE(prefs_.SetInt64(kKey, kint64max));
string value;
EXPECT_TRUE(base::ReadFileToString(prefs_dir_.Append(kKey), &value));
@@ -192,7 +189,6 @@
}
TEST_F(PrefsTest, SetInt64Min) {
- const char kKey[] = "test-min-int";
EXPECT_TRUE(prefs_.SetInt64(kKey, kint64min));
string value;
EXPECT_TRUE(base::ReadFileToString(prefs_dir_.Append(kKey), &value));
@@ -200,7 +196,6 @@
}
TEST_F(PrefsTest, GetBooleanFalse) {
- const char kKey[] = "test-key";
ASSERT_TRUE(SetValue(kKey, " \n false \t "));
bool value;
EXPECT_TRUE(prefs_.GetBoolean(kKey, &value));
@@ -257,8 +252,6 @@
}
TEST_F(PrefsTest, ExistsWorks) {
- const char kKey[] = "exists-key";
-
// test that the key doesn't exist before we set it.
EXPECT_FALSE(prefs_.Exists(kKey));
@@ -268,8 +261,6 @@
}
TEST_F(PrefsTest, DeleteWorks) {
- const char kKey[] = "delete-key";
-
// test that it's alright to delete a non-existent key.
EXPECT_TRUE(prefs_.Delete(kKey));
@@ -281,4 +272,66 @@
EXPECT_FALSE(prefs_.Exists(kKey));
}
+class MockPrefsObserver : public PrefsInterface::ObserverInterface {
+ public:
+ MOCK_METHOD1(OnPrefSet, void(const string&));
+ MOCK_METHOD1(OnPrefDeleted, void(const string& key));
+};
+
+TEST_F(PrefsTest, ObserversCalled) {
+ MockPrefsObserver mock_obserser;
+ prefs_.AddObserver(kKey, &mock_obserser);
+
+ EXPECT_CALL(mock_obserser, OnPrefSet(Eq(kKey)));
+ EXPECT_CALL(mock_obserser, OnPrefDeleted(_)).Times(0);
+ prefs_.SetString(kKey, "value");
+ testing::Mock::VerifyAndClearExpectations(&mock_obserser);
+
+ EXPECT_CALL(mock_obserser, OnPrefSet(_)).Times(0);
+ EXPECT_CALL(mock_obserser, OnPrefDeleted(Eq(kKey)));
+ prefs_.Delete(kKey);
+ testing::Mock::VerifyAndClearExpectations(&mock_obserser);
+
+ prefs_.RemoveObserver(kKey, &mock_obserser);
+}
+
+TEST_F(PrefsTest, OnlyCalledOnObservedKeys) {
+ MockPrefsObserver mock_obserser;
+ const char kUnusedKey[] = "unused-key";
+ prefs_.AddObserver(kUnusedKey, &mock_obserser);
+
+ EXPECT_CALL(mock_obserser, OnPrefSet(_)).Times(0);
+ EXPECT_CALL(mock_obserser, OnPrefDeleted(_)).Times(0);
+ prefs_.SetString(kKey, "value");
+ prefs_.Delete(kKey);
+
+ prefs_.RemoveObserver(kUnusedKey, &mock_obserser);
+}
+
+TEST_F(PrefsTest, RemovedObserversNotCalled) {
+ MockPrefsObserver mock_obserser_a, mock_obserser_b;
+ prefs_.AddObserver(kKey, &mock_obserser_a);
+ prefs_.AddObserver(kKey, &mock_obserser_b);
+ EXPECT_CALL(mock_obserser_a, OnPrefSet(_)).Times(2);
+ EXPECT_CALL(mock_obserser_b, OnPrefSet(_)).Times(1);
+ EXPECT_TRUE(prefs_.SetString(kKey, "value"));
+ prefs_.RemoveObserver(kKey, &mock_obserser_b);
+ EXPECT_TRUE(prefs_.SetString(kKey, "other value"));
+ prefs_.RemoveObserver(kKey, &mock_obserser_a);
+ EXPECT_TRUE(prefs_.SetString(kKey, "yet another value"));
+}
+
+TEST_F(PrefsTest, UnsuccessfulCallsNotObserved) {
+ MockPrefsObserver mock_obserser;
+ const char kInvalidKey[] = "no spaces or .";
+ prefs_.AddObserver(kInvalidKey, &mock_obserser);
+
+ EXPECT_CALL(mock_obserser, OnPrefSet(_)).Times(0);
+ EXPECT_CALL(mock_obserser, OnPrefDeleted(_)).Times(0);
+ EXPECT_FALSE(prefs_.SetString(kInvalidKey, "value"));
+ EXPECT_FALSE(prefs_.Delete(kInvalidKey));
+
+ prefs_.RemoveObserver(kInvalidKey, &mock_obserser);
+}
+
} // namespace chromeos_update_engine
diff --git a/update_manager/fake_updater_provider.h b/update_manager/fake_updater_provider.h
index 7d2d3f6..44389f4 100644
--- a/update_manager/fake_updater_provider.h
+++ b/update_manager/fake_updater_provider.h
@@ -106,10 +106,12 @@
"curr_channel", kVariableModePoll};
FakeVariable<std::string> var_new_channel_{ // NOLINT(whitespace/braces)
"new_channel", kVariableModePoll};
- FakeVariable<bool> var_p2p_enabled_{ // NOLINT(whitespace/braces)
- "p2p_enabled", kVariableModePoll};
- FakeVariable<bool> var_cellular_enabled_{ // NOLINT(whitespace/braces)
- "cellular_enabled", kVariableModePoll};
+ FakeVariable<bool> var_p2p_enabled_{// NOLINT(whitespace/braces)
+ "p2p_enabled",
+ kVariableModeAsync};
+ FakeVariable<bool> var_cellular_enabled_{// NOLINT(whitespace/braces)
+ "cellular_enabled",
+ kVariableModeAsync};
FakeVariable<unsigned int>
var_consecutive_failed_update_checks_{ // NOLINT(whitespace/braces)
"consecutive_failed_update_checks", kVariableModePoll};
diff --git a/update_manager/real_updater_provider.cc b/update_manager/real_updater_provider.cc
index 14d60d2..1c2868d 100644
--- a/update_manager/real_updater_provider.cc
+++ b/update_manager/real_updater_provider.cc
@@ -298,25 +298,42 @@
};
// A variable class for reading Boolean prefs values.
-class BooleanPrefVariable : public UpdaterVariableBase<bool> {
+class BooleanPrefVariable
+ : public AsyncCopyVariable<bool>,
+ public chromeos_update_engine::PrefsInterface::ObserverInterface {
public:
- BooleanPrefVariable(const string& name, SystemState* system_state,
- const char* key, bool default_val)
- : UpdaterVariableBase<bool>(name, kVariableModePoll, system_state),
- key_(key), default_val_(default_val) {}
+ BooleanPrefVariable(const string& name,
+ chromeos_update_engine::PrefsInterface* prefs,
+ const char* key,
+ bool default_val)
+ : AsyncCopyVariable<bool>(name),
+ prefs_(prefs),
+ key_(key),
+ default_val_(default_val) {
+ prefs->AddObserver(key, this);
+ OnPrefSet(key);
+ }
+ ~BooleanPrefVariable() {
+ prefs_->RemoveObserver(key_, this);
+ }
private:
- const bool* GetValue(TimeDelta /* timeout */, string* errmsg) override {
+ // Reads the actual value from the Prefs instance and updates the Variable
+ // value.
+ void OnPrefSet(const string& key) override {
bool result = default_val_;
- chromeos_update_engine::PrefsInterface* prefs = system_state()->prefs();
- if (prefs && prefs->Exists(key_) && !prefs->GetBoolean(key_, &result)) {
- if (errmsg)
- *errmsg = string("Could not read boolean pref ") + key_;
- return nullptr;
- }
- return new bool(result);
+ if (prefs_ && prefs_->Exists(key_) && !prefs_->GetBoolean(key_, &result))
+ result = default_val_;
+ // AsyncCopyVariable will take care of values that didn't change.
+ SetValue(result);
}
+ void OnPrefDeleted(const string& key) override {
+ SetValue(default_val_);
+ }
+
+ chromeos_update_engine::PrefsInterface* prefs_;
+
// The Boolean preference key and default value.
const char* const key_;
const bool default_val_;
@@ -415,12 +432,12 @@
var_curr_channel_(new CurrChannelVariable("curr_channel", system_state_)),
var_new_channel_(new NewChannelVariable("new_channel", system_state_)),
var_p2p_enabled_(
- new BooleanPrefVariable("p2p_enabled", system_state_,
+ new BooleanPrefVariable("p2p_enabled", system_state_->prefs(),
chromeos_update_engine::kPrefsP2PEnabled,
false)),
var_cellular_enabled_(
new BooleanPrefVariable(
- "cellular_enabled", system_state_,
+ "cellular_enabled", system_state_->prefs(),
chromeos_update_engine::kPrefsUpdateOverCellularPermission,
false)),
var_consecutive_failed_update_checks_(
diff --git a/update_manager/real_updater_provider_unittest.cc b/update_manager/real_updater_provider_unittest.cc
index 4187427..2f0f9dd 100644
--- a/update_manager/real_updater_provider_unittest.cc
+++ b/update_manager/real_updater_provider_unittest.cc
@@ -24,8 +24,8 @@
#include <update_engine/dbus-constants.h>
#include "update_engine/fake_clock.h"
+#include "update_engine/fake_prefs.h"
#include "update_engine/fake_system_state.h"
-#include "update_engine/mock_prefs.h"
#include "update_engine/mock_update_attempter.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/update_manager/umtest_utils.h"
@@ -33,14 +33,13 @@
using base::Time;
using base::TimeDelta;
using chromeos_update_engine::FakeClock;
+using chromeos_update_engine::FakePrefs;
using chromeos_update_engine::FakeSystemState;
-using chromeos_update_engine::MockPrefs;
using chromeos_update_engine::OmahaRequestParams;
using std::string;
using std::unique_ptr;
using testing::Return;
using testing::SetArgPointee;
-using testing::StrEq;
using testing::_;
namespace {
@@ -76,30 +75,13 @@
protected:
void SetUp() override {
fake_clock_ = fake_sys_state_.fake_clock();
+ fake_sys_state_.set_prefs(&fake_prefs_);
provider_.reset(new RealUpdaterProvider(&fake_sys_state_));
ASSERT_NE(nullptr, provider_.get());
// Check that provider initializes correctly.
ASSERT_TRUE(provider_->Init());
}
- // Sets up mock expectations for testing a variable that reads a Boolean pref
- // |key|. |key_exists| determines whether the key is present. If it is, then
- // |get_boolean_success| determines whether reading it is successful, and if
- // so |output| is the value being read.
- void SetupReadBooleanPref(const char* key, bool key_exists,
- bool get_boolean_success, bool output) {
- MockPrefs* const mock_prefs = fake_sys_state_.mock_prefs();
- EXPECT_CALL(*mock_prefs, Exists(StrEq(key))).WillOnce(Return(key_exists));
- if (key_exists) {
- auto& get_boolean = EXPECT_CALL(
- *fake_sys_state_.mock_prefs(), GetBoolean(StrEq(key), _));
- if (get_boolean_success)
- get_boolean.WillOnce(DoAll(SetArgPointee<1>(output), Return(true)));
- else
- get_boolean.WillOnce(Return(false));
- }
- }
-
// Sets up mock expectations for testing the update completed time reporting.
// |valid| determines whether the returned time is valid. Returns the expected
// update completed time value.
@@ -120,6 +102,7 @@
FakeSystemState fake_sys_state_;
FakeClock* fake_clock_; // Short for fake_sys_state_.fake_clock()
+ FakePrefs fake_prefs_;
unique_ptr<RealUpdaterProvider> provider_;
};
@@ -389,57 +372,39 @@
}
TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefDoesntExist) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- false, false, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefReadsFalse) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, true, false);
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefReadsTrue) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, true, true);
+TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledReadWhenInitialized) {
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, true);
+ SetUp();
UmTestUtils::ExpectVariableHasValue(true, provider_->var_p2p_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledFailCannotReadPref) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, false, false);
- UmTestUtils::ExpectVariableNotSet(provider_->var_p2p_enabled());
+TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledUpdated) {
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, false);
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, true);
+ UmTestUtils::ExpectVariableHasValue(true, provider_->var_p2p_enabled());
+ fake_prefs_.Delete(chromeos_update_engine::kPrefsP2PEnabled);
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefDoesntExist) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- false, false, false);
- UmTestUtils::ExpectVariableHasValue(false, provider_->var_cellular_enabled());
-}
-
-TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefReadsFalse) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, true, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_cellular_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefReadsTrue) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, true, true);
+ fake_prefs_.SetBoolean(
+ chromeos_update_engine::kPrefsUpdateOverCellularPermission, true);
UmTestUtils::ExpectVariableHasValue(true, provider_->var_cellular_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledFailCannotReadPref) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, false, false);
- UmTestUtils::ExpectVariableNotSet(provider_->var_cellular_enabled());
-}
-
TEST_F(UmRealUpdaterProviderTest, GetUpdateCompletedTimeOkay) {
Time expected = SetupUpdateCompletedTime(true);
UmTestUtils::ExpectVariableHasValue(expected,