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 66f1c31650f7799701f8aba763b515e7c340aaeb..9589d98fb894be819b7ef6b75b03265c45e8aeb6 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -1225,6 +1225,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) {
@@ -1453,6 +1454,12 @@ bool RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(
     return true;
   }
 
+  if (composition_range_.is_empty()) {
+    *actual_range = selection_range_.ToNSRange();
+    *rect = NSRectFromCGRect(first_selection_rect_.ToCGRect());
+    return true;
+  }
+
   const gfx::Range request_range_in_composition =
       ConvertCharacterRangeToCompositionRange(gfx::Range(range));
   if (request_range_in_composition == gfx::Range::InvalidRange())
@@ -2887,10 +2894,30 @@ 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;
+  gfx::Range expected_range;

[yukawa, 2015/08/21 03:46:13]

How about making sure if |range| is not reversed as follows?  Seems that
gfx::Range has a special constructor for NSRange.

  const gfx::Range requested_range(range);
  if (requested_range.is_reversed())
    return nil;

[Shu Chen, 2015/08/21 05:17:04]

Done.

+  // Using pointer to avoid string copy.
+  const base::string16* expected_text;

[yukawa, 2015/08/21 03:46:13]

Can we use StringPiece16 instead?

[Shu Chen, 2015/08/21 05:17:04]

Using StringPiece16 cannot make the code more concise. But I've revised the code
to use it anyway.

[yukawa, 2015/08/21 05:45:24]

I see.  Well, sorry for asking you for different thing again but I'm a bit
worried about the risk of unexpected use-after-free here if someone changes
internal details of |renderWidgetHostView_->composition_range()| or
|renderWidgetHostView_->selection_text()|.  How about simply copying the string
here?  It's up to you, but I suppose string copy is acceptable, probably.

[Shu Chen, 2015/08/21 05:59:49]

Sorry I don't understand. The |exptected_text| is used temporarily and
synchronously.
Would there be a chance that internal state is changed in a multi-threading
environment while this method is executing?

[yukawa, 2015/08/21 06:19:26]

const std::string selection_text() {
  return std::string("test");
}

void test1() {
  const std::string* ptr = &selection_text();
  // |ptr| points a destroyed object.
}

void test2() {
  StringPiece str = selection_text();
  // |str| points a destroyed object.
}

Basically we have to be careful about the return type of |selection_text()|,
which I think is hard to check when reading the caller site only.

[yukawa, 2015/08/21 06:25:00]

So, it's not the internal details but the object type.  Sorry for confusing you.

[Shu Chen, 2015/08/21 06:36:49]

OK. The string copy sounds a reasonable price to pay to avoid future headaches.
:)

+
+  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->length());
+  }
+
+  if (expected_range.start() > range.location ||

[yukawa, 2015/08/21 03:46:13]

  if (!expected_range.Contains(expected_range))

[yukawa, 2015/08/21 03:49:36]

Correction:
  if (!expected_range.Contains(requested_range))

[Shu Chen, 2015/08/21 05:17:04]

Done.

+      expected_range.end() < range.location + range.length) {
+    return nil;
+  }
+
+  // Uses wide string for Unicode string truncation.
+  std::string result_text = base::WideToUTF8(
+      base::UTF16ToWide(*expected_text).substr(
+          range.location - expected_range.start(), range.length));
+  NSString *nstr = [[NSString alloc] initWithUTF8String:result_text.c_str()];
+  return [[NSAttributedString alloc] initWithString:nstr];
 }
 
 - (NSInteger)conversationIdentifier {