SafeBrowsing PVer4: Send mutable response to the database and the stores

components/safe_browsing_db/v4_database_unittest.cc

 // Copyright 2016 The Chromium 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 "base/bind.h"
 #include "base/debug/leak_annotations.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/test/test_simple_task_runner.h"
@@ skipping 110 matching lines @@
     }
 
     EXPECT_EQ(expected_resets_successfully_, v4_database->ResetDatabase());
 
     EXPECT_FALSE(created_and_called_back_);
     created_and_called_back_ = true;
 
     v4_database_ = std::move(v4_database);
   }
 
-  void PopulateFakeUpdateResponse(StoreStateMap store_state_map,
-                                  std::vector<ListUpdateResponse>* responses) {
+  std::unique_ptr<ParsedServerResponse> CreateFakeServerResponse(
+      StoreStateMap store_state_map) {
+    std::unique_ptr<ParsedServerResponse> parsed_server_response(
+        new ParsedServerResponse);

[Scott Hess - ex-Googler, 2016/06/28 03:58:43]

Is this your formatting or auto-formatting?  Once the line is broken, I
generally prefer to format like
  std::unique_ptr<ParsedServerResponse>
    parsed_server_response(new ParsedServerResponse);

[Nathan Parker, 2016/06/28 19:00:38]

I'd say if the auto-formatting is non-optimal, we should still use it but then
file a bug to fix it.  Otherwise, we'll run into this everytime someone hits
auto-format.

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

I wouldn't call it a bug. It's just something that some of us dislike slightly
more than the alternative. At best, we could consider starting a discussion
about it.

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

$ git cl format

[Scott Hess - ex-Googler, 2016/06/28 22:22:25]

Acknowledged.  All hail our formatting overlords.

     for (const auto& store_state_iter : store_state_map) {
       UpdateListIdentifier identifier = store_state_iter.first;
-      ListUpdateResponse list_update_response;
-      list_update_response.set_platform_type(identifier.platform_type);
-      list_update_response.set_threat_entry_type(identifier.threat_entry_type);
-      list_update_response.set_threat_type(identifier.threat_type);
-      list_update_response.set_new_client_state(store_state_iter.second);
-      responses->push_back(list_update_response);
+      ListUpdateResponse* list_update_response = new ListUpdateResponse;
+      list_update_response->set_platform_type(identifier.platform_type);
+      list_update_response->set_threat_entry_type(identifier.threat_entry_type);
+      list_update_response->set_threat_type(identifier.threat_type);
+      list_update_response->set_new_client_state(store_state_iter.second);
+      parsed_server_response->push_back(base::WrapUnique(list_update_response));
     }
+    return parsed_server_response;
   }
 
   void VerifyExpectedStoresState(bool expect_new_stores) {
     const StoreMap* new_store_map = v4_database_->store_map_.get();
     std::unique_ptr<StoreStateMap> new_store_state_map =
         v4_database_->GetStoreStateMap();
     EXPECT_EQ(expected_store_state_map_.size(), new_store_map->size());
     EXPECT_EQ(expected_store_state_map_.size(), new_store_state_map->size());
     for (const auto& expected_iter : expected_store_state_map_) {
       const UpdateListIdentifier& identifier = expected_iter.first;
@@ skipping 77 matching lines @@
   // The database has now been created. Time to try to update it.
   EXPECT_TRUE(v4_database_);
   const StoreMap* db_stores = v4_database_->store_map_.get();
   EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
   for (const auto& store_iter : *db_stores) {
     V4Store* store = store_iter.second.get();
     expected_store_state_map_[store_iter.first] = store->state() + "_fake";
     old_stores_map_[store_iter.first] = store;
   }
 
-  std::vector<ListUpdateResponse> update_response;
-  PopulateFakeUpdateResponse(expected_store_state_map_, &update_response);
-  v4_database_->ApplyUpdate(update_response, callback_db_updated_);
+  std::unique_ptr<ParsedServerResponse> parsed_server_response =
+      CreateFakeServerResponse(expected_store_state_map_);
+  v4_database_->ApplyUpdate(std::move(parsed_server_response),
+                            callback_db_updated_);

[Scott Hess - ex-Googler, 2016/06/28 03:58:43]

Would the following work as:
  v4_database_->ApplyUpdate(
      CreateFakeServerResponse(expected_store_state_map_),
      callback_db_updated_);
???  Right now it feels like it's creating a long-winded temporary for no
particular purpose.

Otherwise, I see no reason not to use |response|.  There aren't other responses
here, so recapitulating the entire type doesn't make it clearer.

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

Done.

 
   task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
 
   VerifyExpectedStoresState(true);
 }
 
 // Test to ensure no state updates leads to no store updates.
 TEST_F(SafeBrowsingV4DatabaseTest, TestApplyUpdateWithNoNewState) {
   expected_resets_successfully_ = true;
   RegisterFactory(!expected_resets_successfully_);
 
   V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
                      callback_db_ready_);
   created_but_not_called_back_ = true;
   task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
 
   // The database has now been created. Time to try to update it.
   EXPECT_TRUE(v4_database_);
   const StoreMap* db_stores = v4_database_->store_map_.get();
   EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
   for (const auto& store_iter : *db_stores) {
     V4Store* store = store_iter.second.get();
     expected_store_state_map_[store_iter.first] = store->state();
     old_stores_map_[store_iter.first] = store;
   }
 
-  std::vector<ListUpdateResponse> update_response;
-  PopulateFakeUpdateResponse(expected_store_state_map_, &update_response);
-  v4_database_->ApplyUpdate(update_response, callback_db_updated_);
+  std::unique_ptr<ParsedServerResponse> parsed_server_response =
+      CreateFakeServerResponse(expected_store_state_map_);
+  v4_database_->ApplyUpdate(std::move(parsed_server_response),
+                            callback_db_updated_);
 
   task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
 
   VerifyExpectedStoresState(false);
 }
 
 // Test to ensure no updates leads to no store updates.
 TEST_F(SafeBrowsingV4DatabaseTest, TestApplyUpdateWithEmptyUpdate) {
   expected_resets_successfully_ = true;
   RegisterFactory(!expected_resets_successfully_);
 
   V4Database::Create(task_runner_, database_dirname_, store_file_name_map_,
                      callback_db_ready_);
   created_but_not_called_back_ = true;
   task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
 
   // The database has now been created. Time to try to update it.
   EXPECT_TRUE(v4_database_);
   const StoreMap* db_stores = v4_database_->store_map_.get();
   EXPECT_EQ(expected_store_paths_.size(), db_stores->size());
   for (const auto& store_iter : *db_stores) {
     V4Store* store = store_iter.second.get();
     expected_store_state_map_[store_iter.first] = store->state();
     old_stores_map_[store_iter.first] = store;
   }
 
-  std::vector<ListUpdateResponse> update_response;
-  v4_database_->ApplyUpdate(update_response, callback_db_updated_);
+  std::unique_ptr<ParsedServerResponse> parsed_server_response(
+      new ParsedServerResponse);
+  v4_database_->ApplyUpdate(std::move(parsed_server_response),
+                            callback_db_updated_);
 
   task_runner_->RunPendingTasks();
   base::RunLoop().RunUntilIdle();
 
   VerifyExpectedStoresState(false);
 }
 
 }  // namespace safe_browsing