Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(228)

Unified Diff: components/password_manager/content/browser/credential_manager_dispatcher.cc

Issue 866983003: GetLoginsRequest: Use ScopedVector to express ownership of forms (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@324291_scopedvector
Patch Set: Just rebased on mkwst's changes Created 5 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: components/password_manager/content/browser/credential_manager_dispatcher.cc
diff --git a/components/password_manager/content/browser/credential_manager_dispatcher.cc b/components/password_manager/content/browser/credential_manager_dispatcher.cc
index fdb6f5f82f79ce4f16e26be86f99d0316d50d7fa..b251bd7d8bd32460566a7faaa72b0ff8188f035d 100644
--- a/components/password_manager/content/browser/credential_manager_dispatcher.cc
+++ b/components/password_manager/content/browser/credential_manager_dispatcher.cc
@@ -28,6 +28,7 @@ namespace password_manager {
class CredentialManagerDispatcher::PendingRequestTask
: public PasswordStoreConsumer {
public:
+ // TODO(mkwst): De-inline the methods in this class. http://goo.gl/RmFwKd
PendingRequestTask(CredentialManagerDispatcher* const dispatcher,
int request_id,
bool request_zero_click_only,
@@ -45,52 +46,7 @@ class CredentialManagerDispatcher::PendingRequestTask
// PasswordStoreConsumer implementation.
void OnGetPasswordStoreResults(
- const std::vector<autofill::PasswordForm*>& results) override {
- // We own the PasswordForm instances, so we're responsible for cleaning
- // up the instances we don't add to |local_results| or |federated_results|.
- //
- // TODO(mkwst): Switch this and PromptUserToChooseCredentials() to use
- // ScopedVector.
- std::vector<autofill::PasswordForm*> local_results;
- std::vector<autofill::PasswordForm*> federated_results;
- for (autofill::PasswordForm* form : results) {
- if (form->origin == origin_)
- local_results.push_back(form);
- else if (federations_.count(form->origin.spec()))
- federated_results.push_back(form);
- else
- delete form;
- }
-
- if ((local_results.empty() && federated_results.empty()) ||
- dispatcher_->web_contents()->GetLastCommittedURL().GetOrigin() !=
- origin_) {
- dispatcher_->SendCredential(id_, CredentialInfo());
- return;
- }
- if (local_results.size() == 1 && dispatcher_->IsZeroClickAllowed()) {
- // TODO(mkwst): Use the `one_time_disable_zero_click` flag on the result
- // to prevent auto-sign-in, once that flag is implemented.
- CredentialInfo info(*local_results[0],
- local_results[0]->federation_url.is_empty()
- ? CredentialType::CREDENTIAL_TYPE_LOCAL
- : CredentialType::CREDENTIAL_TYPE_FEDERATED);
- STLDeleteElements(&local_results);
- STLDeleteElements(&federated_results);
- dispatcher_->SendCredential(id_, info);
- return;
- }
-
- if (zero_click_only_ ||
- !dispatcher_->client()->PromptUserToChooseCredentials(
- local_results, federated_results,
- base::Bind(&CredentialManagerDispatcher::SendCredential,
- base::Unretained(dispatcher_), id_))) {
- STLDeleteElements(&local_results);
- STLDeleteElements(&federated_results);
- dispatcher_->SendCredential(id_, CredentialInfo());
- }
- }
+ ScopedVector<autofill::PasswordForm> results) override;
private:
// Backlink to the CredentialManagerDispatcher that owns this object.
@@ -104,6 +60,46 @@ class CredentialManagerDispatcher::PendingRequestTask
DISALLOW_COPY_AND_ASSIGN(PendingRequestTask);
};
+void CredentialManagerDispatcher::PendingRequestTask::OnGetPasswordStoreResults(
+ ScopedVector<autofill::PasswordForm> results) {
+ ScopedVector<autofill::PasswordForm> local_results;
+ ScopedVector<autofill::PasswordForm> federated_results;
+ for (auto& form : results) {
+ if (form->origin == origin_) {
+ local_results.push_back(form);
+ form = nullptr;
+ } else if (federations_.count(form->origin.spec())) {
+ federated_results.push_back(form);
+ form = nullptr;
+ }
+ }
+
+ if ((local_results.empty() && federated_results.empty()) ||
+ dispatcher_->web_contents()->GetLastCommittedURL().GetOrigin() !=
+ origin_) {
+ dispatcher_->SendCredential(id_, CredentialInfo());
+ return;
+ }
+ if (local_results.size() == 1 && dispatcher_->IsZeroClickAllowed()) {
+ // TODO(mkwst): Use the `skip_zero_click` flag on the result to prevent
+ // auto-sign-in.
+ CredentialInfo info(*local_results[0],
+ local_results[0]->federation_url.is_empty()
+ ? CredentialType::CREDENTIAL_TYPE_LOCAL
+ : CredentialType::CREDENTIAL_TYPE_FEDERATED);
+ dispatcher_->SendCredential(id_, info);
+ return;
+ }
+
+ if (zero_click_only_ ||
+ !dispatcher_->client()->PromptUserToChooseCredentials(
+ local_results.Pass(), federated_results.Pass(),
+ base::Bind(&CredentialManagerDispatcher::SendCredential,
+ base::Unretained(dispatcher_), id_))) {
+ dispatcher_->SendCredential(id_, CredentialInfo());
+ }
+}
+
// CredentialManagerDispatcher::PendingSignedOutTask ---------------------------
class CredentialManagerDispatcher::PendingSignedOutTask
@@ -116,7 +112,7 @@ class CredentialManagerDispatcher::PendingSignedOutTask
// PasswordStoreConsumer implementation.
void OnGetPasswordStoreResults(
- const std::vector<autofill::PasswordForm*>& results) override;
+ ScopedVector<autofill::PasswordForm> results) override;
private:
// Backlink to the CredentialManagerDispatcher that owns this object.
@@ -139,18 +135,16 @@ void CredentialManagerDispatcher::PendingSignedOutTask::AddOrigin(
}
void CredentialManagerDispatcher::PendingSignedOutTask::
- OnGetPasswordStoreResults(
- const std::vector<autofill::PasswordForm*>& results) {
+ OnGetPasswordStoreResults(ScopedVector<autofill::PasswordForm> results) {
PasswordStore* store = dispatcher_->GetPasswordStore();
for (autofill::PasswordForm* form : results) {
if (origins_.count(form->origin.spec())) {
form->skip_zero_click = true;
+ // Note that UpdateLogin ends up copying the form while posting a task to
+ // update the PasswordStore, so it's fine to let |results| delete the
+ // original at the end of this method.
store->UpdateLogin(*form);
}
- // We own the PasswordForms, so we need to dispose of them. This is safe to
- // do, as ::UpdateLogin ends up copying the form while posting a task to
- // update the PasswordStore.
- delete form;
}
dispatcher_->DoneSigningOut();

Powered by Google App Engine
This is Rietveld 408576698