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

content/browser/renderer_host/text_input_manager.cc

 // Copyright 2016 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/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 {
 
 namespace {
 
 bool AreDifferentTextInputStates(const content::TextInputState& old_state,
                                  const content::TextInputState& new_state) {
 #if defined(USE_AURA)
   return old_state.type != new_state.type || old_state.mode != new_state.mode ||
@@ skipping 23 matching lines @@
     views.push_back(pair.first);
 
   for (auto view : views)
     Unregister(view);
 }
 
 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(
     RenderWidgetHostViewBase* view,
     const TextInputState& text_input_state) {
   DCHECK(IsRegistered(view));
 
   // Since |view| is registgered, we already have a previous value for its
   // TextInputState.
   bool changed = AreDifferentTextInputStates(text_input_state_map_[view],
@@ skipping 44 matching lines @@
 
 void TextInputManager::NotifyObserversAboutInputStateUpdate(
     RenderWidgetHostViewBase* updated_view,
     bool did_update_state) {
   FOR_EACH_OBSERVER(
       Observer, observer_list_,
       OnUpdateTextInputStateCalled(this, updated_view, did_update_state));
 }
 
 }  // namespace content