NTP: Record TileType metrics also on desktop

chrome/renderer/searchbox/searchbox_extension.cc

Index: chrome/renderer/searchbox/searchbox_extension.cc
diff --git a/chrome/renderer/searchbox/searchbox_extension.cc b/chrome/renderer/searchbox/searchbox_extension.cc
index c2134402bcf620617c53fe502aac6b83f4e9e1a8..3ba1b8df4ee3e47440caf2c6d48a66c8ee632bad 100644
--- a/chrome/renderer/searchbox/searchbox_extension.cc
+++ b/chrome/renderer/searchbox/searchbox_extension.cc
@@ -23,6 +23,7 @@
 #include "chrome/renderer/searchbox/searchbox.h"
 #include "components/crx_file/id_util.h"
 #include "components/ntp_tiles/tile_source.h"
+#include "components/ntp_tiles/tile_visual_type.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
@@ -994,18 +995,23 @@ void SearchBoxExtensionWrapper::LogMostVisitedImpression(
   if (!render_frame)
     return;
 
-  if (args.Length() < 2 || !args[0]->IsNumber() || !args[1]->IsNumber()) {
+  if (args.Length() < 3 || !args[0]->IsNumber() || !args[1]->IsNumber() ||
+      !args[2]->IsNumber()) {
     ThrowInvalidParameters(args);
     return;
   }
 
   DVLOG(1) << render_frame << " LogMostVisitedImpression";
 
-  if (args[1]->Uint32Value() <= static_cast<int>(ntp_tiles::TileSource::LAST)) {
+  if (args[1]->Uint32Value() <= static_cast<int>(ntp_tiles::TileSource::LAST) &&
+      args[2]->Uint32Value() <= ntp_tiles::MAX_RECORDED_TILE_TYPE) {

[sfiera, 2017/04/06 08:53:28]

Can we instead check here only that args[2] is a valid enum value, and let
ntp_tiles::metrics decide whether that value is recorded or not? As written,
this would change (0, TileSource::TOP_SITES, TileVisualType::UNKNOWN) to stop
logging the CLIENT impression because UNKNOWN isn't recorded. (even if that
tuple of arguments shouldn't actually happen)

[Marc Treib, 2017/04/06 09:16:44]

Done.

     ntp_tiles::TileSource tile_source =
         static_cast<ntp_tiles::TileSource>(args[1]->Uint32Value());
+    ntp_tiles::TileVisualType tile_type =

[sfiera, 2017/04/06 08:53:28]

Optional: with the static_cast<>, seems like a good place for auto.

[Marc Treib, 2017/04/06 09:16:44]

Done.

+        static_cast<ntp_tiles::TileVisualType>(args[2]->Uint32Value());
     SearchBox::Get(render_frame)
-        ->LogMostVisitedImpression(args[0]->IntegerValue(), tile_source);
+        ->LogMostVisitedImpression(args[0]->IntegerValue(), tile_source,
+                                   tile_type);
   }
 }
 
@@ -1024,11 +1030,15 @@ void SearchBoxExtensionWrapper::LogMostVisitedNavigation(
 
   DVLOG(1) << render_frame << " LogMostVisitedNavigation";
 
-  if (args[1]->Uint32Value() <= static_cast<int>(ntp_tiles::TileSource::LAST)) {
+  if (args[1]->Uint32Value() <= static_cast<int>(ntp_tiles::TileSource::LAST) &&
+      args[2]->Uint32Value() <= ntp_tiles::MAX_RECORDED_TILE_TYPE) {
     ntp_tiles::TileSource tile_source =
         static_cast<ntp_tiles::TileSource>(args[1]->Uint32Value());
+    ntp_tiles::TileVisualType tile_type =
+        static_cast<ntp_tiles::TileVisualType>(args[2]->Uint32Value());
     SearchBox::Get(render_frame)
-        ->LogMostVisitedNavigation(args[0]->IntegerValue(), tile_source);
+        ->LogMostVisitedNavigation(args[0]->IntegerValue(), tile_source,
+                                   tile_type);
   }
 }