Refactor omnibox focus API into a single method.

chrome/browser/ui/search/instant_controller.cc

Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc
index 8dcc23badcd2c8ac4034bb6080f57d5ad555c2a2..7901607fd77214f3c8d9eeaa16a9b5c52d80aa9e 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -1169,31 +1169,24 @@ void InstantController::ShowInstantOverlay(const content::WebContents* contents,
     ShowOverlay(height, units);
 }
 
-void InstantController::FocusOmnibox(const content::WebContents* contents) {
+void InstantController::FocusOmnibox(const content::WebContents* contents,
+                                     OmniboxFocusState state) {
   if (!extended_enabled_)
     return;
 
   DCHECK(IsContentsFrom(instant_tab(), contents));
-  browser_->FocusOmnibox(true);
-}
-
-void InstantController::StartCapturingKeyStrokes(
-    const content::WebContents* contents) {
-  if (!extended_enabled_)
-    return;
-
-  DCHECK(IsContentsFrom(instant_tab(), contents));
-  browser_->FocusOmnibox(false);
-}
-
-void InstantController::StopCapturingKeyStrokes(
-    content::WebContents* contents) {
-  // Nothing to do if omnibox doesn't have invisible focus.
-  if (!extended_enabled_ || omnibox_focus_state_ != OMNIBOX_FOCUS_INVISIBLE)
-    return;
-
-  DCHECK(IsContentsFrom(instant_tab(), contents));
-  contents->GetView()->Focus();
+  switch (state) {
+    case OMNIBOX_FOCUS_VISIBLE:
+      browser_->FocusOmnibox(true);
+      break;
+    case OMNIBOX_FOCUS_INVISIBLE:
+      browser_->FocusOmnibox(false);
+      break;
+    case OMNIBOX_FOCUS_NONE:
+      if (omnibox_focus_state_ != OMNIBOX_FOCUS_INVISIBLE)
+        contents->GetView()->Focus();
+      break;
+  }

[jln (very slow on Chromium), 2013/04/05 21:32:45]

I'm surprised the compiler didn't complain about default: here.

Please, add a default: statement and a "CHECK(false);  // An attacker may have
sent a malicious state index".

Don't make it a DCHECK or a NOTREACHED(), perhaps even add a comment that this
should be a CHECK so that no-one is tempted to change it.

[sreeram, 2013/04/05 21:47:13]

All the enum values are explicitly handled here, so that's why the compiler
doesn't complain. I think it's probably undefined behaviour (note: I Am Not A
Standards Expert) if |state| is not one of those values, and the compiler is
free to do whatever it wants.

This is in fact, the recommended way to write switch() statements for enums. If
somebody later adds a new value for the enum without updating this code, the
compiler will complain that it's not being handled here.
This will defeat the compile time check mentioned above, but I see what you are
saying about the IPC having untrusted values. Is it really a problem though? Say
|state| was some arbitrary value. Assuming the compiler doesn't cause the
program to crash-and-burn due to undefined behaviour, the worst that happens is
that this method does nothing. I think that's a fine tradeoff (given the ability
to catch at compile time any new value being added to the enum).

[jln (very slow on Chromium), 2013/04/05 21:53:59]

It's all about review-ability. It's easy to assert that nothing will go wrong
for security if you have a CHECK(). Otherwise, a security reviewer, often not
familiar at all with your code, will have a really hard time assessing that.
Also it could regress.

Undefined behavior, or hard-to-define behavior is the enemy for security.

In this particular case, not a huge deal. But a goal of these new IPC security
review is to help developers understand the paranoid mindset :)

[sreeram, 2013/04/05 22:03:38]

Okay, how about we don't add the "default:" clause, to help catch errors at
compile time, but also document the situation for the reasons you mentioned.
Something like:
// An attacker may send a bogus |state| value, but we won't do anything in that
// case, since we only handle the following explicitly listed values.
(@kmadhusu will likely come up with better wording)

[kmadhusu, 2013/04/05 22:08:51]

sreeram@: How about adding a COMPILE_ASSERT macro in the default case? By doing
that, we can address both the concerns.

Ref:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[jln (very slow on Chromium), 2013/04/05 22:32:44]

This wouldn't work and needs to be a run-time check. The problem is that we're
sending an IPC, so "type checking" would need to happen at run-time.
Unfortunately, the IPC run-time checking isn't as strict as it should.

What's more, in this situation a cast from an int will happen implicitly and the
compiler would in theory be at liberty to optimize out the default: case.

Adding Tom in case he has some thoughts about how we could improve this at the
IPC layer.

[sreeram, 2013/04/05 22:38:48]

So it turns out enum variables are explicitly allowed to have any value that
their underlying type allows. I.e., if this enum is stored as an int, all int
values (-2^31 to 2^31-1) are legal.

So there's no undefined behaviour here. Given that, what's the point of a CHECK?
Say an evil renderer sent us an IPC with a bogus |state|. Why should we crash
the browser? I'd be in favour of killing the renderer, if anything. I'm also
fine with the "ignore it, do nothing, keep calm and carry on" approach so far in
this CL.

[jln (very slow on Chromium), 2013/04/05 22:43:50]

As explained above, it's much harder to assess the security implications if we
just "carry on". Also this can regress and become a security issue if we do
"nothing" in this function. It's very easy to expect this function to never to
nothing.

A CHECK here won't have any run-time impact, can catch bugs and can also catch
an attacker, what's not to like?

To re-iterate, I'm not particularly worried about this particular case, but it's
good design / practice.

[kmadhusu, 2013/04/05 23:51:37]

Added a detailed comment. PTAL.

 }
 
 void InstantController::NavigateToURL(const content::WebContents* contents,