Omnibox: Show visible URL while navigating away from NTP

This was broken recently by crrev.com/c/800931.

Bug: 792401
Change-Id: Idd269b49bd282a7faa6ca13a259f60384b2b5a08
Reviewed-on: https://chromium-review.googlesource.com/810788
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522404}

chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc

@@ -69,23 +69,25 @@ bool ChromeToolbarModelDelegate::ShouldDisplayURL() const {
   //   of view-source:chrome://newtab, which should display its URL despite what
   //   chrome://newtab says.
   content::NavigationEntry* entry = GetNavigationEntry();
-  if (entry) {
-    if (entry->IsViewSourceMode() ||
-        entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL) {
-      return true;
-    }
-
-    GURL url = entry->GetURL();
-    GURL virtual_url = entry->GetVirtualURL();
-    if (url.SchemeIs(content::kChromeUIScheme) ||
-        virtual_url.SchemeIs(content::kChromeUIScheme)) {
-      if (!url.SchemeIs(content::kChromeUIScheme))
-        url = virtual_url;
-      return url.host() != chrome::kChromeUINewTabHost;
-    }
+  if (!entry)
+    return true;
+
+  if (entry->IsViewSourceMode() ||
+      entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL) {
+    return true;
+  }
+
+  GURL url = entry->GetURL();
+  GURL virtual_url = entry->GetVirtualURL();
+  if (url.SchemeIs(content::kChromeUIScheme) ||
+      virtual_url.SchemeIs(content::kChromeUIScheme)) {
+    if (!url.SchemeIs(content::kChromeUIScheme))
+      url = virtual_url;
+    return url.host() != chrome::kChromeUINewTabHost;
   }
 
-  return !search::IsInstantNTP(GetActiveWebContents());
+  Profile* profile = GetProfile();
+  return !profile || !search::IsInstantNTPURL(url, profile);
 }
 
 security_state::SecurityLevel ChromeToolbarModelDelegate::GetSecurityLevel()

chrome/browser/ui/toolbar/toolbar_model_unittest.cc

@@ -39,10 +39,21 @@ struct TestItem {
         GURL("view-source:http://www.google.com"),
         base::ASCIIToUTF16("view-source:www.google.com"),
     },
+    {
+        GURL("chrome://newtab/"), base::string16(),
+    },
     {
         GURL("view-source:chrome://newtab/"),
         base::ASCIIToUTF16("view-source:chrome://newtab"),
     },
+    {
+        GURL("chrome-search://local-ntp/local-ntp.html"), base::string16(),
+    },
+    {
+        GURL("view-source:chrome-search://local-ntp/local-ntp.html"),
+        base::ASCIIToUTF16(
+            "view-source:chrome-search://local-ntp/local-ntp.html"),
+    },
     {
         GURL("chrome-extension://fooooooooooooooooooooooooooooooo/bar.html"),
         base::ASCIIToUTF16(
@@ -120,23 +131,23 @@ void ToolbarModelTest::NavigateAndCheckText(
     const base::string16& expected_text) {
   // Check while loading.
   content::NavigationController* controller =
-      &browser()->tab_strip_model()->GetWebContentsAt(0)->GetController();
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
   controller->LoadURL(url, content::Referrer(), ui::PAGE_TRANSITION_LINK,
                       std::string());
   ToolbarModel* toolbar_model = browser()->toolbar_model();
   EXPECT_EQ(expected_text, toolbar_model->GetFormattedURL(nullptr));
-  EXPECT_TRUE(toolbar_model->ShouldDisplayURL());
+  EXPECT_NE(expected_text.empty(), toolbar_model->ShouldDisplayURL());
 
   // Check after commit.
   CommitPendingLoad(controller);
   EXPECT_EQ(expected_text, toolbar_model->GetFormattedURL(nullptr));
-  EXPECT_TRUE(toolbar_model->ShouldDisplayURL());
+  EXPECT_NE(expected_text.empty(), toolbar_model->ShouldDisplayURL());
 }
 
 void ToolbarModelTest::NavigateAndCheckElided(const GURL& url) {
   // Check while loading.
   content::NavigationController* controller =
-      &browser()->tab_strip_model()->GetWebContentsAt(0)->GetController();
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
   controller->LoadURL(url, content::Referrer(), ui::PAGE_TRANSITION_LINK,
                       std::string());
   ToolbarModel* toolbar_model = browser()->toolbar_model();
@@ -174,3 +185,31 @@ TEST_F(ToolbarModelTest, ShouldElideLongURLs) {
       GURL(std::string("https://www.foo.com/?") + long_text));
   NavigateAndCheckElided(GURL(std::string("data:abc") + long_text));
 }
+
+// Regression test for crbug.com/792401.
+TEST_F(ToolbarModelTest, ShouldDisplayURLWhileNavigatingAwayFromNTP) {
+  ToolbarModel* toolbar_model = browser()->toolbar_model();
+
+  // Open an NTP. Its URL should not be displayed.
+  AddTab(browser(), GURL("chrome://newtab"));
+  ASSERT_FALSE(toolbar_model->ShouldDisplayURL());
+  ASSERT_TRUE(toolbar_model->GetFormattedURL(nullptr).empty());
+
+  const std::string other_url = "https://www.foo.com";
+
+  // Start loading another page. Its URL should be displayed, even though the
+  // current page is still the NTP.
+  content::NavigationController* controller =
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController();
+  controller->LoadURL(GURL(other_url), content::Referrer(),
+                      ui::PAGE_TRANSITION_LINK, std::string());
+  EXPECT_TRUE(toolbar_model->ShouldDisplayURL());
+  EXPECT_EQ(base::ASCIIToUTF16(other_url),
+            toolbar_model->GetFormattedURL(nullptr));
+
+  // Of course the same should still hold after committing.
+  CommitPendingLoad(controller);
+  EXPECT_TRUE(toolbar_model->ShouldDisplayURL());
+  EXPECT_EQ(base::ASCIIToUTF16(other_url),
+            toolbar_model->GetFormattedURL(nullptr));
+}

components/omnibox/browser/omnibox_client.h

@@ -67,7 +67,7 @@ class OmniboxClient {
   // Returns the favicon of the current page.
   virtual gfx::Image GetFavicon() const;
 
-  // Returns true if the visible entry is a New Tab Page rendered by Instant.
+  // Returns true if the current page is a New Tab Page rendered by Instant.
   virtual bool IsInstantNTP() const;
 
   // Returns true if the committed entry is a search results page.