[reland] Browser Side Text Input State Tracking for OOPIF (Aura Only)

content/public/test/text_input_test_utils.cc

Index: content/public/test/text_input_test_utils.cc
diff --git a/content/public/test/text_input_test_utils.cc b/content/public/test/text_input_test_utils.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6e95fff9c481b5524e4fd054e3af6ac5f26a449b
--- /dev/null
+++ b/content/public/test/text_input_test_utils.cc
@@ -0,0 +1,313 @@
+// 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/public/test/text_input_test_utils.h"
+
+#include "content/browser/renderer_host/render_widget_host_view_aura.h"
+#include "content/browser/renderer_host/render_widget_host_view_base.h"
+#include "content/browser/renderer_host/render_widget_host_view_base_observer.h"
+#include "content/browser/renderer_host/text_input_manager.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/text_input_state.h"
+#include "content/public/browser/render_widget_host_view.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_utils.h"
+#include "ui/base/ime/input_method.h"
+#include "ui/base/ime/input_method_observer.h"
+
+namespace ui {
+class TextInputClient;
+}
+
+namespace content {
+
+namespace {
+
+// The implementation of the TestTextInputManagerObserver which is observing the
+// content::TextInputManager for the given WebContentsImpl.
+class TextInputManagerObserver : public TestTextInputManagerObserver,

[Charlie Reis, 2016/05/18 20:46:05]

Why is this a subclass of TestTextInputManagerObserver?  I don't see any other
implementations, and unlike InputMethodObserverAura, this doesn't seem to be
platform-specific.

I think it would be much simpler and easier to follow if we just had one
implementation of TestTextInputManagerObserver.  That also means most of these
methods don't need to be virtual.

[EhsanK, 2016/05/24 20:42:47]

The reason is that TextInputManager is not publicly visible. The only two
solutions I could think of were this, or, creating a wrapper class which entails
an implementation TextInputManager::Observer. I went this way. I did not change
this for now to get your suggestions on the best way to do this.
I think that would be the case were TestTextInputManagerObserver has a memeber
|observer_| which implements TextInputManager::Observer, right?

[Charlie Reis, 2016/05/26 06:22:03]

Ah, I see.  Yes, let's just have an inner class to do the observing of the
non-public TextInputManager.  That avoids unnecessary inheritance, keeps the
interface simpler (by having a constructor) and still hides the implementation
details.

+                                 public TextInputManager::Observer {
+ public:
+  // TestTextInputManagerObserver implementations.

[Charlie Reis, 2016/05/18 20:46:05]

nit: Move this comment down to SetUpdateCallback, since it doesn't really apply
to the constructor/destructor.

[EhsanK, 2016/05/24 20:42:47]

Done.

+  TextInputManagerObserver(WebContents* web_contents)
+      : updated_view_(nullptr), changed_(false) {
+    text_input_manager_ =
+        static_cast<WebContentsImpl*>(web_contents)->GetTextInputManager();
+    DCHECK(!!text_input_manager_);
+    text_input_manager_->AddObserver(this);
+  }
+
+  ~TextInputManagerObserver() override {
+    text_input_manager_->RemoveObserver(this);
+  }
+
+  void SetUpdateCallback(const content::TestTextInputManagerObserver::Callback&

[Charlie Reis, 2016/05/18 20:46:05]

nit: We're already in the content namespace, so you shouldn't need content::
here or below.

[EhsanK, 2016/05/24 20:42:47]

Acknowledged.

+                             callback) override {
+    update_callback_.reset(
+        new content::TestTextInputManagerObserver::Callback(callback));

[Charlie Reis, 2016/05/18 20:46:05]

You shouldn't need to wrap/copy this.  Just pass callback.

[EhsanK, 2016/05/24 20:42:46]

Acknowledged.

+  }
+
+  void SetUpdateCalledCallback(
+      const content::TestTextInputManagerObserver::Callback& callback)
+      override {
+    update_called_callback_.reset(
+        new content::TestTextInputManagerObserver::Callback(callback));
+  }
+
+  bool GetTextInputValue(std::string& value) const override {

[Charlie Reis, 2016/05/18 20:46:05]

We don't use non-const refs.  This should be const std::string&, or std::string*
if you're treating it as an out parameter (in which case it must be documented
in the interface).

[EhsanK, 2016/05/24 20:42:46]

Done.

+    const content::TextInputState* state =
+        text_input_manager_->GetTextInputState();
+    if (!state)
+      return false;
+    value = state->value;
+    return true;
+  }
+
+  ui::TextInputType GetTextInputType() const override {
+    const content::TextInputState* state =
+        text_input_manager_->GetTextInputState();
+    return !!state ? state->type : ui::TEXT_INPUT_TYPE_NONE;
+  }
+
+  const RenderWidgetHostView* GetActiveView() const override {
+    return text_input_manager_->GetActiveView();
+  }
+
+  const RenderWidgetHostView* GetUpdatedView() const override {
+    return updated_view_;
+  }
+
+  bool IsTextInputStateChanged() const override { return changed_; }
+
+ private:
+  // TextInputManager::Observer implementations.
+  void OnTextInputStateUpdated(
+      TextInputManager* text_input_manager,
+      RenderWidgetHostViewBase* updated_view) override {
+    if (text_input_manager_ != text_input_manager)
+      return;
+    changed_ = true;
+    updated_view_ = updated_view;
+    if (update_callback_)
+      update_callback_->Run(this);
+  }
+
+  void OnTextInputStateUpdateCalled(
+      TextInputManager* text_input_manager) override {
+    if (text_input_manager_ != text_input_manager)
+      return;
+    if (update_called_callback_)
+      update_called_callback_->Run(this);
+  }
+
+  content::TextInputManager* text_input_manager_;
+  content::RenderWidgetHostViewBase* updated_view_;
+  bool changed_;
+  std::unique_ptr<content::TestTextInputManagerObserver::Callback>
+      update_callback_;
+  std::unique_ptr<content::TestTextInputManagerObserver::Callback>
+      update_called_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextInputManagerObserver);
+};
+
+class RenderWidgetHostViewBaseObserverImpl

[Charlie Reis, 2016/05/18 20:46:05]

I'm unclear about why this one is a subclass, too.  Can we just move all this
into RenderWidgetHostViewDestructionObserver directly?

[EhsanK, 2016/05/24 20:42:46]

Same reason as above. What is the best approach?

+    : public RenderWidgetHostViewBaseObserver,
+      public RenderWidgetHostViewDestructionObserver {
+ public:
+  RenderWidgetHostViewBaseObserverImpl(RenderWidgetHostViewBase* view)
+      : view_(view), destroyed_(false) {
+    view->AddObserver(this);
+  }
+
+  void Wait() override {
+    if (destroyed_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner();
+    message_loop_runner_->Run();
+  }
+
+ private:
+  void OnRenderWidgetHostViewBaseDestroyed(
+      RenderWidgetHostViewBase* view) override {
+    DCHECK(view_ == view);

[Charlie Reis, 2016/05/18 20:46:05]

nit: DCHECK_EQ

[EhsanK, 2016/05/24 20:42:47]

Acknowledged.

+    destroyed_ = true;
+    view->RemoveObserver(this);
+    if (message_loop_runner_)
+      message_loop_runner_->Quit();
+  }
+
+  RenderWidgetHostView* view_;
+  bool destroyed_;
+  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewBaseObserverImpl);
+};
+
+#ifdef USE_AURA
+class InputMethodObserverAura : public TestInputMethodObserver,
+                                public ui::InputMethodObserver {
+ public:
+  explicit InputMethodObserverAura(ui::InputMethod* input_method)
+      : input_method_(input_method), text_input_client_(nullptr) {
+    input_method_->AddObserver(this);
+  }
+  ~InputMethodObserverAura() override {
+    if (input_method_)
+      input_method_->RemoveObserver(this);
+  }
+
+  // TestInputMethodObserver implementations.
+  ui::TextInputType GetTextInputTypeFromClient() const override {
+    if (text_input_client_)
+      return text_input_client_->GetTextInputType();
+
+    return ui::TEXT_INPUT_TYPE_NONE;
+  }
+  void SetOnTextInputTypeChangedCallback(

[Charlie Reis, 2016/05/18 20:46:05]

nit: Let's be consistent on whether there's a blank line between these methods
or not.

[EhsanK, 2016/05/24 20:42:46]

Acknowledged.

+      const base::Closure& callback) override {
+    on_text_input_type_changed_callback_.reset(new base::Closure(callback));

[Charlie Reis, 2016/05/18 20:46:05]

Again, no need to make a copy of callback.

[EhsanK, 2016/05/24 20:42:47]

Acknowledged.

+  }
+
+  void SetOnShowImeIfNeededCallback(const base::Closure& callback) override {
+    on_show_ime_if_needed_callback_.reset(new base::Closure(callback));
+  }
+
+ private:
+  // ui::InputMethodObserver implementations.
+  void OnTextInputTypeChanged(const ui::TextInputClient* client) override {
+    text_input_client_ = client;
+    if (on_text_input_type_changed_callback_)
+      on_text_input_type_changed_callback_->Run();
+  }
+
+  void OnFocus() override {}
+  void OnBlur() override {}
+  void OnCaretBoundsChanged(const ui::TextInputClient* client) override {}
+  void OnTextInputStateChanged(const ui::TextInputClient* client) override {}
+  void OnInputMethodDestroyed(const ui::InputMethod* input_method) override {}
+
+  void OnShowImeIfNeeded() override {
+    if (on_show_ime_if_needed_callback_)
+      on_show_ime_if_needed_callback_->Run();
+  }
+
+  ui::InputMethod* input_method_;
+  const ui::TextInputClient* text_input_client_;
+  std::unique_ptr<base::Closure> on_text_input_type_changed_callback_;
+  std::unique_ptr<base::Closure> on_show_ime_if_needed_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(InputMethodObserverAura);
+};
+#endif
+
+}  // namespace
+
+// TestTextInputManagerObserver Implementations.
+TestTextInputManagerObserver::~TestTextInputManagerObserver() {}
+
+// static
+std::unique_ptr<TestTextInputManagerObserver>
+TestTextInputManagerObserver::Create(WebContents* web_contents) {
+  return base::WrapUnique(new TextInputManagerObserver(web_contents));
+}
+
+// static
+std::unordered_map<const RenderWidgetHostView*, ui::TextInputType>
+TestTextInputManagerObserver::GetTextInputTypeMap(WebContents* web_contents) {
+  TextInputManager* manager =
+      static_cast<WebContentsImpl*>(web_contents)->GetTextInputManager();
+  std::unordered_map<const RenderWidgetHostView*, ui::TextInputType> result;
+  for (const auto& pair : manager->text_input_state_map_)
+    result[pair.first] = pair.second.type;

[Charlie Reis, 2016/05/18 20:46:05]

Is there a reason we need to make a copy of the whole map to return, rather than
just returning a reference to it?  It's test code, so I'm not concerned about
the caller mutating the map.

[EhsanK, 2016/05/24 20:42:47]

The key in the internal map is RenderWidgetHostViewBase. Unless, I change that
one into RenderWidgetHostView, this might not work since
RenderWidgetHostViewBase is not in content/public.

To clarify, I am running browser_tests (and I guess) I cannot include content/
code. At least for extension tests (later for <webview>) I believe this is the
case?

[Charlie Reis, 2016/05/26 06:22:03]

Ah, that's really subtle.  Please add a comment explaining that we need to
convert the map from RWHVB to the public RWHV type.

Or even better, let's replace this API with GetTextInputTypeForView, as I
mention on line 157 of the test file.

+  return result;
+}
+
+// RenderWidgetHostViewDestructionObserver implementations.
+RenderWidgetHostViewDestructionObserver::
+    ~RenderWidgetHostViewDestructionObserver() {}
+
+// static
+std::unique_ptr<RenderWidgetHostViewDestructionObserver>
+RenderWidgetHostViewDestructionObserver::Create(RenderWidgetHostView* view) {
+  return base::WrapUnique(new RenderWidgetHostViewBaseObserverImpl(
+      static_cast<RenderWidgetHostViewBase*>(view)));
+}
+
+ui::TextInputType GetTextInputTypeFromWebContents(WebContents* web_contents) {
+  const TextInputState* state = static_cast<WebContentsImpl*>(web_contents)
+                                    ->GetTextInputManager()
+                                    ->GetTextInputState();
+  return !!state ? state->type : ui::TEXT_INPUT_TYPE_NONE;
+}
+
+RenderWidgetHostView* GetActiveViewFromWebContents(WebContents* web_contents) {
+  return static_cast<WebContentsImpl*>(web_contents)
+      ->GetTextInputManager()
+      ->GetActiveView();
+}
+
+// TextInputStateSender implementations.
+TextInputStateSender::TextInputStateSender(RenderWidgetHostView* view)
+    : text_input_state_(new TextInputState()),
+      view_(static_cast<RenderWidgetHostViewBase*>(view)) {}
+
+TextInputStateSender::~TextInputStateSender() {}
+
+void TextInputStateSender::Send() {
+  if (view_)
+    view_->TextInputStateChanged(*text_input_state_);
+}
+
+void TextInputStateSender::SetFromCurrentState() {
+  if (view_) {
+    *text_input_state_ =

[Charlie Reis, 2016/05/18 20:46:05]

Shouldn't this be a .reset call for a unique pointer?  (I may be wrong.)

[EhsanK, 2016/05/24 20:42:46]

I think we should not.

Two reasons I can think of:

1- The TextInputManager::GetTextInputState() returns a const pointer which
cannot be owned.

2- We do not want to take ownership of TextInputManager::GetTextInputState()
anyway since it will be the actual struct TextInputState for the specific view.

[Charlie Reis, 2016/05/26 06:22:03]

Acknowledged.

+        *RenderWidgetHostImpl::From(view_->GetRenderWidgetHost())
+             ->delegate()
+             ->GetTextInputManager()
+             ->GetTextInputState();
+  }
+}
+
+void TextInputStateSender::SetType(ui::TextInputType type) {
+  text_input_state_->type = type;
+}
+
+void TextInputStateSender::SetMode(ui::TextInputMode mode) {
+  text_input_state_->mode = mode;
+}
+
+void TextInputStateSender::SetFlags(int flags) {
+  text_input_state_->flags = flags;
+}
+
+void TextInputStateSender::SetCanComposeInline(bool can_compose_inline) {
+  text_input_state_->can_compose_inline = can_compose_inline;
+}
+
+void TextInputStateSender::SetShowImeIfNeeded(bool show_ime_if_needed) {
+  text_input_state_->show_ime_if_needed = show_ime_if_needed;
+}
+
+void TextInputStateSender::SetIsNonImeChange(bool is_non_ime_change) {
+  text_input_state_->is_non_ime_change = is_non_ime_change;
+}
+
+// TestInputMethodObserver implementations.
+TestInputMethodObserver::~TestInputMethodObserver() {}
+
+// static
+std::unique_ptr<TestInputMethodObserver> TestInputMethodObserver::Create(
+    WebContents* web_contents) {
+  std::unique_ptr<TestInputMethodObserver> observer;
+
+#ifdef USE_AURA
+  RenderWidgetHostViewAura* view = static_cast<RenderWidgetHostViewAura*>(
+      web_contents->GetRenderWidgetHostView());
+  observer.reset(new InputMethodObserverAura(view->GetInputMethod()));
+#endif
+  return observer;
+}
+
+}  // namespace content