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

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 <utility>
 
@@ skipping 79 matching lines @@
   // The following methods can be used to obtain information about IME-related
   // state for the active RenderWidgetHost. If the active widget is nullptr, the
   // methods below will return nullptr as well.
   // Users of these methods should not hold on to the pointers as they become
   // dangling if the TextInputManager or |active_view_| are destroyed.
 
   // Returns the currently stored TextInputState. An state of nullptr can be
   // 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;
 
   // The following method returns the text selection state for the given |view|.
   // If |view| == nullptr, it will assume |active_view_| and return its state.
   // In the case of |active_view_| == nullptr, the method will return nullptr.
   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.
 
   // Updates the TextInputState for |view|.
   void UpdateTextInputState(RenderWidgetHostViewBase* view,
                             const TextInputState& state);
 
   // The current IME composition has been cancelled on the renderer side for
   // the widget corresponding to |view|.
@@ skipping 39 matching lines @@
   // TextInputManager.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
   RenderWidgetHostViewBase* active_view_for_testing() { return active_view_; }
   size_t GetRegisteredViewsCountForTesting();
   ui::TextInputType GetTextInputTypeForViewForTesting(
       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 {
     CompositionRangeInfo();
     CompositionRangeInfo(const CompositionRangeInfo& other);
     ~CompositionRangeInfo();
 
     std::vector<gfx::Rect> character_bounds;
   };
 
@@ skipping 19 matching lines @@
   ViewMap<CompositionRangeInfo> composition_range_info_map_;
   ViewMap<TextSelection> text_selection_map_;
 
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(TextInputManager);
 };
 }
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_TEXT_INPUT_MANAGER_H__