Make OmniboxViewTest.BasicTextOperations Work on Linux

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

Index: chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
index 10f517e44e87805e4d44c9b331c2fd5471d1de3f..1f12b476d4deeb59239f5758b163d776e24e6f94 100644
--- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
+++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
@@ -742,8 +742,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, EscapeToDefaultMatch) {
 }
 
 // Flaky on Windows: http://crbug.com/146619
-// Fails on Linux: http://crbug.com/408637
-#if defined(OS_WIN) || defined(OS_LINUX)
+#if defined(OS_WIN)
 #define MAYBE_BasicTextOperations DISABLED_BasicTextOperations
 #else
 #define MAYBE_BasicTextOperations BasicTextOperations
@@ -762,6 +761,11 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_BasicTextOperations) {
 
   size_t start, end;
   omnibox_view->GetSelectionBounds(&start, &end);
+#if defined(OS_WIN) || defined(OS_LINUX)

[msw, 2014/09/10 23:17:27]

I'd suggest using "#if defined(TOOLKIT_VIEWS)", but I think that'll break Mac,
which now defines that symbol but goes on to instantiate an OmniboxViewMac...
So, I guess this is fine (or moot; see below).

[Mark P, 2014/09/15 22:54:53]

Acknowledged.

+  // In Views textfields (i.e., Windows and Linux), select-all highlights end

[msw, 2014/09/10 23:17:27]

nit: consider "Views textfields select-all in reverse to show the leading text."

[Mark P, 2014/09/15 22:54:53]

Sounds good to me.

+  // to start in order to show the leading text.
+  std::swap(start, end);

[msw, 2014/09/10 23:17:27]

Hmm, does it make sense to replace these EXPECT_EQ calls of specific range
bounds with something more robust, like checking the range length (if we're not
particular about the reversal of the bounds)?

[Mark P, 2014/09/15 22:54:53]

I think it was a conscious user-interface decision to have the bounds reversed
in certain cases.  I think we should leave this test as it currently is, testing
this property, not make it more forgiving of something that may be important.

+#endif
   EXPECT_EQ(0U, start);
   EXPECT_EQ(old_text.size(), end);
 
@@ -776,6 +780,11 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_BasicTextOperations) {
 
   // Make sure the cursor is placed correctly.
   omnibox_view->GetSelectionBounds(&start, &end);
+#if defined(OS_WIN) || defined(OS_LINUX)
+  // In Views textfields (i.e., Windows and Linux), select-all highlights end
+  // to start in order to show the leading text.
+  std::swap(start, end);

[msw, 2014/09/10 23:17:27]

Why is this swap needed? The bounds are expected to be the same.

[Mark P, 2014/09/15 22:54:53]

It's not needed.  I added it because I liked the consistency of having every
GetSelectionBounds call annotated with this reminder.

[msw, 2014/09/16 18:59:44]

But there's no reason to expect consistency in the reversal of selection ranges
that aren't for select-all, when that quirk is only needed for cases that are
select-all. In fact, it's [trivially] incorrect. I think you should remove this.

[Mark P, 2014/09/16 21:31:00]

Done.

+#endif
   EXPECT_EQ(old_text.size(), start);
   EXPECT_EQ(old_text.size(), end);
 
@@ -792,6 +801,11 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_BasicTextOperations) {
   omnibox_view->SelectAll(true);
   EXPECT_TRUE(omnibox_view->IsSelectAll());
   omnibox_view->GetSelectionBounds(&start, &end);
+#if defined(OS_WIN) || defined(OS_LINUX)
+  // In Views textfields (i.e., Windows and Linux), select-all highlights end
+  // to start in order to show the leading text.
+  std::swap(start, end);
+#endif
   EXPECT_EQ(0U, start);
   EXPECT_EQ(old_text.size(), end);