Measure how often PSL and same-organization name credentials are suppressed.

components/password_manager/core/browser/form_fetcher_impl.cc

Index: components/password_manager/core/browser/form_fetcher_impl.cc
diff --git a/components/password_manager/core/browser/form_fetcher_impl.cc b/components/password_manager/core/browser/form_fetcher_impl.cc
index 8b7b55f52f0c5807f1925298dccb62cf476aa79c..79dfffe7d4b30fb515c042fb1c57c29fc4f6d929 100644
--- a/components/password_manager/core/browser/form_fetcher_impl.cc
+++ b/components/password_manager/core/browser/form_fetcher_impl.cc
@@ -15,6 +15,7 @@
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/browser/psl_matching_helper.h"
 #include "components/password_manager/core/browser/statistics_table.h"
 
 using autofill::PasswordForm;
@@ -46,6 +47,40 @@ std::vector<std::unique_ptr<PasswordForm>> SplitFederatedMatches(
   return federated_matches;
 }
 
+void SplitSuppressedFormsAndAssignTo(
+    const PasswordStore::FormDigest& observed_form_digest,
+    std::vector<std::unique_ptr<PasswordForm>> suppressed_forms,

[kolos1, 2017/05/30 07:02:29]

Should we pass |suppressed_forms| by value, by not by point (since we change the
vector)?

[engedy, 2017/05/30 08:47:43]

We do pass it by value.

[kolos1, 2017/05/30 09:16:36]

oops, actually I suggested the opposite. I believe we can pass it by point.

[engedy, 2017/05/30 13:51:35]

I dunno -- passing it by value allows for std::move at the call site, which
expresses nicely that it is consumed. WDYT?

+    std::vector<std::unique_ptr<PasswordForm>>* same_origin_https_forms,
+    std::vector<std::unique_ptr<PasswordForm>>* psl_matching_forms,
+    std::vector<std::unique_ptr<PasswordForm>>* same_organization_name_forms) {

[kolos1, 2017/05/30 07:02:29]

Optional: add DCHECKs for vectors here 

[engedy, 2017/05/30 08:47:43]

Done.

+  same_origin_https_forms->clear();
+  psl_matching_forms->clear();
+  same_organization_name_forms->clear();
+  for (auto& form : suppressed_forms) {
+    switch (GetMatchResult(*form, observed_form_digest)) {
+      case MatchResult::PSL_MATCH:
+        psl_matching_forms->push_back(std::move(form));
+        break;
+      case MatchResult::NO_MATCH:
+        if (form->origin.host() != observed_form_digest.origin.host()) {
+          same_organization_name_forms->push_back(std::move(form));
+        } else if (form->origin.SchemeIs(url::kHttpsScheme) &&
+                   observed_form_digest.origin.SchemeIs(url::kHttpScheme)) {
+          same_origin_https_forms->push_back(std::move(form));
+        } else {
+          // HTTP form suppressed on HTTPS observed page: The HTTP->HTTPS
+          // migration can leave tons of such HTTP forms behind, ignore these.
+        }
+        break;
+      case MatchResult::EXACT_MATCH:
+      case MatchResult::FEDERATED_MATCH:
+      case MatchResult::FEDERATED_PSL_MATCH:
+        NOTREACHED() << "Suppressed match cannot be exact or federated.";
+        break;
+    }
+  }
+}
+
 // Create a vector of const PasswordForm from a vector of
 // unique_ptr<PasswordForm> by applying get() item-wise.
 std::vector<const PasswordForm*> MakeWeakCopies(
@@ -74,12 +109,11 @@ std::vector<std::unique_ptr<PasswordForm>> MakeCopies(
 FormFetcherImpl::FormFetcherImpl(PasswordStore::FormDigest form_digest,
                                  const PasswordManagerClient* client,
                                  bool should_migrate_http_passwords,
-                                 bool should_query_suppressed_https_forms)
+                                 bool should_query_suppressed_forms)
     : form_digest_(std::move(form_digest)),
       client_(client),
       should_migrate_http_passwords_(should_migrate_http_passwords),
-      should_query_suppressed_https_forms_(
-          should_query_suppressed_https_forms) {}
+      should_query_suppressed_forms_(should_query_suppressed_forms) {}
 
 FormFetcherImpl::~FormFetcherImpl() = default;
 
@@ -111,11 +145,21 @@ const std::vector<const PasswordForm*>& FormFetcherImpl::GetFederatedMatches()
 
 const std::vector<const PasswordForm*>&
 FormFetcherImpl::GetSuppressedHTTPSForms() const {
-  return weak_suppressed_https_forms_;
+  return weak_suppressed_same_origin_https_forms_;
+}
+
+const std::vector<const PasswordForm*>&
+FormFetcherImpl::GetSuppressedPSLMatchingForms() const {
+  return weak_suppressed_psl_matching_forms_;
 }
 
-bool FormFetcherImpl::DidCompleteQueryingSuppressedHTTPSForms() const {
-  return did_complete_querying_suppressed_https_forms_;
+const std::vector<const PasswordForm*>&
+FormFetcherImpl::GetSuppressedSameOrganizationNameForms() const {
+  return weak_suppressed_same_organization_name_forms_;
+}
+
+bool FormFetcherImpl::DidCompleteQueryingSuppressedForms() const {
+  return did_complete_querying_suppressed_forms_;
 }
 
 void FormFetcherImpl::OnGetPasswordStoreResults(
@@ -144,12 +188,11 @@ void FormFetcherImpl::OnGetPasswordStoreResults(
   //
   // These results are used only for recording metrics at PasswordFormManager
   // desctruction time, this is why they are requested so late.
-  if (should_query_suppressed_https_forms_ &&
+  if (should_query_suppressed_forms_ &&
       form_digest_.scheme == PasswordForm::SCHEME_HTML &&
-      form_digest_.origin.SchemeIs(url::kHttpScheme)) {
-    suppressed_https_form_fetcher_ =
-        base::MakeUnique<SuppressedHTTPSFormFetcher>(form_digest_.signon_realm,
-                                                     client_, this);
+      GURL(form_digest_.signon_realm).SchemeIsHTTPOrHTTPS()) {
+    suppressed_form_fetcher_ = base::MakeUnique<SuppressedFormFetcher>(
+        form_digest_.signon_realm, client_, this);
   }
 
   if (should_migrate_http_passwords_ && results.empty() &&
@@ -174,12 +217,19 @@ void FormFetcherImpl::ProcessMigratedForms(
   ProcessPasswordStoreResults(std::move(forms));
 }
 
-void FormFetcherImpl::ProcessSuppressedHTTPSForms(
+void FormFetcherImpl::ProcessSuppressedForms(
     std::vector<std::unique_ptr<autofill::PasswordForm>> forms) {
-  did_complete_querying_suppressed_https_forms_ = true;
-
-  suppressed_https_forms_ = std::move(forms);
-  weak_suppressed_https_forms_ = MakeWeakCopies(suppressed_https_forms_);
+  did_complete_querying_suppressed_forms_ = true;
+  SplitSuppressedFormsAndAssignTo(form_digest_, std::move(forms),
+                                  &suppressed_same_origin_https_forms_,
+                                  &suppressed_psl_matching_forms_,
+                                  &suppressed_same_organization_name_forms_);
+  weak_suppressed_same_origin_https_forms_ =
+      MakeWeakCopies(suppressed_same_origin_https_forms_);
+  weak_suppressed_psl_matching_forms_ =
+      MakeWeakCopies(suppressed_psl_matching_forms_);
+  weak_suppressed_same_organization_name_forms_ =
+      MakeWeakCopies(suppressed_same_organization_name_forms_);
 }
 
 void FormFetcherImpl::Fetch() {
@@ -223,17 +273,26 @@ std::unique_ptr<FormFetcher> FormFetcherImpl::Clone() {
   // Create the copy without the "HTTPS migration" activated. If it was needed,
   // then it was done by |this| already.
   auto result = base::MakeUnique<FormFetcherImpl>(
-      form_digest_, client_, false, should_query_suppressed_https_forms_);
+      form_digest_, client_, false, should_query_suppressed_forms_);

[kolos1, 2017/05/30 07:02:29]

Shall we create a copy with SuppressedFormsFetcher? Or should
|should_query_suppressed_forms| always be false?

[engedy, 2017/05/30 08:47:43]

The SuppressedFormFetcher has no state, so I guess implementing a Clone()
operation on it would be over-engineering it. :-) It is probably fine to have it
created on the next call to Start() in the clone.

Note that if you clone |this| right after construction, without ever having
called Start(), |suppressed*_forms_| will all be empty, so I'd be uneasy about
setting |should_query_suppressed_forms| to false in the clone.

[kolos1, 2017/05/30 09:16:36]

Sorry for confusing comment again :(

HTTP migration is suppressed in the copy of FormFetcher (look at |false| and
comment before). I believe we should do the same for SuppressedFormFetcher.

The comment to FormFetcher::Clone says that it should not cause additional
fetches. The same comment in tests
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

My suggestion:
* replace |should_query_suppressed_forms_| with |false|
* update FormFetcherImplTest, Clone_* tests

[engedy, 2017/05/30 13:51:35]

Added a unittest to enforce that no fetch is made directly in response to the
Clone call.

 
   result->non_federated_ = MakeCopies(this->non_federated_);
   result->federated_ = MakeCopies(this->federated_);
   result->interactions_stats_ = this->interactions_stats_;
-  result->suppressed_https_forms_ = MakeCopies(this->suppressed_https_forms_);
+  result->suppressed_same_origin_https_forms_ =
+      MakeCopies(this->suppressed_same_origin_https_forms_);
+  result->suppressed_psl_matching_forms_ =
+      MakeCopies(this->suppressed_psl_matching_forms_);
+  result->suppressed_same_organization_name_forms_ =
+      MakeCopies(this->suppressed_same_organization_name_forms_);
 
   result->weak_non_federated_ = MakeWeakCopies(result->non_federated_);
   result->weak_federated_ = MakeWeakCopies(result->federated_);
-  result->weak_suppressed_https_forms_ =
-      MakeWeakCopies(result->suppressed_https_forms_);
+  result->weak_suppressed_same_origin_https_forms_ =
+      MakeWeakCopies(result->suppressed_same_origin_https_forms_);
+  result->weak_suppressed_psl_matching_forms_ =
+      MakeWeakCopies(result->suppressed_psl_matching_forms_);
+  result->weak_suppressed_same_organization_name_forms_ =
+      MakeWeakCopies(result->suppressed_same_organization_name_forms_);
 
   result->filtered_count_ = this->filtered_count_;
   result->state_ = this->state_;