Add UMA to break down data usage by http/https and video/non_video

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
index 8e3a0d46236cfc3d2322ee37a4f9ab385344ed82..574bb1f76355d02390ec85547d8acb46ea8978dc 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
@@ -72,6 +72,7 @@ using TestNetworkDelegate = net::NetworkDelegateImpl;
 const char kOtherProxy[] = "testproxy:17";
 
 const char kTestURL[] = "http://www.google.com/";
+const char kSecureTestURL[] = "https://www.google.com/";
 
 #if defined(OS_ANDROID)
 const Client kClient = Client::CHROME_ANDROID;
@@ -180,6 +181,8 @@ class TestLoFiUIService : public LoFiUIService {
   bool on_lofi_response_;
 };
 
+enum ProxyTestConfig { USE_SECURE_PROXY, USE_INSECURE_PROXY, BYPASS_PROXY };
+
 class DataReductionProxyNetworkDelegateTest : public testing::Test {
  public:
   DataReductionProxyNetworkDelegateTest()
@@ -191,24 +194,35 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
         net::GetTestCertsDirectory(), "unittest.selfsigned.der");
   }
 
-  void Init(bool use_secure_proxy, bool enable_brotli_globally) {
-    net::ProxyServer proxy_server =
-        use_secure_proxy
-            ? net::ProxyServer::FromURI("https://origin.net:443",
-                                        net::ProxyServer::SCHEME_HTTPS)
-            : net::ProxyServer::FromURI("http://origin.net:80",
-                                        net::ProxyServer::SCHEME_HTTP);
-
+  void Init(ProxyTestConfig proxy_config, bool enable_brotli_globally) {
+    net::ProxyServer proxy_server;
+    switch (proxy_config) {
+      case BYPASS_PROXY:

[sclittle, 2017/04/11 20:36:11]

nit: instead of switching on an enum here, have you considered just passing in
the net::ProxyServer here, and having net::ProxyServer constants for the secure
and insecure kinds of proxies?

e.g.,

void Init(const net::ProxyServer& proxy_server, bool enable_brotli_globally) {
  // ... initializes using |proxy_server| ...
}

You could handle the BYPASS_PROXY check below by checking whether the
|proxy_server| is direct.

WDYT?

[ajo1, 2017/04/11 23:08:49]

I like this approach better, since I'd want to create a new proxy on each test
invocation, which makes having net::ProxyServer constants trickier(i.e. I'd be
replace USE_INSECURE_PROXY with GetInsecureProxy())

+        proxy_server = net::ProxyServer::Direct();
+        break;
+      case USE_SECURE_PROXY:
+        proxy_server = net::ProxyServer::FromURI(
+            "https://origin.net:443", net::ProxyServer::SCHEME_HTTPS);
+        break;
+      case USE_INSECURE_PROXY:
+        proxy_server = net::ProxyServer::FromURI("http://origin.net:80",
+                                                 net::ProxyServer::SCHEME_HTTP);
+        break;
+    }
     proxy_service_ =
         net::ProxyService::CreateFixedFromPacResult(proxy_server.ToPacString());
     context_.set_proxy_service(proxy_service_.get());
-    test_context_ = (DataReductionProxyTestContext::Builder()
-                         .WithClient(kClient)
-                         .WithMockClientSocketFactory(&mock_socket_factory_)
-                         .WithURLRequestContext(&context_)
-                         .WithProxiesForHttp({DataReductionProxyServer(
-                             proxy_server, ProxyServer::UNSPECIFIED_TYPE)})
-                         .Build());
+    DataReductionProxyTestContext::Builder builder;
+    builder = builder.WithClient(kClient)
+                  .WithMockClientSocketFactory(&mock_socket_factory_)
+                  .WithURLRequestContext(&context_);
+
+    if (proxy_config != BYPASS_PROXY) {
+      builder = builder.WithProxiesForHttp({DataReductionProxyServer(
+          proxy_server, ProxyServer::UNSPECIFIED_TYPE)});
+    }
+
+    test_context_ = builder.Build();
 
     context_.set_client_socket_factory(&mock_socket_factory_);
     test_context_->AttachToURLRequestContext(&context_storage_);
@@ -552,6 +566,10 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
     return &test_network_quality_estimator_;
   }
 
+  net::SSLSocketDataProvider* ssl_socket_data_provider() {
+    return &ssl_socket_data_provider_;
+  }
+
  private:
   base::MessageLoopForIO message_loop_;
   net::MockClientSocketFactory mock_socket_factory_;
@@ -573,7 +591,7 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
 };
 
 TEST_F(DataReductionProxyNetworkDelegateTest, AuthenticationTest) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   std::unique_ptr<net::URLRequest> fake_request(
       FetchURLRequest(GURL(kTestURL), nullptr, std::string(), 0, 0));
 
@@ -603,7 +621,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, AuthenticationTest) {
 }
 
 TEST_F(DataReductionProxyNetworkDelegateTest, LoFiTransitions) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   // Enable Lo-Fi.
   const struct {
     bool lofi_switch_enabled;
@@ -754,7 +772,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, LoFiTransitions) {
 }
 
 TEST_F(DataReductionProxyNetworkDelegateTest, RequestDataConfigurations) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   const struct {
     bool lofi_on;
     bool used_data_reduction_proxy;
@@ -830,7 +848,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, RequestDataConfigurations) {
 
 TEST_F(DataReductionProxyNetworkDelegateTest,
        RequestDataHoldbackConfigurations) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   const struct {
     bool data_reduction_proxy_enabled;
     bool used_direct;
@@ -877,7 +895,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
 }
 
 TEST_F(DataReductionProxyNetworkDelegateTest, RedirectRequestDataCleared) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   net::ProxyInfo data_reduction_proxy_info;
   std::string data_reduction_proxy;
   base::TrimString(params()->DefaultOrigin(), "/", &data_reduction_proxy);
@@ -944,7 +962,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, RedirectRequestDataCleared) {
 }
 
 TEST_F(DataReductionProxyNetworkDelegateTest, NetHistograms) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   const std::string kReceivedValidOCLHistogramName =
       "Net.HttpContentLengthWithValidOCL";
   const std::string kOriginalValidOCLHistogramName =
@@ -961,6 +979,11 @@ TEST_F(DataReductionProxyNetworkDelegateTest, NetHistograms) {
       "Net.HttpContentLengthDifferenceWithValidOCL.LoFiOn";
 
   const std::string kReceivedHistogramName = "Net.HttpContentLength";
+  const std::string kReceivedInsecureHistogramName =
+      "Net.HttpContentLength_Http";
+  const std::string kReceivedSecureHistogramName =
+      "Net.HttpContentLength_Https";

[sclittle, 2017/04/11 20:36:11]

nit: Assuming you split this test up (see below), these string constants seem
unnecessary, could you inline these strings instead to make it easier to see
which histogram checks refer to which histograms?

+  const std::string kReceivedVideoHistogramName = "Net.HttpContentLength_Video";
   const std::string kOriginalHistogramName = "Net.HttpOriginalContentLength";
   const std::string kDifferenceHistogramName =
       "Net.HttpContentLengthDifference";
@@ -1002,6 +1025,12 @@ TEST_F(DataReductionProxyNetworkDelegateTest, NetHistograms) {
       kOriginalContentLength - kResponseContentLength, 1);
   histogram_tester.ExpectUniqueSample(kReceivedHistogramName,
                                       kResponseContentLength, 1);
+  histogram_tester.ExpectUniqueSample(kReceivedInsecureHistogramName,
+                                      kResponseContentLength, 1);
+  histogram_tester.ExpectTotalCount(kReceivedSecureHistogramName, 0);
+  histogram_tester.ExpectTotalCount(kReceivedVideoHistogramName, 0);
+  histogram_tester.ExpectUniqueSample(kReceivedInsecureHistogramName,
+                                      kResponseContentLength, 1);
   histogram_tester.ExpectUniqueSample(kOriginalHistogramName,
                                       kOriginalContentLength, 1);
   histogram_tester.ExpectUniqueSample(
@@ -1016,6 +1045,24 @@ TEST_F(DataReductionProxyNetworkDelegateTest, NetHistograms) {
   histogram_tester.ExpectUniqueSample(kCacheable24HoursHistogramName,
                                       kResponseContentLength, 1);
 
+  // Check video
+  std::string video_response_headers =

[sclittle, 2017/04/11 20:36:11]

Could you split this video request into it's own test?

[ajo1, 2017/04/11 23:08:49]

Done.

+      "HTTP/1.1 200 OK\r\n"
+      "Date: Wed, 28 Nov 2007 09:40:09 GMT\r\n"
+      "Expires: Mon, 24 Nov 2014 12:45:26 GMT\r\n"
+      "Content-Type: video/mp4\r\n"
+      "Via: 1.1 Chrome-Compression-Proxy\r\n"
+      "x-original-content-length: " +
+      base::Int64ToString(kOriginalContentLength) + "\r\n\r\n";
+
+  FetchURLRequest(GURL(kTestURL), nullptr, video_response_headers,
+                  kResponseContentLength, 0);
+
+  histogram_tester.ExpectTotalCount(kReceivedInsecureHistogramName, 2);
+  histogram_tester.ExpectTotalCount(kReceivedSecureHistogramName, 0);
+  histogram_tester.ExpectUniqueSample(kReceivedVideoHistogramName,
+                                      kResponseContentLength, 1);
+
   // Check Lo-Fi histograms.
   const struct {

[sclittle, 2017/04/11 20:36:11]

nit: While you're at it, could you split these LoFi tests into their own test?

[ajo1, 2017/04/11 23:08:49]

I don't want to mix that into this change if I can avoid it.

     bool lofi_enabled_through_switch;
@@ -1092,8 +1139,40 @@ TEST_F(DataReductionProxyNetworkDelegateTest, NetHistograms) {
   }
 }
 
+TEST_F(DataReductionProxyNetworkDelegateTest, NetSSLHistograms) {
+  Init(BYPASS_PROXY, false);
+
+  const std::string kReceivedInsecureHistogramName =
+      "Net.HttpContentLength_Http";
+  const std::string kReceivedSecureHistogramName =
+      "Net.HttpContentLength_Https";
+  const std::string kReceivedVideoHistogramName = "Net.HttpContentLength_Video";
+  const int64_t kResponseContentLength = 100;
+  const int64_t kOriginalContentLength = 200;
+
+  base::HistogramTester histogram_tester;
+
+  // Check https
+  std::string secure_response_headers =
+      "HTTP/1.1 200 OK\r\n"
+      "Date: Wed, 28 Nov 2007 09:40:09 GMT\r\n"
+      "Expires: Mon, 24 Nov 2014 12:45:26 GMT\r\n"
+      "Via: 1.1 Chrome-Compression-Proxy\r\n"
+      "x-original-content-length: " +
+      base::Int64ToString(kOriginalContentLength) + "\r\n\r\n";
+
+  mock_socket_factory()->AddSSLSocketDataProvider(ssl_socket_data_provider());
+  FetchURLRequest(GURL(kSecureTestURL), nullptr, secure_response_headers,
+                  kResponseContentLength, 0);
+
+  histogram_tester.ExpectTotalCount(kReceivedInsecureHistogramName, 0);
+  histogram_tester.ExpectUniqueSample(kReceivedSecureHistogramName,
+                                      kResponseContentLength, 1);
+  histogram_tester.ExpectTotalCount(kReceivedVideoHistogramName, 0);
+}
+
 TEST_F(DataReductionProxyNetworkDelegateTest, OnCompletedInternalLoFi) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   // Enable Lo-Fi.
   const struct {
     bool lofi_response;
@@ -1121,7 +1200,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, OnCompletedInternalLoFi) {
 
 TEST_F(DataReductionProxyNetworkDelegateTest,
        TestLoFiTransformationTypeHistogram) {
-  Init(false, false);
+  Init(USE_INSECURE_PROXY, false);
   const char kLoFiTransformationTypeHistogram[] =
       "DataReductionProxy.LoFi.TransformationType";
   base::HistogramTester histogram_tester;
@@ -1152,7 +1231,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
 // disabled globally.
 TEST_F(DataReductionProxyNetworkDelegateTest,
        BrotliAdvertisement_BrotliDisabled) {
-  Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);
 
   ReadBrotliFile();
 
@@ -1174,7 +1253,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
 // is fetched from an insecure proxy.
 TEST_F(DataReductionProxyNetworkDelegateTest,
        BrotliAdvertisementInsecureProxy) {
-  Init(false /* use_secure_proxy */, true /* enable_brotli_globally */);
+  Init(USE_INSECURE_PROXY, true /* enable_brotli_globally */);
   std::string response_headers =
       "HTTP/1.1 200 OK\r\n"
       "Content-Length: 140\r\n"
@@ -1200,7 +1279,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
 // disabled via data reduction proxy field trial.
 TEST_F(DataReductionProxyNetworkDelegateTest,
        BrotliAdvertisementDisabledViaFieldTrial) {
-  Init(true /* use_secure_proxy */, true /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, true /* enable_brotli_globally */);
 
   base::FieldTrialList field_trial_list(nullptr);
   ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
@@ -1222,7 +1301,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
 // Test that Brotli is correctly added to the accept-encoding header when it is
 // enabled globally.
 TEST_F(DataReductionProxyNetworkDelegateTest, BrotliAdvertisement) {
-  Init(true /* use_secure_proxy */, true /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, true /* enable_brotli_globally */);
 
   std::string response_headers =
       "HTTP/1.1 200 OK\r\n"
@@ -1239,7 +1318,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, BrotliAdvertisement) {
 
 TEST_F(DataReductionProxyNetworkDelegateTest, IncrementingMainFramePageId) {
   // This is unaffacted by brotil and insecure proxy.
-  Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);
 
   io_data()->request_options()->SetSecureSession("new-session");
 
@@ -1252,7 +1331,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest, IncrementingMainFramePageId) {
 
 TEST_F(DataReductionProxyNetworkDelegateTest, ResetSessionResetsId) {
   // This is unaffacted by brotil and insecure proxy.
-  Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);
 
   io_data()->request_options()->SetSecureSession("new-session");
 
@@ -1265,14 +1344,14 @@ TEST_F(DataReductionProxyNetworkDelegateTest, ResetSessionResetsId) {
 
 TEST_F(DataReductionProxyNetworkDelegateTest, SubResourceNoPageId) {
   // This is unaffacted by brotil and insecure proxy.
-  Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);
   io_data()->request_options()->SetSecureSession("new-session");
   FetchURLRequestAndVerifyPageIdDirective(std::string(), false);
 }
 
 TEST_F(DataReductionProxyNetworkDelegateTest, RedirectSharePid) {
   // This is unaffacted by brotil and insecure proxy.
-  Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);
 
   io_data()->request_options()->SetSecureSession("new-session");
 
@@ -1285,7 +1364,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
   // state changing in between redirects within an URLRequest's lifetime.
 
   // This is unaffacted by brotil and insecure proxy.
-  Init(false /* use_secure_proxy */, false /* enable_brotli_globally */);
+  Init(USE_INSECURE_PROXY, false /* enable_brotli_globally */);
   net::ProxyInfo data_reduction_proxy_info;
   std::string data_reduction_proxy;
   base::TrimString(params()->DefaultOrigin(), "/", &data_reduction_proxy);