Extracting placeholder information from Webkit to Blimp

blimp/engine/session/blimp_engine_session.cc

Index: blimp/engine/session/blimp_engine_session.cc
diff --git a/blimp/engine/session/blimp_engine_session.cc b/blimp/engine/session/blimp_engine_session.cc
index 33fac46bf27e5bccfaa1bae6aa0cebd32cbc72b3..c0406682a99220bcc15ef44e3e4e4a921a526fd9 100644
--- a/blimp/engine/session/blimp_engine_session.cc
+++ b/blimp/engine/session/blimp_engine_session.cc
@@ -40,6 +40,7 @@
 #include "blimp/net/null_blimp_message_processor.h"
 #include "blimp/net/tcp_engine_transport.h"
 #include "blimp/net/thread_pipe_manager.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
@@ -250,8 +251,6 @@ BlimpEngineSession::BlimpEngineSession(
 }
 
 BlimpEngineSession::~BlimpEngineSession() {
-  window_tree_host_->GetInputMethod()->RemoveObserver(this);
-
   // Ensure that all tabs are torn down first, since teardown will
   // trigger RenderViewDeleted callbacks to their observers, which will in turn
   // send messages to net_components_, which is already deleted due to the line
@@ -284,8 +283,6 @@ void BlimpEngineSession::Initialize() {
   window_tree_client_.reset(
       new BlimpWindowTreeClient(window_tree_host_->window()));
 
-  window_tree_host_->GetInputMethod()->AddObserver(this);
-
   window_tree_host_->SetBounds(gfx::Rect(screen_->GetPrimaryDisplay().size()));
 
   RegisterFeatures();
@@ -381,51 +378,25 @@ void BlimpEngineSession::HandleResize(float device_pixel_ratio,
   }
 }
 
-void BlimpEngineSession::OnTextInputTypeChanged(
-    const ui::TextInputClient* client) {}
-
-void BlimpEngineSession::OnFocus() {}
-
-void BlimpEngineSession::OnBlur() {}
+void BlimpEngineSession::OnUpdateTextInputStateCalled(

[EhsanK, 2016/09/28 16:14:11]

I guess I see why you had to observe TextInputManager. Basically, you need to
know the RWH which changed its TextInputState. This is a totally different
structure then the rest of InputMethod and TextInputClient that I know of.

Passing some info about RWH to InputMethod is also not a good idea since AFAIK
InputMethod does not really care which RWH is focused or not; as long as there
is a client which consumes the results methods.

But could there be something added to WebContentsDelegate about the change
instead of exposing all this? We are exposing TextInputManager,
RenderWidgetHostViewAura and WebContentsImpl which are all content/ internal.

[shaktisahu, 2016/09/28 20:17:23]

I think I pipe this call to RenderWidgetHostDelegate -> WebContentsDelegate ->
BlimpEngineSession. That makes more sense and in that way we don't have to
expose any of the content internal stuff.

[EhsanK, 2016/10/03 14:53:10]

I too think going through the delegates is better.

+    content::TextInputManager* text_input_manager,
+    content::RenderWidgetHostViewBase* updated_view,
+    bool did_update_state) {
+  const content::TextInputState* state =
+      text_input_manager->GetTextInputState();
 
-void BlimpEngineSession::OnCaretBoundsChanged(
-    const ui::TextInputClient* client) {}
-
-// Called when either:
-//  - the TextInputClient is changed (e.g. by a change of focus)
-//  - the TextInputType of the TextInputClient changes
-void BlimpEngineSession::OnTextInputStateChanged(
-    const ui::TextInputClient* client) {
-  if (!tab_ || !tab_->web_contents()->GetRenderWidgetHostView())
+  if (!state)
     return;
 
-  ui::TextInputType type =
-      client ? client->GetTextInputType() : ui::TEXT_INPUT_TYPE_NONE;
+  if (state->show_ime_if_needed) {
+    render_widget_feature_.SendShowImeRequest(
+        tab_->tab_id(), updated_view->GetRenderWidgetHost(), state);

[EhsanK, 2016/09/28 16:14:11]

RWHV::GetRenderWidgetHost() may return nullptr. I could not tell if this would
cause issues down the stack. As I see it it will be used for a map to retrieve a
RWHId. I assume the map will not find any id for a nullptr entry?

[shaktisahu, 2016/09/28 20:17:23]

Ya, this is checked. The function will return 0 for nullptr entry and we are
DCHECKing the id for non-zero after that. So indirectly the DCHECK checks
thatthe GetRenderWidgetHost() should return non-null.

[EhsanK, 2016/10/03 14:53:10]

Acknowledged.

+  }
 
-  // TODO(shaktisahu): Propagate the new type to the client.
-  // Hide IME, when text input is out of focus, i.e. if the text input type
-  // changes to ui::TEXT_INPUT_TYPE_NONE. For other text input types,
-  // OnShowImeIfNeeded is used instead to send show IME request to client.
-  if (type == ui::TEXT_INPUT_TYPE_NONE)
+  if (state->type == ui::TEXT_INPUT_TYPE_NONE) {
     render_widget_feature_.SendHideImeRequest(
-        tab_->tab_id(),
-        tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost());
-}
-
-void BlimpEngineSession::OnInputMethodDestroyed(
-    const ui::InputMethod* input_method) {}
-
-// Called when a user input should trigger showing the IME.
-void BlimpEngineSession::OnShowImeIfNeeded() {
-  TRACE_EVENT0("blimp", "BlimpEngineSession::OnShowImeIfNeeded");
-  if (!tab_ || !tab_->web_contents()->GetRenderWidgetHostView() ||
-      !window_tree_host_->GetInputMethod()->GetTextInputClient())
-    return;
-
-  render_widget_feature_.SendShowImeRequest(
-      tab_->tab_id(),
-      tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost(),
-      window_tree_host_->GetInputMethod()->GetTextInputClient());
+        tab_->tab_id(), updated_view->GetRenderWidgetHost());
+  }
 }
 
 void BlimpEngineSession::ProcessMessage(
@@ -528,6 +499,9 @@ void BlimpEngineSession::RequestToLockMouse(content::WebContents* web_contents,
 
 void BlimpEngineSession::CloseContents(content::WebContents* source) {
   if (source == tab_->web_contents()) {
+    static_cast<content::WebContentsImpl*>(source)
+        ->GetTextInputManager()
+        ->RemoveObserver(this);
     tab_.reset();
   }
 }
@@ -566,6 +540,10 @@ void BlimpEngineSession::PlatformSetContents(
     parent->AddChild(content);
   content->Show();
 
+  static_cast<content::WebContentsImpl*>(new_contents.get())
+      ->GetTextInputManager()
+      ->AddObserver(this);
+
   tab_ = base::MakeUnique<Tab>(std::move(new_contents), target_tab_id,
                                &render_widget_feature_,
                                navigation_message_sender_.get());