Record UMA histogram when Form-Not-Secure warning is shown

Record UMA histogram when Form-Not-Secure warning is shown

This records a histogram, at most once per main-frame navigation, when "Login
not secure" is shown in a password form. This will allow us to measure the
frequency of the warning (e.g. the number of times the warning is shown per
million navigations.)

BUG=687823
Review-Url: https://codereview.chromium.org/2670733002
Cr-Commit-Position: refs/heads/master@{#447960}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9ab459b6cce4f6d327b83ef49a8684a8839b9b

[estark, 2017-02-02 03:08:10]

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

[commit-bot: I haz the power, 2017-02-02 03:08:32]

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-02 03:09:19]

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

[estark, 2017-02-02 03:09:20]

One more Form-Not-Secure metric...
holte for histograms.xml
engedy for the rest
Thanks! This will be the last one, I promise :)

[commit-bot: I haz the power, 2017-02-02 06:17:04]

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

[commit-bot: I haz the power, 2017-02-02 06:17:05]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-02-02 13:13:34]

LGTM % nits, thanks!

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

https://codereview.chromium.org/2670733002/diff/1/components/password_manager...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:809:
histograms.ExpectBucketCount(kHistogram, true, 1);
nit: ExpectUniqueSample to both ensure and make it clear to readers that no
other buckets should have values (same below in two place).

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

https://codereview.chromium.org/2670733002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_metrics_util.cc:153:
UMA_HISTOGRAM_BOOLEAN(
Just checking: it is intentional that we never record `false` values here,
because we are going to use some other histogram as a baseline corresponding to
100%, right?

[engedy, 2017-02-02 13:13:55]

Also thank you for the unit test. :)

[estark, 2017-02-02 21:59:36]

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

[commit-bot: I haz the power, 2017-02-02 22:00:04]

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

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

https://codereview.chromium.org/2670733002/diff/1/components/password_manager...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:809:
histograms.ExpectBucketCount(kHistogram, true, 1);
On 2017/02/02 13:13:34, engedy wrote:
> nit: ExpectUniqueSample to both ensure and make it clear to readers that no
> other buckets should have values (same below in two place).

Done.

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

https://codereview.chromium.org/2670733002/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager_metrics_util.cc:153:
UMA_HISTOGRAM_BOOLEAN(
On 2017/02/02 13:13:34, engedy wrote:
> Just checking: it is intentional that we never record `false` values here,
> because we are going to use some other histogram as a baseline corresponding
to
> 100%, right?

Right. I'm interested in metrics such as the # of times we show the warning per
million page loads. I'll add a comment to clarify that.

[commit-bot: I haz the power, 2017-02-02 23:47:43]

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

[commit-bot: I haz the power, 2017-02-02 23:47:44]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-02-03 02:59:59]

histograms lgtm

[estark, 2017-02-03 03:02:31]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-03 03:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2670733002/#ps20001
(title: "engedy comments")

[commit-bot: I haz the power, 2017-02-03 03:03:35]

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-03 03:49:48]

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

[commit-bot: I haz the power, 2017-02-03 03:49:50]

Failed to apply patch for
components/password_manager/core/browser/password_autofill_manager_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
components/password_manager/core/browser/password_autofill_manager_unittest.cc:9
  error:
components/password_manager/core/browser/password_autofill_manager_unittest.cc:
patch does not apply

Patch:      
components/password_manager/core/browser/password_autofill_manager_unittest.cc
Index:
components/password_manager/core/browser/password_autofill_manager_unittest.cc
diff --git
a/components/password_manager/core/browser/password_autofill_manager_unittest.cc
b/components/password_manager/core/browser/password_autofill_manager_unittest.cc
index
713b83cd24bf89bc48060ee0d689bb016e0a4777..313b4797d1bd605949103b36c7939eb06501b46e
100644
---
a/components/password_manager/core/browser/password_autofill_manager_unittest.cc
+++
b/components/password_manager/core/browser/password_autofill_manager_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/suggestion_test_helpers.h"
@@ -788,4 +789,47 @@ TEST_F(PasswordAutofillManagerTest,
SecurePasswordFieldHttpWarningMessage) {
       autofill::IS_PASSWORD_FIELD, element_bounds);
 }
 
+// Tests that the Form-Not-Secure warning is recorded in UMA, at most once per
+// navigation.
+TEST_F(PasswordAutofillManagerTest, ShowedFormNotSecureHistogram) {
+  const char kHistogram[] =
+      "PasswordManager.ShowedFormNotSecureWarningOnCurrentNavigation";
+  base::HistogramTester histograms;
+  SetHttpWarningEnabled();
+
+  auto client = base::MakeUnique<TestPasswordManagerClient>();
+  auto autofill_client = base::MakeUnique<MockAutofillClient>();
+  InitializePasswordAutofillManager(client.get(), autofill_client.get());
+
+  // Test that the standalone warning (with no autofill suggestions) records
the
+  // histogram.
+  gfx::RectF element_bounds;
+  password_autofill_manager_->OnShowNotSecureWarning(base::i18n::RIGHT_TO_LEFT,
+                                                     element_bounds);
+  histograms.ExpectUniqueSample(kHistogram, true, 1);
+
+  // Simulate a navigation, because the histogram is recorded at most once per
+  // navigation.
+  password_autofill_manager_->DidNavigateMainFrame();
+
+  // Test that the warning, when included with the normal autofill dropdown,
+  // records the histogram.
+  int dummy_key = 0;
+  autofill::PasswordFormFillData data;
+  data.username_field.value = test_username_;
+  data.password_field.value = test_password_;
+  data.origin = GURL("http://foo.test");
+  password_autofill_manager_->OnAddPasswordFormMapping(dummy_key, data);
+
+  password_autofill_manager_->OnShowPasswordSuggestions(
+      dummy_key, base::i18n::RIGHT_TO_LEFT, test_username_,
+      autofill::IS_PASSWORD_FIELD, element_bounds);
+  histograms.ExpectUniqueSample(kHistogram, true, 2);
+
+  // The histogram should not be recorded again on the same navigation.
+  password_autofill_manager_->OnShowNotSecureWarning(base::i18n::RIGHT_TO_LEFT,
+                                                     element_bounds);
+  histograms.ExpectUniqueSample(kHistogram, true, 2);
+}
+
 }  // namespace password_manager

[estark, 2017-02-03 06:43:51]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-03 06:43:51]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2670733002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-03 06:47:16]

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-03 07:43:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486104231035720,
"parent_rev": "d6e50d4b7dec6e8aff173a1fd567758973b65130", "commit_rev":
"ce9ab459b6cce4f6d327b83ef49a8684a8839b9b"}

[commit-bot: I haz the power, 2017-02-03 07:44:06]

Description was changed from

==========
Record UMA histogram when Form-Not-Secure warning is shown

This records a histogram, at most once per main-frame navigation, when "Login
not secure" is shown in a password form. This will allow us to measure the
frequency of the warning (e.g. the number of times the warning is shown per
million navigations.)

BUG=687823
==========

to

==========
Record UMA histogram when Form-Not-Secure warning is shown

This records a histogram, at most once per main-frame navigation, when "Login
not secure" is shown in a password form. This will allow us to measure the
frequency of the warning (e.g. the number of times the warning is shown per
million navigations.)

BUG=687823

Review-Url: https://codereview.chromium.org/2670733002
Cr-Commit-Position: refs/heads/master@{#447960}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce9ab459b6cce4f6d327b83ef49a...
==========

[commit-bot: I haz the power, 2017-02-03 07:44:07]

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