Add HTTP-specific metrics for password forms

Add HTTP-specific metrics for password forms

These metrics will help us track the effect of the Form-Not-Secure warnings
(which show a "Login not secure" message in the autofill dropdown on HTTP
pages).

BUG=677291
Review-Url: https://codereview.chromium.org/2667983006
Cr-Commit-Position: refs/heads/master@{#447867}
Committed: https://chromium.googlesource.com/chromium/src/+/0cd28be01ef0b1f4a51d2806c4601264a53d4c53

[estark, 2017-02-01 00:40:50]

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

[estark, 2017-02-01 00:41:50]

estark@chromium.org changed reviewers:
+ engedy@chromium.org, holte@chromium.org

[estark, 2017-02-01 00:41:50]

+engedy for components/password_manager
+holte for histograms.xml
Thanks!

[commit-bot: I haz the power, 2017-02-01 00:42:03]

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-02-01 01:49:47]

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

[commit-bot: I haz the power, 2017-02-01 01:49:48]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-02-01 10:32:54]

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.cc:250: if
(!observed_form_.origin.SchemeIsCryptographic()) {
I am somewhat lacking context here, so wanted to check the following.

In another CL [1] that I reviewed yesterday we were using
content::IsOriginSecure, and were checking the origin of the main frame
document.

Here we are using SchemeIsCryptographic() which is mostly similar but has
different behavior in some (mostly edge) cases, e.g. localhost, file:// schemes.
Furthermore, we are checking the origin of the frame hosting the form, which
might be different than the origin of the main frame document.

Provided there is no misunderstanding and the two CLs are indeed about the same
feature :-), could you please advise which is the desired behavior?

[1]:
https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...

[estark, 2017-02-01 19:16:07]

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

[commit-bot: I haz the power, 2017-02-01 19:16:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-02-01 19:24:10]

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

[commit-bot: I haz the power, 2017-02-01 19:24:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-02-01 19:26:25]

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.cc:250: if
(!observed_form_.origin.SchemeIsCryptographic()) {
On 2017/02/01 10:32:54, engedy wrote:
> I am somewhat lacking context here, so wanted to check the following.
> 
> In another CL [1] that I reviewed yesterday we were using
> content::IsOriginSecure, and were checking the origin of the main frame
> document.
> 
> Here we are using SchemeIsCryptographic() which is mostly similar but has
> different behavior in some (mostly edge) cases, e.g. localhost, file://
schemes.
> Furthermore, we are checking the origin of the frame hosting the form, which
> might be different than the origin of the main frame document.
> 
> Provided there is no misunderstanding and the two CLs are indeed about the
same
> feature :-), could you please advise which is the desired behavior?
> 
> [1]:
>
https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...

You're correct, these are the same feature. Thanks for catching this. The
main-frame vs current frame was an oversight on my part. I had somehow convinced
myself that using SchemeIsCryptographic here was okay to avoid introducing
another the //content dependency on IsOriginSecure, but now I can't figure out
why I thought that made sense. :)

So, I've now introduced a PasswordManagerClient method to check whether the main
frame origin is secure -- PTAL.

[commit-bot: I haz the power, 2017-02-01 21:21:18]

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

[commit-bot: I haz the power, 2017-02-01 21:21:19]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-02-02 01:39:00]

histograms lgtm

[engedy, 2017-02-02 12:26:55]

LGTM.

It would be really great to have some unit tests for these histograms. In the
past we have had some subtle bugs around here, leading to unreliable metrics,
causing sorrow and regret. But given that presently there are no such unit tests
whatsoever, this not a request, but merely a disclaimer. I filed
crbug.com/687932 to track this.

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2667983006/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.cc:250: if
(!observed_form_.origin.SchemeIsCryptographic()) {
On 2017/02/01 19:26:25, estark wrote:
> On 2017/02/01 10:32:54, engedy wrote:
> > I am somewhat lacking context here, so wanted to check the following.
> > 
> > In another CL [1] that I reviewed yesterday we were using
> > content::IsOriginSecure, and were checking the origin of the main frame
> > document.
> > 
> > Here we are using SchemeIsCryptographic() which is mostly similar but has
> > different behavior in some (mostly edge) cases, e.g. localhost, file://
> schemes.
> > Furthermore, we are checking the origin of the frame hosting the form, which
> > might be different than the origin of the main frame document.
> > 
> > Provided there is no misunderstanding and the two CLs are indeed about the
> same
> > feature :-), could you please advise which is the desired behavior?
> > 
> > [1]:
> >
>
https://codereview.chromium.org/2660643002/diff/1/components/autofill/content...
> 
> You're correct, these are the same feature. Thanks for catching this. The
> main-frame vs current frame was an oversight on my part. I had somehow
convinced
> myself that using SchemeIsCryptographic here was okay to avoid introducing
> another the //content dependency on IsOriginSecure, but now I can't figure out
> why I thought that made sense. :)
> 
> So, I've now introduced a PasswordManagerClient method to check whether the
main
> frame origin is secure -- PTAL.

Thank you for explaining, sounds good to me.

[estark, 2017-02-02 21:47:51]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2017-02-02 21:48:19]

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-02-02 22:58:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486072071053880,
"parent_rev": "f2d9e2d2a37375e9aba8f2e3f33e0ed7548b85ac", "commit_rev":
"0cd28be01ef0b1f4a51d2806c4601264a53d4c53"}

[commit-bot: I haz the power, 2017-02-02 22:58:36]

Description was changed from

==========
Add HTTP-specific metrics for password forms

These metrics will help us track the effect of the Form-Not-Secure warnings
(which show a "Login not secure" message in the autofill dropdown on HTTP
pages).

BUG=677291
==========

to

==========
Add HTTP-specific metrics for password forms

These metrics will help us track the effect of the Form-Not-Secure warnings
(which show a "Login not secure" message in the autofill dropdown on HTTP
pages).

BUG=677291

Review-Url: https://codereview.chromium.org/2667983006
Cr-Commit-Position: refs/heads/master@{#447867}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cd28be01ef0b1f4a51d2806c460...
==========

[commit-bot: I haz the power, 2017-02-02 22:58:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0cd28be01ef0b1f4a51d2806c460...