Avoid sync IPCs for firstRectForCharacterRange/attributedSubstringForProposedRange.

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 9500038092b8c8630da89909a8f4d2ae95452169..0a27810bbac370c0f34bf78b063e5a154ae2001b 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -1226,6 +1226,7 @@ void RenderWidgetHostViewMac::SelectionBoundsChanged(
     const ViewHostMsg_SelectionBounds_Params& params) {
   if (params.anchor_rect == params.focus_rect)
     caret_rect_ = params.anchor_rect;
+  first_selection_rect_ = params.anchor_rect;
 }
 
 void RenderWidgetHostViewMac::SetShowingContextMenu(bool showing) {
@@ -1446,16 +1447,26 @@ bool RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(
   TRACE_EVENT0("browser",
                "RenderWidgetHostViewMac::GetFirstRectForCharacterRange");
 
+  const gfx::Range requested_range(range);
   // If requested range is same as caret location, we can just return it.
-  if (selection_range_.is_empty() && gfx::Range(range) == selection_range_) {
+  if (selection_range_.is_empty() && requested_range == selection_range_) {
     if (actual_range)
       *actual_range = range;
     *rect = NSRectFromCGRect(caret_rect_.ToCGRect());
     return true;
   }
 
+  if (composition_range_.is_empty()) {
+    if (!selection_range_.Contains(requested_range))
+      return false;
+    if (actual_range)
+      *actual_range = selection_range_.ToNSRange();
+    *rect = NSRectFromCGRect(first_selection_rect_.ToCGRect());
+    return true;
+  }
+
   const gfx::Range request_range_in_composition =
-      ConvertCharacterRangeToCompositionRange(gfx::Range(range));
+      ConvertCharacterRangeToCompositionRange(requested_range);
   if (request_range_in_composition == gfx::Range::InvalidRange())
     return false;
 
@@ -2895,10 +2906,36 @@ extern NSString *NSTextInputReplacementRangeAttributeName;
   // TODO(thakis): Pipe |actualRange| through TextInputClientMac machinery.
   if (actualRange)
     *actualRange = range;
-  NSAttributedString* str =
-      TextInputClientMac::GetInstance()->GetAttributedSubstringFromRange(
-          renderWidgetHostView_->render_widget_host_, range);
-  return str;
+
+  const gfx::Range requested_range(range);
+  if (requested_range.is_reversed())
+    return nil;
+
+  gfx::Range expected_range;
+  const base::string16* expected_text;
+
+  if (!renderWidgetHostView_->composition_range().is_empty()) {
+    expected_text = &markedText_;
+    expected_range = renderWidgetHostView_->composition_range();
+  } else {
+    expected_text = &renderWidgetHostView_->selection_text();
+    size_t offset = renderWidgetHostView_->selection_text_offset();
+    expected_range = gfx::Range(offset, offset + expected_text->size());

[Shu Chen, 2015/09/02 05:04:36]

from palmer@:

How do you know this range is valid? Can offset + expected_text->size() go
outside the available space?

[Shu Chen, 2015/09/02 05:41:22]

RenderWidgetHostViewBase must make sure selection_text() and
selection_text_offset() is valid in the whole range.

+  }
+
+  if (!expected_range.Contains(requested_range))
+    return nil;
+
+  // Gets the raw bytes to avoid unnecessary string copies for generating
+  // NSString.
+  const base::char16* bytes =
+      &(*expected_text)[requested_range.start() - expected_range.start()];

[Shu Chen, 2015/09/02 05:04:36]

from palmer@:

I don't understand this code. What is going on?

What guarantees that the expression "requested_range.start() -
expected_range.start()" describes a valid range of memory?

[Shu Chen, 2015/09/02 05:41:22]

The above "expected_range.Contains(requested_range)" check makes sure of that.

+  NSUInteger bytes_len = requested_range.length() * sizeof(base::char16);

[Shu Chen, 2015/09/02 05:04:36]

from palmer@:

Is NSUInteger guaranteed to have the same range as a size_t? If not, there could
be integer truncation here.

You might want to make use of base/numerics/safe_conversions.h.

[Shu Chen, 2015/09/02 05:41:22]

Doc says "When building 32-bit applications, NSUInteger is a 32-bit unsigned
integer. A 64-bit application treats NSUInteger as a 64-bit unsigned integer".
And size_t is "unsigned int" in 32-bit while "long unsigned int" in 64-bit.
So I don't see potential risks of integer truncation.

But anyway, I've used strict_cast() to make it safer.

+  base::scoped_nsobject<NSString> ns_string(
+      [[NSString alloc] initWithBytes:bytes
+                               length:bytes_len
+                             encoding:NSUTF16LittleEndianStringEncoding]);
+  return [[[NSAttributedString alloc] initWithString:ns_string] autorelease];
 }
 
 - (NSInteger)conversationIdentifier {