Fix TextInputManager for Android

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"
 #include "content/common/view_messages.h"
 #include "ui/gfx/geometry/rect.h"
@@ skipping 84 matching lines @@
 
   if (text_input_state.type == ui::TEXT_INPUT_TYPE_NONE &&
       active_view_ != view) {
     // We reached here because an IPC is received to reset the TextInputState
     // for |view|. But |view| != |active_view_|, which suggests that at least
     // one other view has become active and we have received the corresponding
     // IPC from their RenderWidget sooner than this one. That also means we have
     // already synthesized the loss of TextInputState for the |view| before (see
     // below). So we can forget about this method ever being called (no observer
     // calls necessary).
+    // NOTE: Android requires state to be returned even when the current state
+    // is/becomes NONE. Otherwise IME may become irresponsive.
+#if !defined(OS_ANDROID)
     return;

[EhsanK, 2017/03/10 15:43:16]

This should be good since we will not have more than one RWHV on a page to
overwrite each other's state to begin with. And as I understand we were throwing
away some state updates from renderer when the type was NONE, which seems
incorrect.

(If everything is working fine for normal Android then ignore the rest of this
comment pls).

But wouldn't we also have to change the code inside
RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled() a bit?
Right now the state it is reporting to content_view_core_ is the one from active
widget, which is the one with TextInputState.type != NONE. Don't we need to
sometimes update the ime_adapter_ with states where the type is NONE?

If that is the case then I suggest changing the signature of GetTextInputState
to pass an optional parameter for view and get the state from the view, e.g.:

void TextInputState::GetTextInputState(RenderWidgetHostViewBase* view = nullptr)
{
if (view == nullptr) view = active_view_;
return view ? text_input_state_map_[view] : nullptr;
}

And inside RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled do:

const TextInputState* state = text_input_manager_->IsRegisteredView(view) ?
text_input_manager_->GetTextInputState(view) : nullptr.

The last line is to make sure we are not in destruction (view is still
registered) because when a RWHV is destroyed the TextInputManager does not hold
its state anymore (not doing this causes crash https:://crbug.com/669375).

I also need to rework this anyway and base it on focused RenderWidgetHost since
this currently might break things for OOPIF Android (https://crbug.com/631798).

[Changwan Ryu, 2017/03/10 16:01:38]

As long as it returns something, it should work correctly.

[EhsanK, 2017/03/10 16:46:18]

Right. So here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

if active_view_= nullptr (which is the case in the beginning or might be set at
line 142 here), we will be updating content view core with TextInputState()
default and seems to be OK. Then I think this is fine.

Also I will soon upload a patch to possibly change this whole method to use the
focused widget. I don't think relying on just TextInputState.type is good enough
(not for android). I will add you as reviewer to that CL.

+#endif
   }
 
   // Since |view| is registered, we already have a previous value for its
   // TextInputState.
   bool changed = ShouldUpdateTextInputState(text_input_state_map_[view],
                                             text_input_state);
 
   text_input_state_map_[view] = text_input_state;
 
   // If |view| is different from |active_view| and its |TextInputState.type| is
@@ skipping 226 matching lines @@
     if (pos >= text.length()) {
       LOG(WARNING) << "The text cannot cover range (selection range's starting "
                       "point exceeds text length).";
     } else {
       selected_text_.append(text.substr(pos, n));
     }
   }
 }
 
 }  // namespace content