[omnibox] Fix page classification detection for interactions from WebUI NTP

Omnibox page classification detection logic is failing to correctly
report interactions from the WebUI NTP. This is due to the WebUI NTP
not using the instant process. This CL fixes that by making the
ChromeLocationBarModelDelegate::IsInstantNTP logic correctly identify
both the local NTP and the WebUI NTP without the need for checking
whether the NTP uses the instant process.

As a result interactions from the WebUI NTP will continue to be reported
as OmniboxEventProto::INSTANT_NTP_WITH_[OMNIBOX|FAKEBOX]_AS_STARTING_FOCUS
without the NTP actually using the instant process.

This logic *should* eventually be refactord into
chrome/browser/search/search.cc once the WebUI NTP is fully launched and
the references to the instant process are removed.

Bug: 1098513
Change-Id: I6d4a9e75a45cd70232ffa3541b122c639a428a4a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2265415
Commit-Queue: Lei Zhang <thestig@chromium.org>
Auto-Submit: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783086}

chrome/browser/BUILD.gn

@@ -1433,6 +1433,8 @@ static_library("browser") {
     "resource_coordinator/utils.h",
     "resources_util.cc",
     "resources_util.h",
+    "search/ntp_features.cc",
+    "search/ntp_features.h",
     "search/search.cc",
     "search/search.h",
     "search/suggestions/suggestions_service_factory.cc",
@@ -3474,8 +3476,6 @@ static_library("browser") {
       "search/most_visited_iframe_source.h",
       "search/ntp_custom_background_enabled_policy_handler.cc",
       "search/ntp_custom_background_enabled_policy_handler.h",
-      "search/ntp_features.cc",
-      "search/ntp_features.h",
       "search/ntp_icon_source.cc",
       "search/ntp_icon_source.h",
       "search/one_google_bar/one_google_bar_data.cc",

chrome/browser/ui/android/toolbar/location_bar_model_android.cc

@@ -77,7 +77,7 @@ content::WebContents* LocationBarModelAndroid::GetActiveWebContents() const {
   return content::WebContents::FromJavaWebContents(jweb_contents);
 }
 
-bool LocationBarModelAndroid::IsInstantNTP() const {
+bool LocationBarModelAndroid::IsNewTabPage() const {
   GURL url;
   if (!GetURL(&url))
     return false;

chrome/browser/ui/android/toolbar/location_bar_model_android.h

@@ -37,7 +37,7 @@ class LocationBarModelAndroid : public ChromeLocationBarModelDelegate {
 
   // ChromeLocationBarModelDelegate:
   content::WebContents* GetActiveWebContents() const override;
-  bool IsInstantNTP() const override;
+  bool IsNewTabPage() const override;
 
  private:
   std::unique_ptr<LocationBarModel> location_bar_model_;

chrome/browser/ui/toolbar/chrome_location_bar_model_delegate.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/ntp_features.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
@@ -195,11 +196,26 @@ bool ChromeLocationBarModelDelegate::IsOfflinePage() const {
 #endif
 }
 
-bool ChromeLocationBarModelDelegate::IsInstantNTP() const {
-  return search::IsInstantNTP(GetActiveWebContents());
+bool ChromeLocationBarModelDelegate::IsNewTabPage() const {
+  content::NavigationEntry* const entry = GetNavigationEntry();
+  if (!entry)
+    return false;
+
+  Profile* const profile = GetProfile();
+  if (!profile)
+    return false;
+
+  if (!search::DefaultSearchProviderIsGoogle(profile))
+    return false;
+
+  GURL ntp_url(base::FeatureList::IsEnabled(ntp_features::kWebUI)
+                   ? chrome::kChromeUINewTabPageURL
+                   : chrome::kChromeSearchLocalNtpUrl);
+  return ntp_url.scheme_piece() == entry->GetURL().scheme_piece() &&
+         ntp_url.host_piece() == entry->GetURL().host_piece();
 }
 
-bool ChromeLocationBarModelDelegate::IsNewTabPage(const GURL& url) const {
+bool ChromeLocationBarModelDelegate::IsNewTabPageURL(const GURL& url) const {
   return url.spec() == chrome::kChromeUINewTabURL;
 }
 

chrome/browser/ui/toolbar/chrome_location_bar_model_delegate.h

@@ -42,8 +42,8 @@ class ChromeLocationBarModelDelegate : public LocationBarModelDelegate {
   scoped_refptr<net::X509Certificate> GetCertificate() const override;
   const gfx::VectorIcon* GetVectorIconOverride() const override;
   bool IsOfflinePage() const override;
-  bool IsInstantNTP() const override;
-  bool IsNewTabPage(const GURL& url) const override;
+  bool IsNewTabPage() const override;
+  bool IsNewTabPageURL(const GURL& url) const override;
   bool IsHomePage(const GURL& url) const override;
   AutocompleteClassifier* GetAutocompleteClassifier() override;
   TemplateURLService* GetTemplateURLService() override;

chrome/browser/ui/toolbar/chrome_location_bar_model_delegate_unittest.cc

+// Copyright 2020 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 "chrome/browser/ui/toolbar/chrome_location_bar_model_delegate.h"
+
+#include <memory>
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/search/ntp_features.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/base/search_test_utils.h"
+#include "components/search_engines/template_url_service.h"
+
+// Concrete implementation of ChromeLocationBarModelDelegate.
+class TestChromeLocationBarModelDelegate
+    : public ChromeLocationBarModelDelegate {
+ public:
+  explicit TestChromeLocationBarModelDelegate(Browser* browser)
+      : browser_(browser) {}
+  ~TestChromeLocationBarModelDelegate() override = default;
+
+  // Not copyable or movable.
+  TestChromeLocationBarModelDelegate(
+      const TestChromeLocationBarModelDelegate&) = delete;
+  TestChromeLocationBarModelDelegate& operator=(
+      const TestChromeLocationBarModelDelegate&) = delete;
+
+  // ChromeLocationBarModelDelegate:
+  content::WebContents* GetActiveWebContents() const override {
+    browser_->tab_strip_model()->GetActiveWebContents();
+    return browser_->tab_strip_model()->GetActiveWebContents();
+  }
+
+ private:
+  Browser* const browser_;
+};
+
+class ChromeLocationBarModelDelegateTest
+    : public BrowserWithTestWindowTest,
+      public ::testing::WithParamInterface<bool> {
+ protected:
+  ChromeLocationBarModelDelegateTest() {
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeature(ntp_features::kWebUI);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(ntp_features::kWebUI);
+    }
+  }
+  void SetUp() override {
+    BrowserWithTestWindowTest::SetUp();
+
+    TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+        profile(),
+        base::BindRepeating(&TemplateURLServiceFactory::BuildInstanceFor));
+    TemplateURLService* template_url_service =
+        TemplateURLServiceFactory::GetForProfile(profile());
+    search_test_utils::WaitForTemplateURLServiceToLoad(template_url_service);
+
+    delegate_ = std::make_unique<TestChromeLocationBarModelDelegate>(browser());
+  }
+
+  void SetSearchProvider(bool set_ntp_url) {
+    TemplateURLService* template_url_service =
+        TemplateURLServiceFactory::GetForProfile(profile());
+    TemplateURLData data;
+    data.SetShortName(base::ASCIIToUTF16("foo.com"));
+    data.SetURL("http://foo.com/url?bar={searchTerms}");
+    if (set_ntp_url) {
+      data.new_tab_url = "https://foo.com/newtab";
+    }
+
+    TemplateURL* template_url =
+        template_url_service->Add(std::make_unique<TemplateURL>(data));
+    template_url_service->SetUserSelectedDefaultSearchProvider(template_url);
+  }
+
+  GURL GetURL() {
+    GURL url;
+    EXPECT_TRUE(delegate_->GetURL(&url));
+    return url;
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+  std::unique_ptr<TestChromeLocationBarModelDelegate> delegate_;
+};
+
+// Tests whether ChromeLocationBarModelDelegate::IsNewTabPage and
+// ChromeLocationBarModelDelegate::IsNewTabPageURL return the expected results
+// for various NTP scenarios.
+TEST_P(ChromeLocationBarModelDelegateTest, IsNewTabPage) {
+  chrome::NewTab(browser());
+  // New Tab URL with Google DSP resolves to the local or the WebUI NTP URL.
+  GURL ntp_url(base::FeatureList::IsEnabled(ntp_features::kWebUI)
+                   ? chrome::kChromeUINewTabPageURL
+                   : chrome::kChromeSearchLocalNtpUrl);
+  EXPECT_EQ(ntp_url, search::GetNewTabPageURL(profile()));
+
+  EXPECT_TRUE(delegate_->IsNewTabPage());
+  EXPECT_TRUE(delegate_->IsNewTabPageURL(GetURL()));
+
+  SetSearchProvider(false);
+  chrome::NewTab(browser());
+  // New Tab URL with a user selected DSP without an NTP URL resolves to
+  // chrome-search://local-ntp/local-ntp.html.
+  EXPECT_EQ(GURL(chrome::kChromeSearchLocalNtpUrl),
+            search::GetNewTabPageURL(profile()));
+
+  EXPECT_FALSE(delegate_->IsNewTabPage());
+  EXPECT_TRUE(delegate_->IsNewTabPageURL(GetURL()));
+
+  SetSearchProvider(true);
+  chrome::NewTab(browser());
+  // New Tab URL with a user selected DSP resolves to the DSP's NTP URL.
+  EXPECT_EQ("https://foo.com/newtab", search::GetNewTabPageURL(profile()));
+
+  EXPECT_FALSE(delegate_->IsNewTabPage());
+  EXPECT_TRUE(delegate_->IsNewTabPageURL(GetURL()));
+}
+
+INSTANTIATE_TEST_SUITE_P(,
+                         ChromeLocationBarModelDelegateTest,
+                         ::testing::Bool());

chrome/test/BUILD.gn

@@ -4408,6 +4408,7 @@ test("unit_tests") {
       "../browser/ui/media_router/media_router_ui_service_factory_unittest.cc",
       "../browser/ui/media_router/query_result_manager_unittest.cc",
       "../browser/ui/passwords/manage_passwords_ui_controller_unittest.cc",
+      "../browser/ui/toolbar/chrome_location_bar_model_delegate_unittest.cc",
       "../browser/ui/toolbar/media_router_action_controller_unittest.cc",
       "../browser/ui/toolbar/media_router_contextual_menu_unittest.cc",
       "../browser/ui/toolbar/mock_media_router_action_controller.cc",

components/omnibox/browser/location_bar_model_delegate.cc

@@ -39,11 +39,11 @@ bool LocationBarModelDelegate::IsOfflinePage() const {
   return false;
 }
 
-bool LocationBarModelDelegate::IsInstantNTP() const {
+bool LocationBarModelDelegate::IsNewTabPage() const {
   return false;
 }
 
-bool LocationBarModelDelegate::IsNewTabPage(const GURL& url) const {
+bool LocationBarModelDelegate::IsNewTabPageURL(const GURL& url) const {
   return false;
 }
 

components/omnibox/browser/location_bar_model_delegate.h

@@ -72,11 +72,13 @@ class LocationBarModelDelegate {
   // previously-downloaded content.
   virtual bool IsOfflinePage() const;
 
-  // Returns true if the current page is a New Tab Page rendered by Instant.
-  virtual bool IsInstantNTP() const;
+  // Returns true if the current page is the default new tab page, i.e., the
+  // page shown when Google is the DSP, NTP is not provided by an extension,
+  // the profile is not off-the-record, and the browser is not in Guest mode.
+  virtual bool IsNewTabPage() const;
 
   // Returns whether |url| corresponds to the new tab page.
-  virtual bool IsNewTabPage(const GURL& url) const;
+  virtual bool IsNewTabPageURL(const GURL& url) const;
 
   // Returns whether |url| corresponds to the user's home page.
   virtual bool IsHomePage(const GURL& url) const;

components/omnibox/browser/location_bar_model_impl.cc

@@ -155,7 +155,7 @@ LocationBarModelImpl::GetPageClassification(OmniboxFocusSource focus_source) {
 
   if (focus_source == OmniboxFocusSource::SEARCH_BUTTON)
     return OmniboxEventProto::SEARCH_BUTTON_AS_STARTING_FOCUS;
-  if (delegate_->IsInstantNTP()) {
+  if (delegate_->IsNewTabPage()) {
     // Note that we treat OMNIBOX as the source if focus_source_ is INVALID,
     // i.e., if input isn't actually in progress.
     return (focus_source == OmniboxFocusSource::FAKEBOX)
@@ -164,7 +164,7 @@ LocationBarModelImpl::GetPageClassification(OmniboxFocusSource focus_source) {
   }
   if (!gurl.is_valid())
     return OmniboxEventProto::INVALID_SPEC;
-  if (delegate_->IsNewTabPage(gurl))
+  if (delegate_->IsNewTabPageURL(gurl))
     return OmniboxEventProto::NTP;
   if (gurl.spec() == url::kAboutBlankURL)
     return OmniboxEventProto::BLANK;
@@ -279,4 +279,4 @@ bool LocationBarModelImpl::IsOfflinePage() const {
 
 bool LocationBarModelImpl::ShouldPreventElision() const {
   return delegate_->ShouldPreventElision();
-}
+}
\ No newline at end of file

components/omnibox/browser/location_bar_model_impl_unittest.cc

@@ -65,9 +65,9 @@ class FakeLocationBarModelDelegate : public LocationBarModelDelegate {
     return state;
   }
 
-  bool IsInstantNTP() const override { return false; }
+  bool IsNewTabPage() const override { return false; }
 
-  bool IsNewTabPage(const GURL& url) const override { return false; }
+  bool IsNewTabPageURL(const GURL& url) const override { return false; }
 
   bool IsHomePage(const GURL& url) const override { return false; }
 

ios/chrome/browser/ui/location_bar/location_bar_model_delegate_ios.h

@@ -35,8 +35,8 @@ class LocationBarModelDelegateIOS : public LocationBarModelDelegate {
   scoped_refptr<net::X509Certificate> GetCertificate() const override;
   const gfx::VectorIcon* GetVectorIconOverride() const override;
   bool IsOfflinePage() const override;
-  bool IsInstantNTP() const override;
-  bool IsNewTabPage(const GURL& url) const override;
+  bool IsNewTabPage() const override;
+  bool IsNewTabPageURL(const GURL& url) const override;
   bool IsHomePage(const GURL& url) const override;
 
  private:

ios/chrome/browser/ui/location_bar/location_bar_model_delegate_ios.mm

@@ -114,7 +114,7 @@ bool LocationBarModelDelegateIOS::IsOfflinePage() const {
       ->presenting_offline_page();
 }
 
-bool LocationBarModelDelegateIOS::IsInstantNTP() const {
+bool LocationBarModelDelegateIOS::IsNewTabPage() const {
   // This is currently only called by the OmniboxEditModel to determine if the
   // Google landing page is showing.
   //
@@ -127,7 +127,7 @@ bool LocationBarModelDelegateIOS::IsInstantNTP() const {
   return currentURL == kChromeUINewTabURL;
 }
 
-bool LocationBarModelDelegateIOS::IsNewTabPage(const GURL& url) const {
+bool LocationBarModelDelegateIOS::IsNewTabPageURL(const GURL& url) const {
   return url.spec() == kChromeUINewTabURL;
 }