Add a histogram for each time the HTTP Bad UI is shown

chrome/browser/ssl/chrome_security_state_model_client_unittest.cc

 // Copyright 2016 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.
 
 #include "chrome/browser/ssl/chrome_security_state_model_client.h"
 
+#include "base/command_line.h"
+#include "base/test/histogram_tester.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/security_state/security_state_model.h"
+#include "components/security_state/switches.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/security_style_explanation.h"
 #include "content/public/browser/security_style_explanations.h"
+#include "content/public/browser/ssl_status.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 // Tests that SecurityInfo flags for subresources with certificate
 // errors are reflected in the SecurityStyleExplanations produced by
 // ChromeSecurityStateModelClient.
@@ skipping 215 matching lines @@
   content::SecurityStyleExplanations explanations;
   security_info.security_level = security_state::SecurityStateModel::NONE;
   security_info.displayed_private_user_data_input_on_http = true;
   blink::WebSecurityStyle security_style =
       ChromeSecurityStateModelClient::GetSecurityStyle(security_info,
                                                        &explanations);
   EXPECT_EQ(blink::WebSecurityStyleUnauthenticated, security_style);
   EXPECT_EQ(1u, explanations.info_explanations.size());
 }
 
+class ChromeSecurityStateModelClientHistogramTest
+    : public ChromeRenderViewHostTestHarness {
+ public:
+  ChromeSecurityStateModelClientHistogramTest() {}
+  ~ChromeSecurityStateModelClientHistogramTest() override {}
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+
+    ChromeSecurityStateModelClient::CreateForWebContents(web_contents());
+    client = ChromeSecurityStateModelClient::FromWebContents(web_contents());
+    navigateToHTTP();
+  }
+
+ protected:
+  void signalPassword() {

[estark, 2016/10/26 20:30:56]

style: should be signal_password

[elawrence, 2016/10/26 21:02:39]

Done.

+    web_contents()->OnPasswordInputShownOnHttp();
+    client->VisibleSSLStateChanged();
+  }
+
+  void navigateToHTTP() { NavigateAndCommit(GURL("http://example.test")); }

[estark, 2016/10/26 20:30:56]

style: should be navigate_to_http

[elawrence, 2016/10/26 21:02:39]

Done.

+
+  ChromeSecurityStateModelClient* client;

[estark, 2016/10/26 20:30:56]

style: should be client_

Also non-private data members are technically style guide violations, though
you'll occasionally see tests with protected members. It really should be
private with a protected client() accessor method.

[elawrence, 2016/10/26 21:02:39]

Done.

+  const char* kHTTPBadHistogram =

[estark, 2016/10/26 20:30:56]

No need for this to be a class member, I'd just put it in the anonymous
namespace at the top of the file (line 22)

[elawrence, 2016/10/26 21:02:39]

Done.

+      "Security.HTTPBad.UserWarnedAboutSensitiveInput";
+};
+
+// Tests that UMA logs the omnibox warning when security level is
+// HTTP_SHOW_WARNING.
+TEST_F(ChromeSecurityStateModelClientHistogramTest,
+       HTTPOmniboxWarningHistogram) {
+  // Show Warning Chip

[estark, 2016/10/26 20:30:56]

nit: end comments with periods, here and below. It's in the style guide:
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G....
which I reference only to prove that it's not my own personal pedantry :P

[elawrence, 2016/10/26 21:02:39]

Done.

+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpWithPasswordsOrCcWithChip);
+
+  base::HistogramTester histograms;
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 1);

[estark, 2016/10/26 20:30:56]

I think `true` instead of `1` for the second arg would be clearer. (1 made me
think it's an enum value.)

[elawrence, 2016/10/26 21:02:39]

Done.

+
+  // Fire again and ensure no sample is recorded
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 1);
+
+  // Navigate to a new page and ensure a sample is recorded
+  navigateToHTTP();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 1);
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 2);
+}
+
+// Tests that UMA logs the console warning when security level is NONE.
+TEST_F(ChromeSecurityStateModelClientHistogramTest,
+       HTTPConsoleWarningHistogram) {
+  // Show Neutral for HTTP
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpAsNeutral);
+
+  base::HistogramTester histograms;
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 1);

[estark, 2016/10/26 20:30:56]

same comment about false instead of 0

[elawrence, 2016/10/26 21:02:39]

Done.

+
+  // Fire again and ensure no sample is recorded
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 1);
+
+  // Navigate to a new page and ensure a sample is recorded
+  navigateToHTTP();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 1);
+  signalPassword();
+  histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 2);
+}
+
 }  // namespace