Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only)

content/browser/renderer_host/render_widget_host_view_aura.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 "content/browser/renderer_host/render_widget_host_view_aura.h"
 
 #include <set>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 1420 matching lines @@
     cursor_client->ShowCursor();
   }
 
   host_->LostMouseLock();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, ui::TextInputClient implementation:
 void RenderWidgetHostViewAura::SetCompositionText(
     const ui::CompositionText& composition) {
   // TODO(wjmaclean): can host_ ever be null?

[kenrb, 2016/06/13 19:37:47]

Nit: That comment can be removed.

[EhsanK, 2016/06/17 22:36:19]

Acknowledged.

-  if (!host_)
+  if (!text_input_manager_ || !text_input_manager_->GetActiveWidget())
     return;
 
   // TODO(suzhe): convert both renderer_host and renderer to use
   // ui::CompositionText.
   std::vector<blink::WebCompositionUnderline> underlines;
   underlines.reserve(composition.underlines.size());
   for (std::vector<ui::CompositionUnderline>::const_iterator it =
            composition.underlines.begin();
        it != composition.underlines.end(); ++it) {
     underlines.push_back(
         blink::WebCompositionUnderline(static_cast<unsigned>(it->start_offset),
                                        static_cast<unsigned>(it->end_offset),
                                        it->color,
                                        it->thick,
                                        it->background_color));
   }
 
   // TODO(suzhe): due to a bug of webkit, we can't use selection range with
   // composition string. See: https://bugs.webkit.org/show_bug.cgi?id=37788
-  host_->ImeSetComposition(composition.text, underlines,
-                           gfx::Range::InvalidRange(),
-                           composition.selection.end(),
-                           composition.selection.end());
+  text_input_manager_->GetActiveWidget()->ImeSetComposition(

[kenrb, 2016/06/13 19:37:47]

I have a slight concern about nullptr derefs here.
RenderWidgetHostViewChildFrame has its view_ pointer cleared when Destroy() is
called, but it doesn't clear itself from TextInputManager's list until it is
actually deleted, and there is a delay between those things because
RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, I'd
like to investigate removing that deferred deletion when I have time.

[EhsanK, 2016/06/17 22:36:19]

Agreed. I think it is safe to unregister here rather than the dtor of the base
class.

[kenrb, 2016/06/20 16:04:06]

You need to either address that problem before this patch lands, or check for
null return on GetActiveWidget().

[EhsanK, 2016/06/22 18:26:32]

We already early return on top when GetActiveWidget() is nullptr (line 1442).

As far as the functionality of TextInputManager goes, unregistering at base dtor
is fine. But there will be a period of time between here and then that the
TextInputManager will return the child frame's widget as active and that seems
wrong. So unregistering here should be safer.

[kenrb, 2016/06/22 19:39:50]

Okay, I had missed the line above. This seems fine.

I don't quite understand that last comment, though. What do you mean by
unregistering 'here'? I don't think RenderWidgetHostViewAura should be
unregistering other RenderWidgetHostViews from the TextInputManager view list.

[EhsanK, 2016/06/22 20:56:43]

Sorry for that confusing line. I was thinking of another cl
here:https://codereview.chromium.org/2054163003/
where I decided to unregister a RWHVCF in its Destroy function.

+      composition.text, underlines, gfx::Range::InvalidRange(),
+      composition.selection.end(), composition.selection.end());
 
   has_composition_text_ = !composition.text.empty();
 }
 
 void RenderWidgetHostViewAura::ConfirmCompositionText() {
   // TODO(wjmaclean): can host_ ever be null?
-  if (host_ && has_composition_text_) {
-    host_->ImeConfirmComposition(base::string16(), gfx::Range::InvalidRange(),
-                                 false);
+  if (text_input_manager_ && text_input_manager_->GetActiveWidget() &&
+      has_composition_text_) {
+    text_input_manager_->GetActiveWidget()->ImeConfirmComposition(
+        base::string16(), gfx::Range::InvalidRange(), false);
   }
   has_composition_text_ = false;
 }
 
 void RenderWidgetHostViewAura::ClearCompositionText() {
   // TODO(wjmaclean): can host_ ever be null?
-  if (host_ && has_composition_text_)
-    host_->ImeCancelComposition();
+  if (text_input_manager_ && text_input_manager_->GetActiveWidget() &&
+      has_composition_text_)
+    text_input_manager_->GetActiveWidget()->ImeCancelComposition();
   has_composition_text_ = false;
 }
 
 void RenderWidgetHostViewAura::InsertText(const base::string16& text) {
   DCHECK_NE(GetTextInputType(), ui::TEXT_INPUT_TYPE_NONE);
 
   // TODO(wjmaclean): can host_ ever be null?
-  if (host_)
-    host_->ImeConfirmComposition(text, gfx::Range::InvalidRange(), false);
+  if (text_input_manager_ && text_input_manager_->GetActiveWidget())
+    text_input_manager_->GetActiveWidget()->ImeConfirmComposition(
+        text, gfx::Range::InvalidRange(), false);
   has_composition_text_ = false;
 }
 
 void RenderWidgetHostViewAura::InsertChar(const ui::KeyEvent& event) {
   if (popup_child_host_view_ && popup_child_host_view_->NeedsInputGrab()) {
     popup_child_host_view_->InsertChar(event);
     return;
   }
 
   // Ignore character messages for VKEY_RETURN sent on CTRL+M. crbug.com/315547
@@ skipping 1514 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewBase, public:
 
 // static
 void RenderWidgetHostViewBase::GetDefaultScreenInfo(WebScreenInfo* results) {
   GetScreenInfoForWindow(results, NULL);
 }
 
 }  // namespace content