Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

ui/views/cocoa/bridged_content_view.mm

 // Copyright 2014 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.
 
 #import "ui/views/cocoa/bridged_content_view.h"
 
 #include "base/logging.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/strings/sys_string_conversions.h"
 #include "skia/ext/skia_utils_mac.h"
@@ skipping 38 matching lines @@
 bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
   MenuController* menuController = MenuController::GetActiveInstance();
   if (menuController && menuController->owner() == widget) {
     if (menuController->OnWillDispatchKeyEvent(0, key_code) ==
         ui::POST_DISPATCH_NONE)
       return true;
   }
   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 ()
 
 // Translates keycodes and modifiers on |theEvent| to ui::KeyEvents and passes
 // the event to the InputMethod for dispatch.
 - (void)handleKeyEvent:(NSEvent*)theEvent;
 
 // Handles an NSResponder Action Message by mapping it to a corresponding text
 // editing command from ui_strings.grd and, when not being sent to a
@@ skipping 516 matching lines @@
   }
 
   if ([self respondsToSelector:selector])
     [self performSelector:selector withObject:nil];
   else
     [[self nextResponder] doCommandBySelector:selector];
 }
 
 - (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 {
   return textInputClient_ && textInputClient_->HasCompositionText();
 }
 
 - (void)insertText:(id)text replacementRange:(NSRange)replacementRange {
   if (!hostedView_)
     return;
 
@@ skipping 115 matching lines @@
   }
 
   return [super accessibilityAttributeValue:attribute];
 }
 
 - (id)accessibilityHitTest:(NSPoint)point {
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 @end