Adding DRP specfic UMA for FirstContentfulPaint

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc

Index: chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
diff --git a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
index 1026d5c4169b1513260c8943de3b81359d5debdb..60bb54df45b9ed2c8f7db34337158dce8ffffc41 100644
--- a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
+++ b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
@@ -1,33 +1,42 @@
 // Copyright 2014 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/renderer_host/chrome_resource_dispatcher_host_delegate.h"
 
 #include <stddef.h>
+
+#include <memory>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/cloud/policy_header_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/renderer_host/chrome_navigation_data.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/policy_header_io_helper.h"
 #include "components/policy/core/common/cloud/policy_header_service.h"
 #include "components/policy/core/common/policy_switches.h"
+#include "content/public/browser/navigation_data.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/resource_dispatcher_host.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/browser_test_utils.h"
 #include "net/http/http_request_headers.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "net/url_request/url_request.h"
 
 using content::ResourceType;
 
 namespace {
 static const char kTestPolicyHeader[] = "test_header";
@@ -51,21 +60,21 @@ std::unique_ptr<net::test_server::HttpResponse> HandleTestRequest(
     std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
         new net::test_server::BasicHttpResponse);
     http_response->set_code(net::HTTP_OK);
     http_response->set_content("Success");
     return std::move(http_response);
   }
 }
 
 class TestDispatcherHostDelegate : public ChromeResourceDispatcherHostDelegate {
  public:
-  TestDispatcherHostDelegate() {}
+  TestDispatcherHostDelegate() : add_data_reduction_proxy_data_(false) {}
   ~TestDispatcherHostDelegate() override {}
 
   void RequestBeginning(
       net::URLRequest* request,
       content::ResourceContext* resource_context,
       content::AppCacheService* appcache_service,
       ResourceType resource_type,
       ScopedVector<content::ResourceThrottle>* throttles) override {
     ChromeResourceDispatcherHostDelegate::RequestBeginning(
         request,
@@ -81,26 +90,77 @@ class TestDispatcherHostDelegate : public ChromeResourceDispatcherHostDelegate {
                            content::ResourceContext* resource_context,
                            content::ResourceResponse* response) override {
     ChromeResourceDispatcherHostDelegate::OnRequestRedirected(
         redirect_url,
         request,
         resource_context,
         response);
     request_headers_.MergeFrom(request->extra_request_headers());
   }
 
+  std::unique_ptr<content::NavigationData> GetNavigationData(
+      net::URLRequest* request) const override {
+    if (add_data_reduction_proxy_data_) {
+      data_reduction_proxy::DataReductionProxyData* data =
+          data_reduction_proxy::DataReductionProxyData::DataForRequest(request,
+                                                                       true);
+      data->set_used_data_reduction_proxy(true);
+    }
+    return ChromeResourceDispatcherHostDelegate::GetNavigationData(request);
+  }
+
+  bool add_data_reduction_proxy_data_;

[bengr, 2016/04/29 21:14:21]

Ideally, you'd rename as should_add_data_reduction_proxy_data_to_request_;
should_add_data_reduction_proxy_data_ is ok too. Also do this for line 151.

[RyanSturm, 2016/05/02 19:52:19]

Done.

   net::HttpRequestHeaders request_headers_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TestDispatcherHostDelegate);
 };
 
+// Helper class to track DidFinishNavigation and it verify NavigationData being

[bengr, 2016/04/29 21:14:21]

"and it verify" ?

[RyanSturm, 2016/05/02 19:52:20]

Done.

+// added to NavigationHandle and pause/resume execution of the test.
+class DidFinishNavigationObserver : public content::WebContentsObserver {
+ public:
+  explicit DidFinishNavigationObserver(content::WebContents* web_contents,
+                                       bool add_data_reduction_proxy_data)
+      : content::WebContentsObserver(web_contents),
+        add_data_reduction_proxy_data_(add_data_reduction_proxy_data),
+        message_loop_runner_(new content::MessageLoopRunner) {}
+  ~DidFinishNavigationObserver() override {}
+
+  // Runs a nested message loop and blocks until the full load has
+  // completed.
+  void Wait() { message_loop_runner_->Run(); }
+
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    ChromeNavigationData* data = static_cast<ChromeNavigationData*>(
+        navigation_handle->GetNavigationData());
+    if (!add_data_reduction_proxy_data_) {
+      EXPECT_TRUE(!data->GetDataReductionProxyData());

[bengr, 2016/04/29 21:14:21]

I would flip this around:
EXPECT_FALSE(data->GetDataREductionProxyData());

and the next would would be EXPECT_TRUE.

Alternatively, you could explicitly check that it is equal to nullptr and not
nullptr in line 140.

[RyanSturm, 2016/05/02 19:52:19]

Done.

+    } else {
+      EXPECT_FALSE(!data->GetDataReductionProxyData());
+      EXPECT_TRUE(
+          data->GetDataReductionProxyData()->get_used_data_reduction_proxy());
+    }
+    if (message_loop_runner_->loop_running())
+      message_loop_runner_->Quit();
+  }
+
+  std::unique_ptr<content::NavigationData> cloned_data;

[bengr, 2016/04/29 21:14:21]

Where is this used?

[RyanSturm, 2016/05/02 19:52:20]

Done.

+
+ private:
+  bool add_data_reduction_proxy_data_;
+  // The MessageLoopRunner used to spin the message loop.

[bengr, 2016/04/29 21:14:21]

This comment is pretty useless. Delete.

[RyanSturm, 2016/05/02 19:52:19]

Done.

+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  DISALLOW_COPY_AND_ASSIGN(DidFinishNavigationObserver);
+};
+
 }  // namespace
 
 class ChromeResourceDispatcherHostDelegateBrowserTest :
     public InProcessBrowserTest {
  public:
   ChromeResourceDispatcherHostDelegateBrowserTest() {}
 
   void SetUpOnMainThread() override {
     InProcessBrowserTest::SetUpOnMainThread();
     // Hook navigations with our delegate.
@@ -128,25 +188,28 @@ class ChromeResourceDispatcherHostDelegateBrowserTest :
       (*it)->SetServerURLForTest(dm_url_.spec());
       (*it)->UpdateHeader(kTestPolicyHeader);
     }
   }
 
   void TearDownOnMainThread() override {
     content::ResourceDispatcherHost::Get()->SetDelegate(NULL);
     dispatcher_host_delegate_.reset();
   }
 
+  void SetAddDataReductionProxyData(bool add_data) {

[bengr, 2016/04/29 21:14:21]

should_add_data

[RyanSturm, 2016/05/02 19:52:20]

Done.

+    dispatcher_host_delegate_->add_data_reduction_proxy_data_ = add_data;
+  }
+
  protected:
   // The fake URL for DMServer we are using.
   GURL dm_url_;
   std::unique_ptr<TestDispatcherHostDelegate> dispatcher_host_delegate_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ChromeResourceDispatcherHostDelegateBrowserTest);
 };
 
 
 IN_PROC_BROWSER_TEST_F(ChromeResourceDispatcherHostDelegateBrowserTest,
                        NoPolicyHeader) {
   // When fetching non-DMServer URLs, we should not add a policy header to the
   // request.
   DCHECK(!embedded_test_server()->base_url().spec().empty());
@@ -175,10 +238,35 @@ IN_PROC_BROWSER_TEST_F(ChromeResourceDispatcherHostDelegateBrowserTest,
   redirect_url += kServerRedirectUrl;
   redirect_url += "?";
   redirect_url += dm_url_.spec();
   ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL(
       redirect_url));
   std::string value;
   ASSERT_TRUE(dispatcher_host_delegate_->request_headers_.GetHeader(
       policy::kChromePolicyHeader, &value));
   ASSERT_EQ(kTestPolicyHeader, value);
 }
+
+IN_PROC_BROWSER_TEST_F(ChromeResourceDispatcherHostDelegateBrowserTest,
+                       NavigationDataProcessed) {
+  ui_test_utils::NavigateToURL(browser(), embedded_test_server()->base_url());
+  {
+    DidFinishNavigationObserver nav_observer(
+        browser()->tab_strip_model()->GetActiveWebContents(), false);
+    EXPECT_TRUE(content::ExecuteScript(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        "window.location.href = '" +
+            embedded_test_server()->GetURL("/google/google.html").spec() +
+            "';"));
+    nav_observer.Wait();
+  }
+  SetAddDataReductionProxyData(true);
+  {
+    DidFinishNavigationObserver nav_observer(
+        browser()->tab_strip_model()->GetActiveWebContents(), true);
+    EXPECT_TRUE(content::ExecuteScript(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        "window.location.href = '" + embedded_test_server()->base_url().spec() +

[bengr, 2016/04/29 21:14:21]

This is weird to me. Are you just checking that you navigated to the url?

[RyanSturm, 2016/05/02 19:52:20]

Fixed the weirdness.

+            "';"));
+    nav_observer.Wait();
+  }
+}