search::IsInstantNTP: Prefer committed over visible URL

The visible URL doesn't always correspond to the actual page that's
loaded. For example, if the user types in a URL that ends up being a
download, then the visible URL corresponds to the download, but the NTP
is still there and should remain functional.

Bug: 592273, 624410
Change-Id: Ib2e0a7df8658e55030ee5e67959a0ebc486e495d
Reviewed-on: https://chromium-review.googlesource.com/800931Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521359}

chrome/browser/search/search.cc

@@ -276,8 +276,14 @@ bool IsInstantNTP(const content::WebContents* contents) {
   if (!contents)
     return false;
 
-  return NavEntryIsInstantNTP(contents,
-                              contents->GetController().GetVisibleEntry());
+  if (contents->ShowingInterstitialPage())
+    return false;
+
+  const content::NavigationEntry* entry =
+      contents->GetController().GetLastCommittedEntry();
+  if (!entry)
+    entry = contents->GetController().GetVisibleEntry();
+  return NavEntryIsInstantNTP(contents, entry);
 }
 
 bool NavEntryIsInstantNTP(const content::WebContents* contents,

chrome/browser/search/search.h

@@ -36,8 +36,9 @@ bool DefaultSearchProviderIsGoogle(
 // to an NTP, but aren't an NTP themselves (such as the NTP's service worker).
 bool IsNTPURL(const GURL& url, Profile* profile);
 
-// Returns true if the visible entry of |contents| is a New Tab page rendered
-// in an Instant process.
+// Returns true if the active navigation entry of |contents| is a New Tab page
+// rendered in an Instant process. This is the last committed entry if it
+// exists, and otherwise the visible entry.
 bool IsInstantNTP(const content::WebContents* contents);
 
 // Same as IsInstantNTP but uses |nav_entry| to determine the URL for the page

chrome/browser/search/search_unittest.cc

@@ -275,8 +275,7 @@ const SearchTestCase kInstantNTPTestCases[] = {
 
 TEST_F(SearchTest, InstantNTPExtendedEnabled) {
   AddTab(browser(), GURL("chrome://blank"));
-  for (size_t i = 0; i < arraysize(kInstantNTPTestCases); ++i) {
-    const SearchTestCase& test = kInstantNTPTestCases[i];
+  for (const SearchTestCase& test : kInstantNTPTestCases) {
     NavigateAndCommitActiveTab(GURL(test.url));
     const content::WebContents* contents =
         browser()->tab_strip_model()->GetWebContentsAt(0);
@@ -287,8 +286,7 @@ TEST_F(SearchTest, InstantNTPExtendedEnabled) {
 
 TEST_F(SearchTest, InstantNTPCustomNavigationEntry) {
   AddTab(browser(), GURL("chrome://blank"));
-  for (size_t i = 0; i < arraysize(kInstantNTPTestCases); ++i) {
-    const SearchTestCase& test = kInstantNTPTestCases[i];
+  for (const SearchTestCase& test : kInstantNTPTestCases) {
     NavigateAndCommitActiveTab(GURL(test.url));
     content::WebContents* contents =
         browser()->tab_strip_model()->GetWebContentsAt(0);
@@ -300,12 +298,14 @@ TEST_F(SearchTest, InstantNTPCustomNavigationEntry) {
                                          false,
                                          std::string(),
                                          contents->GetBrowserContext()));
-    // The active entry is chrome://blank and not an NTP.
-    EXPECT_FALSE(IsInstantNTP(contents));
+    // The visible entry is now chrome://blank, but this is still an NTP.
+    EXPECT_FALSE(NavEntryIsInstantNTP(contents, controller.GetVisibleEntry()));
     EXPECT_EQ(test.expected_result,
               NavEntryIsInstantNTP(contents,
                                    controller.GetLastCommittedEntry()))
         << test.url << " " << test.comment;
+    EXPECT_EQ(test.expected_result, IsInstantNTP(contents))
+        << test.url << " " << test.comment;
   }
 }
 

chrome/browser/ui/search/search_ipc_router_policy_unittest.cc

@@ -4,14 +4,12 @@
 
 #include "chrome/browser/ui/search/search_ipc_router.h"
 
-#include "build/build_config.h"
-#include "base/command_line.h"
 #include "chrome/browser/ui/search/search_ipc_router_policy_impl.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "content/public/browser/page_navigator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -30,7 +28,7 @@ class SearchIPCRouterPolicyTest : public BrowserWithTestWindowTest {
   SearchIPCRouter::Policy* GetSearchIPCRouterPolicy() {
     SearchTabHelper* search_tab_helper =
         SearchTabHelper::FromWebContents(web_contents());
-    EXPECT_NE(static_cast<SearchTabHelper*>(NULL), search_tab_helper);
+    EXPECT_NE(nullptr, search_tab_helper);
     return search_tab_helper->ipc_router_for_testing().policy_for_testing();
   }
 
@@ -46,6 +44,28 @@ TEST_F(SearchIPCRouterPolicyTest, ProcessFocusOmnibox) {
   EXPECT_TRUE(GetSearchIPCRouterPolicy()->ShouldProcessFocusOmnibox(true));
 }
 
+// Regression test for crbug.com/592273.
+TEST_F(SearchIPCRouterPolicyTest, ProcessFocusOmniboxAfterDownload) {
+  // Open an NTP.
+  NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
+  ASSERT_TRUE(GetSearchIPCRouterPolicy()->ShouldProcessFocusOmnibox(true));
+
+  // Simulate a download by opening a URL without committing it.
+  browser()->OpenURL(content::OpenURLParams(
+      GURL("http://foo/download.zip"), content::Referrer(),
+      WindowOpenDisposition::CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
+
+  // Now the visible URL corresponds to the download, but the last committed URL
+  // is still the NTP.
+  const content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_EQ(GURL("http://foo/download.zip"), tab->GetVisibleURL());
+  ASSERT_EQ(GURL(chrome::kChromeSearchLocalNtpUrl), tab->GetLastCommittedURL());
+
+  // In this state, we should still accept IPC messages.
+  EXPECT_TRUE(GetSearchIPCRouterPolicy()->ShouldProcessFocusOmnibox(true));
+}
+
 TEST_F(SearchIPCRouterPolicyTest, DoNotProcessFocusOmnibox) {
   // Process message only if the underlying page is an InstantNTP.
   NavigateAndCommitActiveTab(GURL("chrome-search://foo/bar"));