Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum

chrome/renderer/searchbox/searchbox_extension.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/searchbox/searchbox_extension.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/command_line.h"
@@ skipping 1042 matching lines @@
   }
 }
 
 // static
 void SearchBoxExtensionWrapper::LogMostVisitedImpression(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   content::RenderView* render_view = GetRenderViewWithCheckedOrigin(
       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
 void SearchBoxExtensionWrapper::LogMostVisitedNavigation(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   content::RenderView* render_view = GetRenderViewWithCheckedOrigin(
       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
 void SearchBoxExtensionWrapper::Paste(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   content::RenderView* render_view = GetRenderView();
   if (!render_view) return;
 
   base::string16 text;
   if (!args[0]->IsUndefined())
@@ skipping 57 matching lines @@
   if (!render_view) return;
 
   bool display_instant_results =
       SearchBox::Get(render_view)->display_instant_results();
   DVLOG(1) << render_view << " GetDisplayInstantResults" <<
       display_instant_results;
   args.GetReturnValue().Set(display_instant_results);
 }
 
 }  // namespace extensions_v8