Remove ScopedVector from chrome/browser/.

chrome/browser/sync/test/integration/sync_test.cc

 // Copyright (c) 2012 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/test/integration/sync_test.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 450 matching lines @@
   profiles_.push_back(profile);
 
   return browsers_[browsers_.size() - 1];
 }
 
 ProfileSyncServiceHarness* SyncTest::GetClient(int index) {
   if (clients_.empty())
     LOG(FATAL) << "SetupClients() has not yet been called.";
   if (index < 0 || index >= static_cast<int>(clients_.size()))
     LOG(FATAL) << "GetClient(): Index is out of bounds.";
-  return clients_[index];
+  return clients_[index].get();
+}
+
+std::vector<ProfileSyncServiceHarness*> SyncTest::GetSyncClients() {
+  std::vector<ProfileSyncServiceHarness*> clients(clients_.size());
+  for (size_t i = 0; i < clients_.size(); ++i)
+    clients[i] = clients_[i].get();
+  return clients;
 }
 
 ProfileSyncService* SyncTest::GetSyncService(int index) {
   return ProfileSyncServiceFactory::GetForProfile(GetProfile(index));
 }
 
 std::vector<ProfileSyncService*> SyncTest::GetSyncServices() {
   std::vector<ProfileSyncService*> services;
   for (int i = 0; i < num_clients(); ++i) {
     services.push_back(GetSyncService(i));
@@ skipping 96 matching lines @@
     profile_sync_service->OverrideNetworkResourcesForTest(
         base::WrapUnique<syncer::NetworkResources>(
             new fake_server::FakeServerNetworkResources(
                 fake_server_->AsWeakPtr())));
   }
 
   ProfileSyncServiceHarness::SigninType singin_type = UsingExternalServers()
           ? ProfileSyncServiceHarness::SigninType::UI_SIGNIN
           : ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN;
 
+  clients_[index].release();

[Avi (use Gerrit), 2017/03/23 02:44:48]

Is this right? I know that this preserves the code's behavior, but making it
explicit like this exposes the wat.

[leonhsl(Using Gerrit), 2017/03/23 15:03:17]

Yeah I also felt a little weak here.. After checking around I think we can use a
DCHECK(!clients_[index]) here, original code is not intentionally releasing
ownership of a non-null pointer. WDYT? Thanks.

Also changed to use DCHECK for below 2X.

[skym, 2017/03/23 19:28:05]

I agree that the intention is that each index is written to exactly once.

I don't quite follow why the old code was leaking, was this a quirk of
ScopedVector? I definitely don't think we want to leak. If there was for some
reason already an entry at |index|, I think it should be destructed when
overriden. In this new std::vector<std::unique_ptr<X>> world, will assigning to
|index| correctly destruct the existing object? I don't really think adding a
DCHECK above this really adds anything.

[Avi (use Gerrit), 2017/03/23 19:36:33]

Yes. If you assigned through operator[] of ScopedVector it would not delete the
old value. If this is not intentional, and the old code was *accidentally*
leaking, we can fix it.

We just wanted to check to see if for some reason it was intentional.
Yes. If that's the desired behavior (and it seems like it was the behavior you
were expecting and not getting) we can switch to it.

[Nicolas Zea, 2017/03/23 20:06:47]

Agreed with Sky. I'm fairly sure this wasn't intentionally leaking, and was just
an oversight.

   clients_[index] =
       ProfileSyncServiceHarness::Create(GetProfile(index),
                                         username_,
                                         password_,
                                         singin_type);
   EXPECT_NE(nullptr, GetClient(index)) << "Could not create Client " << index;
   InitializeInvalidations(index);
 }
 
 void SyncTest::InitializeInvalidations(int index) {
@@ skipping 24 matching lines @@
             static_cast<invalidation::ProfileInvalidationProvider*>(
                 invalidation::ProfileInvalidationProviderFactory::
                     GetInstance()->SetTestingFactoryAndUse(
                         GetProfile(index),
                         TestUsesSelfNotifications() ?
                             BuildSelfNotifyingP2PProfileInvalidationProvider :
                             BuildRealisticP2PProfileInvalidationProvider))->
                                 GetInvalidationService());
     p2p_invalidation_service->UpdateCredentials(username_, password_);
     // Start listening for and emitting notifications of commits.
+    invalidation_forwarders_[index].release();

[Avi (use Gerrit), 2017/03/23 02:44:48]

ditto

     invalidation_forwarders_[index] =
-        new P2PInvalidationForwarder(clients_[index]->service(),
-                                     p2p_invalidation_service);
+        base::MakeUnique<P2PInvalidationForwarder>(clients_[index]->service(),
+                                                   p2p_invalidation_service);
   }
 }
 
 bool SyncTest::SetupSync() {
   // Create sync profiles and clients if they haven't already been created.
   if (profiles_.empty()) {
     if (!SetupClients()) {
       LOG(FATAL) << "SetupClients() failed.";
       return false;
     }
@@ skipping 35 matching lines @@
 
   // SyncRefresher is used instead of invalidations to notify other profiles to
   // do a sync refresh on committed data sets. This is only needed when running
   // tests against external live server, otherwise invalidation service is used.
   // With external live servers, the profiles commit data on first sync cycle
   // automatically after signing in. To avoid misleading sync commit
   // notifications at start up, we start the SyncRefresher observers post
   // client set up.
   if (UsingExternalServers()) {
     for (int i = 0; i < num_clients_; ++i) {
-      sync_refreshers_[i] =
-          new P2PSyncRefresher(GetProfile(i), clients_[i]->service());
+      sync_refreshers_[i].release();

[Avi (use Gerrit), 2017/03/23 02:44:48]

ditto

+      sync_refreshers_[i] = base::MakeUnique<P2PSyncRefresher>(
+          GetProfile(i), clients_[i]->service());
     }
 
     // OneClickSigninSyncStarter observer is created with a real user sign in.
     // It is deleted on certain conditions which are not satisfied by our tests,
     // and this causes the SigninTracker observer to stay hanging at shutdown.
     // Calling LoginUIService::SyncConfirmationUIClosed forces the observer to
     // be removed. http://crbug.com/484388
     for (int i = 0; i < num_clients_; ++i) {
       LoginUIServiceFactory::GetForProfile(GetProfile(i))->
           SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
@@ skipping 375 matching lines @@
 bool SyncTest::IsEncryptionComplete(int index) {
   return ::IsEncryptionComplete(GetClient(index)->service());
 }
 
 bool SyncTest::AwaitEncryptionComplete(int index) {
   ProfileSyncService* service = GetClient(index)->service();
   return EncryptionChecker(service).Wait();
 }
 
 bool SyncTest::AwaitQuiescence() {
-  return ProfileSyncServiceHarness::AwaitQuiescence(clients());
+  return ProfileSyncServiceHarness::AwaitQuiescence(GetSyncClients());
 }
 
 bool SyncTest::UsingExternalServers() {
   return server_type_ == EXTERNAL_LIVE_SERVER;
 }
 
 bool SyncTest::ServerSupportsNotificationControl() const {
   EXPECT_NE(SERVER_TYPE_UNDECIDED, server_type_);
 
   // Supported only if we're using the python testserver.
@@ skipping 115 matching lines @@
 bool SyncTest::ClearServerData(ProfileSyncServiceHarness* harness) {
   // At this point our birthday is good.
   base::RunLoop run_loop;
   harness->service()->ClearServerDataForTest(run_loop.QuitClosure());
   run_loop.Run();
 
   // Our birthday is invalidated on the server here so restart sync to get
   // the new birthday from the server.
   return harness->RestartSyncService();
 }