PolicyManager: Move Init() from the Provider class to the classes that require it.
Not all the providers require an Init() method, and not all of them
can fail. For example, none of the Fake version of the providers
require an special initialization on a separated method.
This patch moves those Init() methods from the base Provider class
to the classes that require it. When you create a class Foo instance,
it is your responsibility to call Foo::Init(), and since you know the
type of the instance you are creating, you know what to expect from
its Init() method. This patch simplifies the FakeState class while
removing the Init() methods from those classes.
Init() methods are simply a way to have a constructor that can fail
without using exceptions and it is the style's guide recommended way
to have the same functionality.
BUG=chromium:358269
TEST=Build and unit test still passing.
Change-Id: Ida7d2cc1a494998144ca5c488513c85223def626
Reviewed-on: https://chromium-review.googlesource.com/196971
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
diff --git a/SConstruct b/SConstruct
index 5688690..032bf3e 100644
--- a/SConstruct
+++ b/SConstruct
@@ -302,7 +302,6 @@
policy_manager/boxed_value_unittest.cc
policy_manager/evaluation_context_unittest.cc
policy_manager/event_loop_unittest.cc
- policy_manager/fake_state.cc
policy_manager/generic_variables_unittest.cc
policy_manager/pmtest_utils.cc
policy_manager/policy_manager_unittest.cc
diff --git a/fake_system_state.cc b/fake_system_state.cc
index 40b98db..8c73aa1 100644
--- a/fake_system_state.cc
+++ b/fake_system_state.cc
@@ -32,7 +32,7 @@
fake_system_rebooted_(false) {
mock_payload_state_.Initialize(this);
mock_update_attempter_.Init();
- fake_policy_manager_.Init(FakeState::Construct());
+ fake_policy_manager_.Init(new FakeState());
}
} // namespace chromeos_update_engine
diff --git a/policy_manager/fake_config_provider.h b/policy_manager/fake_config_provider.h
index 3a31eb3..7c30190 100644
--- a/policy_manager/fake_config_provider.h
+++ b/policy_manager/fake_config_provider.h
@@ -16,8 +16,6 @@
FakeConfigProvider() {}
protected:
- virtual bool DoInit() override { return true; }
-
virtual FakeVariable<bool>* var_is_oobe_enabled() override {
return &var_is_oobe_enabled_;
}
diff --git a/policy_manager/fake_device_policy_provider.h b/policy_manager/fake_device_policy_provider.h
index 252a5e5..4fa592f 100644
--- a/policy_manager/fake_device_policy_provider.h
+++ b/policy_manager/fake_device_policy_provider.h
@@ -60,8 +60,6 @@
}
private:
- virtual bool DoInit() override { return true; }
-
FakeVariable<bool> var_device_policy_is_loaded_{
"policy_is_loaded", kVariableModePoll};
FakeVariable<std::string> var_release_channel_{
diff --git a/policy_manager/fake_random_provider.h b/policy_manager/fake_random_provider.h
index 64f08a4..bfec2eb 100644
--- a/policy_manager/fake_random_provider.h
+++ b/policy_manager/fake_random_provider.h
@@ -18,8 +18,6 @@
virtual FakeVariable<uint64_t>* var_seed() override { return &var_seed_; }
private:
- virtual bool DoInit() override { return true; }
-
FakeVariable<uint64_t> var_seed_{"seed", kVariableModePoll};
DISALLOW_COPY_AND_ASSIGN(FakeRandomProvider);
diff --git a/policy_manager/fake_shill_provider.h b/policy_manager/fake_shill_provider.h
index c18b0d2..ffe8751 100644
--- a/policy_manager/fake_shill_provider.h
+++ b/policy_manager/fake_shill_provider.h
@@ -33,8 +33,6 @@
}
private:
- virtual bool DoInit() override { return true; }
-
FakeVariable<bool> var_is_connected_{"is_connected", kVariableModePoll};
FakeVariable<ConnectionType> var_conn_type_{"conn_type", kVariableModePoll};
FakeVariable<ConnectionTethering> var_conn_tethering_{
diff --git a/policy_manager/fake_state.cc b/policy_manager/fake_state.cc
deleted file mode 100644
index 6fa663b..0000000
--- a/policy_manager/fake_state.cc
+++ /dev/null
@@ -1,25 +0,0 @@
-// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "update_engine/policy_manager/fake_state.h"
-
-#include "base/memory/scoped_ptr.h"
-
-namespace chromeos_policy_manager {
-
-FakeState* FakeState::Construct() {
- scoped_ptr<FakeState> fake_state(new FakeState());
- if (!(fake_state->config_provider_.Init() &&
- fake_state->device_policy_provider_.Init() &&
- fake_state->random_provider_.Init() &&
- fake_state->shill_provider_.Init() &&
- fake_state->system_provider_.Init() &&
- fake_state->time_provider_.Init() &&
- fake_state->updater_provider_.Init())) {
- return NULL;
- }
- return fake_state.release();
-}
-
-} // namespace chromeos_policy_manager
diff --git a/policy_manager/fake_state.h b/policy_manager/fake_state.h
index 9489050..b6495e5 100644
--- a/policy_manager/fake_state.h
+++ b/policy_manager/fake_state.h
@@ -17,15 +17,23 @@
namespace chromeos_policy_manager {
// A fake State class that creates fake providers for all the providers.
+// This fake can be used in unit testing of Policy subclasses. To fake out the
+// value a variable is exposing, just call FakeVariable<T>::SetValue() on the
+// variable you fake out. For example:
+//
+// FakeState fake_state_;
+// fake_state_.random_provider_->var_seed()->SetValue(new uint64_t(12345));
+//
+// You can call SetValue more than once and the FakeVariable will take care of
+// the memory, but only the last value will remain.
class FakeState : public State {
public:
- // Creates and initializes the FakeState using fake providers. Returns NULL
- // if the initialization fails.
- static FakeState* Construct();
+ // Creates and initializes the FakeState using fake providers.
+ FakeState() {}
virtual ~FakeState() {}
- // Downcasted getters, to allow access to the fake instances during testing.
+ // Downcasted detters to access the fake instances during testing.
virtual FakeConfigProvider* config_provider() override {
return &config_provider_;
}
@@ -55,8 +63,6 @@
}
private:
- FakeState() {}
-
FakeConfigProvider config_provider_;
FakeDevicePolicyProvider device_policy_provider_;
FakeRandomProvider random_provider_;
diff --git a/policy_manager/fake_system_provider.h b/policy_manager/fake_system_provider.h
index fbdff49..5886828 100644
--- a/policy_manager/fake_system_provider.h
+++ b/policy_manager/fake_system_provider.h
@@ -24,8 +24,6 @@
}
private:
- virtual bool DoInit() override { return true; }
-
FakeVariable<bool> var_is_normal_boot_mode_{
"is_normal_boot_mode", kVariableModeConst};
FakeVariable<bool> var_is_official_build_{
diff --git a/policy_manager/fake_time_provider.h b/policy_manager/fake_time_provider.h
index a73346a..d160289 100644
--- a/policy_manager/fake_time_provider.h
+++ b/policy_manager/fake_time_provider.h
@@ -24,8 +24,6 @@
}
private:
- virtual bool DoInit() override { return true; }
-
FakeVariable<base::Time> var_curr_date_{"curr_date", kVariableModePoll};
FakeVariable<int> var_curr_hour_{"curr_hour", kVariableModePoll};
diff --git a/policy_manager/fake_updater_provider.h b/policy_manager/fake_updater_provider.h
index aa12ce7..17060ce 100644
--- a/policy_manager/fake_updater_provider.h
+++ b/policy_manager/fake_updater_provider.h
@@ -56,8 +56,6 @@
}
private:
- virtual bool DoInit() { return true; }
-
FakeVariable<base::Time> var_last_checked_time_{
"last_checked_time", kVariableModePoll};
FakeVariable<base::Time> var_update_completed_time_{
diff --git a/policy_manager/policy_manager_unittest.cc b/policy_manager/policy_manager_unittest.cc
index 9632609..d3a847e 100644
--- a/policy_manager/policy_manager_unittest.cc
+++ b/policy_manager/policy_manager_unittest.cc
@@ -38,8 +38,7 @@
protected:
virtual void SetUp() {
- fake_state_ = FakeState::Construct();
- PMTEST_ASSERT_NOT_NULL(fake_state_);
+ fake_state_ = new FakeState();
EXPECT_TRUE(pmut_.Init(fake_state_));
}
diff --git a/policy_manager/provider.h b/policy_manager/provider.h
index 9ee5e8a..a4e9c54 100644
--- a/policy_manager/provider.h
+++ b/policy_manager/provider.h
@@ -10,23 +10,12 @@
// Abstract base class for a policy provider.
class Provider {
public:
- Provider() : is_initialized_(false) {}
virtual ~Provider() {}
- // Initializes the provider at most once. Returns true on success.
- bool Init() {
- return is_initialized_ || (is_initialized_ = DoInit());
- }
-
protected:
- // Performs the actual initialization. To be implemented by concrete
- // subclasses.
- virtual bool DoInit() = 0;
+ Provider() {}
private:
- // Whether the provider was already initialized.
- bool is_initialized_;
-
DISALLOW_COPY_AND_ASSIGN(Provider);
};
diff --git a/policy_manager/real_config_provider.cc b/policy_manager/real_config_provider.cc
index d30db12..df15ab3 100644
--- a/policy_manager/real_config_provider.cc
+++ b/policy_manager/real_config_provider.cc
@@ -25,7 +25,7 @@
namespace chromeos_policy_manager {
-bool RealConfigProvider::DoInit() {
+bool RealConfigProvider::Init() {
KeyValueStore store;
if (hardware_->IsNormalBootMode()) {
diff --git a/policy_manager/real_config_provider.h b/policy_manager/real_config_provider.h
index d79227f..b3a30e7 100644
--- a/policy_manager/real_config_provider.h
+++ b/policy_manager/real_config_provider.h
@@ -22,6 +22,9 @@
chromeos_update_engine::HardwareInterface* hardware)
: hardware_(hardware) {}
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+
Variable<bool>* var_is_oobe_enabled() override {
return var_is_oobe_enabled_.get();
}
@@ -29,8 +32,6 @@
private:
friend class PmRealConfigProviderTest;
- virtual bool DoInit() override;
-
// Used for testing. Sets the root prefix, which is by default "". Call this
// method before calling Init() in order to mock out the place where the files
// are being read from.
diff --git a/policy_manager/real_device_policy_provider.cc b/policy_manager/real_device_policy_provider.cc
index 46e4416..d0356da 100644
--- a/policy_manager/real_device_policy_provider.cc
+++ b/policy_manager/real_device_policy_provider.cc
@@ -29,7 +29,7 @@
CancelMainLoopEvent(scheduled_refresh_);
}
-bool RealDevicePolicyProvider::DoInit() {
+bool RealDevicePolicyProvider::Init() {
CHECK(policy_provider_ != nullptr);
// On Init() we try to get the device policy and keep updating it.
diff --git a/policy_manager/real_device_policy_provider.h b/policy_manager/real_device_policy_provider.h
index dbd0879..bfbb299 100644
--- a/policy_manager/real_device_policy_provider.h
+++ b/policy_manager/real_device_policy_provider.h
@@ -24,6 +24,9 @@
: policy_provider_(policy_provider) {}
~RealDevicePolicyProvider();
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+
virtual Variable<bool>* var_device_policy_is_loaded() override {
return &var_device_policy_is_loaded_;
}
@@ -70,9 +73,6 @@
FRIEND_TEST(PmRealDevicePolicyProviderTest, NonExistentDevicePolicyReloaded);
FRIEND_TEST(PmRealDevicePolicyProviderTest, ValuesUpdated);
- // Provider override.
- bool DoInit() override;
-
// Schedules a call to periodically refresh the device policy.
void RefreshDevicePolicyAndReschedule();
diff --git a/policy_manager/real_random_provider.cc b/policy_manager/real_random_provider.cc
index 3ab4ff4..d29590b 100644
--- a/policy_manager/real_random_provider.cc
+++ b/policy_manager/real_random_provider.cc
@@ -63,7 +63,7 @@
DISALLOW_COPY_AND_ASSIGN(RandomSeedVariable);
};
-bool RealRandomProvider::DoInit(void) {
+bool RealRandomProvider::Init(void) {
FILE* fp = fopen(kRandomDevice, "r");
if (!fp)
return false;
diff --git a/policy_manager/real_random_provider.h b/policy_manager/real_random_provider.h
index 7dd8fab..cc27c07 100644
--- a/policy_manager/real_random_provider.h
+++ b/policy_manager/real_random_provider.h
@@ -16,9 +16,10 @@
virtual Variable<uint64_t>* var_seed() override { return var_seed_.get(); }
- private:
- virtual bool DoInit(void) override;
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+ private:
// The seed() scoped variable.
scoped_ptr<Variable<uint64_t>> var_seed_;
diff --git a/policy_manager/real_shill_provider.cc b/policy_manager/real_shill_provider.cc
index ffd851c..4fbb6b2 100644
--- a/policy_manager/real_shill_provider.cc
+++ b/policy_manager/real_shill_provider.cc
@@ -63,7 +63,7 @@
return ConnectionTethering::kUnknown;
}
-bool RealShillProvider::DoInit() {
+bool RealShillProvider::Init() {
// Obtain a DBus connection.
GError* error = NULL;
connection_ = dbus_->BusGet(DBUS_BUS_SYSTEM, &error);
diff --git a/policy_manager/real_shill_provider.h b/policy_manager/real_shill_provider.h
index 9c15700..5af3232 100644
--- a/policy_manager/real_shill_provider.h
+++ b/policy_manager/real_shill_provider.h
@@ -31,6 +31,9 @@
virtual ~RealShillProvider();
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+
virtual Variable<bool>* var_is_connected() override {
return &var_is_connected_;
}
@@ -53,8 +56,6 @@
const char* tethering_str);
private:
- virtual bool DoInit() override;
-
// Return a DBus proxy for a given |path| and |interface| within shill.
DBusGProxy* GetProxy(const char* path, const char* interface);
diff --git a/policy_manager/real_system_provider.cc b/policy_manager/real_system_provider.cc
index ade9b6b..6b19268 100644
--- a/policy_manager/real_system_provider.cc
+++ b/policy_manager/real_system_provider.cc
@@ -18,7 +18,7 @@
namespace chromeos_policy_manager {
-bool RealSystemProvider::DoInit() {
+bool RealSystemProvider::Init() {
var_is_normal_boot_mode_.reset(
new ConstCopyVariable<bool>("is_normal_boot_mode",
VbGetSystemPropertyInt("devsw_boot") != 0));
diff --git a/policy_manager/real_system_provider.h b/policy_manager/real_system_provider.h
index 2de07a1..324a360 100644
--- a/policy_manager/real_system_provider.h
+++ b/policy_manager/real_system_provider.h
@@ -16,6 +16,9 @@
public:
RealSystemProvider() {}
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+
virtual Variable<bool>* var_is_normal_boot_mode() override {
return var_is_normal_boot_mode_.get();
}
@@ -25,8 +28,6 @@
}
private:
- virtual bool DoInit() override;
-
scoped_ptr<Variable<bool>> var_is_normal_boot_mode_;
scoped_ptr<Variable<bool>> var_is_official_build_;
diff --git a/policy_manager/real_time_provider.cc b/policy_manager/real_time_provider.cc
index b2ddd2f..1693c73 100644
--- a/policy_manager/real_time_provider.cc
+++ b/policy_manager/real_time_provider.cc
@@ -60,7 +60,7 @@
DISALLOW_COPY_AND_ASSIGN(CurrHourVariable);
};
-bool RealTimeProvider::DoInit() {
+bool RealTimeProvider::Init() {
var_curr_date_.reset(new CurrDateVariable("curr_date", clock_));
var_curr_hour_.reset(new CurrHourVariable("curr_hour", clock_));
return true;
diff --git a/policy_manager/real_time_provider.h b/policy_manager/real_time_provider.h
index 69b0fe9..84cbf1d 100644
--- a/policy_manager/real_time_provider.h
+++ b/policy_manager/real_time_provider.h
@@ -18,6 +18,9 @@
RealTimeProvider(chromeos_update_engine::ClockInterface* clock)
: clock_(clock) {}
+ // Initializes the provider and returns whether it succeeded.
+ bool Init();
+
virtual Variable<base::Time>* var_curr_date() override {
return var_curr_date_.get();
}
@@ -27,8 +30,6 @@
}
private:
- virtual bool DoInit() override;
-
// A clock abstraction (fakeable).
chromeos_update_engine::ClockInterface* const clock_;
diff --git a/policy_manager/real_updater_provider.h b/policy_manager/real_updater_provider.h
index 7ff8e69..adba799 100644
--- a/policy_manager/real_updater_provider.h
+++ b/policy_manager/real_updater_provider.h
@@ -22,6 +22,9 @@
explicit RealUpdaterProvider(
chromeos_update_engine::SystemState* system_state);
+ // Initializes the provider and returns whether it succeeded.
+ bool Init() { return true; }
+
virtual Variable<base::Time>* var_last_checked_time() override {
return var_last_checked_time_.get();
}
@@ -63,8 +66,6 @@
}
private:
- virtual bool DoInit() { return true; }
-
// A pointer to the update engine's system state aggregator.
chromeos_update_engine::SystemState* system_state_;