Add metrics for PasswordForm submission types

Add metrics for PasswordForm submission types

The Password Manager uses a bunch of different events as triggers for checking whether a password form was submitted successfully. This CL adds UMA histograms to record the event used as the trigger for each successful password form submission.

BUG=None

Review-Url: https://codereview.chromium.org/2859243004
Cr-Commit-Position: refs/heads/master@{#470584}
Committed: https://chromium.googlesource.com/chromium/src/+/3ea544d4e19eff2b81e4a0f9d434a597203e2089

[dvadym, 2017-05-09 15:25:41]

Description was changed from

==========
Metrics for Password Form submssions types

BUG=None
==========

to

==========
Metrics for Password Form submssions types

Password Manager uses a bunch of different events as triggers for checking
whether a password form was submitted successfuly. This CL adds UMA, which event
was used for each successful password form submission.

BUG=None
==========

[dvadym, 2017-05-09 15:25:41]

dvadym@chromium.org changed reviewers:
+ engedy@chromium.org

[dvadym, 2017-05-09 15:29:12]

dvadym@chromium.org changed reviewers:
+ mkwst@chromium.org

[dvadym, 2017-05-09 15:29:12]

engedy@ could you please review everything except mojo and *xml?

mkwst@ could you please review mojo?

[dvadym, 2017-05-09 17:51:25]

dvadym@chromium.org changed reviewers:
+ isherman@chromium.org

[dvadym, 2017-05-09 17:51:26]

isherman@ could you please review histograms.xml?

[engedy, 2017-05-09 18:21:04]

Description was changed from

==========
Metrics for Password Form submssions types

Password Manager uses a bunch of different events as triggers for checking
whether a password form was submitted successfuly. This CL adds UMA, which event
was used for each successful password form submission.

BUG=None
==========

to

==========
Add metrics for PasswordForm submission types

The Password Manager uses a bunch of different events as triggers for checking
whether a password form was submitted successfully. This CL adds UMA histograms
to record the event used as the trigger for each successful password form
submission.

BUG=None
==========

[engedy, 2017-05-09 18:25:02]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 18:26:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 19:09:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:09:57]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Ilya Sherman, 2017-05-09 21:35:35]

metrics lgtm

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

It's been a while, so please take my comments with a grain of salt. :-)

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1038: void
PasswordAutofillAgent::OnSameDocumentNavigationCompleted(
nit: Now this sounds like: OnBanana(bool is_banana). WDYT about renaming this to
something like OnMaybeSubmittedFormWithoutChangingDocument?

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1051: if
(form_util::IsFormVisible(frame, provisionally_saved_form_.form_element(),
It seems to me that this is called on *every* successful XHR in the document
once the user has typed anything into the password field.

Given that this can lead to CSS style recalc and layout tree update, I wonder if
we should at least count how many times this check is is performed (and
evaluates to false).

We could also add some TRACE_EVENTs and do some local testing to see if this
leads to any premature (and potentially expensive) style recalcs?

On the other hand, I don't know what else we could do here. It seems reasonable
that we cannot stop after the first XHR -- maybe Jochen's change to detect
disappearing forms, and optionally AND'ing that signal with having had an XHR or
same document navigation, could solve this.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1062:
GetPasswordManagerDriver()->InPageNavigation(password_form);
nit: Then this could be updated to
PasswordFormSuccessfullySubmitted(InPage|WithoutChangingDocument|WithoutNavigation),
to better reflect that we have already made the authoritative decision about it
all being a success.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:89: enum class SubmissionEvent {
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.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:294: SubmissionEvent
submission_event;
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 :-/.

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:717: if
(provisional_save_manager_->submitted_form())
style nit: {}

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:718:
metrics_util::LogPasswordSuccessfulSubmissionEvent(
optional nit: Do you think it would make sense to log this to
chrome://pm-internals?

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:27099: +  <int value="2" label="InPage
navigation"/>
nit: Same document navigation, which is the `cool` term nowadays.

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:49640: +    The type of event which
Password Manager used for detecting a successful
wording nit: s/which/that the/

[engedy, 2017-05-09 22:03:51]

engedy@chromium.org changed reviewers:
+ mathp@chromium.org

[engedy, 2017-05-09 22:03:51]

@Mathieu, would you be the right person to review components/autofill changes?

[engedy, 2017-05-09 22:06:25]

engedy@chromium.org changed reviewers:
- isherman@chromium.org

[engedy, 2017-05-09 22:07:03]

On 2017/05/09 22:03:51, engedy wrote:
> @Mathieu, would you be the right person to review components/autofill changes?

Ahh, nevermind me. I am only seeing the per-file OWNER rules just now.

[Mike West, 2017-05-10 06:52:28]

mkwst@chromium.org changed reviewers:
+ isherman@chromium.org

[Mike West, 2017-05-10 06:52:28]

mojom looks reasonable; ping me when the rest of the patch is finished, just so
I don't approve something that shifts out from under me? :)

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

engedy, PTAL

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1038: void
PasswordAutofillAgent::OnSameDocumentNavigationCompleted(
On 2017/05/09 22:02:58, engedy wrote:
> nit: Now this sounds like: OnBanana(bool is_banana). WDYT about renaming this
to
> something like OnMaybeSubmittedFormWithoutChangingDocument?

Absolutely agree. But I'd prefer to use this CL just for adding metrics. I'll
rename it in another CL.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1051: if
(form_util::IsFormVisible(frame, provisionally_saved_form_.form_element(),
On 2017/05/09 22:02:59, engedy wrote:
> It seems to me that this is called on *every* successful XHR in the document
> once the user has typed anything into the password field.
> 
> Given that this can lead to CSS style recalc and layout tree update, I wonder
if
> we should at least count how many times this check is is performed (and
> evaluates to false).
> 
> We could also add some TRACE_EVENTs and do some local testing to see if this
> leads to any premature (and potentially expensive) style recalcs?
> 
> On the other hand, I don't know what else we could do here. It seems
reasonable
> that we cannot stop after the first XHR -- maybe Jochen's change to detect
> disappearing forms, and optionally AND'ing that signal with having had an XHR
or
> same document navigation, could solve this.

Yeah, for me it also looks strange. It was implemented a long time ago for
fixing some specific cases and it's completely non-clear how it's useful. The
metric from this CL will answer at least whether this useful or not.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1062:
GetPasswordManagerDriver()->InPageNavigation(password_form);
On 2017/05/09 22:02:59, engedy wrote:
> nit: Then this could be updated to
>
PasswordFormSuccessfullySubmitted(InPage|WithoutChangingDocument|WithoutNavigation),
> to better reflect that we have already made the authoritative decision about
it
> all being a success.

Absolutely agree. But I'd prefere to use this CL just for adding metrics. I'll
rename it in another CL.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:89: enum class SubmissionEvent {
On 2017/05/09 22:02:59, engedy wrote:
> 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.

Thanks, it makes sense. Renamed to SubmissionIndicatorEvent.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:294: SubmissionEvent
submission_event;
On 2017/05/09 22:02:59, engedy wrote:
> 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 :-/.

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 :).

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:717: if
(provisional_save_manager_->submitted_form())
On 2017/05/09 22:02:59, engedy wrote:
> style nit: {}

Done.

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:718:
metrics_util::LogPasswordSuccessfulSubmissionEvent(
On 2017/05/09 22:02:59, engedy wrote:
> optional nit: Do you think it would make sense to log this to
> chrome://pm-internals?

Sure, I'll do this in separate CL.

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:27099: +  <int value="2" label="InPage
navigation"/>
On 2017/05/09 22:02:59, engedy wrote:
> nit: Same document navigation, which is the `cool` term nowadays.

Done.

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2859243004/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:49640: +    The type of event which
Password Manager used for detecting a successful
On 2017/05/09 22:02:59, engedy wrote:
> wording nit: s/which/that the/

Done.

[dvadym, 2017-05-10 12:10:26]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 12:11:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-10 13:07:43]

LGTM % nits.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1038: void
PasswordAutofillAgent::OnSameDocumentNavigationCompleted(
On 2017/05/10 12:10:12, dvadym wrote:
> On 2017/05/09 22:02:58, engedy wrote:
> > nit: Now this sounds like: OnBanana(bool is_banana). WDYT about renaming
this
> to
> > something like OnMaybeSubmittedFormWithoutChangingDocument?
> 
> Absolutely agree. But I'd prefer to use this CL just for adding metrics. I'll
> rename it in another CL. 

Acknowledged.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1051: if
(form_util::IsFormVisible(frame, provisionally_saved_form_.form_element(),
On 2017/05/10 12:10:12, dvadym wrote:
> On 2017/05/09 22:02:59, engedy wrote:
> > It seems to me that this is called on *every* successful XHR in the document
> > once the user has typed anything into the password field.
> > 
> > Given that this can lead to CSS style recalc and layout tree update, I
wonder
> if
> > we should at least count how many times this check is is performed (and
> > evaluates to false).
> > 
> > We could also add some TRACE_EVENTs and do some local testing to see if this
> > leads to any premature (and potentially expensive) style recalcs?
> > 
> > On the other hand, I don't know what else we could do here. It seems
> reasonable
> > that we cannot stop after the first XHR -- maybe Jochen's change to detect
> > disappearing forms, and optionally AND'ing that signal with having had an
XHR
> or
> > same document navigation, could solve this.
> 
> Yeah, for me it also looks strange. It was implemented a long time ago for
> fixing some specific cases and it's completely non-clear how it's useful. The
> metric from this CL will answer at least whether this useful or not.

Can we file a tracking bug for these metrics and mention this as a follow-up
task either there, or on a separate bug?

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1062:
GetPasswordManagerDriver()->InPageNavigation(password_form);
On 2017/05/10 12:10:11, dvadym wrote:
> On 2017/05/09 22:02:59, engedy wrote:
> > nit: Then this could be updated to
> >
>
PasswordFormSuccessfullySubmitted(InPage|WithoutChangingDocument|WithoutNavigation),
> > to better reflect that we have already made the authoritative decision about
> it
> > all being a success.
> 
> Absolutely agree. But I'd prefere to use this CL just for adding metrics. I'll
> rename it in another CL. 

Acknowledged.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:89: enum class SubmissionEvent {
On 2017/05/10 12:10:12, dvadym wrote:
> On 2017/05/09 22:02:59, engedy wrote:
> > 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.
> 
> Thanks, it makes sense. Renamed to SubmissionIndicatorEvent.

Acknowledged.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:294: SubmissionEvent
submission_event;
On 2017/05/10 12:10:12, dvadym wrote:
> On 2017/05/09 22:02:59, engedy wrote:
> > 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 :-/.
> 
> 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 :).

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

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:718:
metrics_util::LogPasswordSuccessfulSubmissionEvent(
On 2017/05/10 12:10:12, dvadym wrote:
> On 2017/05/09 22:02:59, engedy wrote:
> > optional nit: Do you think it would make sense to log this to
> > chrome://pm-internals?
> 
> Sure, I'll do this in separate CL. 

Sounds good to me. +1 for tracking bug.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_types_struct_traits.cc (right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_types_struct_traits.cc:246:
mojom::PasswordFormSubmissionIndicatorEvent
ad-hoc musings: Can you please reassure me that this is generated? :)

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/content/renderer/provisionally_saved_password_form.h
(right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/content/renderer/provisionally_saved_password_form.h:38:
const PasswordForm& password_form() const {
nit: What's the reason this is const? A comment would be nice here to highlight
why we need the workaround (and why modifying this ultimately is acceptable).

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:87: // Events that Password
Manager uses as triggers for checking a success of a
Phrasing nit:

// Events observed by the Password Manager that indicate either that a form is
potentially being submitted, or that a form has already been successfully
submitted. Recorded into a UMA histogram, so order of enumerators should not be
changed.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:92: INPAGE_NAVIGATION,
nit: SAME_DOCUMENT_NAVIGATION for consistency

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:95: BY_CONTEXT_MENU_CLICK,
nit: This sounds like an implementation detail. Have you considered naming this
MANUAL_SAVE_OVERRIDE?

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:96: SUBMISSION_EVENT_COUNT
nit: SUBMISSION_INDICATOR_EVENT_COUNT

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:293: // Type of event which was
used for this form submission.
Phrasing nit: 

// The type of the event that was taken as an indication that this form is being
or has already been submitted. This field is not persisted and filled out only
for submitted forms.

https://codereview.chromium.org/2859243004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2859243004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49662: +    The type of event that the
Password Manager used for detecting a successful
nit: Mention when this is recorded (i.e. for each successful form submission).

[commit-bot: I haz the power, 2017-05-10 13:19:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 13:19:18]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-05-10 14:09:26]

Thanks engedy for review!

I've created 2 tracking bugs:
crbug.com/720346 for renaming methods and RPC.
crbug.com/720347 for adding performance metrics.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1051: if
(form_util::IsFormVisible(frame, provisionally_saved_form_.form_element(),
On 2017/05/10 13:07:42, engedy wrote:
> On 2017/05/10 12:10:12, dvadym wrote:
> > On 2017/05/09 22:02:59, engedy wrote:
> > > It seems to me that this is called on *every* successful XHR in the
document
> > > once the user has typed anything into the password field.
> > > 
> > > Given that this can lead to CSS style recalc and layout tree update, I
> wonder
> > if
> > > we should at least count how many times this check is is performed (and
> > > evaluates to false).
> > > 
> > > We could also add some TRACE_EVENTs and do some local testing to see if
this
> > > leads to any premature (and potentially expensive) style recalcs?
> > > 
> > > On the other hand, I don't know what else we could do here. It seems
> > reasonable
> > > that we cannot stop after the first XHR -- maybe Jochen's change to detect
> > > disappearing forms, and optionally AND'ing that signal with having had an
> XHR
> > or
> > > same document navigation, could solve this.
> > 
> > Yeah, for me it also looks strange. It was implemented a long time ago for
> > fixing some specific cases and it's completely non-clear how it's useful.
The
> > metric from this CL will answer at least whether this useful or not.
> 
> Can we file a tracking bug for these metrics and mention this as a follow-up
> task either there, or on a separate bug? 

Done.

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/80001/components/autofill/cor...
components/autofill/core/common/password_form.h:294: SubmissionEvent
submission_event;
On 2017/05/10 13:07:42, engedy wrote:
> On 2017/05/10 12:10:12, dvadym wrote:
> > On 2017/05/09 22:02:59, engedy wrote:
> > > 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 :-/.
> > 
> > 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 :).
> 
> Fair enough, and glad to hear that this abomination is ultimately going away.
> ;-)

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

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:718:
metrics_util::LogPasswordSuccessfulSubmissionEvent(
On 2017/05/10 13:07:42, engedy wrote:
> On 2017/05/10 12:10:12, dvadym wrote:
> > On 2017/05/09 22:02:59, engedy wrote:
> > > optional nit: Do you think it would make sense to log this to
> > > chrome://pm-internals?
> > 
> > Sure, I'll do this in separate CL. 
> 
> Sounds good to me. +1 for tracking bug.

Even not sure whether we need tracking bug for such small thing. I'll do this in
a couple of working days.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_types_struct_traits.cc (right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_types_struct_traits.cc:246:
mojom::PasswordFormSubmissionIndicatorEvent
On 2017/05/10 13:07:42, engedy wrote:
> ad-hoc musings: Can you please reassure me that this is generated? :)

No, I did it manually :). It took ~ 2 mins, writing a script would take more.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/content/renderer/provisionally_saved_password_form.h
(right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/content/renderer/provisionally_saved_password_form.h:38:
const PasswordForm& password_form() const {
On 2017/05/10 13:07:42, engedy wrote:
> nit: What's the reason this is const? A comment would be nice here to
highlight
> why we need the workaround (and why modifying this ultimately is acceptable).

I think const was introduced in order to decrease number of places where
password_form is changed and to increase incapsulation. Of course we can remove
const. But my feeling that in this CL it's better to use a separate method to
SetSubmissionIndicatorEvent, otherwise we would need to write 
if (password_form_)
in all call places, that looks like too related on implementation of
ProvisionallySavedPasswordForm

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:87: // Events that Password
Manager uses as triggers for checking a success of a
On 2017/05/10 13:07:43, engedy wrote:
> Phrasing nit:
> 
> // Events observed by the Password Manager that indicate either that a form is
> potentially being submitted, or that a form has already been successfully
> submitted. Recorded into a UMA histogram, so order of enumerators should not
be
> changed. 

Thanks, done

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:92: INPAGE_NAVIGATION,
On 2017/05/10 13:07:42, engedy wrote:
> nit: SAME_DOCUMENT_NAVIGATION for consistency

Done.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:95: BY_CONTEXT_MENU_CLICK,
On 2017/05/10 13:07:43, engedy wrote:
> nit: This sounds like an implementation detail. Have you considered naming
this
> MANUAL_SAVE_OVERRIDE?

Done.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:96: SUBMISSION_EVENT_COUNT
On 2017/05/10 13:07:42, engedy wrote:
> nit: SUBMISSION_INDICATOR_EVENT_COUNT

Done.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:293: // Type of event which was
used for this form submission.
On 2017/05/10 13:07:42, engedy wrote:
> Phrasing nit: 
> 
> // The type of the event that was taken as an indication that this form is
being
> or has already been submitted. This field is not persisted and filled out only
> for submitted forms.

Thanks, done

https://codereview.chromium.org/2859243004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2859243004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49662: +    The type of event that the
Password Manager used for detecting a successful
On 2017/05/10 13:07:43, engedy wrote:
> nit: Mention when this is recorded (i.e. for each successful form submission).

Done.

[dvadym, 2017-05-10 14:12:30]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 14:12:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-10 14:15:57]

Still LGTM.

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2859243004/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:718:
metrics_util::LogPasswordSuccessfulSubmissionEvent(
On 2017/05/10 14:09:25, dvadym wrote:
> On 2017/05/10 13:07:42, engedy wrote:
> > On 2017/05/10 12:10:12, dvadym wrote:
> > > On 2017/05/09 22:02:59, engedy wrote:
> > > > optional nit: Do you think it would make sense to log this to
> > > > chrome://pm-internals?
> > > 
> > > Sure, I'll do this in separate CL. 
> > 
> > Sounds good to me. +1 for tracking bug.
> 
> Even not sure whether we need tracking bug for such small thing. I'll do this
in
> a couple of working days.

Acknowledged.

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
File components/autofill/content/renderer/provisionally_saved_password_form.h
(right):

https://codereview.chromium.org/2859243004/diff/120001/components/autofill/co...
components/autofill/content/renderer/provisionally_saved_password_form.h:38:
const PasswordForm& password_form() const {
On 2017/05/10 14:09:25, dvadym wrote:
> On 2017/05/10 13:07:42, engedy wrote:
> > nit: What's the reason this is const? A comment would be nice here to
> highlight
> > why we need the workaround (and why modifying this ultimately is
acceptable).
> 
> I think const was introduced in order to decrease number of places where
> password_form is changed and to increase incapsulation. Of course we can
remove
> const. But my feeling that in this CL it's better to use a separate method to
> SetSubmissionIndicatorEvent, otherwise we would need to write 
> if (password_form_)
> in all call places, that looks like too related on implementation of
> ProvisionallySavedPasswordForm 

Acknowledged.

[Mike West, 2017-05-10 14:59:43]

mojo LGTM, thanks.

[dvadym, 2017-05-10 15:00:17]

The CQ bit was unchecked by dvadym@chromium.org

[dvadym, 2017-05-10 15:00:23]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2017-05-10 15:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2859243004/#ps140001
(title: "addressed reviewer comments")

[commit-bot: I haz the power, 2017-05-10 15:00:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 15:45:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494428423752660,
"parent_rev": "02b01701047edf186f7d4164af8bcc6a0c78594f", "commit_rev":
"3ea544d4e19eff2b81e4a0f9d434a597203e2089"}

[commit-bot: I haz the power, 2017-05-10 15:45:58]

Description was changed from

==========
Add metrics for PasswordForm submission types

The Password Manager uses a bunch of different events as triggers for checking
whether a password form was submitted successfully. This CL adds UMA histograms
to record the event used as the trigger for each successful password form
submission.

BUG=None
==========

to

==========
Add metrics for PasswordForm submission types

The Password Manager uses a bunch of different events as triggers for checking
whether a password form was submitted successfully. This CL adds UMA histograms
to record the event used as the trigger for each successful password form
submission.

BUG=None

Review-Url: https://codereview.chromium.org/2859243004
Cr-Commit-Position: refs/heads/master@{#470584}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ea544d4e19eff2b81e4a0f9d434...
==========

[commit-bot: I haz the power, 2017-05-10 15:46:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3ea544d4e19eff2b81e4a0f9d434...