Allow deleting autofill password suggestions on Shift+Delete

components/autofill/core/browser/autofill_external_delegate.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 "components/autofill/core/browser/autofill_external_delegate.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autocomplete_history_manager.h"
 #include "components/autofill/core/browser/autofill_driver.h"
 #include "components/autofill/core/browser/autofill_manager.h"
@@ skipping 17 matching lines @@
       has_shown_autofill_popup_for_current_edit_(false),
       weak_ptr_factory_(this) {
   DCHECK(autofill_manager);
 }
 
 AutofillExternalDelegate::~AutofillExternalDelegate() {}
 
 void AutofillExternalDelegate::OnQuery(int query_id,
                                        const FormData& form,
                                        const FormFieldData& field,
+                                       const PasswordForm& password_form,
                                        const gfx::RectF& element_bounds,
                                        bool display_warning_if_disabled) {
   autofill_query_form_ = form;
   autofill_query_field_ = field;
+  //autofill_query_password_form_ = password_form;

[vabr (Chromium), 2014/01/29 16:09:36]

If you don't need this line, then please remove it. Also remove the
password_form argument, if it's not used.

[riadh.chtara, 2014/02/04 16:22:03]

Done.

   display_warning_if_disabled_ = display_warning_if_disabled;
   autofill_query_id_ = query_id;
   element_bounds_ = element_bounds;
 }
 
 void AutofillExternalDelegate::OnSuggestionsReturned(
     int query_id,
     const std::vector<base::string16>& autofill_values,
     const std::vector<base::string16>& autofill_labels,
     const std::vector<base::string16>& autofill_icons,
@@ skipping 58 matching lines @@
         icons,
         ids,
         GetWeakPtr());
   }
 }
 
 void AutofillExternalDelegate::OnShowPasswordSuggestions(
     const std::vector<base::string16>& suggestions,
     const std::vector<base::string16>& realms,
     const FormFieldData& field,
+    const PasswordForm& password_form,
     const gfx::RectF& element_bounds) {
   autofill_query_field_ = field;
+  autofill_query_password_form_ = password_form;
   element_bounds_ = element_bounds;
 
   if (suggestions.empty()) {
     autofill_manager_->delegate()->HideAutofillPopup();
     return;
   }
 
   std::vector<base::string16> empty(suggestions.size());
   std::vector<int> password_ids(suggestions.size(),
                                 WebAutofillClient::MenuItemIDPasswordEntry);
@@ skipping 61 matching lines @@
     FillAutofillFormData(identifier, false);
   }
 
   autofill_manager_->delegate()->HideAutofillPopup();
 }
 
 void AutofillExternalDelegate::RemoveSuggestion(const base::string16& value,
                                                 int identifier) {
   if (identifier > 0) {
     autofill_manager_->RemoveAutofillProfileOrCreditCard(identifier);
+  } else if (identifier == WebAutofillClient::MenuItemIDPasswordEntry) {
+    autofill_query_password_form_.username_value = value;

[vabr (Chromium), 2014/01/29 16:09:36]

It looks like you mix the |value| with the rest of
|autofill_query_password_form_|. Are you sure that's right?

Can it happen, that |autofill_query_password_form_| will contain the credentials
pair ("Joe User", "secret password"), and then this line replaces "Joe User"
with "Ann User", but keeps the "secret password"?

[riadh.chtara, 2014/02/04 16:22:03]

This is the line that solved the issue which was:
the |username_value|  of  |autofill_query_password_form_| is the actual value in
the form. So if you have two users : Joe and JoeUser. And you put in the user
input field Jo, |username_value| of |autofill_query_password_form_| is going to
be Jo.
The popup is going to be shown with two values. And if you select JoeUser form
the popup without validating your choice (by pressing enter), |value| is going
to be Joe.
Before, this line was added, we try to remove the saved login that have Jo as
its username and this why I got login not found issue.
Now if we add this line the problem is solved.
For the value of the password I don't have a problem with it.
Even if we have many credentials with different passwords, and we have also a
different password in the password field, every thing works fine.
I'm not sure how removing credential happens but I guess when deleting a
credential:
* it checks whether or not a completely match exists: with the same password and
user name, otherwise it checks if there is a partial match (only username is
matching).
or
* it just checks for the username and not the password, because it's not
possible to have two credentials with same user name and different passwords. 
But it works fine.

[vabr (Chromium), 2014/02/05 20:34:20]

Thanks for explaining the problem (with Jo insetad of Joe being in the form
submitted for deletion)!

But instead of modifying autofill_query_password_form_, could you please create
a local copy of autofill_query_password_form_ in this code block, change the
value for that (please add a comment explaining you are replacing the user-typed
prefix with the full name from the suggestion), and pass the local copy to
RemovePasswordSuggestion?

My concern is that if you modify autofill_query_password_form_, and the user
hits Submit without selecting anything, they could possibly store the password
for Joe under the username Jo.

On 2014/02/04 16:22:03, riadh.chtara wrote:

+    bool success = password_autofill_manager_.RemovePasswordSuggestion(
+        autofill_query_field_, autofill_query_password_form_);
+    DCHECK(success);
   } else {
     autofill_manager_->RemoveAutocompleteEntry(autofill_query_field_.name,
                                                value);
   }
 }
 
 void AutofillExternalDelegate::DidEndTextFieldEditing() {
   autofill_manager_->delegate()->HideAutofillPopup();
 
   has_shown_autofill_popup_for_current_edit_ = false;
@@ skipping 136 matching lines @@
   // Set the values that all datalist elements share.
   autofill_icons->insert(autofill_icons->begin(),
                          data_list_values_.size(),
                          base::string16());
   autofill_unique_ids->insert(autofill_unique_ids->begin(),
                               data_list_values_.size(),
                               WebAutofillClient::MenuItemIDDataListEntry);
 }
 
 }  // namespace autofill