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

content/browser/renderer_host/text_input_manager.h

+// 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.
+
+#ifndef CONTENT_BROWSER_RENDERER_HOST_TEXT_INPUT_MANAGER_H__
+#define CONTENT_BROWSER_RENDERER_HOST_TEXT_INPUT_MANAGER_H__
+
+#include <unordered_map>
+
+#include "base/observer_list.h"
+#include "content/common/content_export.h"
+#include "content/common/text_input_state.h"
+
+namespace content {
+
+class RenderWidgetHostView;
+class RenderWidgetHostViewBase;
+class WebContents;
+
+// A class which receives updates of TextInputState from  multiple sources and

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

nit: Remove extra space between "from" and "multiple."

[EhsanK, 2016/05/30 15:06:08]

Done.

+// decides what the new TextInputState is. It also notifies the observers when
+// text input state is updated.
+class CONTENT_EXPORT TextInputManager {
+ public:
+  // The tab's top-level RWHV should be an observer of TextInputManager to get
+  // notifications about changes in TextInputState or other IME related state
+  // for child frames.
+  class CONTENT_EXPORT Observer {
+   public:
+    // Called when a view has called UpdateTextInputState on TextInputManager.
+    // If the call has led to a change in TextInputState, |did_update_state| is
+    // true. In some plaforms, we need this update even when the state has not
+    // changed (e.g., Aura for updating IME).
+    virtual void OnUpdateTextInputStateCalled(
+        TextInputManager* text_input_manager,
+        RenderWidgetHostViewBase* updated_view,
+        bool did_update_state) {}
+
+    // Called when the |text_input_manager| is being destroyed.
+    virtual void OnDestroyingTextInputManager(

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

Please remove this for now.  We can add it later when there's a use for it.

[EhsanK, 2016/05/30 15:06:08]

Done.

I removed it, but there is something about not having such a call which does not
add up for me. RWHVs register with TextInputManager to make it track their
state. Those who use these states and updates, i.e., tab's RWHV, will add
themselves as observers. So we have two categories of entities here: registered
views, and observers.

Now, when TextInputManager goes away it will unregister all RWHVs, where during
the process, they all lose their reference to TextInputManager.

But the observers will never know this. As for tab's RWHV it is fine because we
also register the RWHV even if there are no <input> (in the ctor of RWHVAura).
So we are fine; but I think not for the right reason. In other words, RWHVAura
is on the clear because it is a view, not because it is an observer.

[Charlie Reis, 2016/06/02 22:03:29]

I'm not sure I follow that in depth, but my intention here is to not add code
until it's used.  I'm fine with either of these two options:

1) Include OnDestroyingTextInputManager and a place that uses it (e.g., in a
test).
2) Put a TODO to add OnDestroyingTextInputManager when it's needed for
observers.

+        TextInputManager* text_input_manager) {}
+  };
+
+  TextInputManager();
+  ~TextInputManager();
+
+  // Returns the currently active view (,i.e., the RWHV with a focused <input>

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

nit: No comma after open paren.

[EhsanK, 2016/05/30 15:06:08]

Done.

+  // element), or nullptr if none exist. The |active_view_| cannot have a
+  // |TextInputState.type| of ui::TEXT_INPUT_TYPE_NONE.
+  RenderWidgetHostViewBase* GetActiveView() const;
+
+  // Returns the TextInputState corresponding to |active_view_|.
+  // Users of this method should not hold on to the pointer as it might become
+  // dangling if the TextInputManager or |active_view_| might go away.
+  const TextInputState* GetTextInputState();
+
+  // Updates the TextInputState for |view|.
+  void UpdateTextInputState(RenderWidgetHostViewBase* view,
+                            const TextInputState& state);
+
+  // Registers the given |view| for tracking its TextInputState.
+  // TextInputManager will observe the lifetime of the |view|.
+  void Register(RenderWidgetHostViewBase* view);
+
+  // Clears the TextInputState from the |view|. If |view == active_view_|, this
+  // call will lead to a TextInputState update since the TextInputState.type
+  // should be reset to none.
+  void Unregister(RenderWidgetHostViewBase* view);
+
+  // Returns true if |view| is already registered.
+  bool IsRegistered(RenderWidgetHostViewBase* view) const;
+
+  // Add and remove observers for lifetime event notifications, as well as

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

nit: There's no lifetime event notifications in these observers (for the time
being) once we remove the OnDestroying one.

[EhsanK, 2016/05/30 15:06:08]

Acknowledged.

+  // updates in the TextInputState. Clients must be sure to remove themselves
+  // before they go away.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ private:
+  friend std::unordered_map<RenderWidgetHostView*, ui::TextInputType>
+  GetTextInputTypeMapFromWebContents(WebContents* web_contents);

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

We won't need this if we expose a GetTextInputTypeForView instead.

[EhsanK, 2016/05/30 15:06:08]

I am not sure if we need a GetTextInputTypeForView() in TextInputManager. I have
added the method in test utils, but other than that, I don't quite see
application of it anywhere else.

That being said, we might later on have to do such a thing, if not for Type,
maybe for another IME related state which is kept in TextInputManager.

If you think we should expose it nonetheless, I could modify the definition of
TextInputManager::GetTextInputState()
to:

TextInputManager::GetTextInputState(RenderWidgetHostViewBase* view = nullptr) {
 if (!view)
   view = active_view_;
 if (!IsRegistered(view))
   return nullptr;

 return !!view ? &text_input_state_map_[view] : nullptr;
}

What do you think?

[Charlie Reis, 2016/06/02 22:03:29]

Sorry, that's not what I meant.  I just meant we wouldn't need to friend
GetTextInputTypeMapFromWebContents.  I agree with keeping
GetTextInputTypeForView as a test method.

It would still be nice to avoid the friend declaration if possible.  Maybe we
can have a public GetTextInputTypeForTesting(view) on TextInputManager, and call
it from the GetTextInputTypeForView test method?

+
+  void NotifyObserversAboutInputStateUpdate(RenderWidgetHostViewBase* view,
+                                            bool did_update_state);
+
+  // The view with active text input state.
+  RenderWidgetHostViewBase* active_view_;
+
+  std::unordered_map<RenderWidgetHostViewBase*, TextInputState>
+      text_input_state_map_;
+
+  base::ObserverList<Observer> observer_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextInputManager);
+};
+}
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_TEXT_INPUT_MANAGER_H__