Add metrics for PasswordForm submission types

components/autofill/core/common/password_form.h

 // 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.
 
 #ifndef COMPONENTS_AUTOFILL_CORE_COMMON_PASSWORD_FORM_H__
 #define COMPONENTS_AUTOFILL_CORE_COMMON_PASSWORD_FORM_H__
 
 #include <map>
 #include <memory>
 #include <string>
@@ skipping 66 matching lines @@
   enum class Layout {
     // Forms which either do not need to be classified, or cannot be classified
     // meaningfully.
     LAYOUT_OTHER,
     // Login and signup forms combined in one <form>, to distinguish them from,
     // e.g., change-password forms.
     LAYOUT_LOGIN_AND_SIGNUP,
     LAYOUT_LAST = LAYOUT_LOGIN_AND_SIGNUP
   };
 
+  // Events that Password Manager uses as triggers for checking a success of a
+  // password form submission.
+  enum class SubmissionEvent {

[engedy, 2017/05/09 22:02:59]

nit: To avoid using overloaded words in this context, have you considered naming
this something along the lines of:
 -- SubmissionDetectionTechnique, or
 -- SubmissionIndicatorEvent, or
 -- ProvisionalSaveTrigger, etc.

[dvadym, 2017/05/10 12:10:12]

Thanks, it makes sense. Renamed to SubmissionIndicatorEvent.

[engedy, 2017/05/10 13:07:42]

Acknowledged.

+    NONE,
+    HTML_FORM_SUBMISSION,
+    INPAGE_NAVIGATION,
+    XHR_SUCCEEDED,
+    FRAME_DETACHED,
+    BY_CONTEXT_MENU_CLICK,
+    SUBMISSION_EVENT_COUNT
+  };
+
   // The "Realm" for the sign-on. This is scheme, host, port for SCHEME_HTML.
   // Dialog based forms also contain the HTTP realm. Android based forms will
   // contain a string of the form "android://<hash of cert>@<package name>"
   //
   // The signon_realm is effectively the primary key used for retrieving
   // data from the database, so it must not be empty.
   std::string signon_realm;
 
   // An origin URL consists of the scheme, host, port and path; the rest is
   // stripped. This is the primary data used by the PasswordManager to decide
@@ skipping 174 matching lines @@
   // If true, this match was found using public suffix matching.
   bool is_public_suffix_match;
 
   // If true, this is a credential saved through an Android application, and
   // found using affiliation-based match.
   bool is_affiliation_based_match;
 
   // If true, this form looks like SignUp form according to local heuristics.
   bool does_look_like_signup_form;
 
+  // Type of event which was used for this form submission.
+  SubmissionEvent submission_event;

[engedy, 2017/05/09 22:02:59]

I wonder if Vasilii and Vaclav are going to be really sad once they are back and
see that PasswordForm grew even larger during their absence.

While I can certainly see the appeal of making this value part of PasswordForm
to facilitate plumbing it through all the layers, have you considered the
following?

 -- Getting this value through Mojo is quite painless: We would need to add this
as an argument to PasswordManagerDriver::InPageNavigation, but in all other
scenarios the PasswordManager can already tell the submission detection type
based on what method was called on it.

 -- The better question is how to store this on the browser side. We could store
this entirely in the PasswordManager (after all, the histogram is recorded
there), or add it as an additional field to PasswordFormManager. The latter
seems nicer, but there are quite a few call sites in unittests to
PFM::ProvisionallySave :-/.

[dvadym, 2017/05/10 12:10:12]

Don't worry, with the refactoring PasswordForm will not be on renderer anymore. 
And I think that this is the right place, since submission event type is a
concept of password form, why it should be in PasswordFormManager, which is
actually also overloaded with information :).

[engedy, 2017/05/10 13:07:42]

Fair enough, and glad to hear that this abomination is ultimately going away.
;-)

[dvadym, 2017/05/10 14:09:25]

Here many fields in PasswordForm is the smallest problem :). But of course it's
very visible.

+
   // Return true if we consider this form to be a change password form.
   // We use only client heuristics, so it could include signup forms.
   bool IsPossibleChangePasswordForm() const;
 
   // Return true if we consider this form to be a change password form
   // without username field. We use only client heuristics, so it could
   // include signup forms.
   bool IsPossibleChangePasswordFormWithoutUsername() const;
 
   // Equality operators for testing.
@@ skipping 17 matching lines @@
 };
 
 // Converts a vector of possible usernames to string.
 base::string16 OtherPossibleUsernamesToString(
     const PossibleUsernamesVector& possible_usernames);
 
 // For testing.
 std::ostream& operator<<(std::ostream& os, PasswordForm::Layout layout);
 std::ostream& operator<<(std::ostream& os, const PasswordForm& form);
 std::ostream& operator<<(std::ostream& os, PasswordForm* form);
+std::ostream& operator<<(std::ostream& os,
+                         PasswordForm::SubmissionEvent submission_event);
 
 }  // namespace autofill
 
 #endif  // COMPONENTS_AUTOFILL_CORE_COMMON_PASSWORD_FORM_H__