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

content/browser/renderer_host/text_input_manager.cc

Index: content/browser/renderer_host/text_input_manager.cc
diff --git a/content/browser/renderer_host/text_input_manager.cc b/content/browser/renderer_host/text_input_manager.cc
index 258c05ee00602ff2cf4a8ed6a1f0a76b71d15b46..c80f3807a0452485b0435e9e3c5f2b024c7dcb05 100644
--- a/content/browser/renderer_host/text_input_manager.cc
+++ b/content/browser/renderer_host/text_input_manager.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/renderer_host/text_input_manager.h"
 
+#include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
 
 namespace content {
@@ -47,8 +48,10 @@ const TextInputState* TextInputManager::GetTextInputState() {
   return !!active_view_ ? &text_input_state_map_[active_view_] : nullptr;
 }
 
-RenderWidgetHostViewBase* TextInputManager::GetActiveView() const {
-  return active_view_;
+RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const {

[Charlie Reis, 2016/06/21 21:13:56]

There's a bunch of places that just call GetActiveWidget()->GetView(), with
additional null checks.  Maybe we should have both GetActiveWidget() and
GetActiveView(), to avoid the "there and back again" under the hood?

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

I found three instances:
in 'text_input_test_utils.cc':
GetActiveViewFromWebContents()
TextInputManagerTester::GetActiveView().
and in 'render_widget_host_view_aura_unittest.cc':
InputMethodAuraTest::ActivateView() (which is now refactored into
ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest).

Since all the instances are for testing only, should we still have the dedicated
method for view?

[Charlie Reis, 2016/06/22 19:19:15]

Yes.  Also, I missed this in the previous review, but since the implementation
is trivial, it should be inlined into the .h file as active_view().

That said, if you want to steer people towards using GetActiveWidget() in
practice, I'm fine with calling it active_view_for_testing().

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

Charlie: I had asked for removal of GetActiveView() in an earlier round of
review on this CL, since it was only used in tests. Generally, IME code in the
views is on the control flow path that comes up from the renderer and doesn't
come through TextInputManager. 'Downward' IME flows like SetCompositionFlows
that need this class go straight to the RenderWidgetHostImpl, so
GetActiveWidget() makes sense here while GetActiveView() doesn't, really.

[Charlie Reis, 2016/06/22 20:23:08]

Ok, I'm fine with steering people towards GetActiveWidget().

It still seems weird to store the active_view_ internally and have to do a bunch
of work to get back to it in several places, so having a
active_view_for_testing() seems useful to me.  (I suppose we could store
active_widget_ instead of active_view_ instead?  That's probably disruptive,
though.)

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

I also agree with Ken on this. Views are used as key in maps since the stats are
reported by the view. But the only use of knowing the active view/widget as of
now is routing IME methods.

I have added |active_view_for_testing()| following Charlie's second suggestion
as it seems like the active view is only used for tests.

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

Charlie: I actually think |active_widget_| rather than |active_view_| might work
just fine now that I have a better understanding of the whole IME in content/.
The only reason I used |active_view_| was
RenderWidgetHostViewBase::TextInputStateChanged. We might be able to remove it
and report it directly from RenderWidgetHost. But it will be a bigger change
than this CL is about. I have added a |active_view_for_testing()|.

+  return !!active_view_ ? static_cast<RenderWidgetHostImpl*>(
+                              active_view_->GetRenderWidgetHost())
+                        : nullptr;
 }
 
 void TextInputManager::UpdateTextInputState(