Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

ui/views/cocoa/bridged_native_widget_unittest.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_native_widget.h"
 
 #import <Cocoa/Cocoa.h>
 
 #import "base/mac/foundation_util.h"
 #import "base/mac/mac_util.h"
 #import "base/mac/sdk_forward_declarations.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #import "testing/gtest_mac.h"
 #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"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/root_view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ skipping 91 matching lines @@
     // before the tests covering the Init() flow are ready to do that.
     init_params_.type = Widget::InitParams::TYPE_WINDOW_FRAMELESS;
 
     // To control the lifetime without an actual window that must be closed,
     // tests in this file need to use WIDGET_OWNS_NATIVE_WIDGET.
     init_params_.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
 
     // 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_);
   }
 
  protected:
   scoped_ptr<Widget> widget_;
   MockNativeWidgetMac* native_widget_mac_;  // Weak. Owned by |widget_|.
 
   // Make the InitParams available to tests to cover initialization codepaths.
   Widget::InitParams init_params_;
 };
@@ skipping 26 matching lines @@
 
 BridgedNativeWidgetTest::BridgedNativeWidgetTest() {
 }
 
 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
   // have them delivered to the textfield. Note that focusing a textfield
   // schedules a task to flash the cursor, so this requires |message_loop_|.
   textfield->RequestFocus();
 
   [ns_view_ setTextInputClient:textfield];
 }
 
@@ skipping 311 matching lines @@
   EXPECT_EQ("a", GetText());
   EXPECT_EQ_RANGE(NSMakeRange(1, 0), [ns_view_ selectedRange]);
 
   // Should succeed after moving left first.
   [ns_view_ doCommandBySelector:@selector(moveLeft:)];
   [ns_view_ doCommandBySelector:@selector(deleteForward:)];
   EXPECT_EQ("", GetText());
   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
 // operation begins, and then fails and must abort. This notification sequence
 // was determined by posting delayed tasks to toggle fullscreen state and then
 // mashing Ctrl+Left/Right to keep OSX in a transition between Spaces to cause
 // the fullscreen transition to fail.
 TEST_F(BridgedNativeWidgetSimulateFullscreenTest, FailToEnterAndExit) {
   if (base::mac::IsOSSnowLeopard())
     return;
@@ skipping 58 matching lines @@
   [center postNotificationName:NSWindowDidExitFullScreenNotification
                         object:window];
   EXPECT_EQ(1, [window ignoredToggleFullScreenCount]);  // No change.
   EXPECT_FALSE(bridge()->target_fullscreen_state());
 
   widget_->CloseNow();
 }
 
 }  // namespace test
 }  // namespace views