NewTabPage.LoadTime histograms: Add splits for .Google vs .Other

Semi-related cleanup: Merge the DefaultSearchProviderIsGoogle
implementations between search.cc and local_ntp_source.cc.

Bug: 583292
Change-Id: I47f0a2b1eeb2dfd6e117e81b332a44f09b7e8ee8
Reviewed-on: https://chromium-review.googlesource.com/527437Reviewed-by: Chris Pickel <sfiera@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478672}

chrome/browser/search/local_ntp_source.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/search/one_google_bar/one_google_bar_data.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service_factory.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
@@ -165,15 +166,6 @@ std::string GetLocalNtpPath() {
          std::string(chrome::kChromeSearchLocalNtpHost) + "/";
 }
 
-bool DefaultSearchProviderIsGoogleImpl(
-    const TemplateURLService* template_url_service) {
-  const TemplateURL* default_provider =
-      template_url_service->GetDefaultSearchProvider();
-  return default_provider && (default_provider->GetEngineType(
-                                  template_url_service->search_terms_data()) ==
-                              SEARCH_ENGINE_GOOGLE);
-}
-
 std::unique_ptr<base::DictionaryValue> ConvertOGBDataToDict(
     const OneGoogleBarData& og) {
   auto result = base::MakeUnique<base::DictionaryValue>();
@@ -198,7 +190,7 @@ class LocalNtpSource::GoogleSearchProviderTracker
       : service_(service), callback_(callback), is_google_(false) {
     DCHECK(service_);
     service_->AddObserver(this);
-    is_google_ = DefaultSearchProviderIsGoogleImpl(service_);
+    is_google_ = search::DefaultSearchProviderIsGoogle(service_);
   }
 
   ~GoogleSearchProviderTracker() override {
@@ -211,7 +203,7 @@ class LocalNtpSource::GoogleSearchProviderTracker
  private:
   void OnTemplateURLServiceChanged() override {
     bool old_is_google = is_google_;
-    is_google_ = DefaultSearchProviderIsGoogleImpl(service_);
+    is_google_ = search::DefaultSearchProviderIsGoogle(service_);
     if (is_google_ != old_is_google)
       callback_.Run(is_google_);
   }

chrome/browser/search/search.cc

@@ -92,20 +92,6 @@ const TemplateURL* GetDefaultSearchProviderTemplateURL(Profile* profile) {
   return NULL;
 }
 
-bool DefaultSearchProviderIsGoogle(Profile* profile) {
-  TemplateURLService* template_url_service =
-      TemplateURLServiceFactory::GetForProfile(profile);
-  if (!template_url_service)
-    return false;
-  const TemplateURL* default_provider =
-      template_url_service->GetDefaultSearchProvider();
-  if (!default_provider)
-    return false;
-  return default_provider->GetEngineType(
-             template_url_service->search_terms_data()) ==
-         SearchEngineType::SEARCH_ENGINE_GOOGLE;
-}
-
 GURL TemplateURLRefToGURL(const TemplateURLRef& ref,
                           const SearchTermsData& search_terms_data,
                           bool append_extra_query_params,
@@ -300,6 +286,24 @@ bool ShouldUseProcessPerSiteForInstantURL(const GURL& url, Profile* profile) {
           url.host_piece() == chrome::kChromeSearchRemoteNtpHost);
 }
 
+bool DefaultSearchProviderIsGoogle(Profile* profile) {
+  return DefaultSearchProviderIsGoogle(
+      TemplateURLServiceFactory::GetForProfile(profile));
+}
+
+bool DefaultSearchProviderIsGoogle(
+    const TemplateURLService* template_url_service) {
+  if (!template_url_service)
+    return false;
+  const TemplateURL* default_provider =
+      template_url_service->GetDefaultSearchProvider();
+  if (!default_provider)
+    return false;
+  return default_provider->GetEngineType(
+             template_url_service->search_terms_data()) ==
+         SearchEngineType::SEARCH_ENGINE_GOOGLE;
+}
+
 bool IsNTPURL(const GURL& url, Profile* profile) {
   if (!url.is_valid())
     return false;

chrome/browser/search/search.h

@@ -14,6 +14,7 @@
 
 class GURL;
 class Profile;
+class TemplateURLService;
 
 namespace content {
 class BrowserContext;
@@ -47,6 +48,11 @@ bool IsRenderedInInstantProcess(const content::WebContents* contents,
 // Returns true if the Instant |url| should use process per site.
 bool ShouldUseProcessPerSiteForInstantURL(const GURL& url, Profile* profile);
 
+// Returns whether Google is selected as the default search engine.
+bool DefaultSearchProviderIsGoogle(Profile* profile);
+bool DefaultSearchProviderIsGoogle(
+    const TemplateURLService* template_url_service);
+
 // Returns true if |url| corresponds to a New Tab page (it can be either an
 // Instant Extended NTP or a non-extended NTP). A page that matches the search
 // or Instant URL of the default search provider but does not have any search

chrome/browser/ui/search/ntp_user_data_logger.cc

@@ -150,6 +150,12 @@ void NTPUserDataLogger::NavigatedFromURLToURL(const GURL& from,
   }
 }
 
+bool NTPUserDataLogger::DefaultSearchProviderIsGoogle() const {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  return search::DefaultSearchProviderIsGoogle(profile);
+}
+
 void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   // We only send statistics once per page.
   if (has_emitted_) {
@@ -192,15 +198,32 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
     UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.MostVisited", load_time);
   }
 
+  // Note: This could be inaccurate if the default search engine was changed
+  // since the page load started. That's unlikely enough to not warrant special
+  // handling.
+  bool is_google = DefaultSearchProviderIsGoogle();
+
   // Split between Web and Local.
   if (ntp_url_.SchemeIsHTTPOrHTTPS()) {
     UMA_HISTOGRAM_LOAD_TIME("NewTabPage.TilesReceivedTime.Web",
                             tiles_received_time_);
     UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.Web", load_time);
+    // Further split between Google and non-Google.
+    if (is_google) {
+      UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.Web.Google", load_time);
+    } else {
+      UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.Web.Other", load_time);
+    }
   } else {
     UMA_HISTOGRAM_LOAD_TIME("NewTabPage.TilesReceivedTime.LocalNTP",
                             tiles_received_time_);
     UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.LocalNTP", load_time);
+    // Further split between Google and non-Google.
+    if (is_google) {
+      UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.LocalNTP.Google", load_time);
+    } else {
+      UMA_HISTOGRAM_LOAD_TIME("NewTabPage.LoadTime.LocalNTP.Other", load_time);
+    }
   }
 
   // Split between Startup and non-startup.

chrome/browser/ui/search/ntp_user_data_logger.h

@@ -53,12 +53,14 @@ class NTPUserDataLogger
  protected:
   explicit NTPUserDataLogger(content::WebContents* contents);
 
+  void set_ntp_url_for_testing(const GURL& ntp_url) { ntp_url_ = ntp_url; }
+
  private:
   friend class content::WebContentsUserData<NTPUserDataLogger>;
 
+  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLoadTime);
   FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLogMostVisitedImpression);
   FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestNumberOfTiles);
-  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLoadTime);
 
   // Number of Most Visited elements on the NTP for logging purposes.
   static const int kNumMostVisited = 8;
@@ -70,6 +72,10 @@ class NTPUserDataLogger
   // Implementation of NavigationEntryCommitted; separate for test.
   void NavigatedFromURLToURL(const GURL& from, const GURL& to);
 
+  // Returns whether Google is selected as the default search engine. Virtual
+  // for testing.
+  virtual bool DefaultSearchProviderIsGoogle() const;
+
   // Logs a number of statistics regarding the NTP. Called when an NTP tab is
   // about to be deactivated (be it by switching tabs, losing focus or closing
   // the tab/shutting down Chrome), or when the user navigates to a URL.

chrome/browser/ui/search/ntp_user_data_logger_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/metrics/statistics_recorder.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/common/search/ntp_logging_events.h"
+#include "chrome/common/url_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -29,8 +30,16 @@ using Samples = std::vector<Sample>;
 
 class TestNTPUserDataLogger : public NTPUserDataLogger {
  public:
-  TestNTPUserDataLogger() : NTPUserDataLogger(nullptr) {}
+  explicit TestNTPUserDataLogger(const GURL& ntp_url)
+      : NTPUserDataLogger(nullptr) {
+    set_ntp_url_for_testing(ntp_url);
+  }
+
   ~TestNTPUserDataLogger() override {}
+
+  bool DefaultSearchProviderIsGoogle() const override { return is_google_; }
+
+  bool is_google_ = true;
 };
 
 }  // namespace
@@ -41,8 +50,7 @@ TEST(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::HistogramTester histogram_tester;
 
   // Ensure non-zero statistics.
-  TestNTPUserDataLogger logger;
-  logger.ntp_url_ = GURL("chrome://newtab/");
+  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(0);
 
@@ -74,8 +82,7 @@ TEST(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
 
   base::HistogramTester histogram_tester;
 
-  TestNTPUserDataLogger logger;
-  logger.ntp_url_ = GURL("chrome://newtab/");
+  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(0);
 
@@ -199,7 +206,7 @@ TEST(NTPUserDataLoggerTest, TestLogMostVisitedNavigation) {
 
   base::HistogramTester histogram_tester;
 
-  TestNTPUserDataLogger logger;
+  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
   logger.LogMostVisitedNavigation(0, TileSource::SUGGESTIONS_SERVICE,
                                   TileVisualType::THUMBNAIL);
@@ -315,8 +322,7 @@ TEST(NTPUserDataLoggerTest, TestLoadTime) {
 
   base::HistogramTester histogram_tester;
 
-  TestNTPUserDataLogger logger;
-  logger.ntp_url_ = GURL("chrome://newtab/");
+  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
   base::TimeDelta delta_tiles_received = base::TimeDelta::FromMilliseconds(10);
   base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(100);
@@ -402,3 +408,147 @@ TEST(NTPUserDataLoggerTest, TestLoadTime) {
   histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.MostLikely",
                                          delta_tiles_loaded2, 1);
 }
+
+TEST(NTPUserDataLoggerTest, TestLoadTimeLocalNTPGoogle) {
+  base::StatisticsRecorder::Initialize();
+
+  base::HistogramTester histogram_tester;
+
+  TestNTPUserDataLogger logger((GURL(chrome::kChromeSearchLocalNtpUrl)));
+  logger.is_google_ = true;
+
+  base::TimeDelta delta_tiles_received = base::TimeDelta::FromMilliseconds(10);
+  base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(100);
+
+  // Send the ALL_TILES_RECEIVED event.
+  logger.LogEvent(NTP_ALL_TILES_RECEIVED, delta_tiles_received);
+
+  // Send the ALL_TILES_LOADED event, this should trigger emitting histograms.
+  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
+
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP"),
+              SizeIs(1));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP.Google"),
+      SizeIs(1));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP.Other"),
+      IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web"),
+              IsEmpty());
+
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.LocalNTP",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.LocalNTP.Google",
+                                         delta_tiles_loaded, 1);
+}
+
+TEST(NTPUserDataLoggerTest, TestLoadTimeLocalNTPOther) {
+  base::StatisticsRecorder::Initialize();
+
+  base::HistogramTester histogram_tester;
+
+  TestNTPUserDataLogger logger((GURL(chrome::kChromeSearchLocalNtpUrl)));
+  logger.is_google_ = false;
+
+  base::TimeDelta delta_tiles_received = base::TimeDelta::FromMilliseconds(10);
+  base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(100);
+
+  // Send the ALL_TILES_RECEIVED event.
+  logger.LogEvent(NTP_ALL_TILES_RECEIVED, delta_tiles_received);
+
+  // Send the ALL_TILES_LOADED event, this should trigger emitting histograms.
+  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
+
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP"),
+              SizeIs(1));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP.Google"),
+      IsEmpty());
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP.Other"),
+      SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web"),
+              IsEmpty());
+
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.LocalNTP",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.LocalNTP.Other",
+                                         delta_tiles_loaded, 1);
+}
+
+TEST(NTPUserDataLoggerTest, TestLoadTimeRemoteNTPGoogle) {
+  base::StatisticsRecorder::Initialize();
+
+  base::HistogramTester histogram_tester;
+
+  TestNTPUserDataLogger logger(GURL("https://www.google.com/_/chrome/newtab"));
+  logger.is_google_ = true;
+
+  base::TimeDelta delta_tiles_received = base::TimeDelta::FromMilliseconds(10);
+  base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(100);
+
+  // Send the ALL_TILES_RECEIVED event.
+  logger.LogEvent(NTP_ALL_TILES_RECEIVED, delta_tiles_received);
+
+  // Send the ALL_TILES_LOADED event, this should trigger emitting histograms.
+  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
+
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web"),
+              SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web.Google"),
+              SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web.Other"),
+              IsEmpty());
+
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.Web",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.Web.Google",
+                                         delta_tiles_loaded, 1);
+}
+
+TEST(NTPUserDataLoggerTest, TestLoadTimeRemoteNTPOther) {
+  base::StatisticsRecorder::Initialize();
+
+  base::HistogramTester histogram_tester;
+
+  TestNTPUserDataLogger logger(GURL("https://www.notgoogle.com/newtab"));
+  logger.is_google_ = false;
+
+  base::TimeDelta delta_tiles_received = base::TimeDelta::FromMilliseconds(10);
+  base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(100);
+
+  // Send the ALL_TILES_RECEIVED event.
+  logger.LogEvent(NTP_ALL_TILES_RECEIVED, delta_tiles_received);
+
+  // Send the ALL_TILES_LOADED event, this should trigger emitting histograms.
+  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
+
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.LocalNTP"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web"),
+              SizeIs(1));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web.Google"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.Web.Other"),
+              SizeIs(1));
+
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.Web",
+                                         delta_tiles_loaded, 1);
+  histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.Web.Other",
+                                         delta_tiles_loaded, 1);
+}

tools/metrics/histograms/histograms.xml

@@ -91403,14 +91403,21 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 <histogram_suffixes name="NewTabPageTimings" separator=".">
   <suffix name="MostLikely" label="Loaded server-side suggestions."/>
   <suffix name="MostVisited" label="Loaded client-side suggestions."/>
-  <suffix name="LocalNTP" label="Loaded local NTP"/>
-  <suffix name="Web" label="Loaded server-side NTP"/>
+  <suffix name="LocalNTP" label="Loaded local NTP."/>
+  <suffix name="Web" label="Loaded server-side NTP."/>
   <suffix name="Startup" label="NTP loaded during browser startup."/>
   <suffix name="NewTab" label="NTP loaded on a new tab."/>
   <affected-histogram name="NewTabPage.LoadTime"/>
   <affected-histogram name="NewTabPage.TilesReceivedTime"/>
 </histogram_suffixes>
 
+<histogram_suffixes name="NewTabPageTimingsIsGoogle" separator=".">
+  <suffix name="Google" label="Default search provider is Google."/>
+  <suffix name="Other" label="Default search provider is not Google."/>
+  <affected-histogram name="NewTabPage.LoadTime.LocalNTP"/>
+  <affected-histogram name="NewTabPage.LoadTime.Web"/>
+</histogram_suffixes>
+
 <histogram_suffixes name="NextTabState" separator="_">
   <suffix name="Active"
       label="For a tab active which is shown foreground in a browser window."/>