Change StateChangeQueue::NotifyPropertiesUpdated to take const ref
Instead of using std::unique_ptr<>. This simplifies code around the
usage patterns and tests, and causes only one extra copy of the
dictionary in one case.
Change-Id: Ic0d3158116a7be5823078b08f1bf58e1c88c88b7
Reviewed-on: https://weave-review.googlesource.com/1769
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/src/base_api_handler_unittest.cc b/src/base_api_handler_unittest.cc
index ded6315..5c6a8a5 100644
--- a/src/base_api_handler_unittest.cc
+++ b/src/base_api_handler_unittest.cc
@@ -31,7 +31,7 @@
class BaseApiHandlerTest : public ::testing::Test {
protected:
void SetUp() override {
- EXPECT_CALL(mock_state_change_queue_, MockNotifyPropertiesUpdated(_, _))
+ EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_, _))
.WillRepeatedly(Return(true));
command_manager_ = std::make_shared<CommandManager>();
diff --git a/src/states/mock_state_change_queue_interface.h b/src/states/mock_state_change_queue_interface.h
index 78c6c82..fc119cd 100644
--- a/src/states/mock_state_change_queue_interface.h
+++ b/src/states/mock_state_change_queue_interface.h
@@ -16,7 +16,7 @@
class MockStateChangeQueueInterface : public StateChangeQueueInterface {
public:
MOCK_CONST_METHOD0(IsEmpty, bool());
- MOCK_METHOD2(MockNotifyPropertiesUpdated,
+ MOCK_METHOD2(NotifyPropertiesUpdated,
bool(base::Time timestamp,
const base::DictionaryValue& changed_properties));
MOCK_METHOD0(MockGetAndClearRecordedStateChanges,
@@ -28,11 +28,6 @@
MOCK_METHOD1(NotifyStateUpdatedOnServer, void(UpdateID));
private:
- bool NotifyPropertiesUpdated(
- base::Time timestamp,
- std::unique_ptr<base::DictionaryValue> changed_properties) override {
- return MockNotifyPropertiesUpdated(timestamp, *changed_properties);
- }
std::vector<StateChange> GetAndClearRecordedStateChanges() override {
return std::move(MockGetAndClearRecordedStateChanges());
}
diff --git a/src/states/state_change_queue.cc b/src/states/state_change_queue.cc
index ba7bf65..b6c67cd 100644
--- a/src/states/state_change_queue.cc
+++ b/src/states/state_change_queue.cc
@@ -16,13 +16,13 @@
bool StateChangeQueue::NotifyPropertiesUpdated(
base::Time timestamp,
- std::unique_ptr<base::DictionaryValue> changed_properties) {
+ const base::DictionaryValue& changed_properties) {
auto& stored_changes = state_changes_[timestamp];
// Merge the old property set.
if (stored_changes)
- stored_changes->MergeDictionary(changed_properties.get());
+ stored_changes->MergeDictionary(&changed_properties);
else
- stored_changes = std::move(changed_properties);
+ stored_changes.reset(changed_properties.DeepCopy());
while (state_changes_.size() > max_queue_size_) {
// Queue is full.
diff --git a/src/states/state_change_queue.h b/src/states/state_change_queue.h
index 314f746..857ec8b 100644
--- a/src/states/state_change_queue.h
+++ b/src/states/state_change_queue.h
@@ -23,7 +23,7 @@
bool IsEmpty() const override { return state_changes_.empty(); }
bool NotifyPropertiesUpdated(
base::Time timestamp,
- std::unique_ptr<base::DictionaryValue> changed_properties) override;
+ const base::DictionaryValue& changed_properties) override;
std::vector<StateChange> GetAndClearRecordedStateChanges() override;
UpdateID GetLastStateChangeId() const override { return last_change_id_; }
Token AddOnStateUpdatedCallback(
diff --git a/src/states/state_change_queue_interface.h b/src/states/state_change_queue_interface.h
index 631a127..7ddce8c 100644
--- a/src/states/state_change_queue_interface.h
+++ b/src/states/state_change_queue_interface.h
@@ -42,7 +42,7 @@
// Called by StateManager when device state properties are updated.
virtual bool NotifyPropertiesUpdated(
base::Time timestamp,
- std::unique_ptr<base::DictionaryValue> changed_properties) = 0;
+ const base::DictionaryValue& changed_properties) = 0;
// Returns the recorded state changes since last time this method was called.
virtual std::vector<StateChange> GetAndClearRecordedStateChanges() = 0;
diff --git a/src/states/state_change_queue_unittest.cc b/src/states/state_change_queue_unittest.cc
index bb2947a..b639d37 100644
--- a/src/states/state_change_queue_unittest.cc
+++ b/src/states/state_change_queue_unittest.cc
@@ -31,7 +31,7 @@
TEST_F(StateChangeQueueTest, UpdateOne) {
auto timestamp = base::Time::Now();
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp, CreateDictionaryValue("{'prop': {'name': 23}}")));
+ timestamp, *CreateDictionaryValue("{'prop': {'name': 23}}")));
EXPECT_FALSE(queue_->IsEmpty());
EXPECT_EQ(1u, queue_->GetLastStateChangeId());
auto changes = queue_->GetAndClearRecordedStateChanges();
@@ -50,9 +50,9 @@
const std::string state2 =
"{'prop': {'name1': 17, 'name2': 1.0, 'name3': false}}";
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp1, CreateDictionaryValue(state1)));
+ timestamp1, *CreateDictionaryValue(state1)));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp2, CreateDictionaryValue(state2)));
+ timestamp2, *CreateDictionaryValue(state2)));
EXPECT_EQ(2u, queue_->GetLastStateChangeId());
EXPECT_FALSE(queue_->IsEmpty());
@@ -71,16 +71,17 @@
base::TimeDelta time_delta = base::TimeDelta::FromMinutes(1);
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp, CreateDictionaryValue("{'prop': {'name1': 1}}")));
+ timestamp, *CreateDictionaryValue("{'prop': {'name1': 1}}")));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp, CreateDictionaryValue("{'prop': {'name2': 2}}")));
+ timestamp, *CreateDictionaryValue("{'prop': {'name2': 2}}")));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp, CreateDictionaryValue("{'prop': {'name1': 3}}")));
+ timestamp, *CreateDictionaryValue("{'prop': {'name1': 3}}")));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- timestamp + time_delta, CreateDictionaryValue("{'prop': {'name1': 4}}")));
+ timestamp + time_delta,
+ *CreateDictionaryValue("{'prop': {'name1': 4}}")));
auto changes = queue_->GetAndClearRecordedStateChanges();
EXPECT_EQ(4u, queue_->GetLastStateChangeId());
@@ -101,15 +102,16 @@
base::TimeDelta time_delta2 = base::TimeDelta::FromMinutes(3);
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
- start_time, CreateDictionaryValue("{'prop': {'name1': 1, 'name2': 2}}")));
+ start_time,
+ *CreateDictionaryValue("{'prop': {'name1': 1, 'name2': 2}}")));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
start_time + time_delta1,
- CreateDictionaryValue("{'prop': {'name1': 3, 'name3': 4}}")));
+ *CreateDictionaryValue("{'prop': {'name1': 3, 'name3': 4}}")));
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
start_time + time_delta2,
- CreateDictionaryValue("{'prop': {'name10': 10, 'name11': 11}}")));
+ *CreateDictionaryValue("{'prop': {'name10': 10, 'name11': 11}}")));
EXPECT_EQ(3u, queue_->GetLastStateChangeId());
auto changes = queue_->GetAndClearRecordedStateChanges();
@@ -140,7 +142,7 @@
// When queue is not empty, registering a new callback will not trigger it.
ASSERT_TRUE(queue_->NotifyPropertiesUpdated(
base::Time::Now(),
- CreateDictionaryValue("{'prop': {'name1': 1, 'name3': 2}}")));
+ *CreateDictionaryValue("{'prop': {'name1': 1, 'name3': 2}}")));
auto callback = [](StateChangeQueueInterface::UpdateID id) {
FAIL() << "This should not be called";
diff --git a/src/states/state_manager.cc b/src/states/state_manager.cc
index b170b86..128f7d8 100644
--- a/src/states/state_manager.cc
+++ b/src/states/state_manager.cc
@@ -97,9 +97,9 @@
cached_dict_valid_ = false;
- std::unique_ptr<base::DictionaryValue> prop_set{new base::DictionaryValue};
- prop_set->Set(full_property_name, value.DeepCopy());
- state_change_queue_->NotifyPropertiesUpdated(timestamp, std::move(prop_set));
+ base::DictionaryValue prop_set;
+ prop_set.Set(full_property_name, value.DeepCopy());
+ state_change_queue_->NotifyPropertiesUpdated(timestamp, prop_set);
return true;
}
diff --git a/src/states/state_manager_unittest.cc b/src/states/state_manager_unittest.cc
index 6899646..918fb89 100644
--- a/src/states/state_manager_unittest.cc
+++ b/src/states/state_manager_unittest.cc
@@ -63,7 +63,7 @@
void SetUp() override {
// Initial expectations.
EXPECT_CALL(mock_state_change_queue_, IsEmpty()).Times(0);
- EXPECT_CALL(mock_state_change_queue_, MockNotifyPropertiesUpdated(_, _))
+ EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_, _))
.WillRepeatedly(Return(true));
EXPECT_CALL(mock_state_change_queue_, MockGetAndClearRecordedStateChanges())
.Times(0);
@@ -176,7 +176,7 @@
TEST_F(StateManagerTest, SetPropertyValue) {
const std::string state = "{'device': {'state_property': 'Test Value'}}";
EXPECT_CALL(mock_state_change_queue_,
- MockNotifyPropertiesUpdated(timestamp_, IsState(state)))
+ NotifyPropertiesUpdated(timestamp_, IsState(state)))
.WillOnce(Return(true));
ASSERT_TRUE(SetPropertyValue("device.state_property",
base::StringValue{"Test Value"}, nullptr));
@@ -222,7 +222,7 @@
TEST_F(StateManagerTest, GetAndClearRecordedStateChanges) {
EXPECT_CALL(mock_state_change_queue_,
- MockNotifyPropertiesUpdated(timestamp_, _))
+ NotifyPropertiesUpdated(timestamp_, _))
.WillOnce(Return(true));
ASSERT_TRUE(SetPropertyValue("device.state_property",
base::StringValue{"Test Value"}, nullptr));
@@ -245,7 +245,7 @@
TEST_F(StateManagerTest, SetProperties) {
const std::string state = "{'base': {'manufacturer': 'No Name'}}";
EXPECT_CALL(mock_state_change_queue_,
- MockNotifyPropertiesUpdated(_, IsState(state)))
+ NotifyPropertiesUpdated(_, IsState(state)))
.WillOnce(Return(true));
EXPECT_CALL(*this, OnStateChanged()).Times(1);
