Refactoring some of the NativeViewHost and NativeControl focus management so their consumers don't have to explicitly set the focused view.

BUG=None
TEST=Run all tests. Make sure focus is stored/restored properly in Chrome.

Review URL: http://codereview.chromium.org/214029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27113 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 139 insertions, 95 deletions

diff --git a/views/controls/native/native_view_host.cc b/views/controls/native/native_view_host.cc
index 9cbeaf7..b5eb509 100644
--- a/views/controls/native/native_view_host.cc
+++ b/views/controls/native/native_view_host.cc
@@ -7,6 +7,7 @@
 #include "base/logging.h"
 #include "app/gfx/canvas.h"
 #include "views/controls/native/native_view_host_wrapper.h"
+#include "views/focus/focus_manager.h"
 #include "views/widget/widget.h"
 
 namespace views {
@@ -20,7 +21,10 @@ const char NativeViewHost::kViewClassName[] = "views/NativeViewHost";
 NativeViewHost::NativeViewHost()
     : native_view_(NULL),
       fast_resize_(false),
-      focus_view_(NULL) {
+      focus_native_view_(NULL) {
+  // By default we make this view the one that should be set as the focused view
+  // when the native view gets the focus.
+  focus_view_ = this;
   // The native widget is placed relative to the root. As such, we need to
   // know when the position of any ancestor changes, or our visibility relative
   // to other views changed as it'll effect our position relative to the root.
@@ -33,10 +37,12 @@ NativeViewHost::~NativeViewHost() {
 void NativeViewHost::Attach(gfx::NativeView native_view) {
   DCHECK(!native_view_);
   native_view_ = native_view;
-  // If set_focus_view() has not been invoked, this view is the one that should
-  // be seen as focused when the native view receives focus.
-  if (!focus_view_)
-    focus_view_ = this;
+  // If this NativeViewHost is the focus view, than it should obviously be
+  // focusable.  If it is not, then it acts as a container and we don't want it
+  // to get focus through tab-traversal, so we make it not focusable.
+  SetFocusable(focus_view_ == this);
+  if (!focus_native_view_)
+    focus_native_view_ = native_view;
   native_wrapper_->NativeViewAttached();
 }
 
@@ -44,6 +50,7 @@ void NativeViewHost::Detach() {
   DCHECK(native_view_);
   native_wrapper_->NativeViewDetaching();
   native_view_ = NULL;
+  focus_native_view_ = NULL;
 }
 
 void NativeViewHost::SetPreferredSize(const gfx::Size& size) {
@@ -57,6 +64,22 @@ void NativeViewHost::NativeViewDestroyed() {
   native_view_ = NULL;
 }
 
+void NativeViewHost::GotNativeFocus() {
+  // Some NativeViewHost may not have an associated focus view.  This is the
+  // case for containers for example.  They might still get the native focus,
+  // if one non native view they contain gets focused.  In that case, we should
+  // not change the focused view.
+  if (!focus_view_ || !focus_view_->IsFocusable())
+    return;
+
+  FocusManager* focus_manager = GetFocusManager();
+  if (!focus_manager) {
+    NOTREACHED();
+    return;
+  }
+  focus_manager->SetFocusedView(focus_view_);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // NativeViewHost, View overrides:
 
@@ -134,7 +157,12 @@ std::string NativeViewHost::GetClassName() const {
 }
 
 void NativeViewHost::Focus() {
-  native_wrapper_->SetFocus();
+  FocusManager* focus_manager = GetFocusManager();
+  if (!focus_manager) {
+    NOTREACHED();
+    return;
+  }
+  focus_manager->FocusNativeView(focus_native_view_);
 }
 
 }  // namespace views
diff --git a/views/controls/native/native_view_host.h b/views/controls/native/native_view_host.h
index 679e176..d27a816e 100644
--- a/views/controls/native/native_view_host.h
+++ b/views/controls/native/native_view_host.h
@@ -42,15 +42,27 @@ class NativeViewHost : public View {
   // Sets a preferred size for the native view attached to this View.
   void SetPreferredSize(const gfx::Size& size);
 
-  // A NativeViewHost has an associated focus View so that the focus of the
-  // native control and of the View are kept in sync. In simple cases where the
-  // NativeViewHost directly wraps a native window as is, the associated view
-  // is this View. In other cases where the NativeViewHost is part of another
-  // view (such as TextField), the actual View is not the NativeViewHost and
-  // this method must be called to set that.
-  // This method must be called before Attach().
+  // A NativeViewHost must keep the focused view in the focus manager and the
+  // native focus with in sync . There are 2 aspects to this:
+  // - when the native view receives focus, the focus manager must be notified
+  //   that the associated view is now the focused view.
+  //   In simple cases where the NativeViewHost directly wraps a native window
+  //   as is, the associated view is this NativeViewHost. In other cases where
+  //   the NativeViewHost is part of another view (such as for the TextField
+  //   class), the actual View is not the NativeViewHost and set_focus_view()
+  //   must be called to set the associated view before Attach() is called.
+  // - when the view is focused (by calling View::RequestFocus()), it must focus
+  //   the appropriate native view.  In simple cases where the native view does
+  //   not have children or is the native view that should really get the focus,
+  //   this works without doing anything.  In case where the native view that
+  //   should get the focus is not the native view passed to Attach(), then
+  //   the appropriate native view should be specified to
+  //   set_focus_native_view() before Attach() is called.
   void set_focus_view(View* view) { focus_view_ = view; }
-  View* focus_view() { return focus_view_; }
+  void set_focus_native_view(gfx::NativeView native_view) {
+    focus_native_view_ = native_view;
+  }
+  gfx::NativeView focus_native_view() const { return focus_native_view_; }
 
   // Fast resizing will move the native view and clip its visible region, this
   // will result in white areas and will not resize the content (so scrollbars
@@ -64,6 +76,10 @@ class NativeViewHost : public View {
   // Accessor for |native_view_|.
   gfx::NativeView native_view() const { return native_view_; }
 
+  // Called by the NativeViewHostWrapper to notify that the |focus_native_view_|
+  // got focus.
+  void GotNativeFocus();
+
   void NativeViewDestroyed();
 
   // Overridden from View:
@@ -96,6 +112,9 @@ class NativeViewHost : public View {
   // The view that should be given focus when this NativeViewHost is focused.
   View* focus_view_;
 
+  // The native view that should get the focus when this View gets focused.
+  gfx::NativeView focus_native_view_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeViewHost);
 };
 
diff --git a/views/controls/native/native_view_host_gtk.cc b/views/controls/native/native_view_host_gtk.cc
index ea9bd62..4b9fc0c 100644
--- a/views/controls/native/native_view_host_gtk.cc
+++ b/views/controls/native/native_view_host_gtk.cc
@@ -48,7 +48,7 @@ void NativeViewHostGtk::NativeViewAttached() {
   }
 
   if (!focus_signal_id_) {
-    focus_signal_id_ = g_signal_connect(G_OBJECT(host_->native_view()),
+    focus_signal_id_ = g_signal_connect(G_OBJECT(host_->focus_native_view()),
                                         "focus-in-event",
                                         G_CALLBACK(CallFocusIn), this);
   }
@@ -70,7 +70,7 @@ void NativeViewHostGtk::NativeViewDetaching() {
                               destroy_signal_id_);
   destroy_signal_id_ = 0;
 
-  g_signal_handler_disconnect(G_OBJECT(host_->native_view()),
+  g_signal_handler_disconnect(G_OBJECT(host_->focus_native_view()),
                               focus_signal_id_);
   focus_signal_id_ = 0;
 
@@ -164,11 +164,6 @@ void NativeViewHostGtk::HideWidget() {
   gtk_widget_hide(fixed_);
 }
 
-void NativeViewHostGtk::SetFocus() {
-  DCHECK(host_->native_view());
-  gtk_widget_grab_focus(host_->native_view());
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // NativeViewHostGtk, private:
 
@@ -215,19 +210,11 @@ void NativeViewHostGtk::CallDestroy(GtkObject* object,
 }
 
 // static
-void NativeViewHostGtk::CallFocusIn(GtkWidget* widget,
-                                    GdkEventFocus* event,
-                                    NativeViewHostGtk* host) {
-  FocusManager* focus_manager =
-      FocusManager::GetFocusManagerForNativeView(widget);
-  if (!focus_manager) {
-    // TODO(jcampan): http://crbug.com/21378 Reenable this NOTREACHED() when the
-    // options page is only based on views.
-    // NOTREACHED();
-    NOTIMPLEMENTED();
-    return;
-  }
-  focus_manager->SetFocusedView(host->host_->focus_view());
+gboolean NativeViewHostGtk::CallFocusIn(GtkWidget* widget,
+                                        GdkEventFocus* event,
+                                        NativeViewHostGtk* host) {
+  host->host_->GotNativeFocus();
+  return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/views/controls/native/native_view_host_gtk.h b/views/controls/native/native_view_host_gtk.h
index 81a2a7a1..2e58bef 100644
--- a/views/controls/native/native_view_host_gtk.h
+++ b/views/controls/native/native_view_host_gtk.h
@@ -36,7 +36,6 @@ class NativeViewHostGtk : public NativeViewHostWrapper {
   virtual void UninstallClip();
   virtual void ShowWidget(int x, int y, int w, int h);
   virtual void HideWidget();
-  virtual void SetFocus();
 
  private:
   // Create and Destroy the GtkFixed that performs clipping on our hosted
@@ -55,9 +54,9 @@ class NativeViewHostGtk : public NativeViewHostWrapper {
   static void CallDestroy(GtkObject* object, NativeViewHostGtk* host);
 
   // Invoked from the 'focus-in-event' signal.
-  static void CallFocusIn(GtkWidget* widget,
-                          GdkEventFocus* event,
-                          NativeViewHostGtk* button);
+  static gboolean CallFocusIn(GtkWidget* widget,
+                              GdkEventFocus* event,
+                              NativeViewHostGtk* button);
 
   // Our associated NativeViewHost.
   NativeViewHost* host_;
@@ -86,4 +85,3 @@ class NativeViewHostGtk : public NativeViewHostWrapper {
 }  // namespace views
 
 #endif  // VIEWS_CONTROLS_NATIVE_HOST_VIEW_GTK_H_
-
diff --git a/views/controls/native/native_view_host_win.cc b/views/controls/native/native_view_host_win.cc
index d1a5217..3c05e2a 100644
--- a/views/controls/native/native_view_host_win.cc
+++ b/views/controls/native/native_view_host_win.cc
@@ -6,10 +6,13 @@
 
 #include "app/gfx/canvas.h"
 #include "base/logging.h"
+#include "base/win_util.h"
 #include "views/controls/native/native_view_host.h"
 #include "views/focus/focus_manager.h"
 #include "views/widget/widget.h"
 
+const wchar_t* kNativeViewHostWinKey = L"__NATIVE_VIEW_HOST_WIN__";
+
 namespace views {
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -17,7 +20,8 @@ namespace views {
 
 NativeViewHostWin::NativeViewHostWin(NativeViewHost* host)
     : host_(host),
-      installed_clip_(false) {
+      installed_clip_(false),
+      original_wndproc_(NULL) {
 }
 
 NativeViewHostWin::~NativeViewHostWin() {
@@ -37,10 +41,32 @@ void NativeViewHostWin::NativeViewAttached() {
   // Need to set the HWND's parent before changing its size to avoid flashing.
   SetParent(host_->native_view(), host_->GetWidget()->GetNativeView());
   host_->Layout();
+
+  // Subclass the appropriate HWND to get focus notifications.
+  HWND focus_hwnd = host_->focus_native_view();
+  DCHECK(focus_hwnd == host_->native_view() ||
+         ::IsChild(host_->native_view(), focus_hwnd));
+  original_wndproc_ =
+      win_util::SetWindowProc(focus_hwnd,
+                              &NativeViewHostWin::NativeViewHostWndProc);
+
+  // We use a property to retrieve the NativeViewHostWin from the window
+  // procedure.
+  ::SetProp(focus_hwnd, kNativeViewHostWinKey, this);
 }
 
 void NativeViewHostWin::NativeViewDetaching() {
   installed_clip_ = false;
+
+  // Restore the original Windows procedure.
+  DCHECK(original_wndproc_);
+  WNDPROC wndproc = win_util::SetWindowProc(host_->focus_native_view(),
+                                            original_wndproc_);
+  DCHECK(wndproc == &NativeViewHostWin::NativeViewHostWndProc);
+
+  // Also remove the property, it's not needed anymore.
+  HANDLE h = ::RemoveProp(host_->focus_native_view(), kNativeViewHostWinKey);
+  DCHECK(h == this);
 }
 
 void NativeViewHostWin::AddedToWidget() {
@@ -116,8 +142,22 @@ void NativeViewHostWin::HideWidget() {
                SWP_NOREDRAW | SWP_NOOWNERZORDER);
 }
 
-void NativeViewHostWin::SetFocus() {
-  ::SetFocus(host_->native_view());
+// static
+LRESULT CALLBACK NativeViewHostWin::NativeViewHostWndProc(HWND window,
+                                                          UINT message,
+                                                          WPARAM w_param,
+                                                          LPARAM l_param) {
+  NativeViewHostWin* native_view_host =
+      static_cast<NativeViewHostWin*>(::GetProp(window, kNativeViewHostWinKey));
+  DCHECK(native_view_host);
+
+  if (message == WM_SETFOCUS)
+    native_view_host->host_->GotNativeFocus();
+  if (message == WM_DESTROY)
+    native_view_host->host_->Detach();
+
+  return CallWindowProc(native_view_host->original_wndproc_,
+                        window, message, w_param, l_param);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/views/controls/native/native_view_host_win.h b/views/controls/native/native_view_host_win.h
index 51bc616..6224693 100644
--- a/views/controls/native/native_view_host_win.h
+++ b/views/controls/native/native_view_host_win.h
@@ -2,8 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef VIEWS_CONTROLS_HWND_VIEW_H_
-#define VIEWS_CONTROLS_HWND_VIEW_H_
+#ifndef VIEWS_CONTROL_NATIVE_NATIVE_VIEW_HOST_WIN_H_
+#define VIEWS_CONTROL_NATIVE_NATIVE_VIEW_HOST_WIN_H_
+
+#include "windows.h"
 
 #include "base/logging.h"
 #include "views/controls/native/native_view_host_wrapper.h"
@@ -28,9 +30,14 @@ class NativeViewHostWin : public NativeViewHostWrapper {
   virtual void UninstallClip();
   virtual void ShowWidget(int x, int y, int w, int h);
   virtual void HideWidget();
-  virtual void SetFocus();
 
  private:
+  // Our subclass window procedure.
+  static LRESULT CALLBACK NativeViewHostWndProc(HWND window,
+                                                UINT message,
+                                                WPARAM w_param,
+                                                LPARAM l_param);
+
   // Our associated NativeViewHost.
   NativeViewHost* host_;
 
@@ -38,9 +45,12 @@ class NativeViewHostWin : public NativeViewHostWrapper {
   // visible portion of the gfx::NativeView ?
   bool installed_clip_;
 
+  // The window procedure before we subclassed.
+  WNDPROC original_wndproc_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeViewHostWin);
 };
 
 }  // namespace views
 
-#endif  // VIEWS_CONTROLS_HWND_VIEW_H_
+#endif  // VIEWS_CONTROL_NATIVE_NATIVE_VIEW_HOST_WIN_H_
diff --git a/views/controls/native/native_view_host_wrapper.h b/views/controls/native/native_view_host_wrapper.h
index 948f0ce..483aff5 100644
--- a/views/controls/native/native_view_host_wrapper.h
+++ b/views/controls/native/native_view_host_wrapper.h
@@ -51,9 +51,6 @@ class NativeViewHostWrapper {
   // is already hidden.
   virtual void HideWidget() = 0;
 
-  // Sets focus to the gfx::NativeView.
-  virtual void SetFocus() = 0;
-
   // Creates a platform-specific instance of an object implementing this
   // interface.
   static NativeViewHostWrapper* CreateWrapper(NativeViewHost* host);
diff --git a/views/controls/native_control.cc b/views/controls/native_control.cc
index 0c89664..86fd311 100644
--- a/views/controls/native_control.cc
+++ b/views/controls/native_control.cc
@@ -286,8 +286,6 @@ HWND NativeControl::GetNativeControlHWND() {
 }
 
 void NativeControl::NativeControlDestroyed() {
-  if (hwnd_view_)
-    hwnd_view_->Detach();
   container_ = NULL;
 }
 
diff --git a/views/controls/native_control_gtk.cc b/views/controls/native_control_gtk.cc
index 082e610..390e00d 100644
--- a/views/controls/native_control_gtk.cc
+++ b/views/controls/native_control_gtk.cc
@@ -63,26 +63,6 @@ void NativeControlGtk::NativeControlCreated(GtkWidget* native_control) {
 
   // Update the newly created GtkWdigetwith any resident enabled state.
   gtk_widget_set_sensitive(native_view(), IsEnabled());
-
-  // Listen for focus change event to update the FocusManager focused view.
-  g_signal_connect(G_OBJECT(native_control), "focus-in-event",
-                   G_CALLBACK(CallFocusIn), this);
-}
-
-// static
-void NativeControlGtk::CallFocusIn(GtkWidget* widget,
-                                   GdkEventFocus* event,
-                                   NativeControlGtk* control) {
-  FocusManager* focus_manager =
-      FocusManager::GetFocusManagerForNativeView(widget);
-  if (!focus_manager) {
-    // TODO(jcampan): http://crbug.com/21378 Reenable this NOTREACHED() when the
-    // options page is only based on views.
-    // NOTREACHED();
-    NOTIMPLEMENTED();
-    return;
-  }
-  focus_manager->SetFocusedView(control->focus_view());
 }
 
 }  // namespace views
diff --git a/views/controls/native_control_gtk.h b/views/controls/native_control_gtk.h
index ce2f187..69f650d 100644
--- a/views/controls/native_control_gtk.h
+++ b/views/controls/native_control_gtk.h
@@ -36,10 +36,6 @@ class NativeControlGtk : public NativeViewHost {
   virtual void NativeControlCreated(GtkWidget* widget);
 
  private:
-  static void CallFocusIn(GtkWidget* widget,
-                          GdkEventFocus* event,
-                          NativeControlGtk* button);
-
   DISALLOW_COPY_AND_ASSIGN(NativeControlGtk);
 };
 
diff --git a/views/controls/native_control_win.cc b/views/controls/native_control_win.cc
index 9f4d2d6..037066c 100644
--- a/views/controls/native_control_win.cc
+++ b/views/controls/native_control_win.cc
@@ -24,10 +24,9 @@ NativeControlWin::NativeControlWin() {
 NativeControlWin::~NativeControlWin() {
   HWND hwnd = native_view();
   if (hwnd) {
-    // Destroy the hwnd if it still exists. Otherwise we won't have shut things
-    // down correctly, leading to leaking and crashing if another message
-    // comes in for the hwnd.
-    Detach();
+    // Destroy the hwnd if it still exists. Otherwise we won't shut things down
+    // correctly, leading to leaking and crashing if another message comes in
+    // for the hwnd.
     DestroyWindow(hwnd);
   }
 }
@@ -74,9 +73,7 @@ void NativeControlWin::VisibilityChanged(View* starting_from, bool is_visible) {
   if (!is_visible) {
     // We destroy the child control HWND when we become invisible because of the
     // performance cost of maintaining many HWNDs.
-    HWND hwnd = native_view();
-    Detach();
-    DestroyWindow(hwnd);
+    DestroyWindow(native_view());
   } else if (!native_view()) {
     if (GetWidget())
       CreateNativeControl();
@@ -117,14 +114,14 @@ void NativeControlWin::NativeControlCreated(HWND native_control) {
   // Note that we never unset this property. We don't have to.
   SetProp(native_control, kNativeControlWinKey, this);
 
-  // Subclass so we get WM_KEYDOWN and WM_SETFOCUS messages.
+  Attach(native_control);
+  // native_view() is now valid.
+
+  // Subclass so we get WM_KEYDOWN message.
   original_wndproc_ =
       win_util::SetWindowProc(native_control,
                               &NativeControlWin::NativeControlWndProc);
 
-  Attach(native_control);
-  // native_view() is now valid.
-
   // Update the newly created HWND with any resident enabled state.
   EnableWindow(native_view(), IsEnabled());
 
@@ -186,16 +183,10 @@ LRESULT NativeControlWin::NativeControlWndProc(HWND window,
   if (message == WM_KEYDOWN &&
       native_control->OnKeyDown(static_cast<int>(w_param))) {
       return 0;
-  } else if (message == WM_SETFOCUS) {
-    // Let the focus manager know that the focus changed.
-    FocusManager* focus_manager = native_control->GetFocusManager();
-    if (focus_manager) {
-      focus_manager->SetFocusedView(native_control->focus_view());
-    } else {
-      NOTREACHED();
-    }
   } else if (message == WM_DESTROY) {
-    win_util::SetWindowProc(window, native_control->original_wndproc_);
+    WNDPROC old_wndproc =
+        win_util::SetWindowProc(window, native_control->original_wndproc_);
+    DCHECK(old_wndproc == &NativeControlWin::NativeControlWndProc);
   }
 
   return CallWindowProc(native_control->original_wndproc_, window, message,