Cleanup desktop NTP metrics recording

- Remove all logging code from the multi-iframe NTP
- Remove NTPLoggingEventType.NTP_TILE, use NTP_*_SIDE_SUGGESTIONS instead
- Other small cleanups

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2429523002
Cr-Commit-Position: refs/heads/master@{#427056}

chrome/browser/resources/local_ntp/most_visited_single.js

@@ -16,13 +16,10 @@
  * @const
  */
 var LOG_TYPE = {
-  // The suggestion is coming from the server.
+  // A suggestion coming from the server was rendered.
   NTP_SERVER_SIDE_SUGGESTION: 0,
-  // The suggestion is coming from the client.
+  // A suggestion coming from the client was rendered.
   NTP_CLIENT_SIDE_SUGGESTION: 1,
-  // Indicates a tile was rendered, no matter if it's a thumbnail, a gray tile
-  // or an external tile.
-  NTP_TILE: 2,
   // All NTP Tiles have finished loading (successfully or failing).
   NTP_ALL_TILES_LOADED: 11,
 };
@@ -371,7 +368,6 @@ var renderTile = function(data) {
     return tile;
   }
 
-  logEvent(LOG_TYPE.NTP_TILE);
   // The tile will be appended to tiles.
   var position = tiles.children.length;
   logMostVisitedImpression(position, data.tileSource);

chrome/browser/resources/local_ntp/most_visited_thumbnail.js

@@ -11,21 +11,13 @@ window.addEventListener('DOMContentLoaded', function() {
   'use strict';
 
   fillMostVisited(document.location, function(params, data) {
-    function logEvent(eventName) {
-      chrome.embeddedSearch.newTabPage.logEvent(eventName);
-    }
-    function logMostVisitedImpression(tileIndex, tileSource) {
-      chrome.embeddedSearch.newTabPage.logMostVisitedImpression(tileIndex,
-                                                                tileSource);
-    }
     function displayLink(link) {
       document.body.appendChild(link);
       window.parent.postMessage('linkDisplayed', '{{ORIGIN}}');
     }
     function showDomainElement() {
       var link = createMostVisitedLink(
-          params, data.url, data.title, undefined, data.direction,
-          data.tileSource);
+          params, data.url, data.title, undefined, data.direction);
       var domain = document.createElement('div');
       domain.textContent = data.domain;
       link.appendChild(domain);
@@ -35,8 +27,7 @@ window.addEventListener('DOMContentLoaded', function() {
     // externally by the page itself.
     function showEmptyTile() {
       displayLink(createMostVisitedLink(
-          params, data.url, data.title, undefined, data.direction,
-          data.tileSource));
+          params, data.url, data.title, undefined, data.direction));
     }
     // Creates and adds an image.
     function createThumbnail(src, imageClass) {
@@ -46,15 +37,13 @@ window.addEventListener('DOMContentLoaded', function() {
       }
       image.onload = function() {
         var link = createMostVisitedLink(
-            params, data.url, data.title, undefined, data.direction,
-            data.tileSource);
+            params, data.url, data.title, undefined, data.direction);
         // Use blocker to prevent context menu from showing image-related items.
         var blocker = document.createElement('span');
         blocker.className = 'blocker';
         link.appendChild(blocker);
         link.appendChild(image);
         displayLink(link);
-        logEvent(NTP_LOGGING_EVENT_TYPE.NTP_TILE_LOADED);
       };
       image.onerror = function() {
         // If no external thumbnail fallback (etfb), and have domain.
@@ -63,7 +52,6 @@ window.addEventListener('DOMContentLoaded', function() {
         } else {
           showEmptyTile();
         }
-        logEvent(NTP_LOGGING_EVENT_TYPE.NTP_TILE_LOADED);
       };
       image.src = src;
     }
@@ -73,7 +61,6 @@ window.addEventListener('DOMContentLoaded', function() {
       showEmptyTile();
     } else if (useIcons && data.largeIconUrl) {
       createThumbnail(data.largeIconUrl, 'large-icon');
-      // TODO(huangs): Log event for large icons.
     } else if (!useIcons && data.thumbnailUrls && data.thumbnailUrls.length) {
       createThumbnail(data.thumbnailUrls[0], 'thumbnail');
     } else if (data.domain) {
@@ -81,11 +68,5 @@ window.addEventListener('DOMContentLoaded', function() {
     } else {
       showEmptyTile();
     }
-    logEvent(NTP_LOGGING_EVENT_TYPE.NTP_TILE);
-
-    // Log an impression if we know the position of the tile.
-    if (isFinite(params.pos)) {
-      logMostVisitedImpression(parseInt(params.pos, 10), data.tileSource);
-    }
   });
 });

chrome/browser/resources/local_ntp/most_visited_title.js

@@ -13,12 +13,6 @@ window.addEventListener('DOMContentLoaded', function() {
   fillMostVisited(window.location, function(params, data) {
     document.body.appendChild(
         createMostVisitedLink(
-            params, data.url, data.title, data.title, data.direction,
-            data.tileSource));
+            params, data.url, data.title, data.title, data.direction));
   });
 });
-
-window.addEventListener('load', function() {
-  chrome.embeddedSearch.newTabPage.logEvent(
-      NTP_LOGGING_EVENT_TYPE.NTP_TILE_LOADED);
-});

chrome/browser/resources/local_ntp/most_visited_util.js

@@ -10,41 +10,6 @@
 <include src="instant_iframe_validation.js">
 
 
-// TODO(treib): A number of things from this file (e.g. the "enums" below) are
-// duplicated in most_visited_single.js. Pull those out into a shared file.
-
-
-/**
- * The different types of events that are logged from the NTP.  This enum is
- * used to transfer information from the NTP javascript to the renderer and is
- * not used as a UMA enum histogram's logged value.
- * Note: Keep in sync with common/ntp_logging_events.h
- * @enum {number}
- * @const
- */
-var NTP_LOGGING_EVENT_TYPE = {
-  // The suggestion is coming from the server.
-  NTP_SERVER_SIDE_SUGGESTION: 0,
-  // The suggestion is coming from the client.
-  NTP_CLIENT_SIDE_SUGGESTION: 1,
-  // Indicates a tile was rendered, no matter if it's a thumbnail, a gray tile
-  // or an external tile.
-  NTP_TILE: 2,
-  // A NTP Tile has finished loading (successfully or failing).
-  NTP_TILE_LOADED: 10,
-};
-
-/**
- * The different sources that an NTP tile can have.
- * Note: Keep in sync with common/ntp_logging_events.h
- * @enum {number}
- * @const
- */
-var NTPLoggingTileSource = {
-  CLIENT: 0,
-  SERVER: 1,
-};
-
 /**
  * The origin of this request.
  * @const {string}
@@ -83,11 +48,9 @@ function parseQueryParams(location) {
  * @param {string} title The title for the link.
  * @param {string|undefined} text The text for the link or none.
  * @param {string|undefined} direction The text direction.
- * @param {number} tileSource The source from NTPLoggingTileSource.
  * @return {HTMLAnchorElement} A new link element.
  */
-function createMostVisitedLink(
-    params, href, title, text, direction, tileSource) {
+function createMostVisitedLink(params, href, title, text, direction) {
   var styles = getMostVisitedStyles(params, !!text);
   var link = document.createElement('a');
   link.style.color = styles.color;
@@ -135,12 +98,6 @@ function createMostVisitedLink(
       generatePing(DOMAIN_ORIGIN + params.ping);
     }
 
-    var ntpApiHandle = chrome.embeddedSearch.newTabPage;
-    if ('pos' in params && isFinite(params.pos)) {
-      ntpApiHandle.logMostVisitedNavigation(parseInt(params.pos, 10),
-                                            tileSource);
-    }
-
     // Follow <a> normally, so transition type will be LINK.
   };
 
@@ -243,10 +200,6 @@ function fillMostVisited(location, fill) {
   params.rid = parseInt(params.rid, 10);
   if (!isFinite(params.rid) && !params.url)
     return;
-  // Log whether the suggestion was obtained from the server or the client.
-  chrome.embeddedSearch.newTabPage.logEvent(params.url ?
-      NTP_LOGGING_EVENT_TYPE.NTP_SERVER_SIDE_SUGGESTION :
-      NTP_LOGGING_EVENT_TYPE.NTP_CLIENT_SIDE_SUGGESTION);
   var data;
   if (params.url) {
     // Means that the suggestion data comes from the server. Create data object.
@@ -256,15 +209,13 @@ function fillMostVisited(location, fill) {
       thumbnailUrl: params.tu || '',
       title: params.ti || '',
       direction: params.di || '',
-      domain: params.dom || '',
-      tileSource: NTPLoggingTileSource.SERVER
+      domain: params.dom || ''
     };
   } else {
     var apiHandle = chrome.embeddedSearch.searchBox;
     data = apiHandle.getMostVisitedItemData(params.rid);
     if (!data)
       return;
-    data.tileSource = NTPLoggingTileSource.CLIENT;
   }
 
   if (isFinite(params.dummy) && parseInt(params.dummy, 10)) {

chrome/browser/ui/search/search_ipc_router_unittest.cc

@@ -259,14 +259,15 @@ TEST_F(SearchIPCRouterTest, ProcessLogEventMsg) {
   NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_TILE, delta)).Times(1);
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+      .Times(1);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(1)
       .WillOnce(testing::Return(true));
 
   content::WebContents* contents = web_contents();
   OnMessageReceived(ChromeViewHostMsg_LogEvent(
       contents->GetRoutingID(), GetSearchIPCRouterSeqNo(),
-      NTP_TILE, delta));
+      NTP_CLIENT_SIDE_SUGGESTION, delta));
 }
 
 TEST_F(SearchIPCRouterTest, IgnoreLogEventMsg) {
@@ -274,14 +275,15 @@ TEST_F(SearchIPCRouterTest, IgnoreLogEventMsg) {
   NavigateAndCommitActiveTab(GURL("chrome-search://foo/bar"));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_TILE, delta)).Times(0);
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+      .Times(0);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(1)
       .WillOnce(testing::Return(false));
 
   content::WebContents* contents = web_contents();
   OnMessageReceived(ChromeViewHostMsg_LogEvent(
       contents->GetRoutingID(), GetSearchIPCRouterSeqNo(),
-      NTP_TILE, delta));
+      NTP_CLIENT_SIDE_SUGGESTION, delta));
 }
 
 TEST_F(SearchIPCRouterTest, ProcessLogMostVisitedImpressionMsg) {
@@ -486,11 +488,12 @@ TEST_F(SearchIPCRouterTest, IgnoreMessageIfThePageIsNotActive) {
       contents->GetRoutingID(), page_seq_no, OMNIBOX_FOCUS_VISIBLE));
 
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(123);
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_TILE, delta)).Times(0);
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+      .Times(0);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(0);
-  OnMessageReceived(ChromeViewHostMsg_LogEvent(contents->GetRoutingID(),
-                                               page_seq_no,
-                                               NTP_TILE, delta));
+  OnMessageReceived(
+      ChromeViewHostMsg_LogEvent(contents->GetRoutingID(), page_seq_no,
+                                 NTP_CLIENT_SIDE_SUGGESTION, delta));
 
   base::string16 text;
   EXPECT_CALL(*mock_delegate(), PasteIntoOmnibox(text)).Times(0);

chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc

@@ -54,6 +54,21 @@ std::string GetSourceName(NTPLoggingTileSource tile_source) {
   return std::string();
 }
 
+void RecordSyncSessionMetrics(content::WebContents* contents) {
+  if (!contents)
+    return;
+  browser_sync::ProfileSyncService* sync =
+      ProfileSyncServiceFactory::GetForProfile(
+          Profile::FromBrowserContext(contents->GetBrowserContext()));
+  if (!sync)
+    return;
+  sync_sessions::SessionsSyncManager* sessions =
+      static_cast<sync_sessions::SessionsSyncManager*>(
+          sync->GetSessionsSyncableService());
+  sync_sessions::SyncSessionsMetrics::RecordYoungestForeignTabAgeOnNTP(
+      sessions);
+}
+
 }  // namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(NTPUserDataLogger);
@@ -62,7 +77,7 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(NTPUserDataLogger);
 // Log a time event for a given |histogram| at a given |value|. This
 // routine exists because regular histogram macros are cached thus can't be used
 // if the name of the histogram will change at a given call site.
-void logLoadTimeHistogram(const std::string& histogram, base::TimeDelta value) {
+void LogLoadTimeHistogram(const std::string& histogram, base::TimeDelta value) {
   base::HistogramBase* counter = base::Histogram::FactoryTimeGet(
       histogram,
       base::TimeDelta::FromMilliseconds(1),
@@ -110,23 +125,17 @@ void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
   switch (event) {
     case NTP_SERVER_SIDE_SUGGESTION:
       has_server_side_suggestions_ = true;
-      break;
+      number_of_tiles_++;
+      return;
     case NTP_CLIENT_SIDE_SUGGESTION:
       has_client_side_suggestions_ = true;
-      break;
-    case NTP_TILE:
-      // TODO(sfiera): remove NTP_TILE and use NTP_*_SIDE_SUGGESTION.
       number_of_tiles_++;
-      break;
-    case NTP_TILE_LOADED:
-      // We no longer emit statistics for the multi-iframe NTP.
-      break;
+      return;
     case NTP_ALL_TILES_LOADED:
       EmitNtpStatistics(time);
-      break;
-    default:
-      NOTREACHED();
+      return;
   }
+  NOTREACHED();
 }
 
 void NTPUserDataLogger::LogMostVisitedImpression(
@@ -178,27 +187,12 @@ NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
       has_client_side_suggestions_(false),
       number_of_tiles_(0),
       has_emitted_(false),
-      during_startup_(false) {
-  during_startup_ = !AfterStartupTaskUtils::IsBrowserStartupComplete();
-
+      during_startup_(!AfterStartupTaskUtils::IsBrowserStartupComplete()) {
   // We record metrics about session data here because when this class typically
   // emits metrics it is too late. This session data would theoretically have
   // been used to populate the page, and we want to learn about its state when
   // the NTP is being generated.
-  if (contents) {
-    browser_sync::ProfileSyncService* sync =
-        ProfileSyncServiceFactory::GetForProfile(
-            Profile::FromBrowserContext(contents->GetBrowserContext()));
-    if (sync) {
-      sync_sessions::SessionsSyncManager* sessions =
-          static_cast<sync_sessions::SessionsSyncManager*>(
-              sync->GetSessionsSyncableService());
-      if (sessions) {
-        sync_sessions::SyncSessionsMetrics::RecordYoungestForeignTabAgeOnNTP(
-            sessions);
-      }
-    }
-  }
+  RecordSyncSessionMetrics(contents);
 }
 
 // content::WebContentsObserver override
@@ -228,19 +222,19 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   DVLOG(1) << "Emitting NTP load time: " << load_time << ", "
            << "number of tiles: " << number_of_tiles_;
 
-  logLoadTimeHistogram("NewTabPage.LoadTime", load_time);
+  LogLoadTimeHistogram("NewTabPage.LoadTime", load_time);
 
   // Split between ML and MV.
   std::string type = has_server_side_suggestions_ ?
       "MostLikely" : "MostVisited";
-  logLoadTimeHistogram("NewTabPage.LoadTime." + type, load_time);
+  LogLoadTimeHistogram("NewTabPage.LoadTime." + type, load_time);
   // Split between Web and Local.
   std::string source = ntp_url_.SchemeIsHTTPOrHTTPS() ? "Web" : "LocalNTP";
-  logLoadTimeHistogram("NewTabPage.LoadTime." + source, load_time);
+  LogLoadTimeHistogram("NewTabPage.LoadTime." + source, load_time);
 
   // Split between Startup and non-startup.
   std::string status = during_startup_ ? "Startup" : "NewTab";
-  logLoadTimeHistogram("NewTabPage.LoadTime." + status, load_time);
+  LogLoadTimeHistogram("NewTabPage.LoadTime." + status, load_time);
 
   has_server_side_suggestions_ = false;
   has_client_side_suggestions_ = false;

chrome/browser/ui/webui/ntp/ntp_user_data_logger.h

@@ -34,8 +34,8 @@ class NTPUserDataLogger
       content::WebContents* content);
 
   // Called when an event occurs on the NTP that requires a counter to be
-  // incremented. |time| is the delta time in ms from navigation start until
-  // this event happened.
+  // incremented. |time| is the delta time from navigation start until this
+  // event happened.
   void LogEvent(NTPLoggingEventType event, base::TimeDelta time);
 
   // Logs an impression on one of the NTP tiles by a given source.
@@ -50,12 +50,6 @@ class NTPUserDataLogger
  private:
   friend class content::WebContentsUserData<NTPUserDataLogger>;
 
-  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
-                           OnMostVisitedItemsChangedFromServer);
-  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
-                           OnMostVisitedItemsChangedFromClient);
-  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest,
-                           TestLogging);
   FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestLogMostVisitedImpression);
   FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest, TestNumberOfTiles);
 

chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc

@@ -81,15 +81,14 @@ class NTPUserDataLoggerTest : public testing::Test {
 TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::StatisticsRecorder::Initialize();
 
-  // Enusure non-zero statistics.
+  // Ensure non-zero statistics.
   TestNTPUserDataLogger logger;
   logger.ntp_url_ = GURL("chrome://newtab/");
 
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(0);
 
   for (int i = 0; i < 8; ++i)
-    logger.LogEvent(NTP_TILE, delta);
-  logger.LogEvent(NTP_SERVER_SIDE_SUGGESTION, delta);
+    logger.LogEvent(NTP_SERVER_SIDE_SUGGESTION, delta);
 
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
 

chrome/common/search/ntp_logging_events.h

@@ -8,18 +8,17 @@
 // The different types of events that are logged from the NTP. This enum is used
 // to transfer information from the NTP javascript to the renderer and is not
 // used as a UMA enum histogram's logged value.
-// Note: Keep in sync with browser/resources/local_ntp/most_visited_util.js
-// and browser/resources/local_ntp/most_visited_single.js
+// Note: Keep in sync with browser/resources/local_ntp/most_visited_single.js
 enum NTPLoggingEventType {
-  // The suggestion is coming from the server.
+  // A suggestion coming from the server was rendered.
   NTP_SERVER_SIDE_SUGGESTION = 0,
 
-  // The suggestion is coming from the client.
+  // A suggestion coming from the client was rendered.
   NTP_CLIENT_SIDE_SUGGESTION = 1,
 
   // Indicates a tile was rendered, no matter if it's a thumbnail, a gray tile
   // or an external tile.
-  NTP_TILE = 2,
+  // Deleted: NTP_TILE = 2,
 
   // The tile uses a local thumbnail image.
   // Deleted: NTP_THUMBNAIL_TILE = 3,
@@ -45,7 +44,7 @@ enum NTPLoggingEventType {
 
   // A NTP Tile has finished loading (successfully or failing). Logged only by
   // the multi-iframe version of the NTP.
-  NTP_TILE_LOADED = 10,
+  // Deleted: NTP_TILE_LOADED = 10,
 
   // All NTP tiles have finished loading (successfully or failing). Logged only
   // by the single-iframe version of the NTP.
@@ -55,8 +54,7 @@ enum NTPLoggingEventType {
 };
 
 // The source of an NTP tile.
-// Note: Keep in sync with browser/resources/local_ntp/most_visited_util.js and
-// browser/resources/local_ntp/most_visited_single.js.
+// Note: Keep in sync with browser/resources/local_ntp/most_visited_single.js.
 // TODO(treib): Merge this into MostVisitedSource from components/ntp_tiles.
 enum class NTPLoggingTileSource {
   CLIENT = 0,