Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

ui/views/cocoa/bridged_content_view.mm

Index: ui/views/cocoa/bridged_content_view.mm
diff --git a/ui/views/cocoa/bridged_content_view.mm b/ui/views/cocoa/bridged_content_view.mm
index 2167561368c38f1b8a637d99c1b1daa022faae15..2a4eee483a1fa2f7806261a3c4974e9a3ac91dc9 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -56,6 +56,72 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
   return false;
 }
 
+gfx::Rect GetFirstRectForRangeHelper(
+    const ui::TextInputClient* textInputClient_,

[tapted, 2015/12/17 08:42:20]

no trailing underscores on function arguments (also it should be hacker_style
since we're not between @implementation/@end). Maybe calling this `client` will
neaten up the formatting a bit

[karandeepb, 2015/12/18 09:15:06]

Done.

+    const gfx::Range& range,

[tapted, 2015/12/17 08:42:20]

range -> requested_range? (for clarity)

[karandeepb, 2015/12/18 09:15:06]

Done.

+    gfx::Range& actual_range) {

[tapted, 2015/12/17 08:42:20]

chromium style doesn't allow non-const references, so this will need to be a
pointer

[karandeepb, 2015/12/18 09:15:06]

Done.

+  if (!textInputClient_)
+    return gfx::Rect(0, 0, 0, 0);

[tapted, 2015/12/17 08:42:20]

return gfx::Rect();

[karandeepb, 2015/12/18 09:15:06]

Done.

+
+  // Set up default return values, to be returned in case of unusual cases.
+  gfx::Rect caret_bounds = textInputClient_->GetCaretBounds();
+  caret_bounds.set_width(0);
+  actual_range.set_start(range.start());

[tapted, 2015/12/17 08:42:19]

*actual_range = gfx::Range(requested_rangerange.start());

or is there a downside to doing

*actual_range = requested_range?

[karandeepb, 2015/12/18 09:15:06]

actual_range should correspond to the returned rectangle. Hence when we are
returning caret_bounds, it feels more natural to return an empty range. 

+  actual_range.set_end(range.start());
+
+  if (!textInputClient_->HasCompositionText())
+    return caret_bounds;
+
+  if (range.is_reversed())

[tapted, 2015/12/17 08:42:19]

I'm pretty sure it's impossible for requested_range to be reversed -- NSRange
doesn't support reversed ranges since it is (index+unsignedlength) rather than
(start_index,end_index]

Perhaps

  // NSRange doesn't support reversed ranges.
  DCHECK(!requested_range.is_reversed());

[karandeepb, 2015/12/18 09:15:06]

Done.

+    return caret_bounds;
+
+  gfx::Range composition_range;
+  if (!textInputClient_->GetCompositionTextRange(&composition_range))
+    return caret_bounds;
+
+  if (!composition_range.Contains(gfx::Range(range.start())) ||

[tapted, 2015/12/17 08:42:20]

why not just `if (!composition_range.Contains(requested_range)`?

[karandeepb, 2015/12/18 09:15:06]

Done.

+      !composition_range.Contains(gfx::Range(range.end())))
+    return caret_bounds;
+
+  // If range is empty, return a zero width rectangle corresponding to the
+  // queried range.
+  if (range.is_empty()) {
+    size_t end_index = composition_range.end();
+    gfx::Rect current_rect;
+    if (range.start() == end_index) {
+      // Return a rectangle corresponding to the top right corner of the last
+      // compositioned character.
+      if (!textInputClient_->GetCompositionCharacterBounds(
+              end_index - 1 - composition_range.start(), &current_rect))

[tapted, 2015/12/17 08:42:19]

would RenderWidgetHostViewMac::ConvertCharacterRangeToCompositionRange(..) help
isolate some of this arithmetic each time you call
GetCompositionCharacterBounds()?

[karandeepb, 2015/12/18 09:15:06]

Done.

+        return caret_bounds;
+      current_rect.set_origin(current_rect.top_right());
+    } else if (!textInputClient_->GetCompositionCharacterBounds(
+                   range.start() - composition_range.start(), &current_rect))

[tapted, 2015/12/17 08:42:20]

this `else if` needs curlies, since it's attached to an `if` that has curlies

[karandeepb, 2015/12/18 09:15:06]

Done.

+      return caret_bounds;
+    current_rect.set_width(0);
+    return current_rect;
+  }
+
+  // In browser multi-line textfields are not supported by Chrome currently.

[tapted, 2015/12/17 08:42:19]

Perhaps something like

  // Toolkit-views textfields are always single-line, so no need to check for
  // line breaks.

[karandeepb, 2015/12/18 09:15:06]

Done.

+  // Hence we don't check for a line break.
+
+  // Return the union rectangle of the character bounds within the query range.
+  gfx::Rect union_rect, current_rect;
+  for (size_t i = range.start(); i < range.end(); i++) {
+    if (textInputClient_->GetCompositionCharacterBounds(
+            i - composition_range.start(), &current_rect))
+      union_rect.Union(current_rect);
+    else if (i == range.start())
+      return caret_bounds;

[tapted, 2015/12/17 08:42:19]

can this be dealt with outside the loop? (maybe start the loop at range.start()
+ 1).

Perhaps initialize union_rect with the first character (return caret_bounds if
that fails), then move the declaration for current_rect into the for loop.

I feel like maybe you could combine that with the empty-range case as well.

e.g. 

gfx::Rect union_rect;
size_t first_index = /* some logic */
if (!client->GetCompositionCharacterBounds(first_index, &union_rect))
  return caret_bounds;

// If the range is empty, return a zero-width rectangle corresponding..
if (requested_range.is_empty()) {
  // For <case>, use the right-side of the rect instead of the left.
  if (range.start() == end_index)
    union_rect.set_origin(union_rect.top_right());
  union_rect.set_width(0);
  return union_rect;
}

for (size_t i = range.start() + 1; i < range.end(); i++) {
  /* etc */

[karandeepb, 2015/12/18 09:15:06]

Done.

+    else {

[tapted, 2015/12/17 08:42:19]

if any bit of an if/else chain has a curly, it all needs curlies

[karandeepb, 2015/12/18 09:15:06]

Done.

+      actual_range.set_end(i);
+      return union_rect;
+    }
+  }
+  actual_range.set_end(range.end());
+  return union_rect;
+}
+
 }  // namespace
 
 @interface BridgedContentView ()
@@ -592,8 +658,20 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range
                          actualRange:(NSRangePointer)actualRange {
-  NOTIMPLEMENTED();
-  return NSZeroRect;
+  gfx::Range requested_range(range);

[tapted, 2015/12/17 08:42:20]

since this is between @implementation and @end (i.e. it's an ObjC area of code),
we need to use camelCase for variable names (go/objcstyle)

[karandeepb, 2015/12/18 09:15:06]

Done.

+  gfx::Range actual_range;
+  gfx::Rect rect = GetFirstRectForRangeHelper(textInputClient_, requested_range,
+                                              actual_range);
+  NSRect ns_rect = NSRectFromCGRect(rect.ToCGRect());
+  if (NSEqualRects(ns_rect, NSZeroRect)) {

[tapted, 2015/12/17 08:42:19]

nit: move this up a line and do

if (rect.IsEmpty())
  return NSZeroRect;

Also what happens if this early exit is omitted? (E.g. is the only downside that
the for the error case, the popup appears in the top-left rather than the
bottom-left of screen - that might be fine)

[karandeepb, 2015/12/18 09:15:06]

Removed the early exit. Also, its unlikely for textInputClient_ to be nil, so
shouldn't matter.

+    return NSZeroRect;
+  }
+  *actualRange = actual_range.ToNSRange();
+  // ns_rect is in screen coordinates with origin at top left. Convert to Apple

[tapted, 2015/12/17 08:42:20]

return gfx::ScreenRectToNSRect(screen_rect);

[karandeepb, 2015/12/18 09:15:06]

Done.

+  // coordinate system with origin at bottom left
+  NSRect screen_rect = [[NSScreen mainScreen] frame];
+  ns_rect.origin.y = screen_rect.size.height - NSMaxY(ns_rect);
+  return ns_rect;
 }
 
 - (BOOL)hasMarkedText {