[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_TEXT_INPUT_MANAGER_H__

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

nit: Missing RENDERER_HOST

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

Uh yes...I moved this file around a lot. Sorry. :)

+#define CONTENT_BROWSER_TEXT_INPUT_MANAGER_H__
+
+#include <unordered_map>
+
+#include "base/observer_list.h"
+#include "content/browser/renderer_host/render_widget_host_view_base_observer.h"
+#include "content/common/content_export.h"
+#include "content/common/text_input_state.h"
+
+namespace content {
+
+class RenderWidgetHostViewBase;
+class TestTextInputManagerObserver;

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

This shouldn't be needed.

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

Done.

+
+// A class which receives updates of Text Input State from  multiple sources and

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

nit: Remove extra space.
Also, let's be consistent about whether to capitalize Text Input State (this
line and next one).

Can you give some extra context about why this is needed (e.g., that the
multiple sources may be out-of-process iframes, and this class is deciding what
to use for a WebContents?).

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

Acknowledged. I think to be consistent I should have used TextInputState as it
is the name of the struct.

+// decides what the new text input state is. It also notifies the observers when
+// text input state is updated.
+class CONTENT_EXPORT TextInputManager
+    : public RenderWidgetHostViewBaseObserver {

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

It's a bit unfortunate that these two classes need to observe each other, since
it creates a circular dependency.  Can we have RWHVB call a method on
TextInputManager when it's destroyed so that we don't need TextInputManager to
observe it?  That will lead to less overhead on the other observer methods in
RWHVB and break the circular dependency.

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

I was worried about the observing loop to become confusing to follow and was not
sure if I should use a direct API call on RWHVBase.

Following your suggestion, the only observers of TextInputManager are RWHV for
tab and test observers. RWHVBase gets a new method which does the job of
OnDestroyingTextInputManager.

I am still keeping OnDestroyingTextInputManager since I believe it will be used
when fixing the issue for <webview> (and testing).

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

Thanks-- it's much easier to follow with the observing only going in one
direction.
We shouldn't add it until it's used.

+ public:
+  // The top-level tab RWHV should be an observer of TextInputManager to get

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

nit: The tab's top-level RWHV

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

Done.

+  // notifications about changes in TextInputState or other IME related state
+  // for child frames.
+  class CONTENT_EXPORT Observer {
+   public:
+    // Called when the TextInputState for |view| has changed from its previous
+    // value. The change is determined on each platform differently by only
+    // considering the change in the value of a subset of fields in the
+    // TextInputState.
+    virtual void OnTextInputStateUpdated(TextInputManager* text_input_manager,

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

I'm curious why the |text_input_manager| parameter is needed.  Do the observers
listen to multiple managers?  Looks like we just DCHECK it today.

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

Yes that is all we do for now. The only possible case to have two
TextInputManager's involed is when there are inner WebContents. Then a view
might be observing the inner WebContents at first, and then when attached to
outer WebContents, stops observing that as starts observing the outer
WebContents' TextInputManager.

Also, to be honest, I saw this pattern in base/obsever_list.h comments and
presumed that this is part of the style ;). 

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

I think that only matters when you listen to more than one, but it sounds like
that will be possible here.  Fine to keep it.

+                                         RenderWidgetHostViewBase* view) {}
+
+    // Called when a view has called UpdateTextInputState on TextInputManager,
+    // regardless the call having lead to change in TextInputState. In some

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

nit: regardless of whether the call led to

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

Done.

+    // platforms this is necessary as it gives the tab-level |view| the chance
+    // do some other task, e.g., show IME if needed in Aura.
+    virtual void OnTextInputStateUpdateCalled(

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

Why do we need both this and OnTextInputStateUpdated?  Feels like it might be
simpler (and more efficient) to just have this one with a bool did_update_state.

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

Hmm. I think I took the less efficient solution to resolve kenrb@'s comment in
patch 8. I will follow your suggestion as it is simpler and looks better. 

I should also mention that |did_update_state| is only used on Aura. Later, for
Mac and Android I could enclose this in #ifdef since |did_update_state| is
always true on Android. Also on Mac, even though it is not always true, we only
inform RenderWidgetHostViewMac when it is. So it is pointless to pass the
argument in those two platforms. But I am not sure if this tiny optimization
would be in line with good design/coding practices.

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

I wouldn't worry about it unless there's a substantial difference in frequency.

+        TextInputManager* text_input_manager) {}
+
+    // Notifies all the observers that |text_input_manager| is being destroyed.

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

Let's be consistent with how the other observer methods are phrased:
// Called when the |text_input_manager| is being destroyed.

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

Acknowledged.

+    // TODO(ekaramad): All RWHV should get this call, but only the top-level
+    // RWHV for the outermost WebContents should be called on the other observer
+    // methods. We should separate the two classes of observers when resolving
+    // crbug.com/609846.
+    virtual void OnDestroyingTextInputManager(
+        TextInputManager* text_input_manager) {}
+  };
+
+  explicit TextInputManager();

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

No need for explicit unless you have a single parameter.

+  ~TextInputManager() override;
+
+  // Returns the current text input state corresponding to |active_source_|.

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

nit: TextInputState (here and below)
nit: s/|active_source_|/the currently active view/

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

Done.

+  // 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();
+
+  // Returns the currently active view. The active view is the last
+  // RenderWidgetHostViewBase which reported a |TextInputState.type| of other
+  // than ui::TEXT_INPUT_TYPE_NONE.
+  RenderWidgetHostViewBase* GetActiveView() const;

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

Can this ever be null?  Looks like active_view_ is initialized to null, so we
may want to mention when it becomes valid, etc.

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

It is nullptr initially, and whenever we lose focus on <input>. When it is null,
there should not be any TextInputState. I revised the comment.

+
+  // Updates the text input state 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);
+
+  // Clear text input state of |view| and removes TextInputManager from its
+  // observer list. It is important to call this for any RWHV being tracked
+  // before destruction of RWHV or TextInputManager.

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

"or TextInputManager" -> Should we add a DCHECK that all views have been
unregistered in ~TextInputManager?

Also, there is a risk of UaF if we get this wrong.  Should active_view_ be a
weak pointer, or are we confident that it will be cleared any time the view goes
away?

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

It depends whether we want the un-registering to happen in TextInputManager, or
in the view. I think your comment on
RenderWidgetHostViewBase::OnDestroyingTextInputManager suggests that we should
only set |text_input_manager_| to null and let TextInputManager take care of
un-registering in its destructor.

In the current implementation, RWHVs will unregister from TextInputManager
(using their member variable |text_input_manager_|) in the destructor of
RWHVBase. If the |view| is active, then |active_view_| becomes nullptr. I do not
readily see a scenario where |active_view_| is stale. The unregistering part is
similar to an observer call.

The issue we had before (original patch) was that we were obtaining
WebContentsImpl through RenderWidgetHostImpl. That does not always work out fine
in the destruction paths as we lose the reference to |host_| early on before the
dtor of RenderWidgetHostViewBase is called. Now that we have a reference to
|text_input_manager_| in every view, we should be fine with the un-registration
process.

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

Acknowledged.

+  void Unregister(RenderWidgetHostViewBase* view);
+
+  // Returns true if |view| is already registered.
+  bool IsRegisteredView(RenderWidgetHostViewBase* view) const;

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

nit: IsRegistered (to be consistent with Register and Unregister).

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

Done.

+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ private:
+  friend class TestTextInputManagerObserver;
+
+  // RenderWidgetHostViewObserver implementation.

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

nit: Missing "Base" from name.

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

Yes sorry. But I removed this method following the suggestion on line 24.

+  void OnRenderWidgetHostViewBaseDestroyed(
+      RenderWidgetHostViewBase* view) override;
+
+  void NotifyObserversAboutInputStateUpdate(RenderWidgetHostViewBase* view,
+                                            bool state_changed);
+
+  // 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_TEXT_INPUT_MANAGER_H__