[Password Manager] Add federations to sort key on password page

chrome/browser/ui/passwords/password_manager_presenter.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/ui/passwords/password_manager_presenter.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 58 matching lines @@
   if (!is_android_uri)
     return "0";
   if (is_clickable)
     return "1";
   return "2";
 }
 
 // Creates key for sorting password or password exception entries.
 // The key is eTLD+1 followed by subdomains
 // (e.g. secure.accounts.example.com => example.com.accounts.secure).
-// If |username_and_password_in_key == true|, username and password is appended
-// to the key. The entry type code (non-Android, Android w/ or w/o affiliated
-// web realm) is also appended to the key.
+// If |is_password_exception == false|, username, password and federation are
+// appended to the key. The entry type code (non-Android, Android w/ or w/o
+// affiliated web realm) is also appended to the key.
 std::string CreateSortKey(const autofill::PasswordForm& form,
-                          bool username_and_password_in_key) {
+                          bool is_password_exception) {
   bool is_android_uri = false;
   bool is_clickable = false;
   GURL link_url;
   std::string origin = password_manager::GetShownOriginAndLinkUrl(
       form, &is_android_uri, &link_url, &is_clickable);
 
   if (!is_clickable) {  // e.g. android://com.example.r => r.example.com.
     origin = SplitByDotAndReverse(
         StringPiece(&origin[kAndroidAppSchemeAndDelimiterLength],
                     origin.length() - kAndroidAppSchemeAndDelimiterLength));
   }
 
   std::string site_name =
       net::registry_controlled_domains::GetDomainAndRegistry(
           origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
   if (site_name.empty())  // e.g. localhost.
     site_name = origin;
   std::string key =
       site_name + SplitByDotAndReverse(StringPiece(
                       &origin[0], origin.length() - site_name.length()));
 
-  if (username_and_password_in_key) {
+  if (!is_password_exception) {
     key = key + kSortKeyPartsSeparator +
           base::UTF16ToUTF8(form.username_value) + kSortKeyPartsSeparator +
           base::UTF16ToUTF8(form.password_value);
+    if (!form.federation_origin.unique())
+      key = key + kSortKeyPartsSeparator + form.federation_origin.host();
   }
 
   // Since Android and non-Android entries shouldn't be merged into one entry,
   // add the entry type code to the sort key.
   key +=
       kSortKeyPartsSeparator + GetEntryTypeCode(is_android_uri, is_clickable);
   return key;
 }
 
 // Finds duplicates of |form| in |duplicates|, removes them from |store| and
 // from |duplicates|.

[vabr (Chromium), 2016/04/28 10:03:11]

nit: This comment should also explain what is |is_password_exception|. It might
be helpful to note that it should be true for the list of blacklisted passwords,
and false for the list of saved passwords.

 void RemoveDuplicates(const autofill::PasswordForm& form,
                       DuplicatesMap* duplicates,
                       PasswordStore* store,
-                      bool username_and_password_in_key) {
-  std::string key =
-      CreateSortKey(form, username_and_password_in_key);
+                      bool is_password_exception) {
+  std::string key = CreateSortKey(form, is_password_exception);
   std::pair<DuplicatesMap::iterator, DuplicatesMap::iterator> dups =
       duplicates->equal_range(key);
   for (DuplicatesMap::iterator it = dups.first; it != dups.second; ++it)
     store->RemoveLogin(*it->second);
   duplicates->erase(key);
 }
 
 }  // namespace
 
 PasswordManagerPresenter::PasswordManagerPresenter(
@@ skipping 47 matching lines @@
     // |index| out of bounds might come from a compromised renderer
     // (http://crbug.com/362054), or the user removed a password while a request
     // to the store is in progress (i.e. |password_list_| is empty).
     // Don't let it crash the browser.
     return;
   }
   PasswordStore* store = GetPasswordStore();
   if (!store)
     return;
 
-  RemoveDuplicates(*password_list_[index], &password_duplicates_,
-                   store, true);
+  RemoveDuplicates(*password_list_[index], &password_duplicates_, store,
+                   false /* not password exception */);

[vabr (Chromium), 2016/04/28 10:03:11]

For this line and lines 214, 380, 405 also:

Please write the name of the argument variable in the comment, i.e.,
/* is_password_exception */

When you write "false /* not password_exception */", it is not clear if the
false already includes the "not".

An even better way to make such calls easy to read would be to change the type
of the argument from
bool is_password_exception
to
PasswordEntryType entry_type

with the new type declared as
enum class PasswordEntryType {SAVED, BLACKLISTED};

Then the call would be
RemoveDuplicates(..., PasswordEntryType::SAVED);
and would not require any clarifying comments.

   store->RemoveLogin(*password_list_[index]);
   content::RecordAction(
       base::UserMetricsAction("PasswordManager_RemoveSavedPassword"));
 }
 
 void PasswordManagerPresenter::RemovePasswordException(size_t index) {
   if (index >= password_exception_list_.size()) {
     // |index| out of bounds might come from a compromised renderer
     // (http://crbug.com/362054), or the user removed a password exception while
     // a request to the store is in progress (i.e. |password_exception_list_|
     // is empty). Don't let it crash the browser.
     return;
   }
   PasswordStore* store = GetPasswordStore();
   if (!store)
     return;
   RemoveDuplicates(*password_exception_list_[index],
-                   &password_exception_duplicates_, store, false);
+                   &password_exception_duplicates_, store,
+                   true /* password exception */);
   store->RemoveLogin(*password_exception_list_[index]);
   content::RecordAction(
       base::UserMetricsAction("PasswordManager_RemovePasswordException"));
 }
 
 void PasswordManagerPresenter::RequestShowPassword(size_t index) {
 #if !defined(OS_ANDROID)  // This is never called on Android.
   if (index >= password_list_.size()) {
     // |index| out of bounds might come from a compromised renderer
     // (http://crbug.com/362054), or the user requested to show a password while
@@ skipping 69 matching lines @@
   password_view_->SetPasswordList(password_list_);
 }
 
 void PasswordManagerPresenter::SetPasswordExceptionList() {
   password_view_->SetPasswordExceptionList(password_exception_list_);
 }
 
 void PasswordManagerPresenter::SortEntriesAndHideDuplicates(
     std::vector<std::unique_ptr<autofill::PasswordForm>>* list,
     DuplicatesMap* duplicates,
-    bool username_and_password_in_key) {
+    bool is_password_exceptions) {
   std::vector<std::pair<std::string, std::unique_ptr<autofill::PasswordForm>>>
       pairs;
   pairs.reserve(list->size());
   for (auto& form : *list) {
-    pairs.push_back(std::make_pair(
-        CreateSortKey(*form, username_and_password_in_key), std::move(form)));
+    pairs.push_back(std::make_pair(CreateSortKey(*form, is_password_exceptions),
+                                   std::move(form)));
   }
 
   std::sort(
       pairs.begin(), pairs.end(),
       [](const std::pair<std::string, std::unique_ptr<autofill::PasswordForm>>&
              left,
          const std::pair<std::string, std::unique_ptr<autofill::PasswordForm>>&
              right) { return left.first < right.first; });
 
   list->clear();
@@ skipping 49 matching lines @@
     LOG(ERROR) << "No password store! Cannot display passwords.";
   }
 }
 
 void PasswordManagerPresenter::PasswordListPopulater::OnGetPasswordStoreResults(
     ScopedVector<autofill::PasswordForm> results) {
   page_->password_list_ =
       password_manager_util::ConvertScopedVector(std::move(results));
   page_->SortEntriesAndHideDuplicates(&page_->password_list_,
                                       &page_->password_duplicates_,
-                                      true /* use username and password */);
+                                      false /* not password exceptions */);
   page_->SetPasswordList();
 }
 
 PasswordManagerPresenter::PasswordExceptionListPopulater::
     PasswordExceptionListPopulater(PasswordManagerPresenter* page)
         : ListPopulater(page) {
 }
 
 void PasswordManagerPresenter::PasswordExceptionListPopulater::Populate() {
   PasswordStore* store = page_->GetPasswordStore();
   if (store != NULL) {
     cancelable_task_tracker()->TryCancelAll();
     store->GetBlacklistLoginsWithAffiliatedRealms(this);
   } else {
     LOG(ERROR) << "No password store! Cannot display exceptions.";
   }
 }
 
 void PasswordManagerPresenter::PasswordExceptionListPopulater::
     OnGetPasswordStoreResults(ScopedVector<autofill::PasswordForm> results) {
   page_->password_exception_list_ =
       password_manager_util::ConvertScopedVector(std::move(results));
   page_->SortEntriesAndHideDuplicates(&page_->password_exception_list_,
-      &page_->password_exception_duplicates_,
-      false /* don't use username and password*/);
+                                      &page_->password_exception_duplicates_,
+                                      true /* password exceptions */);
   page_->SetPasswordExceptionList();
 }