Remove ScopedVector from base::JSONValueConverter

chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc

 // Copyright 2013 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 "chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/sync_file_system/drive_backend/drive_backend_constants.h"
 #include "chrome/browser/sync_file_system/drive_backend/drive_backend_test_util.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.pb.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_engine_context.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task_manager.h"
 #include "chrome/browser/sync_file_system/sync_file_system_test_util.h"
 #include "components/drive/drive_api_util.h"
@@ skipping 83 matching lines @@
   SyncStatusCode PopulateDatabase(
       const google_apis::FileResource& sync_root,
       const google_apis::FileResource** app_roots,
       size_t app_roots_count) {
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
     std::unique_ptr<MetadataDatabase> database = MetadataDatabase::Create(
         database_path(), in_memory_env_.get(), &status);
     if (status != SYNC_STATUS_OK)
       return status;
 
-    // |app_root_list| must not own the resources here. Be sure to call
-    // weak_clear later.
-    ScopedVector<google_apis::FileResource> app_root_list;
+    // |app_root_list| must not own the resources here. Be sure to release
+    // ownership of all elements later.

[Avi (use Gerrit), 2017/01/09 17:18:29]

This code is scaring me, because we should be enforcing ownership in the code,
and not in a comment. Can we rewrite this code so that ownership is handled in
the code?

+    std::vector<std::unique_ptr<google_apis::FileResource>> app_root_list;
     for (size_t i = 0; i < app_roots_count; ++i) {
-      app_root_list.push_back(
-          const_cast<google_apis::FileResource*>(app_roots[i]));
+      app_root_list.push_back(base::WrapUnique(
+          const_cast<google_apis::FileResource*>(app_roots[i])));
     }
 
     status = database->PopulateInitialData(
         kInitialLargestChangeID, sync_root, app_root_list);
 
-    app_root_list.weak_clear();
+    for_each(app_root_list.begin(), app_root_list.end(),
+             [](std::unique_ptr<google_apis::FileResource>& entry) {
+               entry.release();
+             });

[Avi (use Gerrit), 2017/01/09 17:18:29]

Yikes, I see what's going on here. We're temporarily making a double-ownership
of all the entries because of the API of PopulateInitialData. We surely must be
able to do something more clever with PopulateInitialData to avoid having to do
this.

[leonhsl(Using Gerrit), 2017/01/10 04:39:36]

Yeah I understand the concern and I also felt a little weird when I changed
here. Could I solve this concern with a follow-up CL? I'd like to make clear of
all around PopulateInitialData and revise all related codes within a single CL.

[Avi (use Gerrit), 2017/01/10 04:48:40]

:\

Can you at least put a TODO(you) there to make it clear? Please do follow up;
having two owners for a pointer is a dangerous pattern that we don't want to
keep around.

[leonhsl(Using Gerrit), 2017/01/10 05:00:39]

Done and thank you. I'll start this work soon.

+    app_root_list.clear();
     return status;
   }
 
   std::unique_ptr<google_apis::FileResource> CreateRemoteFolder(
       const std::string& parent_folder_id,
       const std::string& title) {
     google_apis::DriveApiErrorCode error = google_apis::DRIVE_OTHER_ERROR;
     std::unique_ptr<google_apis::FileResource> entry;
     drive::AddNewDirectoryOptions options;
     options.visibility = google_apis::drive::FILE_VISIBILITY_PRIVATE;
@@ skipping 218 matching lines @@
 
   EXPECT_EQ(0u, CountTrackersForFile(sync_root->file_id()));
   EXPECT_FALSE(HasNoParent(sync_root->file_id()));
 
   EXPECT_EQ(1u, CountFileMetadata());
   EXPECT_EQ(1u, CountFileTracker());
 }
 
 }  // namespace drive_backend
 }  // namespace sync_file_system