Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum

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 e2aa8ab777589f1b1d22aa9510ddebf8136be31c..6f307f6276b5e63ecaa6cb2bee0d32361494e325 100644
--- a/chrome/renderer/searchbox/searchbox_extension.cc
+++ b/chrome/renderer/searchbox/searchbox_extension.cc
@@ -1060,15 +1060,19 @@ void SearchBoxExtensionWrapper::LogMostVisitedImpression(
       GURL(chrome::kChromeSearchMostVisitedUrl));
   if (!render_view) return;
 
-  if (args.Length() < 2 || !args[0]->IsNumber() || args[1]->IsUndefined()) {
+  if (args.Length() < 2 || !args[0]->IsNumber() || !args[1]->IsNumber()) {
     ThrowInvalidParameters(args);
     return;
   }
 
   DVLOG(1) << render_view << " LogMostVisitedImpression";
 
-  SearchBox::Get(render_view)->LogMostVisitedImpression(
-      args[0]->IntegerValue(), V8ValueToUTF16(args[1]));
+  if (args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {

[dcheng, 2016/07/06 01:51:25]

Nit: I have a weak preference for making this a DCHECK, since this should
"never" happen. If a renderer wants to shoot itself in the foot, then this
wouldn't really do anything anyway.

[Marc Treib, 2016/07/06 08:49:07]

But the same would apply for all the other validity checks in this file, such as
number and types of arguments.
I guess a ThrowInvalidParameters might be appropriate? TBH, I just copied the
NTPLoggingEventType parsing from above...

[Marc Treib, 2016/07/06 08:55:56]

Hm, then again, looking at the code around, nothing seems to
ThrowInvalidParameters on invalid values. I'm not familiar enough with all this
to say what the Right Thing is, but I also think it won't make much of a
difference in practice.

[dcheng, 2016/07/06 09:01:28]

Shrug, if it's copy and pasted, I guess that's fine. I just personally prefer to
visibly error out--in particular, with a DCHECK, since this really is an assert
of a condition that should always be true--since hitting this case means the
developer has a typo or something.

+    NTPLoggingTileSource tile_source =
+        static_cast<NTPLoggingTileSource>(args[1]->Uint32Value());
+    SearchBox::Get(render_view)->LogMostVisitedImpression(
+        args[0]->IntegerValue(), tile_source);
+  }
 }
 
 // static
@@ -1078,15 +1082,19 @@ void SearchBoxExtensionWrapper::LogMostVisitedNavigation(
       GURL(chrome::kChromeSearchMostVisitedUrl));
   if (!render_view) return;
 
-  if (args.Length() < 2 || !args[0]->IsNumber() || args[1]->IsUndefined()) {
+  if (args.Length() < 2 || !args[0]->IsNumber() || !args[1]->IsNumber()) {
     ThrowInvalidParameters(args);
     return;
   }
 
   DVLOG(1) << render_view << " LogMostVisitedNavigation";
 
-  SearchBox::Get(render_view)->LogMostVisitedNavigation(
-      args[0]->IntegerValue(), V8ValueToUTF16(args[1]));
+  if (args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {
+    NTPLoggingTileSource tile_source =
+        static_cast<NTPLoggingTileSource>(args[1]->Uint32Value());
+    SearchBox::Get(render_view)->LogMostVisitedNavigation(
+        args[0]->IntegerValue(), tile_source);
+  }
 }
 
 // static