[ntp] Instantiate NTPUserDataLogger per NTP

Previously, we instantiated an NTPUserDataLogger per web contents. And
it is possible to navigate to multiple NTPs in a single web contents.
This had a few drawbacks:
* Logging should be scoped to a single NTP. Navigations from and to NTPs
  had to be tracked to reset certain logging state.
* It was harder to reason about the life time of a logger and mismatched
  life times might have caused crashes in the WebUI NTP.
* There is a race condition on the WebUI when navigating quickly from
  and to an NTP that can cause incorrect data logging and hits a DCHECK.
  See crbug/1084360 for details.

Bug: 1144317
Fixed: 1084360
Change-Id: I02f683034aa17bc12efe9b58cae3c15c4d0ee038
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586281
Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839209}

chrome/browser/ui/search/ntp_user_data_logger.cc

@@ -20,9 +20,6 @@
 #include "components/ntp_tiles/metrics.h"
 #include "components/prefs/pref_service.h"
 #include "components/search/ntp_features.h"
-#include "content/public/browser/navigation_details.h"
-#include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/web_contents.h"
 
 namespace {
 
@@ -368,39 +365,15 @@ LogoClickType LoggingEventToLogoClick(NTPLoggingEventType event) {
                              base::TimeDelta::FromMilliseconds(1), \
                              base::TimeDelta::FromSeconds(60), 100)
 
-NTPUserDataLogger::~NTPUserDataLogger() {}
+NTPUserDataLogger::NTPUserDataLogger(Profile* profile, const GURL& ntp_url)
+    : has_emitted_(false),
+      should_record_doodle_load_time_(true),
+      modules_visible_(false),
+      during_startup_(!AfterStartupTaskUtils::IsBrowserStartupComplete()),
+      ntp_url_(ntp_url),
+      profile_(profile) {}
 
-// static
-NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
-    content::WebContents* content) {
-  DCHECK(search::IsInstantNTP(content) ||
-         content->GetMainFrame()->GetSiteInstance()->GetSiteURL() ==
-             GURL(chrome::kChromeUINewTabPageURL));
-
-  // Calling CreateForWebContents when an instance is already attached has no
-  // effect, so we can do this.
-  NTPUserDataLogger::CreateForWebContents(content);
-  NTPUserDataLogger* logger = NTPUserDataLogger::FromWebContents(content);
-
-  // We record the URL of this NTP in order to identify navigations that
-  // originate from it. We use the NavigationController's URL since it might
-  // differ from the WebContents URL which is usually chrome://newtab/.
-  //
-  // We update the NTP URL every time this function is called, because the NTP
-  // URL sometimes changes while it is open, and we care about the final one for
-  // detecting when the user leaves or returns to the NTP. In particular, if the
-  // Google URL changes (e.g. google.com -> google.de), then we fall back to the
-  // local NTP.
-  content::NavigationEntry* entry = content->GetController().GetVisibleEntry();
-  if (entry && (logger->ntp_url_ != entry->GetURL())) {
-    DVLOG(1) << "NTP URL changed from \"" << logger->ntp_url_ << "\" to \""
-             << entry->GetURL() << "\"";
-    logger->ntp_url_ = entry->GetURL();
-  }
-
-  logger->profile_ = Profile::FromBrowserContext(content->GetBrowserContext());
-  return logger;
-}
+NTPUserDataLogger::~NTPUserDataLogger() = default;
 
 // static
 void NTPUserDataLogger::LogOneGoogleBarFetchDuration(
@@ -621,31 +594,6 @@ void NTPUserDataLogger::SetModulesVisible(bool visible) {
   modules_visible_ = visible;
 }
 
-NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
-    : content::WebContentsObserver(contents),
-      has_emitted_(false),
-      should_record_doodle_load_time_(true),
-      modules_visible_(false),
-      during_startup_(!AfterStartupTaskUtils::IsBrowserStartupComplete()) {}
-
-// content::WebContentsObserver override
-void NTPUserDataLogger::NavigationEntryCommitted(
-    const content::LoadCommittedDetails& load_details) {
-  NavigatedFromURLToURL(load_details.previous_main_frame_url,
-                        load_details.entry->GetURL());
-}
-
-void NTPUserDataLogger::NavigatedFromURLToURL(const GURL& from,
-                                              const GURL& to) {
-  // User is returning to NTP, probably via the back button; reset stats.
-  if (from.is_valid() && to.is_valid() && (to == ntp_url_)) {
-    DVLOG(1) << "Returning to New Tab Page";
-    logged_impressions_.fill(base::nullopt);
-    has_emitted_ = false;
-    should_record_doodle_load_time_ = true;
-  }
-}
-
 bool NTPUserDataLogger::DefaultSearchProviderIsGoogle() const {
   return search::DefaultSearchProviderIsGoogle(profile_);
 }
@@ -782,5 +730,3 @@ void NTPUserDataLogger::RecordAction(const char* action) {
 
   base::RecordAction(base::UserMetricsAction(action));
 }
-
-WEB_CONTENTS_USER_DATA_KEY_IMPL(NTPUserDataLogger)

chrome/browser/ui/search/ntp_user_data_logger.h

@@ -9,7 +9,6 @@
 
 #include <array>
 
-#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/time/time.h"
@@ -18,29 +17,17 @@
 #include "chrome/common/search/ntp_logging_events.h"
 #include "components/ntp_tiles/constants.h"
 #include "components/ntp_tiles/ntp_tile_impression.h"
-#include "content/public/browser/web_contents_observer.h"
-#include "content/public/browser/web_contents_user_data.h"
 
 #if defined(OS_ANDROID)
 #error "Instant is only used on desktop";
 #endif
 
-namespace content {
-class WebContents;
-}
-
 // Helper class for logging data from the NTP. Attached to each NTP instance.
-class NTPUserDataLogger
-    : public content::WebContentsObserver,
-      public content::WebContentsUserData<NTPUserDataLogger> {
+class NTPUserDataLogger {
  public:
-  ~NTPUserDataLogger() override;
-
-  // Gets the associated NTPUserDataLogger, creating it if necessary.
-  //
-  // MUST be called only when the NTP is active.
-  static NTPUserDataLogger* GetOrCreateFromWebContents(
-      content::WebContents* content);
+  // Creates a NTPUserDataLogger. MUST be called only when the NTP is active.
+  NTPUserDataLogger(Profile* profile, const GURL& ntp_url);
+  virtual ~NTPUserDataLogger();
 
   // Called when a One Google Bar fetch has been completed after |duration|.
   // |success| is true if the fetch was successful.
@@ -81,28 +68,7 @@ class NTPUserDataLogger
   // Sets visibility of modules to be later logged.
   void SetModulesVisible(bool visible);
 
- 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, ShouldRecordLoadTime);
-  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, ShouldRecordNumberOfTiles);
-  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest,
-                           ShouldNotRecordImpressionsForBinsBeyondMax);
-  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest,
-                           ShouldRecordImpressionsAgainAfterNavigating);
-
-  // content::WebContentsObserver override
-  void NavigationEntryCommitted(
-      const content::LoadCommittedDetails& load_details) override;
-
-  // 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;
@@ -159,8 +125,6 @@ class NTPUserDataLogger
   // The profile in which this New Tab Page was loaded.
   Profile* profile_;
 
-  WEB_CONTENTS_USER_DATA_KEY_DECL();
-
   DISALLOW_COPY_AND_ASSIGN(NTPUserDataLogger);
 };
 

chrome/browser/ui/search/ntp_user_data_logger_unittest.cc

@@ -69,9 +69,7 @@ std::vector<base::Bucket> FillImpressions(int numImpressions, int count) {
 class TestNTPUserDataLogger : public NTPUserDataLogger {
  public:
   explicit TestNTPUserDataLogger(const GURL& ntp_url)
-      : NTPUserDataLogger(nullptr) {
-    set_ntp_url_for_testing(ntp_url);
-  }
+      : NTPUserDataLogger(nullptr, ntp_url) {}
 
   ~TestNTPUserDataLogger() override {}
 
@@ -125,15 +123,6 @@ TEST_F(NTPUserDataLoggerTest, ShouldRecordNumberOfTiles) {
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
   EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
               ElementsAre(Bucket(ntp_tiles::kMaxNumTiles, 1)));
-
-  // Navigating away and back resets stats.
-  logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
-                               GURL("http://chromium.org"));
-  logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
-                               GURL("chrome://newtab/"));
-  logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
-              ElementsAre(Bucket(0, 1), Bucket(ntp_tiles::kMaxNumTiles, 1)));
 }
 
 TEST_F(NTPUserDataLoggerTest, ShouldNotRecordImpressionsBeforeAllTilesLoaded) {
@@ -367,82 +356,6 @@ TEST_F(NTPUserDataLoggerTest, ShouldNotRecordImpressionsForBinsBeyondMax) {
               IsEmpty());
 }
 
-TEST_F(NTPUserDataLoggerTest, ShouldRecordImpressionsAgainAfterNavigating) {
-  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
-
-  // Record some previous tile impressions.
-  for (int bin = 0; bin < ntp_tiles::kMaxNumTiles; bin++) {
-    logger.LogMostVisitedImpression(MakeNTPTileImpression(
-        bin, TileSource::SUGGESTIONS_SERVICE, TileTitleSource::INFERRED,
-        TileVisualType::ICON_REAL));
-  }
-  logger.LogEvent(NTP_ALL_TILES_LOADED, base::TimeDelta::FromMilliseconds(73));
-
-  // After navigating away from the NTP and back, we should record again.
-  base::HistogramTester histogram_tester;
-
-  logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
-                               GURL("http://chromium.org"));
-  logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
-                               GURL("chrome://newtab/"));
-
-  logger.LogMostVisitedImpression(MakeNTPTileImpression(
-      0, TileSource::SUGGESTIONS_SERVICE, TileTitleSource::INFERRED,
-      TileVisualType::ICON_REAL));
-  logger.LogMostVisitedImpression(
-      MakeNTPTileImpression(1, TileSource::POPULAR, TileTitleSource::MANIFEST,
-                            TileVisualType::ICON_REAL));
-  logger.LogMostVisitedImpression(MakeNTPTileImpression(
-      2, TileSource::SUGGESTIONS_SERVICE, TileTitleSource::INFERRED,
-      TileVisualType::ICON_REAL));
-  logger.LogMostVisitedImpression(
-      MakeNTPTileImpression(3, TileSource::TOP_SITES, TileTitleSource::MANIFEST,
-                            TileVisualType::ICON_DEFAULT));
-  logger.LogEvent(NTP_ALL_TILES_LOADED, base::TimeDelta::FromMilliseconds(73));
-
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
-      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1), Bucket(3, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
-      ElementsAre(Bucket(0, 1), Bucket(2, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
-      ElementsAre(Bucket(3, 1)));
-  EXPECT_THAT(histogram_tester.GetAllSamples(
-                  "NewTabPage.SuggestionsImpression.popular_fetched"),
-              ElementsAre(Bucket(1, 1)));
-  EXPECT_THAT(histogram_tester.GetAllSamples(
-                  "NewTabPage.SuggestionsImpression.popular_baked_in"),
-              IsEmpty());
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType"),
-              ElementsAre(Bucket(ntp_tiles::TileVisualType::ICON_REAL, 3),
-                          Bucket(ntp_tiles::TileVisualType::ICON_DEFAULT, 1)));
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.server"),
-              ElementsAre(Bucket(ntp_tiles::TileVisualType::ICON_REAL, 2)));
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.client"),
-              ElementsAre(Bucket(ntp_tiles::TileVisualType::ICON_DEFAULT, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.TileType.popular_fetched"),
-      ElementsAre(Bucket(ntp_tiles::TileVisualType::ICON_REAL, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.TileType.popular_baked_in"),
-      IsEmpty());
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileTitle"),
-              ElementsAre(Bucket(kManifestTitleSource, 2),
-                          Bucket(kInferredTitleSource, 2)));
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileTitle.server"),
-              ElementsAre(Bucket(kInferredTitleSource, 2)));
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileTitle.client"),
-              ElementsAre(Bucket(kManifestTitleSource, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.TileTitle.popular_fetched"),
-      ElementsAre(Bucket(kManifestTitleSource, 1)));
-  EXPECT_THAT(
-      histogram_tester.GetAllSamples("NewTabPage.TileTitle.popular_baked_in"),
-      IsEmpty());
-}
-
 TEST_F(NTPUserDataLoggerTest, ShouldRecordNavigations) {
   TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
@@ -618,7 +531,7 @@ TEST_F(NTPUserDataLoggerTest, ShouldRecordNavigations) {
   }
 }
 
-TEST_F(NTPUserDataLoggerTest, ShouldRecordLoadTime) {
+TEST_F(NTPUserDataLoggerTest, ShouldRecordMostVisitedLoadTime) {
   base::HistogramTester histogram_tester;
 
   TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
@@ -649,32 +562,32 @@ TEST_F(NTPUserDataLoggerTest, ShouldRecordLoadTime) {
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
   histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
                                          delta_tiles_loaded, 1);
+}
 
-  // After navigating away from the NTP and back, we record again.
-  logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
-                               GURL("http://chromium.org"));
-  logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
-                               GURL("chrome://newtab/"));
+TEST_F(NTPUserDataLoggerTest, ShouldRecordMostLikelyLoadTime) {
+  base::HistogramTester histogram_tester;
+
+  TestNTPUserDataLogger logger(GURL("chrome://newtab/"));
 
-  // This time, log a SUGGESTIONS_SERVICE impression, so the times will end up
-  // in .MostLikely.
+  // Log a SUGGESTIONS_SERVICE impression, so the times will end up in
+  // .MostLikely.
   logger.LogMostVisitedImpression(MakeNTPTileImpression(
       0, TileSource::SUGGESTIONS_SERVICE, TileTitleSource::UNKNOWN,
       TileVisualType::ICON_REAL));
 
-  base::TimeDelta delta_tiles_loaded2 = base::TimeDelta::FromMilliseconds(500);
-  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded2);
+  base::TimeDelta delta_tiles_loaded = base::TimeDelta::FromMilliseconds(500);
+  logger.LogEvent(NTP_ALL_TILES_LOADED, delta_tiles_loaded);
 
-  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(2));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime"), SizeIs(1));
   EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.MostVisited"),
-              SizeIs(1));
+              IsEmpty());
   EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.LoadTime.MostLikely"),
               SizeIs(1));
 
   histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime",
-                                         delta_tiles_loaded2, 1);
+                                         delta_tiles_loaded, 1);
   histogram_tester.ExpectTimeBucketCount("NewTabPage.LoadTime.MostLikely",
-                                         delta_tiles_loaded2, 1);
+                                         delta_tiles_loaded, 1);
 }
 
 TEST_F(NTPUserDataLoggerTest, ShouldRecordLoadTimeLocalNTPGoogle) {

chrome/browser/ui/search/search_tab_helper.cc

@@ -194,16 +194,8 @@ SearchTabHelper::~SearchTabHelper() {
 void SearchTabHelper::OnTabActivated() {
   ipc_router_.OnTabActivated();
 
-  if (search::IsInstantNTP(web_contents_)) {
-    if (instant_service_)
+  if (search::IsInstantNTP(web_contents_) && instant_service_)
     instant_service_->OnNewTabPageOpened();
-
-    // Force creation of NTPUserDataLogger, if we loaded an NTP. The
-    // NTPUserDataLogger tries to detect whether the NTP is being created at
-    // startup or from the user opening a new tab, and if we wait until later,
-    // it won't correctly detect this case.
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_);
-  }
 }
 
 void SearchTabHelper::OnTabDeactivated() {
@@ -275,8 +267,23 @@ void SearchTabHelper::NavigationEntryCommitted(
   if (!load_details.is_main_frame)
     return;
 
-  if (search::IsInstantNTP(web_contents_))
+  if (search::IsInstantNTP(web_contents_)) {
+    // We (re)create the logger here because
+    // 1. The logger tries to detect whether the NTP is being created at startup
+    //    or from the user opening a new tab, and if we wait until later, it
+    //    won't correctly detect this case.
+    // 2. There can be multiple navigations to NTPs in a single web contents.
+    //    The navigations can be user-triggered or automatic, e.g. we fall back
+    //    to the local NTP if a remote NTP fails to load. Since logging should
+    //    be scoped to the life time of a single NTP we reset the logger every
+    //    time we reach a new NTP.
+    logger_ = std::make_unique<NTPUserDataLogger>(
+        Profile::FromBrowserContext(web_contents_->GetBrowserContext()),
+        // We use the NavigationController's URL since it might differ from the
+        // WebContents URL which is usually chrome://newtab/.
+        web_contents_->GetController().GetVisibleEntry()->GetURL());
     ipc_router_.SetInputInProgress(IsInputInProgress());
+  }
 
   if (InInstantProcess(instant_service_, web_contents_))
     ipc_router_.OnNavigationEntryCommitted();
@@ -365,28 +372,28 @@ void SearchTabHelper::OnToggleShortcutsVisibility(bool do_notify) {
 
 void SearchTabHelper::OnLogEvent(NTPLoggingEventType event,
                                  base::TimeDelta time) {
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogEvent(event, time);
+  if (logger_)
+    logger_->LogEvent(event, time);
 }
 
 void SearchTabHelper::OnLogSuggestionEventWithValue(
     NTPSuggestionsLoggingEventType event,
     int data,
     base::TimeDelta time) {
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogSuggestionEventWithValue(event, data, time);
+  if (logger_)
+    logger_->LogSuggestionEventWithValue(event, data, time);
 }
 
 void SearchTabHelper::OnLogMostVisitedImpression(
     const ntp_tiles::NTPTileImpression& impression) {
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogMostVisitedImpression(impression);
+  if (logger_)
+    logger_->LogMostVisitedImpression(impression);
 }
 
 void SearchTabHelper::OnLogMostVisitedNavigation(
     const ntp_tiles::NTPTileImpression& impression) {
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogMostVisitedNavigation(impression);
+  if (logger_)
+    logger_->LogMostVisitedNavigation(impression);
 }
 
 void SearchTabHelper::PasteIntoOmnibox(const base::string16& text) {
@@ -417,11 +424,12 @@ void SearchTabHelper::FileSelected(const base::FilePath& path,
   select_file_dialog_ = nullptr;
   // File selection can happen at any time after NTP load, and is not logged
   // with the event.
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogEvent(NTP_CUSTOMIZE_LOCAL_IMAGE_DONE,
+  if (logger_) {
+    logger_->LogEvent(NTP_CUSTOMIZE_LOCAL_IMAGE_DONE,
+                      base::TimeDelta::FromSeconds(0));
+    logger_->LogEvent(NTP_BACKGROUND_UPLOAD_DONE,
                       base::TimeDelta::FromSeconds(0));
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogEvent(NTP_BACKGROUND_UPLOAD_DONE, base::TimeDelta::FromSeconds(0));
+  }
 
   ipc_router_.SendLocalBackgroundSelected();
 }
@@ -430,11 +438,12 @@ void SearchTabHelper::FileSelectionCanceled(void* params) {
   select_file_dialog_ = nullptr;
   // File selection can happen at any time after NTP load, and is not logged
   // with the event.
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogEvent(NTP_CUSTOMIZE_LOCAL_IMAGE_CANCEL,
+  if (logger_) {
+    logger_->LogEvent(NTP_CUSTOMIZE_LOCAL_IMAGE_CANCEL,
                       base::TimeDelta::FromSeconds(0));
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents())
-      ->LogEvent(NTP_BACKGROUND_UPLOAD_CANCEL, base::TimeDelta::FromSeconds(0));
+    logger_->LogEvent(NTP_BACKGROUND_UPLOAD_CANCEL,
+                      base::TimeDelta::FromSeconds(0));
+  }
 }
 
 void SearchTabHelper::OnResultChanged(AutocompleteController* controller,

chrome/browser/ui/search/search_tab_helper.h

@@ -47,6 +47,7 @@ class Image;
 class AutocompleteController;
 class GURL;
 class InstantService;
+class NTPUserDataLogger;
 class Profile;
 class SearchIPCRouterTest;
 class SearchSuggestService;
@@ -226,6 +227,8 @@ class SearchTabHelper : public content::WebContentsObserver,
 
   FaviconCache favicon_cache_;
 
+  std::unique_ptr<NTPUserDataLogger> logger_;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   base::WeakPtrFactory<SearchTabHelper> weak_factory_{this};

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc

@@ -40,11 +40,11 @@
 #include "chrome/browser/search_provider_logos/logo_service_factory.h"
 #include "chrome/browser/ui/bookmarks/bookmark_stats.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
-#include "chrome/browser/ui/search/ntp_user_data_logger.h"
 #include "chrome/browser/ui/search/omnibox_mojo_utils.h"
 #include "chrome/browser/ui/search/omnibox_utils.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/search/instant_types.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/keyed_service/core/service_access_type.h"
@@ -352,7 +352,6 @@ NewTabPageHandler::NewTabPageHandler(
     Profile* profile,
     InstantService* instant_service,
     content::WebContents* web_contents,
-    NTPUserDataLogger* logger,
     const base::Time& ntp_navigation_start_time)
     : instant_service_(instant_service),
       ntp_background_service_(
@@ -371,7 +370,7 @@ NewTabPageHandler::NewTabPageHandler(
           BitmapFetcherServiceFactory::GetForBrowserContext(profile)),
       web_contents_(web_contents),
       ntp_navigation_start_time_(ntp_navigation_start_time),
-      logger_(logger),
+      logger_(profile, GURL(chrome::kChromeUINewTabPageURL)),
       promo_service_(PromoServiceFactory::GetForProfile(profile)),
       page_{std::move(pending_page)},
       receiver_{this, std::move(pending_page_handler)} {
@@ -381,7 +380,6 @@ NewTabPageHandler::NewTabPageHandler(
   CHECK(one_google_bar_service_);
   CHECK(promo_service_);
   CHECK(web_contents_);
-  CHECK(logger_);
   instant_service_->AddObserver(this);
   ntp_background_service_->AddObserver(this);
   instant_service_->UpdateNtpTheme();
@@ -389,7 +387,7 @@ NewTabPageHandler::NewTabPageHandler(
   OmniboxTabHelper::FromWebContents(web_contents_)->AddObserver(this);
   promo_service_observer_.Add(promo_service_);
   one_google_bar_service_observer_.Add(one_google_bar_service_);
-  logger_->SetModulesVisible(
+  logger_.SetModulesVisible(
       profile_->GetPrefs()->GetBoolean(prefs::kNtpModulesVisible));
 }
 
@@ -419,15 +417,13 @@ void NewTabPageHandler::AddMostVisitedTile(
     AddMostVisitedTileCallback callback) {
   bool success = instant_service_->AddCustomLink(url, title);
   std::move(callback).Run(success);
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-      ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_ADD, base::TimeDelta() /* unused */);
+  logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_ADD, base::TimeDelta() /* unused */);
 }
 
 void NewTabPageHandler::DeleteMostVisitedTile(const GURL& url) {
   if (instant_service_->IsCustomLinksEnabled()) {
     instant_service_->DeleteCustomLink(url);
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-        ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_REMOVE,
+    logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_REMOVE,
                      base::TimeDelta() /* unused */);
   } else {
     instant_service_->DeleteMostVisitedItem(url);
@@ -438,8 +434,7 @@ void NewTabPageHandler::DeleteMostVisitedTile(const GURL& url) {
 void NewTabPageHandler::RestoreMostVisitedDefaults() {
   if (instant_service_->IsCustomLinksEnabled()) {
     instant_service_->ResetCustomLinks();
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-        ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_RESTORE_ALL,
+    logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_RESTORE_ALL,
                      base::TimeDelta() /* unused */);
   } else {
     instant_service_->UndoAllMostVisitedDeletions();
@@ -465,14 +460,12 @@ void NewTabPageHandler::SetMostVisitedSettings(bool custom_links_enabled,
   if (old_visible != visible) {
     instant_service_->ToggleShortcutsVisibility(
         /* do_notify= */ !toggleCustomLinksEnabled);
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-        ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_TOGGLE_VISIBILITY,
+    logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_TOGGLE_VISIBILITY,
                      base::TimeDelta() /* unused */);
   }
   if (toggleCustomLinksEnabled) {
     instant_service_->ToggleMostVisitedOrCustomLinks();
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-        ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_TOGGLE_TYPE,
+    logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_TOGGLE_TYPE,
                      base::TimeDelta() /* unused */);
   }
 }
@@ -480,8 +473,8 @@ void NewTabPageHandler::SetMostVisitedSettings(bool custom_links_enabled,
 void NewTabPageHandler::UndoMostVisitedTileAction() {
   if (instant_service_->IsCustomLinksEnabled()) {
     instant_service_->UndoCustomLinkAction();
-    NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-        ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_UNDO, base::TimeDelta() /* unused */);
+    logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_UNDO,
+                     base::TimeDelta() /* unused */);
   } else if (last_blocklisted_.is_valid()) {
     instant_service_->UndoMostVisitedDeletion(last_blocklisted_);
     last_blocklisted_ = GURL();
@@ -532,8 +525,8 @@ void NewTabPageHandler::UpdateMostVisitedTile(
   bool success = instant_service_->UpdateCustomLink(
       url, new_url != url ? new_url : GURL(), new_title);
   std::move(callback).Run(success);
-  NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_)
-      ->LogEvent(NTP_CUSTOMIZE_SHORTCUT_UPDATE, base::TimeDelta() /* unused */);
+  logger_.LogEvent(NTP_CUSTOMIZE_SHORTCUT_UPDATE,
+                   base::TimeDelta() /* unused */);
 }
 
 void NewTabPageHandler::GetBackgroundCollections(
@@ -728,7 +721,7 @@ void NewTabPageHandler::OnPromoServiceShuttingDown() {
 }
 
 void NewTabPageHandler::OnAppRendered(double time) {
-  logger_->LogEvent(NTP_APP_RENDERED,
+  logger_.LogEvent(NTP_APP_RENDERED,
                    base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
@@ -736,22 +729,22 @@ void NewTabPageHandler::OnMostVisitedTilesRendered(
     std::vector<new_tab_page::mojom::MostVisitedTilePtr> tiles,
     double time) {
   for (size_t i = 0; i < tiles.size(); i++) {
-    logger_->LogMostVisitedImpression(MakeNTPTileImpression(*tiles[i], i));
+    logger_.LogMostVisitedImpression(MakeNTPTileImpression(*tiles[i], i));
   }
   // This call flushes all most visited impression logs to UMA histograms.
   // Therefore, it must come last.
-  logger_->LogEvent(NTP_ALL_TILES_LOADED,
+  logger_.LogEvent(NTP_ALL_TILES_LOADED,
                    base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
 void NewTabPageHandler::OnOneGoogleBarRendered(double time) {
-  logger_->LogEvent(NTP_ONE_GOOGLE_BAR_SHOWN,
+  logger_.LogEvent(NTP_ONE_GOOGLE_BAR_SHOWN,
                    base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
 void NewTabPageHandler::OnPromoRendered(double time,
                                         const base::Optional<GURL>& log_url) {
-  logger_->LogEvent(NTP_MIDDLE_SLOT_PROMO_SHOWN,
+  logger_.LogEvent(NTP_MIDDLE_SLOT_PROMO_SHOWN,
                    base::Time::FromJsTime(time) - ntp_navigation_start_time_);
   if (log_url.has_value() && log_url->is_valid()) {
     Fetch(*log_url, base::BindOnce([](bool, std::unique_ptr<std::string>) {}));
@@ -766,7 +759,7 @@ void NewTabPageHandler::OnMostVisitedTileNavigation(
     bool ctrl_key,
     bool meta_key,
     bool shift_key) {
-  logger_->LogMostVisitedNavigation(MakeNTPTileImpression(*tile, index));
+  logger_.LogMostVisitedNavigation(MakeNTPTileImpression(*tile, index));
 
   if (!base::FeatureList::IsEnabled(
           ntp_features::kNtpHandleMostVisitedNavigationExplicitly))
@@ -877,8 +870,7 @@ void NewTabPageHandler::OnDoodleImageRendered(
     OnDoodleImageRenderedCallback callback) {
   if (type == new_tab_page::mojom::DoodleImageType::kCta ||
       type == new_tab_page::mojom::DoodleImageType::kStatic) {
-    logger_->LogEvent(
-        type == new_tab_page::mojom::DoodleImageType::kCta
+    logger_.LogEvent(type == new_tab_page::mojom::DoodleImageType::kCta
                          ? NTP_CTA_LOGO_SHOWN_FROM_CACHE
                          : NTP_STATIC_LOGO_SHOWN_FROM_CACHE,
                      base::Time::FromJsTime(time) - ntp_navigation_start_time_);
@@ -997,22 +989,22 @@ void NewTabPageHandler::OnVoiceSearchError(
 
 void NewTabPageHandler::OnModuleImpression(const std::string& module_id,
                                            double time) {
-  logger_->LogModuleImpression(
+  logger_.LogModuleImpression(
       module_id, base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
 void NewTabPageHandler::OnModuleLoaded(const std::string& module_id,
                                        double time) {
-  logger_->LogModuleLoaded(
+  logger_.LogModuleLoaded(
       module_id, base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
 void NewTabPageHandler::OnModuleUsage(const std::string& module_id) {
-  logger_->LogModuleUsage(module_id);
+  logger_.LogModuleUsage(module_id);
 }
 
 void NewTabPageHandler::OnModulesRendered(double time) {
-  logger_->LogEvent(NTP_MODULES_SHOWN,
+  logger_.LogEvent(NTP_MODULES_SHOWN,
                    base::Time::FromJsTime(time) - ntp_navigation_start_time_);
 }
 
@@ -1521,7 +1513,7 @@ void NewTabPageHandler::OnRealboxFaviconFetched(int match_index,
 }
 
 void NewTabPageHandler::LogEvent(NTPLoggingEventType event) {
-  logger_->LogEvent(event, base::TimeDelta() /* unused */);
+  logger_.LogEvent(event, base::TimeDelta() /* unused */);
 }
 
 void NewTabPageHandler::Fetch(const GURL& url,

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h

@@ -21,6 +21,7 @@
 #include "chrome/browser/search/promos/promo_service.h"
 #include "chrome/browser/search/promos/promo_service_observer.h"
 #include "chrome/browser/ui/omnibox/omnibox_tab_helper.h"
+#include "chrome/browser/ui/search/ntp_user_data_logger.h"
 #include "chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom.h"
 #include "chrome/common/search/instant_types.h"
 #include "chrome/common/search/ntp_logging_events.h"
@@ -63,7 +64,6 @@ class NewTabPageHandler : public new_tab_page::mojom::PageHandler,
                     Profile* profile,
                     InstantService* instant_service,
                     content::WebContents* web_contents,
-                    NTPUserDataLogger* logger,
                     const base::Time& ntp_navigation_start_time);
   ~NewTabPageHandler() override;
 
@@ -241,7 +241,7 @@ class NewTabPageHandler : public new_tab_page::mojom::PageHandler,
   base::TimeTicks time_of_first_autocomplete_query_;
   content::WebContents* web_contents_;
   base::Time ntp_navigation_start_time_;
-  NTPUserDataLogger* logger_;
+  NTPUserDataLogger logger_;
   std::unordered_map<const network::SimpleURLLoader*,
                      std::unique_ptr<network::SimpleURLLoader>>
       loader_map_;

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc

@@ -5,9 +5,9 @@
 #include "chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/search/instant_service.h"
-#include "chrome/browser/ui/search/ntp_user_data_logger.h"
 #include "chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom.h"
 #include "chrome/common/search/omnibox.mojom.h"
+#include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_web_contents_factory.h"
@@ -29,12 +29,6 @@ class MockInstantService : public InstantService {
   MOCK_METHOD0(UpdateNtpTheme, void());
 };
 
-class MockNTPUserDataLogger : public NTPUserDataLogger {
- public:
-  MockNTPUserDataLogger() : NTPUserDataLogger(nullptr) {}
-  ~MockNTPUserDataLogger() override = default;
-};
-
 class MockPage : public new_tab_page::mojom::Page {
  public:
   MockPage() = default;
@@ -76,7 +70,7 @@ class NewTabPageHandlerTest : public testing::Test {
     handler_ = std::make_unique<NewTabPageHandler>(
         mojo::PendingReceiver<new_tab_page::mojom::PageHandler>(),
         mock_page_.BindAndGetRemote(), &profile_, &mock_instant_service_,
-        web_contents_, &logger_, base::Time::Now());
+        web_contents_, base::Time::Now());
     EXPECT_EQ(handler_.get(), instant_service_observer_);
   }
 
@@ -90,7 +84,6 @@ class NewTabPageHandlerTest : public testing::Test {
   MockInstantService mock_instant_service_;
   content::TestWebContentsFactory factory_;
   content::WebContents* web_contents_;  // Weak. Owned by factory_.
-  MockNTPUserDataLogger logger_;
   base::HistogramTester histogram_tester_;
   std::unique_ptr<NewTabPageHandler> handler_;
   InstantServiceObserver* instant_service_observer_;

chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc

@@ -18,7 +18,6 @@
 #include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/task_module/task_module_handler.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
-#include "chrome/browser/ui/search/ntp_user_data_logger.h"
 #include "chrome/browser/ui/search/omnibox_mojo_utils.h"
 #include "chrome/browser/ui/webui/customize_themes/chrome_customize_themes_handler.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
@@ -410,7 +409,6 @@ void NewTabPageUI::CreatePageHandler(
   page_handler_ = std::make_unique<NewTabPageHandler>(
       std::move(pending_page_handler), std::move(pending_page), profile_,
       instant_service_, web_contents_,
-      NTPUserDataLogger::GetOrCreateFromWebContents(web_contents_),
       navigation_start_time_);
 }