Make OmniboxViewTest.BasicTextOperations Work on Linux

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

 // Copyright (c) 2012 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.
 
 #include <stdio.h>
 
 #include "base/command_line.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 724 matching lines @@
 
   EXPECT_NE(old_text, omnibox_view->GetText());
 
   // Escape shall revert back to the default match item.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_ESCAPE, 0));
   EXPECT_EQ(old_text, omnibox_view->GetText());
   EXPECT_EQ(old_selected_line, popup_model->selected_line());
 }
 
 // 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
 #endif
 
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_BasicTextOperations) {
   ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
   chrome::FocusLocationBar(browser());
 
   OmniboxView* omnibox_view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
 
   base::string16 old_text = omnibox_view->GetText();
   EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL), old_text);
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 
   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);
 
   // Move the cursor to the end.
 #if defined(OS_MACOSX)
   // End doesn't work on Mac trybot.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_E, ui::EF_CONTROL_DOWN));
 #else
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_END, 0));
 #endif
   EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   // 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);
 
   // Insert one character at the end. Make sure we won't insert
   // anything after the special ZWS mark used in gtk implementation.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_A, 0));
   EXPECT_EQ(old_text + base::char16('a'), omnibox_view->GetText());
 
   // Delete one character from the end. Make sure we won't delete the special
   // ZWS mark used in gtk implementation.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
   EXPECT_EQ(old_text, omnibox_view->GetText());
 
   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);
 
   // Delete the content
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_DELETE, 0));
   EXPECT_TRUE(omnibox_view->IsSelectAll());
   omnibox_view->GetSelectionBounds(&start, &end);
   EXPECT_EQ(0U, start);
   EXPECT_EQ(0U, end);
   EXPECT_TRUE(omnibox_view->GetText().empty());
@@ skipping 994 matching lines @@
   omnibox_view->Update();
   EXPECT_EQ(url_c, omnibox_view->GetText());
 }
 
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest, EscDisablesSearchTermReplacement) {
   browser()->toolbar_model()->set_url_replacement_enabled(true);
   chrome::FocusLocationBar(browser());
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_ESCAPE, 0));
   EXPECT_FALSE(browser()->toolbar_model()->url_replacement_enabled());
 }