Emit correct NewTabPage.SuggestionsType metrics

Review URL: https://codereview.chromium.org/1396143002

Cr-Commit-Position: refs/heads/master@{#364190}

9 files changed, 103 insertions, 2 deletions

diff --git a/chrome/browser/resources/local_ntp/most_visited_single.js b/chrome/browser/resources/local_ntp/most_visited_single.js
index 6c51584..98ebea5 100644
--- a/chrome/browser/resources/local_ntp/most_visited_single.js
+++ b/chrome/browser/resources/local_ntp/most_visited_single.js
@@ -300,10 +300,8 @@ var addTile = function(args) {
           window.devicePixelRatio + 'x/' + data.renderViewId + '/' + data.tid;
     }
     tiles.appendChild(renderTile(data));
-    logEvent(LOG_TYPE.NTP_CLIENT_SIDE_SUGGESTION);
   } else if (args.id) {
     tiles.appendChild(renderTile(args));
-    logEvent(LOG_TYPE.NTP_SERVER_SIDE_SUGGESTION);
   } else {
     tiles.appendChild(renderTile(null));
   }
diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc
index f8ea9c1..a75cc22 100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -331,6 +331,7 @@ void InstantService::OnSuggestionsAvailable(
     if (suggestion.has_click_url()) {
       item.click_url = GURL(suggestion.click_url());
     }
+    item.is_server_side_suggestion = true;
     new_suggestions_items.push_back(item);
   }
   suggestions_items_ = new_suggestions_items;
@@ -346,6 +347,7 @@ void InstantService::OnMostVisitedItemsReceived(
     InstantMostVisitedItem item;
     item.url = url.url;
     item.title = url.title;
+    item.is_server_side_suggestion = false;
     new_most_visited_items.push_back(item);
   }
 
diff --git a/chrome/browser/search/instant_service.h b/chrome/browser/search/instant_service.h
index 8892528..5d0b809 100644
--- a/chrome/browser/search/instant_service.h
+++ b/chrome/browser/search/instant_service.h
@@ -107,6 +107,8 @@ class InstantService : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, ProcessIsolation);
   FRIEND_TEST_ALL_PREFIXES(InstantServiceEnabledTest,
                            SendsSearchURLsToRenderer);
+  FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, GetSuggestionFromServiceSide);
+  FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, GetSuggestionFromClientSide);
 
   // KeyedService:
   void Shutdown() override;
diff --git a/chrome/browser/search/instant_service_unittest.cc b/chrome/browser/search/instant_service_unittest.cc
index 4b5d0f7..4b5f0f7 100644
--- a/chrome/browser/search/instant_service_unittest.cc
+++ b/chrome/browser/search/instant_service_unittest.cc
@@ -165,3 +165,25 @@ TEST_F(InstantServiceTest, OmniboxStartMarginChanged) {
               OmniboxStartMarginChanged(new_start_margin)).Times(1);
   UpdateOmniboxStartMargin(new_start_margin);
 }
+
+TEST_F(InstantServiceTest, GetSuggestionFromServiceSide) {
+  auto profile = suggestions::SuggestionsProfile();
+  profile.add_suggestions();
+
+  instant_service_->OnSuggestionsAvailable(profile);
+
+  auto items = instant_service_->suggestions_items_;
+  ASSERT_EQ(1, (int)items.size());
+  ASSERT_TRUE(items[0].is_server_side_suggestion);
+}
+
+TEST_F(InstantServiceTest, GetSuggestionFromClientSide) {
+  history::MostVisitedURLList url_list;
+  url_list.push_back(history::MostVisitedURL());
+
+  instant_service_->OnMostVisitedItemsReceived(url_list);
+
+  auto items = instant_service_->most_visited_items_;
+  ASSERT_EQ(1, (int)items.size());
+  ASSERT_FALSE(items[0].is_server_side_suggestion);
+}
diff --git a/chrome/browser/ui/search/search_tab_helper.cc b/chrome/browser/ui/search/search_tab_helper.cc
index e071a4e..e20bcfe 100644
--- a/chrome/browser/ui/search/search_tab_helper.cc
+++ b/chrome/browser/ui/search/search_tab_helper.cc
@@ -388,6 +388,24 @@ void SearchTabHelper::MostVisitedItemsChanged(
   // our metrics get inconsistent. So we'd rather emit stats now.
   InstantTab::EmitNtpStatistics(web_contents_);
   ipc_router_.SendMostVisitedItems(items);
+  LogMostVisitedItemsSource(items);
+}
+
+void SearchTabHelper::LogMostVisitedItemsSource(
+    const std::vector<InstantMostVisitedItem>& items) {
+  for (auto item : items) {
+    NTPLoggingEventType event;
+    if (item.is_server_side_suggestion) {
+      event = NTP_SERVER_SIDE_SUGGESTION;
+    } else {
+      event = NTP_CLIENT_SIDE_SUGGESTION;
+    }
+    // The metrics are emitted for each suggestion as the design requirement
+    // even the ntp_user_data_logger.cc now only supports the scenario:
+    // all suggestions are provided by server OR
+    // all suggestions are provided by client.
+    this->OnLogEvent(event, base::TimeDelta());
+  }
 }
 
 void SearchTabHelper::OmniboxStartMarginChanged(int omnibox_start_margin) {
diff --git a/chrome/browser/ui/search/search_tab_helper.h b/chrome/browser/ui/search/search_tab_helper.h
index 6c5bea9..1471c7b 100644
--- a/chrome/browser/ui/search/search_tab_helper.h
+++ b/chrome/browser/ui/search/search_tab_helper.h
@@ -125,6 +125,10 @@ class SearchTabHelper : public content::WebContentsObserver,
                            OnHistorySyncCheckSyncing);
   FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
                            OnHistorySyncCheckNotSyncing);
+  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
+                           OnMostVisitedItemsChangedFromServer);
+  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
+                           OnMostVisitedItemsChangedFromClient);
   FRIEND_TEST_ALL_PREFIXES(SearchIPCRouterTest,
                            IgnoreMessageIfThePageIsNotActive);
   FRIEND_TEST_ALL_PREFIXES(SearchIPCRouterTest,
@@ -201,6 +205,10 @@ class SearchTabHelper : public content::WebContentsObserver,
   // Returns the OmniboxView for |web_contents_| or NULL if not available.
   OmniboxView* GetOmniboxView() const;
 
+  // Record whether each suggestion comes from server or client.
+  void LogMostVisitedItemsSource(
+      const std::vector<InstantMostVisitedItem>& items);
+
   typedef bool (*OmniboxHasFocusFn)(OmniboxView*);
 
   void set_omnibox_has_focus_fn(OmniboxHasFocusFn fn) {
diff --git a/chrome/browser/ui/search/search_tab_helper_unittest.cc b/chrome/browser/ui/search/search_tab_helper_unittest.cc
index 6a5d82b..83a613e 100644
--- a/chrome/browser/ui/search/search_tab_helper_unittest.cc
+++ b/chrome/browser/ui/search/search_tab_helper_unittest.cc
@@ -22,6 +22,7 @@
 #include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/ui/search/search_ipc_router.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/ntp/ntp_user_data_logger.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/ntp_logging_events.h"
 #include "chrome/common/render_messages.h"
@@ -316,6 +317,46 @@ TEST_F(SearchTabHelperTest, OnHistorySyncCheckNotSyncing) {
   ASSERT_FALSE(base::get<0>(params));
 }
 
+TEST_F(SearchTabHelperTest, OnMostVisitedItemsChangedFromServer) {
+  InstantMostVisitedItem item;
+  item.is_server_side_suggestion = true;
+  std::vector<InstantMostVisitedItem> items;
+  items.push_back(item);
+
+  SearchTabHelper* search_tab_helper =
+      SearchTabHelper::FromWebContents(web_contents());
+  ASSERT_NE(static_cast<SearchTabHelper*>(NULL), search_tab_helper);
+
+  auto logger = NTPUserDataLogger::GetOrCreateFromWebContents(web_contents());
+  ASSERT_FALSE(logger->has_server_side_suggestions_);
+  ASSERT_FALSE(logger->has_client_side_suggestions_);
+
+  search_tab_helper->MostVisitedItemsChanged(items);
+
+  ASSERT_TRUE(logger->has_server_side_suggestions_);
+  ASSERT_FALSE(logger->has_client_side_suggestions_);
+}
+
+TEST_F(SearchTabHelperTest, OnMostVisitedItemsChangedFromClient) {
+  InstantMostVisitedItem item;
+  item.is_server_side_suggestion = false;
+  std::vector<InstantMostVisitedItem> items;
+  items.push_back(item);
+
+  SearchTabHelper* search_tab_helper =
+      SearchTabHelper::FromWebContents(web_contents());
+  ASSERT_NE(static_cast<SearchTabHelper*>(NULL), search_tab_helper);
+
+  auto logger = NTPUserDataLogger::GetOrCreateFromWebContents(web_contents());
+  ASSERT_FALSE(logger->has_server_side_suggestions_);
+  ASSERT_FALSE(logger->has_client_side_suggestions_);
+
+  search_tab_helper->MostVisitedItemsChanged(items);
+
+  ASSERT_FALSE(logger->has_server_side_suggestions_);
+  ASSERT_TRUE(logger->has_client_side_suggestions_);
+}
+
 class TabTitleObserver : public content::WebContentsObserver {
  public:
   explicit TabTitleObserver(content::WebContents* contents)
diff --git a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
index a1c749a..af3c6f8 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/common/ntp_logging_events.h"
@@ -63,6 +64,11 @@ class NTPUserDataLogger
  private:
   friend class content::WebContentsUserData<NTPUserDataLogger>;
 
+  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
+                           OnMostVisitedItemsChangedFromServer);
+  FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
+                           OnMostVisitedItemsChangedFromClient);
+
   // True if at least one iframe came from a server-side suggestion.
   bool has_server_side_suggestions_;
 
diff --git a/chrome/common/instant_types.h b/chrome/common/instant_types.h
index 617f8f9..fe66a4e 100644
--- a/chrome/common/instant_types.h
+++ b/chrome/common/instant_types.h
@@ -142,6 +142,10 @@ struct InstantMostVisitedItem {
   // The external URL that should be pinged when this item is suggested/clicked.
   GURL impression_url;
   GURL click_url;
+
+  // True if it's a server side suggestion.
+  // Otherwise, it's a client side suggestion.
+  bool is_server_side_suggestion;
 };
 
 // An InstantMostVisitedItem along with its assigned restricted ID.