Local omnibox treats navsuggest suggestions as queries.

chrome/browser/ui/search/instant_extended_interactive_uitest.cc

 // Copyright 2013 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 <sstream>
 
 #include "base/prefs/pref_service.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_controller.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_provider.h"
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_service.h"
 #include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/omnibox/omnibox_view.h"
 #include "chrome/browser/ui/search/instant_commit_type.h"
 #include "chrome/browser/ui/search/instant_ntp.h"
 #include "chrome/browser/ui/search/instant_overlay.h"
 #include "chrome/browser/ui/search/instant_tab.h"
 #include "chrome/browser/ui/search/instant_test_utils.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/theme_source.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/thumbnail_score.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
@@ skipping 765 matching lines @@
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_TRUE(instant_service->IsInstantProcess(
       active_tab->GetRenderProcessHost()->GetID()));
 
   // Navigating elsewhere should not use the Instant render process.
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUIAboutURL));
   EXPECT_FALSE(instant_service->IsInstantProcess(
       active_tab->GetRenderProcessHost()->GetID()));
 }
 
+// Test that a search query will not be displayed for navsuggest queries.
+IN_PROC_BROWSER_TEST_F(
+    InstantExtendedTest, SearchQueryNotDisplayedForNavsuggest) {

[sreeram, 2013/04/29 22:26:31]

Nit: Move "InstantExtendedTest" to the previous line.

[dougw, 2013/04/29 22:48:49]

Done.

+  // Use only the local overlay.
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableLocalOnlyInstantExtendedAPI);
+  ASSERT_TRUE(chrome::IsLocalOnlyInstantExtendedAPIEnabled());
+
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+
+  // The second argument indicates to use only the local overlay and NTP.
+  instant()->SetInstantEnabled(true, true);
+
+  // Focus omnibox and confirm overlay isn't shown.
+  FocusOmniboxAndWaitForInstantSupport();

[sreeram, 2013/04/29 22:26:31]

FocusOmniboxAndWaitForInstant***Extended***Support()?

[dougw, 2013/04/29 22:48:49]

This is intentional. The test hangs if we wait for:
chrome::NOTIFICATION_INSTANT_NTP_SUPPORT_DETERMINED.

The function names are misleading. I wrote a cl to fix this: crrev.com/14050016.


On 2013/04/29 22:26:31, sreeram wrote:

+
+  // Typing in the omnibox should show the overlay.
+  SetOmniboxText("face");
+
+  // Add a navsuggest suggestion.
+  instant()->SetSuggestions(
+      instant()->overlay()->contents(),
+      std::vector<InstantSuggestion>(
+          1,
+          InstantSuggestion(ASCIIToUTF16("http://facemash.com/"),
+                            INSTANT_COMPLETE_NOW,
+                            INSTANT_SUGGESTION_URL,
+                            ASCIIToUTF16("face"))));
+
+  while (!omnibox()->model()->autocomplete_controller()->done()) {
+    content::WindowedNotificationObserver autocomplete_observer(
+        chrome::NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY,
+        content::NotificationService::AllSources());
+    autocomplete_observer.Wait();
+  }
+
+  EXPECT_TRUE(
+      ExecuteScript(
+          "var sortedNativeSuggestions = chrome.embeddedSearch.searchBox.native"
+          "Suggestions.sort(function (a,b) {return b.rankingData.relevance - a."
+          "rankingData.relevance});"));

[sreeram, 2013/04/29 22:26:31]

Yikes. This is really hard to read. Please don't split variables across multiple
lines. How about something like this:
  EXPECT_TRUE(ExecuteScript(
      "var sorted = chrome.embeddedSearch.searchBox.nativeSuggestions.sort("
          "function (a, b) {"
            "return b.rankingData.relevance - a.rankingData.relevance;"
          "});"));

[dougw, 2013/04/29 22:48:49]

Done.

+
+  // Create an event listener that opens the top suggestion in a new tab.

[sreeram, 2013/04/29 22:26:31]

This comment has nothing to do with the next line. Just delete it.

[dougw, 2013/04/29 22:48:49]

Done.

+  content::WebContents* overlay = instant()->GetOverlayContents();
+
+  int suggestions_count;

[sreeram, 2013/04/29 22:26:31]

Initialize this to an invalid value, such as -1.

[dougw, 2013/04/29 22:48:49]

Done.

+  EXPECT_TRUE(GetIntFromJS(
+      overlay, "sortedNativeSuggestions.length", &suggestions_count));
+  EXPECT_TRUE(suggestions_count > 0);

[sreeram, 2013/04/29 22:26:31]

EXPECT_GT(suggestions_count, 0);

[dougw, 2013/04/29 22:48:49]

Done.

+
+  std::string type;
+  EXPECT_TRUE(
+      GetStringFromJS(overlay, "sortedNativeSuggestions[0].type", &type));
+  ASSERT_TRUE(type == "navsuggest");

[sreeram, 2013/04/29 22:26:31]

EXPECT_EQ("navsuggest", type);

[dougw, 2013/04/29 22:48:49]

Done.

+
+  bool is_search_property_exists;

[sreeram, 2013/04/29 22:26:31]

Nit: Fix bad grammar: "is_search_property_defined" or some such.

What's the advantage of testing the definedness of this property? Why not just
ask whether it's true or false? Does anyone care about the difference between it
being undefined versus it being defined and false?

[dougw, 2013/04/29 22:48:49]

Done.

GetBoolFromJS will hang if it is called when is_search isn't defined.

On 2013/04/29 22:26:31, sreeram wrote:

+  EXPECT_TRUE(
+      GetBoolFromJS(
+          overlay, "sortedNativeSuggestions[0].hasOwnProperty('is_search')",
+          &is_search_property_exists));
+
+  if (is_search_property_exists) {
+    bool is_search;
+    EXPECT_TRUE(GetBoolFromJS(
+        overlay, "sortedNativeSuggestions[0].is_search", &is_search));
+    EXPECT_FALSE(is_search);
+  }
+}
+
 // Verification of fix for BUG=176365.  Ensure that each Instant WebContents in
 // a tab uses a new BrowsingInstance, to avoid conflicts in the
 // NavigationController.
 // Flaky: http://crbug.com/177516
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest, DISABLED_UnrelatedSiteInstance) {
   // Setup Instant.
   ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
   FocusOmniboxAndWaitForInstantExtendedSupport();
 
   // Check that the uncommited ntp page and uncommited overlay have unrelated
@@ skipping 903 matching lines @@
   SendEscape();
   EXPECT_TRUE(GetStringFromJS(contents, "chrome.embeddedSearch.searchBox.value",
                               &query));
   EXPECT_EQ("", query);
   EXPECT_EQ("", GetOmniboxText());
 
   EXPECT_TRUE(UpdateSearchState(contents));
   EXPECT_LT(0, on_change_calls_);
   EXPECT_EQ(0, submit_count_);
 }