Refactor PageLoadMetricsObserver completion callback policy

chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc

 // Copyright 2015 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/page_load_metrics/observers/from_gws_page_load_metrics_observer.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h"
 #include "components/page_load_metrics/browser/page_load_metrics_util.h"
@@ skipping 656 matching lines @@
   NavigateAndCommit(GURL("https://example.test"));
   SimulateTimingWithFirstPaint();
   // Simulate user interaction.
   SimulateMouseEvent();
   // Simulate closing the tab.
   DeleteContents();
   histogram_tester().ExpectTotalCount(
       internal::kHistogramFromGWSAbortCloseBeforeInteraction, 0);
 }
 
+TEST_F(FromGWSPageLoadMetricsObserverTest, Intent) {
+  NavigateAndCommit(GURL(kGoogleSearchResultsUrl));
+  NavigateAndCommit(GURL("intent://en.m.wikipedia.org/wiki/Test"));
+  SimulateTimingWithFirstPaint();
+  // Simulate closing the tab.
+  DeleteContents();
+  histogram_tester().ExpectTotalCount(
+      internal::kHistogramFromGWSAbortCloseBeforeInteraction, 0);
+}
+
 TEST_F(FromGWSPageLoadMetricsLoggerTest, IsGoogleSearchHostname) {
   struct {
     bool expected_result;
     const char* url;
   } test_cases[] = {
       {true, "www.google.com"},
       {true, "www.google.co.uk"},
       {true, "www.google.co.in"},
       {false, "other.google.com"},
       {false, "other.www.google.com"},
@@ skipping 136 matching lines @@
   for (const auto& test : test_cases) {
     EXPECT_EQ(test.expected_result,
               FromGWSPageLoadMetricsLogger::QueryContainsComponentPrefix(
                   test.query, test.component))
         << "For query: " << test.query << " with component: " << test.component;
   }
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, Basic) {
   FromGWSPageLoadMetricsLogger logger;
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
+  ASSERT_FALSE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, NoPreviousPage) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL());
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
+  ASSERT_FALSE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, NavigationNotInitiatedViaLink) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.set_navigation_initiated_via_link(false);
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
-}
-
-TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalNonHttpOrHttpsScheme) {

[Charlie Harrison, 2016/07/18 20:02:18]

Looks like the test you added doesn't cover the same cases as these deleted
tests.

[Bryan McQuade, 2016/07/19 12:55:38]

Ah, you mean it not being a provisional load? I added a provisional version of
the new test as well. I think the 2 intent:-based tests I removed were redundant
- they were both just testing different intent: URLs. So I didn't see a need to
replicate both of those provisional tests. I agree it's be good to have a
provisional version of the test so I added that. I also verified that if I
change the intent: URL to an https: URL, both tests fail, so these tests are
verifying that we ignore intent-based navigations.

-  FromGWSPageLoadMetricsLogger logger;
-  logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
-  logger.SetProvisionalUrl(GURL("intent://foo"));
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL::EmptyGURL()));
+  ASSERT_FALSE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalFromGWS) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.SetProvisionalUrl(GURL(kGoogleSearchResultsUrl));
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL::EmptyGURL()));
+  ASSERT_FALSE(logger.ShouldLogFailedProvisionalLoadMetrics());
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalNotFromGWS) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.SetProvisionalUrl(GURL(kExampleUrl));
-  ASSERT_TRUE(logger.ShouldLogMetrics(GURL::EmptyGURL()));
-}
-
-TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalIntent) {
-  FromGWSPageLoadMetricsLogger logger;
-  logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
-  logger.SetProvisionalUrl(GURL("intent://en.m.wikipedia.org/wiki/Test"));
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL::EmptyGURL()));
+  ASSERT_TRUE(logger.ShouldLogFailedProvisionalLoadMetrics());
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalIgnoredAfterCommit1) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.SetProvisionalUrl(GURL(kExampleUrl));
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL(kGoogleSearchResultsUrl)));
+  ASSERT_FALSE(
+      logger.ShouldLogPostCommitMetrics(GURL(kGoogleSearchResultsUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalIgnoredAfterCommit2) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.SetProvisionalUrl(GURL(kGoogleSearchResultsUrl));
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_TRUE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
+  ASSERT_TRUE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, NavigationFromSearch) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_TRUE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
+  ASSERT_TRUE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, NavigationToSearchHostname) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(GURL(kGoogleSearchResultsUrl));
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_FALSE(logger.ShouldLogMetrics(GURL("https://www.google.com/about/")));
+  ASSERT_FALSE(
+      logger.ShouldLogPostCommitMetrics(GURL("https://www.google.com/about/")));
 }
 
 TEST_F(FromGWSPageLoadMetricsLoggerTest, NavigationFromSearchRedirector) {
   FromGWSPageLoadMetricsLogger logger;
   logger.SetPreviouslyCommittedUrl(
       GURL("https://www.google.com/url?source=web"));
   logger.set_navigation_initiated_via_link(true);
-  ASSERT_TRUE(logger.ShouldLogMetrics(GURL(kExampleUrl)));
+  ASSERT_TRUE(logger.ShouldLogPostCommitMetrics(GURL(kExampleUrl)));
 }