Use cached composition bounds for firstRectForCharacterRange

content/browser/renderer_host/render_widget_host_view_mac.mm

Index: content/browser/renderer_host/render_widget_host_view_mac.mm
diff --git a/content/browser/renderer_host/render_widget_host_view_mac.mm b/content/browser/renderer_host/render_widget_host_view_mac.mm
index e6bfb673c4ff0c1c50a7a35d4435727b3d4ced9f..d53c5c4cd9b3c074ddb38b6a91f7130103e204ac 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -647,6 +647,8 @@ void RenderWidgetHostViewMac::ImeCompositionRangeChanged(
   // The RangeChanged message is only sent with valid values. The current
   // caret position (start == end) will be sent if there is no IME range.
   [cocoa_view_ setMarkedRange:range.ToNSRange()];
+  composition_range_ = range;
+  composition_bounds_ = character_bounds;
 }
 
 void RenderWidgetHostViewMac::DidUpdateBackingStore(
@@ -1039,6 +1041,79 @@ void RenderWidgetHostViewMac::AckPendingSwapBuffers() {
   }
 }
 
+bool RenderWidgetHostViewMac::GetLineBreakIdx(
+    const std::vector<gfx::Rect>& bounds,
+    const ui::Range& range,
+    size_t* line_break_point) {
+  DCHECK(line_break_point);
+  if (range.start() >= bounds.size() || range.is_reversed() || range.is_empty())
+    return false;
+
+  // Detect line break by comparing x value because we can't assume the
+  // each character's height is fixed.
+  const size_t loop_end_idx = std::min(bounds.size(), range.end());
+  for (size_t idx = range.start(); idx < loop_end_idx - 1; ++idx) {
+    if (bounds[idx].x() > bounds[idx + 1].x()) {

[James Su, 2012/07/04 01:44:04]

I'm wondering if it works for text with complex layout characters, in which the
characters visual position maybe swapped.

[Seigo Nonaka, 2012/07/04 06:56:58]

Yes, this logic does not work for bidi characters.
However at this moment, this function is used only for composition rectangle.
So if there is some tool which composes bidi characters, this should be fixed,
but I think it's okay to leave TODO.
What do you think?

And I'm not sure about the correct behavior of firstRectForCharRange for bidi
mixed character.


On 2012/07/04 01:44:04, James Su wrote:

+      *line_break_point = idx + 1;
+      return true;
+    }
+  }
+  return false;
+}
+
+gfx::Rect RenderWidgetHostViewMac::GetFirstRectForCompositionRange(
+    const ui::Range& range) {
+  DCHECK(range.start() < composition_bounds_.size());
+  DCHECK(range.end() < composition_bounds_.size());

[James Su, 2012/07/04 01:44:04]

start and end may equal to composition_bounds_.size().

[Seigo Nonaka, 2012/07/04 06:56:58]

Done for range.end().
start point is inclusive, so we can't accept it.
On 2012/07/04 01:44:04, James Su wrote:

+  int right = 0;
+  if (range.is_empty()) {
+    right = composition_bounds_[range.start()].x();
+  } else {
+    size_t line_break_idx;
+    if (GetLineBreakIdx(composition_bounds_, range, &line_break_idx)) {
+      right = composition_bounds_[line_break_idx - 1].right();
+    } else {
+      right = composition_bounds_[range.end() - 1].right();
+    }
+  }
+  return gfx::Rect(
+      composition_bounds_[range.start()].x(),
+      composition_bounds_[range.start()].y(),
+      right - composition_bounds_[range.start()].x(),
+      composition_bounds_[range.start()].height());

[James Su, 2012/07/04 01:44:04]

I think we should return the union rect of all rects within [start, end),
instead of just a rect calculated by start and end.

[Seigo Nonaka, 2012/07/04 06:56:58]

Done.

+}
+
+ui::Range RenderWidgetHostViewMac::ConvertCharacterRangeToCompositionRange(
+    const ui::Range& request_range) {
+  if (composition_range_.is_empty())

[James Su, 2012/07/04 01:44:04]

I think we probably should handle the empty case, as long as it's inside the
composition_range_. Maybe we should return a zero width rect for an empty range.

[Seigo Nonaka, 2012/07/04 06:56:58]

This check means that we don't have any cached composition boundaries. If you
worry about request_range empty case, it's okay because we already handled.

On 2012/07/04 01:44:04, James Su wrote:

+    return ui::Range::InvalidRange();
+
+  if (request_range.is_reversed())
+    return ui::Range::InvalidRange();
+
+  if (request_range.start() < composition_range_.start() ||
+      request_range.start() >= composition_range_.end())

[James Su, 2012/07/04 01:44:04]

request_rang.end() > composition_range_.end() ?

[Seigo Nonaka, 2012/07/04 06:56:58]

The end is exclusive, so treating out of range is safer I think.
On 2012/07/04 01:44:04, James Su wrote:

+    return ui::Range::InvalidRange();
+
+  return ui::Range(
+      request_range.start() - composition_range_.start(),
+      request_range.end() - composition_range_.start());
+}
+
+bool RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(
+  NSRange range,
+  NSRect* rect) {
+  TRACE_EVENT0("browser",
+               "RenderWidgetHostViewMac::GetFirstRectForCharacterRange");
+  const ui::Range request_range_in_composition =
+    ConvertCharacterRangeToCompositionRange(ui::Range(range));
+  if (request_range_in_composition == ui::Range::InvalidRange())
+    return false;
+  *rect = NSRectFromCGRect(
+      GetFirstRectForCompositionRange(request_range_in_composition).ToCGRect());
+  return true;
+}
+
 void RenderWidgetHostViewMac::AcceleratedSurfaceBuffersSwapped(
     const GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params& params,
     int gpu_host_id) {
@@ -2684,8 +2759,12 @@ extern NSString *NSTextInputReplacementRangeAttributeName;
   // TODO(thakis): Pipe |actualRange| through TextInputClientMac machinery.
   if (actualRange)
     *actualRange = theRange;
-  NSRect rect = TextInputClientMac::GetInstance()->GetFirstRectForRange(
-      renderWidgetHostView_->render_widget_host_, theRange);
+  NSRect rect;
+  if (!renderWidgetHostView_->GetCachedFirstRectForCharacterRange(theRange,
+                                                                  &rect)) {
+    rect = TextInputClientMac::GetInstance()->GetFirstRectForRange(
+        renderWidgetHostView_->render_widget_host_, theRange);
+  }
 
   // The returned rectangle is in WebKit coordinates (upper left origin), so
   // flip the coordinate system and then convert it into screen coordinates for