views::Textfield: Implement transpose editing command.

ui/views/controls/textfield/textfield_unittest.cc

Index: ui/views/controls/textfield/textfield_unittest.cc
diff --git a/ui/views/controls/textfield/textfield_unittest.cc b/ui/views/controls/textfield/textfield_unittest.cc
index 4de82b0a20be2f97ede82427d5dcb64d62d9a632..46b18aaed7d968c6c896ada4c59baea8dcbe5387 100644
--- a/ui/views/controls/textfield/textfield_unittest.cc
+++ b/ui/views/controls/textfield/textfield_unittest.cc
@@ -763,7 +763,7 @@ TEST_F(TextfieldTest, KeysWithModifiersTest) {
   SendKeyPress(ui::VKEY_T, shift);
   SendKeyPress(ui::VKEY_E, shift | altgr);
   SendKeyPress(ui::VKEY_X, 0);
-  SendKeyPress(ui::VKEY_T, ctrl);
+  SendKeyPress(ui::VKEY_T, ctrl);  // This causes transpose on Mac.
   SendKeyPress(ui::VKEY_1, alt);
   SendKeyPress(ui::VKEY_2, command);
   SendKeyPress(ui::VKEY_3, 0);
@@ -776,7 +776,7 @@ TEST_F(TextfieldTest, KeysWithModifiersTest) {
   if (TestingNativeCrOs())
     EXPECT_STR_EQ("TeTEx34", textfield_->text());
   else if (TestingNativeMac())
-    EXPECT_STR_EQ("TheTEx134", textfield_->text());
+    EXPECT_STR_EQ("TheTxE134", textfield_->text());
   else
     EXPECT_STR_EQ("TeTEx234", textfield_->text());
 }
@@ -1921,6 +1921,67 @@ TEST_F(TextfieldTest, CutCopyPaste) {
   EXPECT_EQ(ui::CLIPBOARD_TYPE_LAST, GetAndResetCopiedToClipboard());
 }
 
+// Non-Mac platforms don't have a key binding for Transpose.

[tapted, 2016/06/28 04:52:32]

Can we put this in textfield_model_unittest instead? (and run it on all
platforms?)

[karandeepb, 2016/06/29 05:44:38]

Done.

+#if defined(OS_MACOSX)
+TEST_F(TextfieldTest, Transpose) {
+  const base::string16 ltr = base::ASCIIToUTF16("12");
+  const base::string16 rtl = base::WideToUTF16(L"\x0634\x0632");
+  const base::string16 ltr_transposed = base::ASCIIToUTF16("21");
+  const base::string16 rtl_transposed = base::WideToUTF16(L"\x0632\x0634");
+
+  // This is a string with an 'a' between two emojis.
+  const base::string16 surrogate_pairs({0xD83D, 0xDE07, 'a', 0xD83D, 0xDE0E});

[tapted, 2016/06/28 04:52:32]

This is probably invoking std::string's initializer list constructor - I ..
think that's OK now. But it's a fairly recent thing. 

An alternative might be to use base::WideToUTF16 and a full string rather than
an intiializer list

[karandeepb, 2016/06/29 05:44:38]

Had tried that. It won't work since wstring is platform dependent. For example
on Mac, wchar_t is 32 bits, on Windows 16 bits. Hence the wstring representation
will be different on different platforms.

[tapted, 2016/06/29 07:06:14]

yah.. I think there's some macro that can help, but it looks good with
initializer lists - might as well use these language features now that we
finally can.

+  const base::string16 test_strings[] = {ltr, rtl, surrogate_pairs};
+
+  struct {
+    gfx::Range range;
+    base::string16 expected_text;
+    gfx::Range expected_selection;
+  } cases[][4] = {

[tapted, 2016/06/28 04:52:32]

I don't think it's correct to constrain this to 4 test cases per test string --
feels a bit shoe-horned to fit this array declaration. E.g. you test non-zero
selection ranges for rtl/non-rtl but not for emoji. But since we have
initializer lists we can probably do something like

struct TestCase {.. };

std::vector<TestCase> ltr_tests = {
  {gfx::Range(0), ltr, gfx::Range(0)},
  ..,
}

...
std::vector<std::vector<TestCase>> all_tests = {ltr_tests, rtl_tests,
emoji_tests};

etc.

[karandeepb, 2016/06/29 05:44:37]

Done.

+      {// Cases for LTR.
+       {gfx::Range(0), ltr, gfx::Range(0)},
+       {gfx::Range(1), ltr_transposed, gfx::Range(2)},
+       {gfx::Range(2), ltr_transposed, gfx::Range(2)},
+       {gfx::Range(0, 2), ltr, gfx::Range(0, 2)}},
+      {
+          // Cases for RTL.
+          {gfx::Range(0), rtl, gfx::Range(0)},

[tapted, 2016/06/28 04:52:32]

is this what git-cl-format did? it might be a clang-format bug. Giving the test
cases a temporary is probably nicer anyway -- "intrinsic documentation" rather
than having to add the "Cases for foo" comments

[karandeepb, 2016/06/29 05:44:37]

Done. Yeah git cl format did this.

+          {gfx::Range(1), rtl_transposed, gfx::Range(2)},
+          {gfx::Range(2), rtl_transposed, gfx::Range(2)},
+          {gfx::Range(0, 1), rtl, gfx::Range(0, 1)},
+      },
+      {// Cases for surrogate pairs. We only test at valid grapheme boundaries.
+       {gfx::Range(0), surrogate_pairs, gfx::Range(0)},
+       {gfx::Range(2), base::string16({'a', 0xD83D, 0xDE07, 0xD83D, 0xDE0E}),
+        gfx::Range(3)},
+       {gfx::Range(3), base::string16({0xD83D, 0xDE07, 0xD83D, 0xDE0E, 'a'}),
+        gfx::Range(5)},
+       {gfx::Range(5), base::string16({0xD83D, 0xDE07, 0xD83D, 0xDE0E, 'a'}),
+        gfx::Range(5)}}};
+
+  InitTextfield();
+
+  EXPECT_EQ(arraysize(cases), arraysize(test_strings));
+
+  for (size_t i = 0; i < arraysize(cases); i++) {
+    for (size_t j = 0; j < arraysize(cases[i]); j++) {
+      SCOPED_TRACE(base::StringPrintf(
+          "Testing case [%" PRIuS ", %" PRIuS "] with string %s", i, j,
+          base::UTF16ToUTF8(test_strings[i]).c_str()));

[tapted, 2016/06/28 04:52:32]

can you use

SCOPED_TRACE(testing::Message() << "Testing"... << i << ", " << j << " ..)

printf feels a bit clumsy here (Also it might be tempting fate to output emoji
in the log output..)

[karandeepb, 2016/06/29 05:44:37]

Done.

+      auto test_case = cases[i][j];

[tapted, 2016/06/28 04:52:32]

nit: const auto&, (or const TextCase&)

[karandeepb, 2016/06/29 05:44:38]

Done.

+      textfield_->SetText(test_strings[i]);
+      textfield_->SelectRange(test_case.range);
+
+      // Press Ctrl+T.
+      SendKeyPress(ui::VKEY_T, ui::EF_CONTROL_DOWN);
+
+      EXPECT_EQ(test_case.expected_text, textfield_->text());
+      EXPECT_EQ(test_case.expected_selection, textfield_->GetSelectedRange());
+    }
+  }
+}
+#endif
+
 TEST_F(TextfieldTest, OvertypeMode) {
   InitTextfield();
   // Overtype mode should be disabled (no-op [Insert]).