Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

ui/views/cocoa/bridged_native_widget_unittest.mm

Index: ui/views/cocoa/bridged_native_widget_unittest.mm
diff --git a/ui/views/cocoa/bridged_native_widget_unittest.mm b/ui/views/cocoa/bridged_native_widget_unittest.mm
index 3f0d110643b817a4b8e6aeb20a4ec666d68d768d..fde5af4fd3ae9190925667891c014e32f7f00be9 100644
--- a/ui/views/cocoa/bridged_native_widget_unittest.mm
+++ b/ui/views/cocoa/bridged_native_widget_unittest.mm
@@ -17,6 +17,7 @@
 #import "ui/base/cocoa/window_size_constants.h"
 #include "ui/base/ime/input_method.h"
 #import "ui/gfx/test/ui_cocoa_test_helper.h"
+#include "ui/gfx/mac/coordinate_conversion.h"

[tapted, 2015/12/30 06:48:53]

nit: import

[karandeepb, 2015/12/31 02:42:43]

Done.

 #import "ui/views/cocoa/bridged_content_view.h"
 #import "ui/views/cocoa/native_widget_mac_nswindow.h"
 #import "ui/views/cocoa/views_nswindow_delegate.h"
@@ -43,6 +44,37 @@ NSRange EmptyRange() {
   return NSMakeRange(NSNotFound, 0);
 }
 
+// Sets |composition_text| as the composition text with caret placed at
+// |caret_pos| and updates |caret_range|.
+void SetCompositionText(ui::TextInputClient* client,
+                        const base::string16& composition_text,
+                        const int caret_pos,
+                        NSRange* caret_range = nil) {

[tapted, 2015/12/30 06:48:53]

I don't think anything relies on the default argument. Remove?

[karandeepb, 2015/12/31 02:42:43]

Done.

+  ui::CompositionText composition;
+  composition.selection = gfx::Range(caret_pos);
+  composition.text = composition_text;
+  client->SetCompositionText(composition);
+  if (caret_range)
+    *caret_range = NSMakeRange(caret_pos, 0);
+}
+
+// Returns a zero width rectangle corresponding to current caret position.
+gfx::Rect GetCaretBounds(const ui::TextInputClient* client) {
+  gfx::Rect caret_bounds = client->GetCaretBounds();
+  caret_bounds.set_width(0);
+  return caret_bounds;
+}
+
+// Returns a zero width rectangle corresponding to caret bounds when it's placed
+// at |caret_pos| and updates |caret_range|.
+gfx::Rect GetCaretBoundsForPosition(ui::TextInputClient* client,
+                                    const base::string16& composition_text,
+                                    const int caret_pos,
+                                    NSRange* caret_range = nil) {

[tapted, 2015/12/30 06:48:52]

I think there's one call that wants the default - maybe just omit it. default
arguments haven't really taken off in Chrome yet, since they've only recently
been blessed by go/cppguide

[karandeepb, 2015/12/31 02:42:44]

Done.

+  SetCompositionText(client, composition_text, caret_pos, caret_range);
+  return GetCaretBounds(client);
+}
+
 }  // namespace
 
 // Class to override -[NSWindow toggleFullScreen:] to a no-op. This simulates
@@ -128,6 +160,8 @@ class BridgedNativeWidgetTestBase : public ui::CocoaTest {
     // Opacity defaults to "infer" which is usually updated by ViewsDelegate.
     init_params_.opacity = Widget::InitParams::OPAQUE_WINDOW;
 
+    init_params_.bounds = gfx::Rect(100, 100, 100, 100);
+
     native_widget_mac_->GetWidget()->Init(init_params_);
   }
 
@@ -174,6 +208,7 @@ BridgedNativeWidgetTest::~BridgedNativeWidgetTest() {
 void BridgedNativeWidgetTest::InstallTextField(const std::string& text) {
   Textfield* textfield = new Textfield();
   textfield->SetText(ASCIIToUTF16(text));
+  textfield->SetBoundsRect(init_params_.bounds);
   view_->AddChildView(textfield);
 
   // Request focus so the InputMethod can dispatch events to the RootView, and
@@ -505,6 +540,101 @@ TEST_F(BridgedNativeWidgetTest, TextInput_DeleteForward) {
   EXPECT_EQ_RANGE(NSMakeRange(0, 0), [ns_view_ selectedRange]);
 }
 
+// Test firstRectForCharacterRange:actualRange for cases where query range is
+// empty or outside composition range.
+TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange_Caret) {
+  InstallTextField("");
+  ui::TextInputClient* client = [ns_view_ textInputClient];
+
+  // No composition. Ensure bounds and range corresponding to the current caret
+  // position are returned.
+  // Initially selection range will be [0, 0].
+  NSRange caret_range = NSMakeRange(0, 0);
+  NSRange query_range = NSMakeRange(1, 1);
+  NSRange actual_range;
+  NSRect rect = [ns_view_ firstRectForCharacterRange:query_range
+                                         actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBounds(client), gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(caret_range, actual_range);
+
+  // Set composition with caret before second character ('e').
+  const base::string16 kTestString = base::ASCIIToUTF16("test_str");

[tapted, 2015/12/30 06:48:53]

So there's a weird convention (or, the literal interpretation of the style
guide) for naming constants that only primitive types are named as kFooBar (e.g.
char[] or int is fine, but std::string should be `const std::string
test_string;`)

[karandeepb, 2015/12/31 02:42:44]

Can't find this in the cpp style guide -
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#....
Also, similar naming has been followed at other places in the file, should I
refactor it?

[tapted, 2016/01/04 02:28:10]

It says "Variables declared constexpr or const, and whose value is fixed for the
duration of the program". Things with constructors only take on a value when
they're constructed and that value is gone when it's destroyed, so I've had
senior reviewers ping me on this in the past.

So it's something to be aware of, but it's still a "guide" :). You can leave
this as kTestString if you prefer - consistency is important too, and it can
sometimes trump other style concerns. I'll leave it up to you. (Anecdotally,
there's a `kTestString.size()` elsewhere in the file -- this looks really
jarring to me, so I'd support changing them all, but it should be in a separate
CL since there's a lot).

[karandeepb, 2016/01/04 04:33:55]

Done. Will refactor the other usages in a separate CL.

+  const size_t kTextLength = 8;
+  SetCompositionText(client, kTestString, 1, &caret_range);
+
+  // Test empty ranges within the composition range.

[tapted, 2015/12/30 06:48:53]

nit: follow comment with the expectations, E.g. 

"Test empty range after the second character: should return caret bounds
(since..?)"

[karandeepb, 2015/12/31 02:42:43]

Done.

+  query_range = NSMakeRange(1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBoundsForPosition(client, kTestString, 1, &caret_range),
+            gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(query_range, actual_range);
+
+  query_range = NSMakeRange(kTextLength, 0);

[tapted, 2015/12/30 06:48:53]

likewise, "Test empty range at the end of the text: should return <..>"

[karandeepb, 2015/12/31 02:42:44]

Done.

+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_NE(GetCaretBoundsForPosition(client, kTestString, 1, &caret_range),
+            gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ(
+      GetCaretBoundsForPosition(client, kTestString, kTextLength, &caret_range),
+      gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(query_range, actual_range);
+
+  // Query outside composition range. Ensure bounds and range corresponding to
+  // the current caret position are returned.
+  query_range = NSMakeRange(kTextLength + 1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBounds(client), gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(caret_range, actual_range);
+
+  // Make sure not crashing by passing NULL pointer instead of actualRange.

[tapted, 2015/12/30 06:48:52]

nit: NULL->null

[karandeepb, 2015/12/31 02:42:43]

Done.

+  rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL];

[tapted, 2015/12/30 06:48:53]

nit: NULL->nullptr

(again it's convention: `null` in comments, but always `nullptr` in code)

[karandeepb, 2015/12/31 02:42:43]

Done.

[tapted, 2016/01/04 02:28:10]

(ooh, I should say -- and you may have already figured this one out --  'nil' is
also acceptable in an objective C context if it's a pointer to a [subclass of]
NSObject*. Here nullptr is more correct since NSRange is a C-style struct and
not a subtype of NSObject.)

[karandeepb, 2016/01/04 04:33:55]

Didn't know all of it. Thanks!

+}
+
+// Test firstRectForCharacterRange:actualRange for all valid ranges of a test
+// string.
+TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) {
+  InstallTextField("");
+  ui::TextInputClient* client = [ns_view_ textInputClient];
+  const base::string16 kTestString = base::ASCIIToUTF16("test_str");

[tapted, 2015/12/30 06:48:53]

Is there a minimum test string that can give code coverage without requiring the
loops below?

It's usually confusing to put EXPECT_* statements into loops, since you're not
sure what the input data is.

You can add SCOPED_TRACE(testing::Message() << "Range [begin, end) = [" << i <<
", " << j << ")"); to the inner loop, but it's clumsy. No loops would be best.

[karandeepb, 2015/12/31 02:42:43]

Removed the loops. Somehow, this approach did not strike me earlier!

+  const size_t kTextLength = 8;
+
+  std::vector<gfx::Rect> char_bounds(kTextLength);
+  std::vector<gfx::Rect> caret_bounds(kTextLength + 1);
+
+  // Generate caret_bounds between different characters.
+  for (size_t i = 0; i <= kTextLength; i++)
+    caret_bounds[i] = GetCaretBoundsForPosition(client, kTestString, i);
+
+  // Generate individual character bounds from caret positions.
+  for (size_t i = 0; i < kTextLength; i++) {
+    char_bounds[i].set_origin(caret_bounds[i].origin());
+    char_bounds[i].set_width(caret_bounds[i + 1].x() - caret_bounds[i].x());
+    char_bounds[i].set_height(caret_bounds[i].height());
+  }
+
+  // Verify bounds for all valid ranges.
+  NSRange query_range;
+  NSRange actual_range;
+  NSRect rect;
+  gfx::Rect expected_bounds;
+  for (size_t i = 0; i < kTextLength; i++) {
+    for (size_t j = i + 1; j <= kTextLength; j++) {
+      query_range = NSMakeRange(i, j - i);
+      rect = [ns_view_ firstRectForCharacterRange:query_range
+                                      actualRange:&actual_range];
+
+      expected_bounds = gfx::Rect();
+      for (size_t k = i; k < j; k++)
+        expected_bounds.Union(char_bounds[k]);
+
+      EXPECT_EQ(expected_bounds, gfx::ScreenRectFromNSRect(rect));
+      EXPECT_EQ_RANGE(query_range, actual_range);
+    }
+  }
+}
+
 typedef BridgedNativeWidgetTestBase BridgedNativeWidgetSimulateFullscreenTest;
 
 // Simulate the notifications that AppKit would send out if a fullscreen