Tracking SelectionBounds for all RenderWidgets on the Browser Side (Mac)

content/browser/renderer_host/text_input_manager.h

Index: content/browser/renderer_host/text_input_manager.h
diff --git a/content/browser/renderer_host/text_input_manager.h b/content/browser/renderer_host/text_input_manager.h
index b09ae5069420f5c96af8b022c118a5a5e52fc008..76265fecefc92bc07f33e24f9f53653c797e1cdf 100644
--- a/content/browser/renderer_host/text_input_manager.h
+++ b/content/browser/renderer_host/text_input_manager.h
@@ -97,9 +97,6 @@ class CONTENT_EXPORT TextInputManager {
   // interpreted as a ui::TextInputType of ui::TEXT_INPUT_TYPE_NONE.
   const TextInputState* GetTextInputState() const;
 
-  // Returns the rect between selection bounds.
-  gfx::Rect GetSelectionBoundsRect() const;
-
   // Returns a vector of rects representing the character bounds.
   const std::vector<gfx::Rect>* GetCompositionCharacterBounds() const;
 
@@ -109,6 +106,17 @@ class CONTENT_EXPORT TextInputManager {
   const TextSelection* GetTextSelection(
       RenderWidgetHostViewBase* view = nullptr) const;
 
+#if defined(USE_AURA)
+  // Returns the rect between selection bounds.
+  gfx::Rect GetSelectionBoundsRect() const;
+#elif defined(OS_MACOSX)

[Charlie Reis, 2016/08/04 18:44:51]

Hmm, this seems unfortunate to need different selection APIs on different
platforms.  Ifdefs tend to have a code complexity cost, so I generally try to
avoid them unless they're part of a temporary transition, or a fundamental need.

Maybe we're stuck with this, but it's worth thinking whether there's a way to
have a consistent API across platforms here.

[EhsanK, 2016/08/04 23:53:02]

There is more in the .cc file for each specific method.

The whole state tracking logic can be looked at 1) preprocessing the update
received from RWH and 2) making decision after that. Both parts were platform
specific before. The way we did TextInputManager, which might not necessarily be
optimal delegates part (2) to the tab's view for the platform specific
decisions. Part (1) unfortunately has to either be taken care of in RWHVBase, or
here.

I will give this another try tomorrow to see if I can refactor this better. That
being said, another alternative would be to have platform specific
TextInputManager implementations for Aura, Mac, and Android.

Also having the same class for all platforms, such as SelectionRegion below is
fine but I guess not idea since we consume more memory.

[kenrb, 2016/08/08 22:03:04]

What if we just store (more or less) the raw contents of
ViewHostMsg_SelectionBounds_Params in this struct, and have the
RenderWidgetHostView calculate the values that it needs? You might need to store
a little bit extra for first_selection_rect on Mac, but having a data member
that isn't used on Aura isn't that bad.

+  // Returns the first caret bound for the view corresponding to |widget|.
+  const gfx::Rect* GetCaretRect(RenderWidgetHostImpl* widget) const;
+
+  // Returns the first selection bound for the view corresponding to |widget|.
+  const gfx::Rect* GetFirstSelectionRect(RenderWidgetHostImpl* widget) const;
+#endif
+
   // ---------------------------------------------------------------------------
   // The following methods are called by RWHVs on the tab to update their IME-
   // related state.
@@ -168,16 +176,24 @@ class CONTENT_EXPORT TextInputManager {
       RenderWidgetHostViewBase* view);
 
  private:
+#if !defined(OS_ANDROID)
   // Text selection bounds.
   struct SelectionRegion {
     SelectionRegion();
     SelectionRegion(const SelectionRegion& other);
-
+#if defined(USE_AURA)
     // The begining of the selection region.
     gfx::SelectionBound anchor;
     // The end of the selection region (caret position).
     gfx::SelectionBound focus;
+#else   // OS_MACOSX

[Charlie Reis, 2016/08/04 18:44:51]

Same here, and more so.  This will be difficult to reason about and maintain,
and it'll be tough to follow the code in codesearch without cross-linking.

Is there a way to avoid this?

[EhsanK, 2016/08/04 23:53:02]

Let me think about this. Meanwhile, all suggestions are welcome!

+    // The current caret bounds.
+    gfx::Rect caret_rect;
+    // The current first selection bounds.
+    gfx::Rect first_selection_rect;
+#endif  // USE_AURA
   };
+#endif
 
   // Ccomposition range information.
   struct CompositionRangeInfo {