PM: Move the getters/variables on provider base classes to abstract methods.
Also make all DoInit() overrides in subclasses private, not protected
and ensure that all the abstract Provider interface classes has
virtual destructors.
Additionally, remove inlining of virtual methods in derived classes
since it won't matter much in our application. This is because we'll
call these methods only indirectly e.g. through a vtable dispatch via
a method call on the interface class.
BUG=chromium:364763
TEST=Unit tests pass.
Change-Id: I7f6a10d9c0b01c4f5c035125755ef132738e72aa
Reviewed-on: https://chromium-review.googlesource.com/196656
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
diff --git a/policy_manager/fake_device_policy_provider.h b/policy_manager/fake_device_policy_provider.h
index 4863ae5..252a5e5 100644
--- a/policy_manager/fake_device_policy_provider.h
+++ b/policy_manager/fake_device_policy_provider.h
@@ -60,9 +60,7 @@
}
private:
- virtual bool DoInit() override {
- return true;
- }
+ virtual bool DoInit() override { return true; }
FakeVariable<bool> var_device_policy_is_loaded_{
"policy_is_loaded", kVariableModePoll};
diff --git a/policy_manager/fake_random_provider.h b/policy_manager/fake_random_provider.h
index 8d98689..64f08a4 100644
--- a/policy_manager/fake_random_provider.h
+++ b/policy_manager/fake_random_provider.h
@@ -15,13 +15,13 @@
public:
FakeRandomProvider() {}
- protected:
- virtual bool DoInit() {
- set_var_seed(new FakeVariable<uint64_t>("seed", kVariableModeConst));
- return true;
- }
+ 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 d15668d..c18b0d2 100644
--- a/policy_manager/fake_shill_provider.h
+++ b/policy_manager/fake_shill_provider.h
@@ -15,27 +15,26 @@
public:
FakeShillProvider() {}
- virtual inline FakeVariable<bool>* var_is_connected() override {
+ virtual FakeVariable<bool>* var_is_connected() override {
return &var_is_connected_;
}
- virtual inline FakeVariable<ConnectionType>* var_conn_type() override {
+ virtual FakeVariable<ConnectionType>* var_conn_type() override {
return &var_conn_type_;
}
- virtual inline FakeVariable<ConnectionTethering>*
+ virtual FakeVariable<ConnectionTethering>*
var_conn_tethering() override {
return &var_conn_tethering_;
}
- virtual inline FakeVariable<base::Time>* var_conn_last_changed() override {
+ virtual FakeVariable<base::Time>* var_conn_last_changed() override {
return &var_conn_last_changed_;
}
- protected:
- virtual bool DoInit() { return true; }
-
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_system_provider.h b/policy_manager/fake_system_provider.h
index 8e6b92a..fbdff49 100644
--- a/policy_manager/fake_system_provider.h
+++ b/policy_manager/fake_system_provider.h
@@ -15,16 +15,22 @@
public:
FakeSystemProvider() {}
- protected:
- virtual bool DoInit() {
- set_var_is_normal_boot_mode(
- new FakeVariable<bool>("is_normal_boot_mode", kVariableModeConst));
- set_var_is_official_build(
- new FakeVariable<bool>("is_official_build", kVariableModeConst));
- return true;
+ virtual FakeVariable<bool>* var_is_normal_boot_mode() override {
+ return &var_is_normal_boot_mode_;
+ }
+
+ virtual FakeVariable<bool>* var_is_official_build() override {
+ return &var_is_official_build_;
}
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_{
+ "is_official_build", kVariableModeConst};
+
DISALLOW_COPY_AND_ASSIGN(FakeSystemProvider);
};
diff --git a/policy_manager/fake_time_provider.h b/policy_manager/fake_time_provider.h
index 6797f5c..a73346a 100644
--- a/policy_manager/fake_time_provider.h
+++ b/policy_manager/fake_time_provider.h
@@ -15,16 +15,20 @@
public:
FakeTimeProvider() {}
- protected:
- virtual bool DoInit() {
- set_var_curr_date(
- new FakeVariable<base::Time>("curr_date", kVariableModePoll));
- set_var_curr_hour(
- new FakeVariable<int>("curr_hour", kVariableModePoll));
- return true;
+ virtual FakeVariable<base::Time>* var_curr_date() override {
+ return &var_curr_date_;
+ }
+
+ virtual FakeVariable<int>* var_curr_hour() override {
+ return &var_curr_hour_;
}
private:
+ virtual bool DoInit() override { return true; }
+
+ FakeVariable<base::Time> var_curr_date_{"curr_date", kVariableModePoll};
+ FakeVariable<int> var_curr_hour_{"curr_hour", kVariableModePoll};
+
DISALLOW_COPY_AND_ASSIGN(FakeTimeProvider);
};
diff --git a/policy_manager/fake_updater_provider.h b/policy_manager/fake_updater_provider.h
index d208574..aa12ce7 100644
--- a/policy_manager/fake_updater_provider.h
+++ b/policy_manager/fake_updater_provider.h
@@ -55,10 +55,9 @@
return &var_cellular_enabled_;
}
- protected:
+ private:
virtual bool DoInit() { return true; }
- private:
FakeVariable<base::Time> var_last_checked_time_{
"last_checked_time", kVariableModePoll};
FakeVariable<base::Time> var_update_completed_time_{
diff --git a/policy_manager/random_provider.h b/policy_manager/random_provider.h
index b184cf5..a5d76f1 100644
--- a/policy_manager/random_provider.h
+++ b/policy_manager/random_provider.h
@@ -15,23 +15,18 @@
// Provider of random values.
class RandomProvider : public Provider {
public:
+ virtual ~RandomProvider() {}
+
// Return a random number every time it is requested. Note that values
// returned by the variables are cached by the EvaluationContext, so the
// returned value will be the same during the same policy request. If more
// random values are needed use a PRNG seeded with this value.
- Variable<uint64_t>* var_seed() const { return var_seed_.get(); }
+ virtual Variable<uint64_t>* var_seed() = 0;
protected:
RandomProvider() {}
- void set_var_seed(Variable<uint64_t>* var_seed) {
- var_seed_.reset(var_seed);
- }
-
private:
- // The seed() scoped variable.
- scoped_ptr<Variable<uint64_t>> var_seed_;
-
DISALLOW_COPY_AND_ASSIGN(RandomProvider);
};
diff --git a/policy_manager/real_random_provider.cc b/policy_manager/real_random_provider.cc
index 4c8ae85..3ab4ff4 100644
--- a/policy_manager/real_random_provider.cc
+++ b/policy_manager/real_random_provider.cc
@@ -67,7 +67,7 @@
FILE* fp = fopen(kRandomDevice, "r");
if (!fp)
return false;
- set_var_seed(new RandomSeedVariable("seed", fp));
+ var_seed_.reset(new RandomSeedVariable("seed", fp));
return true;
}
diff --git a/policy_manager/real_random_provider.h b/policy_manager/real_random_provider.h
index 75c8f6e..7dd8fab 100644
--- a/policy_manager/real_random_provider.h
+++ b/policy_manager/real_random_provider.h
@@ -14,11 +14,14 @@
public:
RealRandomProvider() {}
- protected:
- // Initializes all the variables.
- virtual bool DoInit(void);
+ virtual Variable<uint64_t>* var_seed() override { return var_seed_.get(); }
private:
+ virtual bool DoInit(void) override;
+
+ // The seed() scoped variable.
+ scoped_ptr<Variable<uint64_t>> var_seed_;
+
DISALLOW_COPY_AND_ASSIGN(RealRandomProvider);
};
diff --git a/policy_manager/real_shill_provider.h b/policy_manager/real_shill_provider.h
index cd5ee06..9c15700 100644
--- a/policy_manager/real_shill_provider.h
+++ b/policy_manager/real_shill_provider.h
@@ -31,19 +31,19 @@
virtual ~RealShillProvider();
- virtual inline Variable<bool>* var_is_connected() override {
+ virtual Variable<bool>* var_is_connected() override {
return &var_is_connected_;
}
- virtual inline Variable<ConnectionType>* var_conn_type() override {
+ virtual Variable<ConnectionType>* var_conn_type() override {
return &var_conn_type_;
}
- virtual inline Variable<ConnectionTethering>* var_conn_tethering() override {
+ virtual Variable<ConnectionTethering>* var_conn_tethering() override {
return &var_conn_tethering_;
}
- virtual inline Variable<base::Time>* var_conn_last_changed() override {
+ virtual Variable<base::Time>* var_conn_last_changed() override {
return &var_conn_last_changed_;
}
@@ -52,10 +52,9 @@
static ConnectionTethering ParseConnectionTethering(
const char* tethering_str);
- protected:
+ private:
virtual bool DoInit() override;
- private:
// 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 72669ff..ade9b6b 100644
--- a/policy_manager/real_system_provider.cc
+++ b/policy_manager/real_system_provider.cc
@@ -19,11 +19,11 @@
namespace chromeos_policy_manager {
bool RealSystemProvider::DoInit() {
- set_var_is_normal_boot_mode(
+ var_is_normal_boot_mode_.reset(
new ConstCopyVariable<bool>("is_normal_boot_mode",
VbGetSystemPropertyInt("devsw_boot") != 0));
- set_var_is_official_build(
+ var_is_official_build_.reset(
new ConstCopyVariable<bool>("var_is_official_build",
VbGetSystemPropertyInt("debug_build") == 0));
diff --git a/policy_manager/real_system_provider.h b/policy_manager/real_system_provider.h
index d5534cb..2de07a1 100644
--- a/policy_manager/real_system_provider.h
+++ b/policy_manager/real_system_provider.h
@@ -16,9 +16,20 @@
public:
RealSystemProvider() {}
+ virtual Variable<bool>* var_is_normal_boot_mode() override {
+ return var_is_normal_boot_mode_.get();
+ }
+
+ virtual Variable<bool>* var_is_official_build() override {
+ return var_is_official_build_.get();
+ }
+
private:
virtual bool DoInit() override;
+ scoped_ptr<Variable<bool>> var_is_normal_boot_mode_;
+ scoped_ptr<Variable<bool>> var_is_official_build_;
+
DISALLOW_COPY_AND_ASSIGN(RealSystemProvider);
};
diff --git a/policy_manager/real_time_provider.cc b/policy_manager/real_time_provider.cc
index d66dd6f..b2ddd2f 100644
--- a/policy_manager/real_time_provider.cc
+++ b/policy_manager/real_time_provider.cc
@@ -61,8 +61,8 @@
};
bool RealTimeProvider::DoInit() {
- set_var_curr_date(new CurrDateVariable("curr_date", clock_));
- set_var_curr_hour(new CurrHourVariable("curr_hour", clock_));
+ 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 49d8896..69b0fe9 100644
--- a/policy_manager/real_time_provider.h
+++ b/policy_manager/real_time_provider.h
@@ -18,13 +18,23 @@
RealTimeProvider(chromeos_update_engine::ClockInterface* clock)
: clock_(clock) {}
- protected:
- virtual bool DoInit();
+ virtual Variable<base::Time>* var_curr_date() override {
+ return var_curr_date_.get();
+ }
+
+ virtual Variable<int>* var_curr_hour() override {
+ return var_curr_hour_.get();
+ }
private:
- // A clock abstraction (mockable).
+ virtual bool DoInit() override;
+
+ // A clock abstraction (fakeable).
chromeos_update_engine::ClockInterface* const clock_;
+ scoped_ptr<Variable<base::Time>> var_curr_date_;
+ scoped_ptr<Variable<int>> var_curr_hour_;
+
DISALLOW_COPY_AND_ASSIGN(RealTimeProvider);
};
diff --git a/policy_manager/real_updater_provider.h b/policy_manager/real_updater_provider.h
index 1b24e16..7ff8e69 100644
--- a/policy_manager/real_updater_provider.h
+++ b/policy_manager/real_updater_provider.h
@@ -62,10 +62,9 @@
return var_cellular_enabled_.get();
}
- protected:
+ private:
virtual bool DoInit() { return true; }
- private:
// A pointer to the update engine's system state aggregator.
chromeos_update_engine::SystemState* system_state_;
diff --git a/policy_manager/shill_provider.h b/policy_manager/shill_provider.h
index b53266f..967faae 100644
--- a/policy_manager/shill_provider.h
+++ b/policy_manager/shill_provider.h
@@ -32,6 +32,8 @@
// Provider for networking related information.
class ShillProvider : public Provider {
public:
+ virtual ~ShillProvider() {}
+
// A variable returning whether we currently have network connectivity.
virtual Variable<bool>* var_is_connected() = 0;
@@ -49,6 +51,9 @@
protected:
ShillProvider() {}
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ShillProvider);
};
} // namespace chromeos_policy_manager
diff --git a/policy_manager/system_provider.h b/policy_manager/system_provider.h
index b7f93c0..d319b8c 100644
--- a/policy_manager/system_provider.h
+++ b/policy_manager/system_provider.h
@@ -16,33 +16,20 @@
// reported by crossystem, the kernel boot command line and the partition table.
class SystemProvider : public Provider {
public:
+ virtual ~SystemProvider() {}
+
// Returns true if the boot mode is normal or if it's unable to
// determine the boot mode. Returns false if the boot mode is
// developer.
- Variable<bool>* var_is_normal_boot_mode() const {
- return var_is_normal_boot_mode_.get();
- }
+ virtual Variable<bool>* var_is_normal_boot_mode() = 0;
// Returns whether this is an official Chrome OS build.
- Variable<bool>* var_is_official_build() const {
- return var_is_official_build_.get();
- }
+ virtual Variable<bool>* var_is_official_build() = 0;
protected:
SystemProvider() {}
- void set_var_is_normal_boot_mode(Variable<bool>* var_is_normal_boot_mode) {
- var_is_normal_boot_mode_.reset(var_is_normal_boot_mode);
- }
-
- void set_var_is_official_build(Variable<bool>* var_is_official_build) {
- var_is_official_build_.reset(var_is_official_build);
- }
-
private:
- scoped_ptr<Variable<bool>> var_is_normal_boot_mode_;
- scoped_ptr<Variable<bool>> var_is_official_build_;
-
DISALLOW_COPY_AND_ASSIGN(SystemProvider);
};
diff --git a/policy_manager/time_provider.h b/policy_manager/time_provider.h
index 7b8bbd9..8349c01 100644
--- a/policy_manager/time_provider.h
+++ b/policy_manager/time_provider.h
@@ -16,16 +16,14 @@
// Provider for time related information.
class TimeProvider : public Provider {
public:
+ virtual ~TimeProvider() {}
+
// Returns the current date. The time of day component will be zero.
- Variable<base::Time>* var_curr_date() const {
- return var_curr_date_.get();
- }
+ virtual Variable<base::Time>* var_curr_date() = 0;
// Returns the current hour (0 to 23) in local time. The type is int to keep
// consistent with base::Time.
- Variable<int>* var_curr_hour() const {
- return var_curr_hour_.get();
- }
+ virtual Variable<int>* var_curr_hour() = 0;
// TODO(garnold) Implement a method/variable for querying whether a given
// point in time was reached.
@@ -33,18 +31,7 @@
protected:
TimeProvider() {}
- void set_var_curr_date(Variable<base::Time>* var_curr_date) {
- var_curr_date_.reset(var_curr_date);
- }
-
- void set_var_curr_hour(Variable<int>* var_curr_hour) {
- var_curr_hour_.reset(var_curr_hour);
- }
-
private:
- scoped_ptr<Variable<base::Time>> var_curr_date_;
- scoped_ptr<Variable<int>> var_curr_hour_;
-
DISALLOW_COPY_AND_ASSIGN(TimeProvider);
};
diff --git a/policy_manager/updater_provider.h b/policy_manager/updater_provider.h
index fa2382f..75f1fb8 100644
--- a/policy_manager/updater_provider.h
+++ b/policy_manager/updater_provider.h
@@ -30,6 +30,8 @@
// Provider for Chrome OS update related information.
class UpdaterProvider : public Provider {
public:
+ virtual ~UpdaterProvider() {}
+
// A variable returning the last update check time.
virtual Variable<base::Time>* var_last_checked_time() = 0;
@@ -68,6 +70,12 @@
// A variable indicating whether updates are allowed over a cellular network.
virtual Variable<bool>* var_cellular_enabled() = 0;
+
+ protected:
+ UpdaterProvider() {}
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(UpdaterProvider);
};
} // namespace chromeos_policy_manager