GetLoginsRequest: Use ScopedVector to express ownership of forms

components/password_manager/core/browser/password_store.cc

 // Copyright 2014 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 "components/password_manager/core/browser/password_store.h"
 
 #include "base/bind.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 11 matching lines @@
 using autofill::PasswordForm;
 
 namespace password_manager {
 
 namespace {
 
 // Calls |consumer| back with the request result, if |consumer| is still alive.
 // Takes ownership of the elements in |result|, passing ownership to |consumer|
 // if it is still alive.
 void MaybeCallConsumerCallback(base::WeakPtr<PasswordStoreConsumer> consumer,
-                               scoped_ptr<std::vector<PasswordForm*>> result) {
-  if (consumer.get())
-    consumer->OnGetPasswordStoreResults(*result);
-  else
-    STLDeleteElements(result.get());
+                               ScopedVector<autofill::PasswordForm> result) {
+  if (consumer) {
+    consumer->results()->clear();

[vasilii, 2015/02/03 19:22:16]

Do you need it?

[vabr (Chromium), 2015/02/04 16:13:44]

I don't, but I left it to make it clear that the current results are dropped. On
the other hand, it's quite clear what is happening, so I'm dropping this line.
Done.

+    consumer->results()->swap(result);
+    consumer->OnGetPasswordStoreResults();
+  }
 }
 
 // http://crbug.com/404012. Let's see where the empty fields come from.
 void CheckForEmptyUsernameAndPassword(const PasswordForm& form) {
   if (form.username_value.empty() &&
       form.password_value.empty() &&
       !form.blacklisted_by_user)
     base::debug::DumpWithoutCrashing();
 }
 
 }  // namespace
 
 PasswordStore::GetLoginsRequest::GetLoginsRequest(
     PasswordStoreConsumer* consumer)
-    : consumer_weak_(consumer->GetWeakPtr()),
-      result_(new std::vector<PasswordForm*>()) {
+    : consumer_weak_(consumer->GetWeakPtr()) {
+  // TODO(vabr): As long as |thread_checker_| gets constructed without failures,
+  // this DCHECK is always trivially true. It looks like we could get rid of the
+  // |thread_checker_| unless it was meant to make sure that threads are
+  // actually present in relevant tests. But in that case, we should do better
+  // than to hog production code because of tests.
   DCHECK(thread_checker_.CalledOnValidThread());

[vasilii, 2015/02/03 19:22:17]

Looks like this is the only place where it's used. What about removing it?

[vabr (Chromium), 2015/02/04 16:13:44]

It is explained in the TODO I added above. We might need it in the tests to make
sure threads are actually present at all. As the TODO states, this is not ideal,
but I prefer to separate that removal from this CL, which is independent of it.

   origin_loop_ = base::MessageLoopProxy::current();
 }
 
 PasswordStore::GetLoginsRequest::~GetLoginsRequest() {
 }
 
 void PasswordStore::GetLoginsRequest::ApplyIgnoreLoginsCutoff() {
   if (!ignore_logins_cutoff_.is_null()) {
-    // Count down rather than up since we may be deleting elements.
-    // Note that in principle it could be more efficient to copy the whole array
-    // since that's worst-case linear time, but we expect that elements will be
-    // deleted rarely and lists will be small, so this avoids the copies.
-    for (size_t i = result_->size(); i > 0; --i) {
-      if ((*result_)[i - 1]->date_created < ignore_logins_cutoff_) {
-        delete (*result_)[i - 1];
-        result_->erase(result_->begin() + (i - 1));
+    ScopedVector<autofill::PasswordForm> remaining_logins;
+    remaining_logins.reserve(result_.size());
+    for (auto& login : result_) {
+      if (login->date_created >= ignore_logins_cutoff_) {
+        remaining_logins.push_back(login);
+        login = nullptr;
       }
     }
+    remaining_logins.swap(result_);
   }
 }
 
 void PasswordStore::GetLoginsRequest::ForwardResult() {
   origin_loop_->PostTask(FROM_HERE,
-                         base::Bind(&MaybeCallConsumerCallback,
-                                    consumer_weak_,
+                         base::Bind(&MaybeCallConsumerCallback, consumer_weak_,
                                     base::Passed(result_.Pass())));

[vasilii, 2015/02/03 19:22:16]

base::Passed(&result_)?

[vabr (Chromium), 2015/02/04 16:13:44]

Done.

 }
 
 PasswordStore::PasswordStore(
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner,
     scoped_refptr<base::SingleThreadTaskRunner> db_thread_runner)
     : main_thread_runner_(main_thread_runner),
       db_thread_runner_(db_thread_runner),
       observers_(new ObserverListThreadSafe<Observer>()),
       shutdown_called_(false) {
 }
@@ skipping 44 matching lines @@
   base::Time ignore_logins_cutoff;  // the null time
   if (form.scheme == PasswordForm::SCHEME_HTML &&
       (form.signon_realm == "http://www.google.com" ||
        form.signon_realm == "http://www.google.com/" ||
        form.signon_realm == "https://www.google.com" ||
        form.signon_realm == "https://www.google.com/")) {
     static const base::Time::Exploded exploded_cutoff =
         { 2012, 1, 0, 1, 0, 0, 0, 0 };  // 00:00 Jan 1 2012
     ignore_logins_cutoff = base::Time::FromUTCExploded(exploded_cutoff);
   }
-  GetLoginsRequest* request = new GetLoginsRequest(consumer);
+  scoped_ptr<GetLoginsRequest> request(new GetLoginsRequest(consumer));
   request->set_ignore_logins_cutoff(ignore_logins_cutoff);
 
-  ConsumerCallbackRunner callback_runner = base::Bind(
-      &PasswordStore::CopyAndForwardLoginsResult, base::Owned(request));
+  ConsumerCallbackRunner callback_runner =
+      base::Bind(&CopyAndForwardLoginsResult, base::Passed(request.Pass()));

[vasilii, 2015/02/03 19:22:16]

base::Passed(&request)?

[vabr (Chromium), 2015/02/04 16:13:44]

Done.

   ScheduleTask(base::Bind(&PasswordStore::GetLoginsImpl, this, form,
                           prompt_policy, callback_runner));
 }
 
 void PasswordStore::GetAutofillableLogins(PasswordStoreConsumer* consumer) {
   Schedule(&PasswordStore::GetAutofillableLoginsImpl, consumer);
 }
 
 void PasswordStore::GetBlacklistLogins(PasswordStoreConsumer* consumer) {
   Schedule(&PasswordStore::GetBlacklistLoginsImpl, consumer);
@@ skipping 42 matching lines @@
 PasswordStore::~PasswordStore() {
   DCHECK(shutdown_called_);
 }
 
 scoped_refptr<base::SingleThreadTaskRunner>
 PasswordStore::GetBackgroundTaskRunner() {
   return db_thread_runner_;
 }
 
 // static
-void PasswordStore::ForwardLoginsResult(GetLoginsRequest* request) {
+void PasswordStore::ForwardLoginsResult(scoped_ptr<GetLoginsRequest> request) {
   request->ApplyIgnoreLoginsCutoff();
   request->ForwardResult();
 }
 
 // static
 void PasswordStore::CopyAndForwardLoginsResult(
-    PasswordStore::GetLoginsRequest* request,
+    scoped_ptr<PasswordStore::GetLoginsRequest> request,
     ScopedVector<autofill::PasswordForm> matched_forms) {
-  // Move the contents of |matched_forms| into the request.
-  request->result()->swap(matched_forms.get());
-  ForwardLoginsResult(request);
+  DCHECK(request->result()->empty());
+  request->result()->swap(matched_forms);
+  ForwardLoginsResult(request.Pass());
 }
 
 void PasswordStore::LogStatsForBulkDeletion(int num_deletions) {
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsDeletedByBulkDelete",
                        num_deletions);
 }
 
 void PasswordStore::LogStatsForBulkDeletionDuringRollback(int num_deletions) {
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsDeletedDuringRollback",
                        num_deletions);
 }
 
 void PasswordStore::NotifyLoginsChanged(
     const PasswordStoreChangeList& changes) {
   DCHECK(GetBackgroundTaskRunner()->BelongsToCurrentThread());
   if (!changes.empty()) {
     observers_->Notify(&Observer::OnLoginsChanged, changes);
 #if defined(PASSWORD_MANAGER_ENABLE_SYNC)
     if (syncable_service_)
       syncable_service_->ActOnPasswordStoreChanges(changes);
 #endif
   }
 }
 
 template <typename BackendFunc>
 void PasswordStore::Schedule(BackendFunc func,

[vasilii, 2015/02/03 19:22:17]

Optional: you can get rid of the template because BackendFunc is just one
concrete, quite simple type.

[vabr (Chromium), 2015/02/04 16:13:44]

Sounds reasonable, the function signature was restricted by the PostTask call
anyway.
Done.

                              PasswordStoreConsumer* consumer) {
-  GetLoginsRequest* request = new GetLoginsRequest(consumer);
+  scoped_ptr<GetLoginsRequest> request(new GetLoginsRequest(consumer));
   consumer->cancelable_task_tracker()->PostTask(
-      GetBackgroundTaskRunner().get(),
-      FROM_HERE,
-      base::Bind(func, this, base::Owned(request)));
+      GetBackgroundTaskRunner().get(), FROM_HERE,
+      base::Bind(func, this, base::Passed(request.Pass())));

[vasilii, 2015/02/03 19:22:17]

base::Passed(&request);

[vabr (Chromium), 2015/02/04 16:13:44]

Done.

 }
 
 void PasswordStore::WrapModificationTask(ModificationTask task) {
   PasswordStoreChangeList changes = task.Run();
   NotifyLoginsChanged(changes);
 }
 
 void PasswordStore::AddLoginInternal(const PasswordForm& form) {
   PasswordStoreChangeList changes = AddLoginImpl(form);
   NotifyLoginsChanged(changes);
@@ skipping 32 matching lines @@
   syncable_service_->InjectStartSyncFlare(flare);
 }
 
 void PasswordStore::DestroySyncableService() {
   DCHECK(GetBackgroundTaskRunner()->BelongsToCurrentThread());
   syncable_service_.reset();
 }
 #endif
 
 }  // namespace password_manager