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..5791deeb7b197c4f7f98e44660ee29b22091509e 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"
 #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"
@@ -128,6 +129,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 +177,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 +509,109 @@ TEST_F(BridgedNativeWidgetTest, TextInput_DeleteForward) {
   EXPECT_EQ_RANGE(NSMakeRange(0, 0), [ns_view_ selectedRange]);
 }
 
+TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) {

[tapted, 2015/12/22 02:33:58]

nit: brief comment summarising what's being tested

[karandeepb, 2015/12/29 07:21:16]

Done.

+  InstallTextField("");
+  ui::TextInputClient* client = [ns_view_ textInputClient];
+  NSRange range;

[tapted, 2015/12/22 02:33:58]

move declarations closer to first use. Typically in Chrome, if a variable can be
assigned when declared, then that's how it's done (in a test it's fine to
re-assign it later, even if that code is unrelated)

[karandeepb, 2015/12/29 07:21:15]

Done.

+  NSRange actual_range = NSMakeRange(-1, -1);

[tapted, 2015/12/22 02:33:58]

NSMakeRange takes unsigned integers. An invalid range is usually
NSMakeRange(NSNotFound, 0)

[karandeepb, 2015/12/29 07:21:16]

Done.

+  NSRect rect;
+  gfx::Rect bounds;
+
+  // Empty composition.
+  rect = [ns_view_ firstRectForCharacterRange:NSMakeRange(0, 0)
+                                  actualRange:&actual_range];
+  bounds = client->GetCaretBounds();

[tapted, 2015/12/22 02:33:58]

Can this be a constant in the test? (if it depends on font sizes and things,
then this is fine, it just needs a comment). There's a bunch of interesting
guidelines - this is something like 'don't put logic in tests'
go/tott/2014/07/episode-338-dont-put-logic-in-tests.html

i.e. something like

// Start with the caret bounds.
gfx::Rect bounds(0, 0, 2, 10);
EXPECT_EQ(bounds, client->GetCaretBounds());

[karandeepb, 2015/12/29 07:21:16]

Not quite sure how to make this a constant, since we are checking that the
actual returned bounds from firstRectForCharacterRange correspond to the
expected caret bounds, when the caret is placed at the appropriate position. I
have separated this logic into a separate function GetCaretBoundsForPosition(..)
to make it clearer.

+  bounds.set_width(0);
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);

[tapted, 2015/12/22 02:33:58]

EXPECT_EQ should work for gfx::Bounds, and give better error messages. More
below.

You can also use EXPECT_NSEQ for NSRects

[karandeepb, 2015/12/29 07:21:16]

Done.

+  EXPECT_TRUE(NSEqualRanges(NSMakeRange(0, 0), actual_range));

[tapted, 2015/12/22 02:33:58]

EXPECT_EQ_RANGE. More below

[karandeepb, 2015/12/29 07:21:15]

Done.

+
+  rect = [ns_view_ firstRectForCharacterRange:NSMakeRange(1, 1)
+                                  actualRange:&actual_range];
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(NSMakeRange(1, 0), actual_range));
+

[tapted, 2015/12/22 02:33:58]

It might be good to split up the test here into separate cases. E.g. a separate
method for the empty cases like

 TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange_Empty)

 TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange)

[karandeepb, 2015/12/29 07:21:15]

Done.

+  // Set composition with caret before second character('e').

[tapted, 2015/12/22 02:33:58]

nit: space before open paren

[karandeepb, 2015/12/29 07:21:16]

Done.

+  ui::CompositionText composition;
+  composition.selection = gfx::Range(1);
+  composition.text = base::UTF8ToUTF16("test_str");

[tapted, 2015/12/22 02:33:58]

nit: ASCIIToUTF16 is what usually gets used for string literals

[karandeepb, 2015/12/29 07:21:16]

Done.

+  size_t count = composition.text.length();

[tapted, 2015/12/22 02:33:58]

`count` is pretty vague here. It also breaks the logic-in-tests thing. Maybe

const size_t kTextLength = 8;

[karandeepb, 2015/12/29 07:21:15]

Done.

+  client->SetCompositionText(composition);
+  bounds = client->GetCaretBounds();
+  bounds.set_width(0);
+
+  range = NSMakeRange(1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(range, actual_range));
+
+  range = NSMakeRange(2, 0);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_FALSE(gfx::ScreenRectFromNSRect(rect) == bounds);

[tapted, 2015/12/22 02:33:58]

comment about this? (what's it testing, and why is it false? can you test
against the rect that it *should* be equal to?)

[karandeepb, 2015/12/29 07:21:16]

Done.

+  EXPECT_TRUE(NSEqualRanges(range, actual_range));
+
+  // Query outside composition range.
+  range = NSMakeRange(count + 1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(range, actual_range));
+
+  // Set composition with caret after last character.
+  composition.selection = gfx::Range(count);
+  client->SetCompositionText(composition);
+  bounds = client->GetCaretBounds();
+  bounds.set_width(0);
+
+  range = NSMakeRange(count, 0);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(range, actual_range));
+
+  range = NSMakeRange(count - 1, 1);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_FALSE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(range, actual_range));
+
+  // Query outside composition range.

[tapted, 2015/12/22 02:33:58]

These all need a bit more commentary. I.e. say why this "Query outside
composition range" is different to the one before.

[karandeepb, 2015/12/29 07:21:15]

Done.

+  range = NSMakeRange(count - 1, 2);
+  rect = [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+  EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+  EXPECT_TRUE(NSEqualRanges(NSMakeRange(count - 1, 0), actual_range));
+
+  std::vector<gfx::Rect> char_bounds(count);

[tapted, 2015/12/22 02:33:58]

It's unusual to see exhaustive testing like this: if a test fails, it's much
harder to see what needs fixing. Is this exercising any new code paths?

Can you use a test string of just two characters and "unroll" all the loops
manually to get coverage of all the cases?

[karandeepb, 2015/12/29 07:21:15]

That would turn out to be a bit messy, since we will still need to compute
individual character bounds from caret bounds, the logic for which seems more
concise when written using loops. Similar exhaustive testing was done in
render_widget_host_view_mac_unittest.mm-
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

+  std::vector<gfx::Rect> caret_bounds(count + 1);
+
+  // Generate caret_bounds between different characters.
+  for (size_t i = 0; i <= count; i++) {
+    composition.selection = gfx::Range(i);
+    client->SetCompositionText(composition);
+    caret_bounds[i] = client->GetCaretBounds();
+  }
+
+  // Generate individual character bounds from caret positions.
+  for (size_t i = 0; i < count; 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(
+        std::max(caret_bounds[i].height(), caret_bounds[i + 1].height()));
+  }
+
+  // Verify bounds for all valid ranges.
+  for (size_t i = 0; i < count; i++) {
+    for (size_t j = i + 1; j <= count; j++) {
+      range = NSMakeRange(i, j - i);
+      rect =
+          [ns_view_ firstRectForCharacterRange:range actualRange:&actual_range];
+
+      bounds = gfx::Rect();
+      for (size_t k = i; k < j; k++)
+        bounds.Union(char_bounds[k]);
+
+      EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds);
+      EXPECT_TRUE(NSEqualRanges(range, actual_range));
+    }
+  }
+}
+
 typedef BridgedNativeWidgetTestBase BridgedNativeWidgetSimulateFullscreenTest;
 
 // Simulate the notifications that AppKit would send out if a fullscreen