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..68f0d97dd56823082b477e5d36685c0c43fb6d30 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,81 @@ 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.
+  // TODO(nona): Bidi support.

[James Su, 2012/07/05 01:44:26]

Yes, Bidi supporting is a big problem, besides it, as I said in earlier comment,
complex text layout is also a problem. For example, in Hindi (and many other
languages), a vowel may be displayed before a consonant even though it's after
the consonant in the buffer.
So I think it's probably more reliable to check y value instead of x.
For example, we may use the average (or max) height of rects in
composition_bounds_ as a reference threshold value, then we can consider it's
the start of a new line, if the y offset is larger than a certain percentage
(e.g. 75%) of the threshold value.

[Seigo Nonaka, 2012/07/05 06:59:45]

Done.

+  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()) {
+      *line_break_point = idx + 1;
+      return true;
+    }
+  }
+  return false;
+}
+
+gfx::Rect RenderWidgetHostViewMac::GetFirstRectForCompositionRange(
+    const ui::Range& range) {
+  DCHECK(range.start() < composition_bounds_.size());

[James Su, 2012/07/05 01:44:26]

I'm still wondering if it's possible that an IME wants to know the end position
of a composition range, so it calls this method with rang.start() == rang.end()
== composition_bounds_.size() ?

[Seigo Nonaka, 2012/07/05 06:59:45]

Done.

+  DCHECK(range.end() <= composition_bounds_.size());
+
+  if (range.is_empty()) {
+    return gfx::Rect(composition_bounds_[range.start()].x(),
+                     composition_bounds_[range.start()].y(),
+                     0,
+                     composition_bounds_[range.start()].height());
+  }
+
+  size_t end_idx;
+  if (!GetLineBreakIdx(composition_bounds_, range, &end_idx)) {
+    end_idx = range.end();
+  }
+  gfx::Rect rect = composition_bounds_[range.start()];
+  for (size_t i = range.start() + 1; i < end_idx; ++i) {
+    rect = rect.Union(composition_bounds_[i]);
+  }
+  return rect;
+}
+
+ui::Range RenderWidgetHostViewMac::ConvertCharacterRangeToCompositionRange(
+    const ui::Range& request_range) {
+  if (composition_range_.is_empty())

[James Su, 2012/07/05 01:44:26]

I think it's better to return correct rect for an empty range, if the range is
inside the composition range.

[Seigo Nonaka, 2012/07/05 06:59:45]

composition_range_.is_empty() == true means there are no cacned composition
rectangles. So we can't construct any rectangle from any range.

On 2012/07/05 01:44:26, 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/05 01:44:26]

request_range.start() may equal to composition_range_.end(), if the ime does
want to know the end position of the composition string.

[Seigo Nonaka, 2012/07/05 06:59:45]

Done.

+    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 +2761,12 @@ extern NSString *NSTextInputReplacementRangeAttributeName;
   // TODO(thakis): Pipe |actualRange| through TextInputClientMac machinery.
   if (actualRange)
     *actualRange = theRange;

[James Su, 2012/07/05 01:44:26]

Can you please help fix the logic of actualRange? We need to set it correctly.

[Seigo Nonaka, 2012/07/05 06:59:45]

Done for chached rectangle. 
On 2012/07/05 01:44:26, James Su wrote:

-  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