Use the correct navigation event when checking for lookalike URL suggestions

The current code uses NavigationEntryCommitted, but it should have used
DidNavigationFinish instead.

Bug: 843361, 847662
Change-Id: Ie16016844e636a14c2114450a6e3b2a94cd2529b
Reviewed-on: https://chromium-review.googlesource.com/1197083Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587767}

chrome/browser/ui/omnibox/lookalike_url_navigation_observer.cc

@@ -16,8 +16,7 @@
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/url_formatter/idn_spoof_checker.h"
 #include "components/url_formatter/url_formatter.h"
-#include "content/public/browser/navigation_details.h"
-#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 namespace {
@@ -54,9 +53,18 @@ LookalikeUrlNavigationObserver::LookalikeUrlNavigationObserver(
 
 LookalikeUrlNavigationObserver::~LookalikeUrlNavigationObserver() {}
 
-void LookalikeUrlNavigationObserver::NavigationEntryCommitted(
-    const content::LoadCommittedDetails& load_details) {
-  const GURL url = load_details.entry->GetVirtualURL();
+void LookalikeUrlNavigationObserver::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(navigation_handle->IsInMainFrame());
+  // If the navigation was not committed, it means either the page was a
+  // download or error 204/205, or the navigation never left the previous
+  // URL. Basically, this isn't a problem since we stayed at the existing URL.
+  // Also ignore same document navigations.
+  if (!navigation_handle->HasCommitted() || navigation_handle->IsSameDocument())
+    return;
+
+  const GURL url = navigation_handle->GetURL();
+
   // If the user has engaged with this site, don't show any lookalike
   // navigation suggestions.
   Profile* profile =
@@ -91,8 +99,7 @@ void LookalikeUrlNavigationObserver::NavigationEntryCommitted(
   if (kMetricsOnly.Get().empty()) {
     RecordEvent(NavigationSuggestionEvent::kInfobarShown);
     AlternateNavInfoBarDelegate::CreateForLookalikeUrlNavigation(
-        web_contents(), base::UTF8ToUTF16(matched_domain), suggested_url,
-        load_details.entry->GetVirtualURL(),
+        web_contents(), base::UTF8ToUTF16(matched_domain), suggested_url, url,
         base::BindOnce(RecordEvent, NavigationSuggestionEvent::kLinkClicked));
   }
 }

chrome/browser/ui/omnibox/lookalike_url_navigation_observer.h

@@ -8,6 +8,10 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace content {
+class NavigationHandle;
+}
+
 class SiteEngagementService;
 
 // Observes navigations and shows an infobar if the navigated domain name
@@ -37,8 +41,8 @@ class LookalikeUrlNavigationObserver
   ~LookalikeUrlNavigationObserver() override;
 
   // content::WebContentsObserver:
-  void NavigationEntryCommitted(
-      const content::LoadCommittedDetails& load_details) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
 
  private:
   // Returns a site that the user has used before that |url| may be attempting

chrome/browser/ui/omnibox/lookalike_url_navigation_observer_browsertest.cc

@@ -75,6 +75,17 @@ class LookalikeUrlNavigationObserverBrowserTest
         ->ResetBaseScoreForURL(url, score);
   }
 
+  // Simulates a link click navigation. We don't use
+  // ui_test_utils::NavigateToURL(const GURL&) because it simulates the user
+  // typing the URL, causing the site to have a site engagement score of at
+  // least LOW.
+  void NavigateToURL(const GURL& url) {
+    NavigateParams params(browser(), url, ui::PAGE_TRANSITION_LINK);
+    params.disposition = WindowOpenDisposition::CURRENT_TAB;
+    params.is_renderer_initiated = true;
+    ui_test_utils::NavigateToURL(&params);
+  }
+
   void TestInfobarNotShown(const GURL& navigated_url) {
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
@@ -82,7 +93,7 @@ class LookalikeUrlNavigationObserverBrowserTest
         InfoBarService::FromWebContents(web_contents);
 
     content::TestNavigationObserver navigation_observer(web_contents, 1);
-    ui_test_utils::NavigateToURL(browser(), navigated_url);
+    NavigateToURL(navigated_url);
     navigation_observer.Wait();
     EXPECT_EQ(0u, infobar_service->infobar_count());
   }
@@ -105,7 +116,7 @@ class LookalikeUrlNavigationObserverBrowserTest
         InfoBarService::FromWebContents(web_contents);
     InfoBarObserver infobar_added_observer(
         infobar_service, InfoBarObserver::Type::kInfoBarAdded);
-    ui_test_utils::NavigateToURL(browser(), navigated_url);
+    NavigateToURL(navigated_url);
     infobar_added_observer.Wait();
 
     infobars::InfoBar* infobar = infobar_service->infobar_at(0);