Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Pro…

chrome/browser/autocomplete/autocomplete_browsertest.cc

Index: chrome/browser/autocomplete/autocomplete_browsertest.cc
diff --git a/chrome/browser/autocomplete/autocomplete_browsertest.cc b/chrome/browser/autocomplete/autocomplete_browsertest.cc
index 00cecb7306f1477aa78bd79a77595cdb7be15cc8..81571b48eae968e8bf8de2f3cae06058ae1365a3 100644
--- a/chrome/browser/autocomplete/autocomplete_browsertest.cc
+++ b/chrome/browser/autocomplete/autocomplete_browsertest.cc
@@ -35,6 +35,7 @@
 #include "components/omnibox/browser/autocomplete_provider.h"
 #include "components/omnibox/browser/omnibox_popup_model.h"
 #include "components/omnibox/browser/omnibox_view.h"
+#include "components/search_engines/template_url_service.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -197,6 +198,8 @@ IN_PROC_BROWSER_TEST_F(AutocompleteBrowserTest, TabAwayRevertSelect) {
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 }
 
+
+

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Don't add these

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

 IN_PROC_BROWSER_TEST_F(AutocompleteBrowserTest, FocusSearch) {
 #if defined(OS_WIN) && defined(USE_ASH)
   // Disable this test in Metro+Ash for now (http://crbug.com/262796).
@@ -208,85 +211,180 @@ IN_PROC_BROWSER_TEST_F(AutocompleteBrowserTest, FocusSearch) {
   WaitForTemplateURLServiceToLoad();
   LocationBar* location_bar = GetLocationBar();
   OmniboxView* omnibox_view = location_bar->GetOmniboxView();
+  OmniboxEditModel* omnibox_model = omnibox_view->model();
+
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(browser()->profile());
+  base::string16 dsp =

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Since this is really a keyword and not a provider itself, and since we
generally avoid abbreviations, I would name this |default_search_keyword|.

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+      template_url_service->GetDefaultSearchProvider()->keyword();
+
+  const char query_text[] = "foo";

[Peter Kasting, 2016/04/08 00:39:56]

Every place below that wants this wants it as a string16.  So make it a const
base::string16.  Then you can eliminate the |query_text_len| temp and just call
.length() on this.

[Tom (Use chromium acct), 2016/04/12 20:03:04]

Done.

+  const size_t query_text_len = sizeof(query_text) - 1;
+
+  size_t selection_start, selection_end;
+
+#define FOCUS_SEARCH_CHECK_PRECONDITIONS()                              \

[Peter Kasting, 2016/04/08 00:39:55]

We try to avoid macros if possible.

Why not put this stuff in a helper function?

[Tom (Use chromium acct), 2016/04/12 20:03:04]

Done.

+  do {                                                                  \
+    EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());         \
+    EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL),                   \
+              omnibox_view->GetText());                                 \
+    EXPECT_EQ(base::string16(), omnibox_model->keyword());              \
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());                     \
+    EXPECT_FALSE(omnibox_model->is_keyword_selected());                 \
+  } while (false)
 
   // Focus search when omnibox is blank
   {
-    EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL), omnibox_view->GetText());
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
 
     location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?"), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
 
-    size_t selection_start, selection_end;
     omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
-    EXPECT_EQ(1U, selection_start);
-    EXPECT_EQ(1U, selection_end);
+    EXPECT_EQ(0U, selection_start);
+    EXPECT_EQ(0U, selection_end);
+
+    omnibox_view->RevertAll();
   }
 
   // Focus search when omnibox is _not_ alread in forced query mode.
   {
-    omnibox_view->SetUserText(base::ASCIIToUTF16("foo"));
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
+
+    omnibox_view->SetUserText(base::ASCIIToUTF16(query_text));
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("foo"), omnibox_view->GetText());
+    EXPECT_EQ(base::ASCIIToUTF16(query_text), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_FALSE(omnibox_model->is_keyword_selected());
 
     location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?"), omnibox_view->GetText());
+    EXPECT_EQ(base::ASCIIToUTF16(query_text), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
 
-    size_t selection_start, selection_end;
     omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
-    EXPECT_EQ(1U, selection_start);
-    EXPECT_EQ(1U, selection_end);
+    EXPECT_EQ(query_text_len, selection_start);
+    EXPECT_EQ(query_text_len, selection_end);
+
+    omnibox_view->RevertAll();
   }
 
   // Focus search when omnibox _is_ already in forced query mode, but no query
   // has been typed.
   {
-    omnibox_view->SetUserText(base::ASCIIToUTF16("?"));
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
+
+    location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?"), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
+
+    omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
+    EXPECT_EQ(0U, selection_start);
+    EXPECT_EQ(0U, selection_end);
 
     location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?"), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
 
-    size_t selection_start, selection_end;
     omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
-    EXPECT_EQ(1U, selection_start);
-    EXPECT_EQ(1U, selection_end);
+    EXPECT_EQ(0U, selection_start);
+    EXPECT_EQ(0U, selection_end);
+
+    omnibox_view->RevertAll();
   }
 
   // Focus search when omnibox _is_ already in forced query mode, and some query
   // has been typed.
   {
-    omnibox_view->SetUserText(base::ASCIIToUTF16("?foo"));
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
+
+    omnibox_view->SetUserText(base::ASCIIToUTF16(query_text));
+    location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?foo"), omnibox_view->GetText());
+    EXPECT_EQ(base::ASCIIToUTF16(query_text), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
+
+    omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
+    EXPECT_EQ(query_text_len, selection_start);
+    EXPECT_EQ(query_text_len, selection_end);
 
     location_bar->FocusSearch();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("?foo"), omnibox_view->GetText());
+    EXPECT_EQ(base::ASCIIToUTF16(query_text), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
 
-    size_t selection_start, selection_end;
     omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
-    EXPECT_EQ(1U, std::min(selection_start, selection_end));
-    EXPECT_EQ(4U, std::max(selection_start, selection_end));
+    EXPECT_EQ(0U,
+              selection_start < selection_end ?
+              selection_start : selection_end);

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Use std::min()

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+    EXPECT_EQ(query_text_len,
+              selection_start > selection_end ?
+              selection_start : selection_end);

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Use std::max()

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+
+    omnibox_view->RevertAll();
   }
 
-  // Focus search when omnibox is in forced query mode with leading whitespace.
+  // If the user got into keyword mode using a keyboard shortcut, and presses

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Tense agreement: got -> gets
(here and below)

[Tom (Use chromium acct), 2016/04/12 20:03:04]

Done.

+  // backspace, they should be left with their original query without their dsp
+  // keyword.
   {
-    omnibox_view->SetUserText(base::ASCIIToUTF16("   ?foo"));
-    EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("   ?foo"), omnibox_view->GetText());
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
 
+    omnibox_view->SetUserText(base::ASCIIToUTF16(query_text));
+    // the user presses Ctrl-K

[Peter Kasting, 2016/04/08 00:39:55]

Nit: Capitalization, trailing period (multiple places)

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

     location_bar->FocusSearch();
+    // the user presses backspace
+    omnibox_model->ClearKeyword();

[Peter Kasting, 2016/04/08 00:39:55]

Can we just send a backspace keypress?  That ensures the keystroke is hooked to
the right method.  (2 places)

[Tom (Use chromium acct), 2016/04/12 20:03:04]

Is there a testing API for pressing backspace or moving around using the arrow
keys?  At the moment, it seems I would have to write platform specific code for
mac vs the others.

[Peter Kasting, 2016/04/12 20:04:13]

I thought there was an API to send any key by key code?

+
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
-    EXPECT_EQ(base::ASCIIToUTF16("   ?foo"), omnibox_view->GetText());
+    EXPECT_EQ(base::ASCIIToUTF16(query_text), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_FALSE(omnibox_model->is_keyword_selected());
 
-    size_t selection_start, selection_end;
-    omnibox_view->GetSelectionBounds(&selection_start, &selection_end);
-    EXPECT_EQ(4U, std::min(selection_start, selection_end));
-    EXPECT_EQ(7U, std::max(selection_start, selection_end));
+    omnibox_view->RevertAll();
+  }
+
+  // If the user got into keyword mode by typing '?', they should be put into
+  // keyword mode with their dsp.  If they press backspace, they should be left

[Peter Kasting, 2016/04/08 00:39:55]

Nit: with their dsp -> for their default search provider

[Tom (Use chromium acct), 2016/04/12 20:03:04]

Done.

+  // with '?' in the omnibox.
+  {
+    FOCUS_SEARCH_CHECK_PRECONDITIONS();
+
+    omnibox_view->SetUserText(base::ASCIIToUTF16("?"));
+
+    EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
+    EXPECT_EQ(base::string16(), omnibox_view->GetText());
+    EXPECT_EQ(dsp, omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_TRUE(omnibox_model->is_keyword_selected());
+
+    // the user presses backspace
+    omnibox_model->ClearKeyword();
+
+    EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
+    EXPECT_EQ(base::ASCIIToUTF16("?"), omnibox_view->GetText());
+    EXPECT_EQ(base::string16(), omnibox_model->keyword());
+    EXPECT_FALSE(omnibox_model->is_keyword_hint());
+    EXPECT_FALSE(omnibox_model->is_keyword_selected());
+
+    omnibox_view->RevertAll();
   }
 }