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);