Redesigns the text input focus handling.

ui/views/controls/webview/webview.cc

 // Copyright (c) 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 "ui/views/controls/webview/webview.h"
 
 #include "content/public/browser/browser_accessibility_state.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 #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"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/views_delegate.h"
 
 namespace views {
 
 // static
 const char WebView::kViewClassName[] = "WebView";
@@ skipping 76 matching lines @@
   PreferredSizeChanged();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // WebView, View overrides:
 
 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());
 }
 
 void WebView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   if (details.is_add)
     AttachWebContents();
 }
 
@@ skipping 64 matching lines @@
 }
 
 bool WebView::EmbedsFullscreenWidget() const {
   DCHECK(wc_owner_.get());
   return embed_fullscreen_widget_mode_enabled_;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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();
   if (focus_manager && focus_manager->GetFocusedView() == this)
     OnFocus();
 }
 
 void WebView::WebContentsDestroyed(content::WebContents* web_contents) {
   // We watch for destruction of WebContents that we host but do not own. If we
   // own a WebContents that is being destroyed, we're doing the destroying, so
@@ skipping 21 matching lines @@
   if (!GetWidget() || !web_contents())
     return;
 
   const gfx::NativeView view_to_attach = is_embedding_fullscreen_widget_ ?
       web_contents()->GetFullscreenRenderWidgetHostView()->GetNativeView() :
       web_contents()->GetView()->GetNativeView();
   if (wcv_holder_->native_view() == view_to_attach)
     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_) {
     web_contents()->SetParentNativeViewAccessible(
         parent()->GetNativeViewAccessible());
   }
 #endif
 }
 
 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.

       // TODO(beng): This should either not be necessary, or be done implicitly
       // by NativeViewHostWin on Detach(). As it stands, this is needed so that
       // the of the detached contents knows to tell the renderer it's been
       // hidden.
       //
       // Moving this out of here would also mean we wouldn't be potentially
       // calling member functions on a half-destroyed WebContents.
       ShowWindow(web_contents()->GetView()->GetNativeView(), SW_HIDE);
 #else
       web_contents()->SetParentNativeViewAccessible(NULL);
@@ skipping 21 matching lines @@
   if (!contents) {
     content::WebContents::CreateParams create_params(
         browser_context, NULL);
     return content::WebContents::Create(create_params);
   }
 
   return contents;
 }
 
 }  // namespace views