Redesigns the text input focus handling.

ui/views/controls/webview/webview.cc

Index: ui/views/controls/webview/webview.cc
diff --git a/ui/views/controls/webview/webview.cc b/ui/views/controls/webview/webview.cc
index 4669c3ede4ac7a7827438711ee68e7c71d9de163..38cf7f908b1baff547f0176af82d3b08beca97d9 100644
--- a/ui/views/controls/webview/webview.cc
+++ b/ui/views/controls/webview/webview.cc
@@ -14,6 +14,7 @@
 #include "ipc/ipc_message.h"
 #include "ui/accessibility/ax_enums.h"
 #include "ui/accessibility/ax_view_state.h"
+#include "ui/base/ui_base_switches_util.h"
 #include "ui/events/event.h"
 #include "ui/views/accessibility/native_view_accessibility.h"
 #include "ui/views/controls/native/native_view_host.h"
@@ -110,6 +111,19 @@ const char* WebView::GetClassName() const {
   return kViewClassName;
 }
 
+ui::TextInputClient* WebView::GetTextInputClient() {
+  if (switches::IsTextInputFocusManagerEnabled() &&
+      web_contents() && !web_contents()->IsBeingDestroyed()) {
+    content::RenderWidgetHostView* host_view =
+        is_embedding_fullscreen_widget_ ?
+        web_contents()->GetFullscreenRenderWidgetHostView() :
+        web_contents()->GetRenderWidgetHostView();
+    if (host_view)
+      return host_view->GetTextInputClient();
+  }
+  return NULL;
+}
+
 void WebView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   wcv_holder_->SetSize(bounds().size());
 }
@@ -194,6 +208,16 @@ bool WebView::EmbedsFullscreenWidget() const {
 ////////////////////////////////////////////////////////////////////////////////
 // WebView, content::WebContentsObserver implementation:
 
+void WebView::RenderViewDeleted(content::RenderViewHost* render_view_host) {
+  // WebView::GetTextInputClient() delegates the text input handling to the

[msw, 2014/03/12 20:56:15]

With three nearly duplicate code and comment blocks, I suggest either:
A) Generalize the comment to mention any RWHV changes, put it in
WebView::GetTextInputClient, then remove the other comments (or put in stub
comments like "Update the TextInputClient as needed; see GetTextInputClient.")
or B) Make a helper function that does this with the comment there.

[Yuki, 2014/03/14 15:25:22]

Done with option A.

+  // underlying content::RenderWidgetHostView.  So when the underlying RWHV is
+  // destroyed, we have to notify the FocusManager that the focused
+  // TextInputClient needs to be updated.
+  FocusManager* const focus_manager = GetFocusManager();
+  if (focus_manager)
+    focus_manager->OnTextInputClientChanged(this);
+}
+
 void WebView::RenderViewHostChanged(content::RenderViewHost* old_host,
                                     content::RenderViewHost* new_host) {
   FocusManager* const focus_manager = GetFocusManager();
@@ -235,12 +259,20 @@ void WebView::AttachWebContents() {
     return;
   wcv_holder_->Attach(view_to_attach);
 
-  // The view will not be focused automatically when it is attached, so we need
-  // to pass on focus to it if the FocusManager thinks the view is focused. Note
-  // that not every Widget has a focus manager.
   FocusManager* const focus_manager = GetFocusManager();
-  if (focus_manager && focus_manager->GetFocusedView() == this)
-    OnFocus();
+  if (focus_manager) {
+    // WebView::GetTextInputClient() delegates the text input handling to the
+    // underlying content::RenderWidgetHostView.  So when the underlying RWHV is
+    // changed, we have to notify the FocusManager that the focused
+    // TextInputClient needs to be updated.
+    focus_manager->OnTextInputClientChanged(this);
+
+    // The view will not be focused automatically when it is attached, so we
+    // need to pass on focus to it if the FocusManager thinks the view is
+    // focused.
+    if (focus_manager->GetFocusedView() == this)
+      OnFocus();
+  }
 
 #if defined(OS_WIN)
   if (!is_embedding_fullscreen_widget_) {
@@ -253,6 +285,15 @@ void WebView::AttachWebContents() {
 void WebView::DetachWebContents() {
   if (web_contents()) {
     wcv_holder_->Detach();
+
+    // WebView::GetTextInputClient() delegates the text input handling to the
+    // underlying content::RenderWidgetHostView.  So when the underlying RWHV is
+    // changed, we have to notify the FocusManager that the focused
+    // TextInputClient needs to be updated.
+    FocusManager* const focus_manager = GetFocusManager();
+    if (focus_manager)
+      focus_manager->OnTextInputClientChanged(this);

[msw, 2014/03/12 20:56:15]

Will WebView::GetTextInputClient return a different/NULL value yet?

[Yuki, 2014/03/14 15:25:22]

Even after the call to wcv_holder_->Detach(), GetTextInputClient() returns the
same pointer (can be either of valid pointer or null).  However, I've moved the
code at the top of the function just in case.

[msw, 2014/03/17 19:48:58]

Actually, that was my concern. [Attach|Detach]WebContents or their callers
should change the output of GetTextInputClient(), to make these
OnTextInputClientChanged() calls necessary. It looks like SetWebContents changes
|web_contents()| and ReattachForFullscreenChange changes
|is_embedding_fullscreen_widget_|, which impact GetTextInputClient(). While
[Attach|Detach]WebContents are called within those functions, perhaps those
those functions themselves are be better places to call
OnTextInputClientChanged(). Hopefully someone that knows WebView better will
review, to give better advice.

Regardless, I would probably move |OnTextInputClientChanged| calls to the end of
whichever functions they are called from, so that any changes are already
applied when GetTextInputClient() is called.

[Yuki, 2014/04/16 09:24:08]

Thanks for the comment.  I'll add a webview reviewer.

As you suggested, I added a helper function NotifyMaybeTextInputClientChanged()
and made SetWebContents() and ReattachForFullscreenChange() call it.

+
 #if defined(OS_WIN)
     if (!is_embedding_fullscreen_widget_) {
 #if !defined(USE_AURA)

[msw, 2014/03/12 20:56:15]

nit: Rebase, this code is no longer present.

[Yuki, 2014/03/14 15:25:22]

Done.