Use cached composition bounds for firstRectForCharacterRange

content/browser/renderer_host/render_widget_host_view_mac.mm

 // Copyright (c) 2012 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.
 
 #include "content/browser/renderer_host/render_widget_host_view_mac.h"
 
 #include <QuartzCore/QuartzCore.h>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 629 matching lines @@
 void RenderWidgetHostViewMac::ImeCancelComposition() {
   [cocoa_view_ cancelComposition];
 }
 
 void RenderWidgetHostViewMac::ImeCompositionRangeChanged(
     const ui::Range& range,
     const std::vector<gfx::Rect>& character_bounds) {
   // 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(
     const gfx::Rect& scroll_rect, int scroll_dx, int scroll_dy,
     const std::vector<gfx::Rect>& copy_rects) {
   GotSoftwareFrame();
 
   if (!is_hidden_) {
     std::vector<gfx::Rect> rects(copy_rects);
 
@@ skipping 372 matching lines @@
           pending_swap_buffers_acks_.front().first,
           pending_swap_buffers_acks_.front().second,
           0);
       if (render_widget_host_)
         render_widget_host_->AcknowledgeSwapBuffersToRenderer();
     }
     pending_swap_buffers_acks_.erase(pending_swap_buffers_acks_.begin());
   }
 }
 
+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) {
   TRACE_EVENT0("browser",
       "RenderWidgetHostViewMac::AcceleratedSurfaceBuffersSwapped");
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   pending_swap_buffers_acks_.push_back(std::make_pair(params.route_id,
                                                       gpu_host_id));
 
@@ skipping 1624 matching lines @@
       TextInputClientMac::GetInstance()->GetCharacterIndexAtPoint(
           renderWidgetHostView_->render_widget_host_,
           gfx::Point(thePoint.x, thePoint.y));
   return index;
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)theRange
                          actualRange:(NSRangePointer)actualRange {
   // 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
   // return.
   NSRect viewFrame = [self frame];
   rect.origin.y = NSHeight(viewFrame) - rect.origin.y;
   rect.origin.y -= rect.size.height;
   rect = [self convertRectToBase:rect];
   rect.origin = [[self window] convertBaseToScreen:rect.origin];
   return rect;
@@ skipping 392 matching lines @@
   if (!string) return NO;
 
   // If the user is currently using an IME, confirm the IME input,
   // and then insert the text from the service, the same as TextEdit and Safari.
   [self confirmComposition];
   [self insertText:string];
   return YES;
 }
 
 @end