Simplified domains: Only call GetPreviousURL on committed/non-error navigations

OmniboxViewViews::DidFinishNavigation calls GetPreviousURL() to determine
if the URL has changed besides the fragment, in which case the full URL
is re-shown. Calling GetPreviousURL() on non-committed or error page
navigations fails a DCHECK. This CL thus adds special checks for whether
the navigation has committed or is an error page.

Speculative fix for https://crbug.com/1127797.

Bug: 1127797
Test: CQ
Change-Id: I245440ea082863b1337c104d15f062e4eb69e378
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2409968Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807203}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -1997,13 +1997,26 @@ void OmniboxViewViews::DidFinishNavigation(
   if (!navigation->IsInMainFrame())
     return;
 
+  // If the navigation didn't commit, and it was renderer-initiated, then no
+  // action is needed, as the URL won't have been updated. But if it was
+  // browser-initiated, then the URL would have been updated to show the URL of
+  // the in-progress navigation; in this case, reset to show the full URL now
+  // that the navigation has finished without committing.
+  if (!navigation->HasCommitted()) {
+    if (navigation->IsRendererInitiated()) {
+      return;
+    }
+    ResetToHideOnInteraction();
+    return;
+  }
+
   // Once a navigation finishes that changes the visible URL (besides just the
   // ref), unelide and reset state so that we'll show the simplified domain on
   // interaction. Same-document navigations that only change the ref are treated
   // specially and don't cause the elision/unelision state to be altered. This
   // is to avoid frequent eliding/uneliding within single-page apps that do
   // frequent fragment navigations.
-  if (!navigation->IsSameDocument() ||
+  if (navigation->IsErrorPage() || !navigation->IsSameDocument() ||
       !navigation->GetPreviousURL().EqualsIgnoringRef(navigation->GetURL())) {
     ResetToHideOnInteraction();
   }

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -327,6 +327,8 @@ class OmniboxViewViews : public OmniboxView,
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       NoStaleGradientMask);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest, ModifierKeys);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
+                           ErrorPageNavigation);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
                            SameDocNavigations);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -292,6 +292,7 @@ void TestingOmniboxView::NavigateAndExpectElided(
   location_bar_model_->set_url_for_display(base::ASCIIToUTF16(url.spec()));
   model()->ResetDisplayTexts();
   RevertAll();
+  navigation.set_has_committed(true);
   DidFinishNavigation(&navigation);
   ExpectElidedToSimplifiedDomain(this, scheme, subdomain, hostname_and_scheme,
                                  path, should_elide_to_registrable_domain);
@@ -311,6 +312,7 @@ void TestingOmniboxView::NavigateAndExpectUnelided(
   location_bar_model_->set_url_for_display(url);
   model()->ResetDisplayTexts();
   RevertAll();
+  navigation.set_has_committed(true);
   DidFinishNavigation(&navigation);
   ExpectUnelidedFromSimplifiedDomain(this->GetRenderText(),
                                      gfx::Range(scheme.size(), url.size()));
@@ -2157,6 +2159,56 @@ TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
   }
 }
 
+// Tests that the hide-on-interaction simplified domain field trial handles
+// non-committed navigations properly.
+TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
+       NonCommittedNavigations) {
+  SetUpSimplifiedDomainTest();
+
+  ASSERT_NO_FATAL_FAILURE(omnibox_view()->NavigateAndExpectUnelided(
+      kSimplifiedDomainDisplayUrl,
+      /*is_same_document=*/false, GURL(), kSimplifiedDomainDisplayUrlScheme));
+  // Simulate a user interaction to elide the URL.
+  omnibox_view()->DidGetUserInteraction(blink::WebKeyboardEvent());
+  ASSERT_NO_FATAL_FAILURE(
+      omnibox_view()->StepSimplifiedDomainInteractionAnimation(
+          /*step_ms=*/1000));
+  ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
+      omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
+      kSimplifiedDomainDisplayUrlSubdomain,
+      kSimplifiedDomainDisplayUrlHostnameAndScheme,
+      kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
+
+  // When a renderer-initiated navigation finishes without committing, the URL
+  // should remain elided; we don't update the display URL until the navigation
+  // commits.
+  {
+    content::MockNavigationHandle navigation;
+    navigation.set_is_renderer_initiated(true);
+    navigation.set_has_committed(false);
+    omnibox_view()->DidStartNavigation(&navigation);
+    omnibox_view()->DidFinishNavigation(&navigation);
+    ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
+        omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
+        kSimplifiedDomainDisplayUrlSubdomain,
+        kSimplifiedDomainDisplayUrlHostnameAndScheme,
+        kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
+  }
+
+  // When a browser-initiated navigation finishes without committing, the URL
+  // updates before commit, so we should reset back to the on-page-load state if
+  // the navigation doesn't eventually commit.
+  content::MockNavigationHandle navigation;
+  navigation.set_is_renderer_initiated(false);
+  navigation.set_has_committed(false);
+  omnibox_view()->DidStartNavigation(&navigation);
+  omnibox_view()->DidFinishNavigation(&navigation);
+  ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
+      omnibox_view()->GetRenderText(),
+      gfx::Range(kSimplifiedDomainDisplayUrlScheme.size(),
+                 kSimplifiedDomainDisplayUrl.size())));
+}
+
 // Tests that mouse clicks do not count as user interactions and do not elide
 // the URL.
 TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest, MouseClick) {
@@ -2879,6 +2931,37 @@ TEST_P(OmniboxViewViewsHideOnInteractionTest, ModifierKeys) {
   EXPECT_FALSE(elide_animation);
 }
 
+// Tests that in the hide-on-interaction field trial, the URL is unelided when
+// navigating to an error page.
+TEST_P(OmniboxViewViewsHideOnInteractionTest, ErrorPageNavigation) {
+  SetUpSimplifiedDomainTest();
+  omnibox_view()->NavigateAndExpectUnelided(kSimplifiedDomainDisplayUrl,
+                                            /*is_same_document=*/false, GURL(),
+                                            kSimplifiedDomainDisplayUrlScheme);
+
+  omnibox_view()->NavigateAndExpectUnelided(kSimplifiedDomainDisplayUrl,
+                                            /*is_same_document=*/false, GURL(),
+                                            kSimplifiedDomainDisplayUrlScheme);
+  // Simulate a user interaction to elide to the simplified domain.
+  omnibox_view()->DidGetUserInteraction(blink::WebKeyboardEvent());
+  OmniboxViewViews::ElideAnimation* elide_animation =
+      omnibox_view()->GetElideAfterInteractionAnimationForTesting();
+  ASSERT_TRUE(elide_animation);
+  EXPECT_TRUE(elide_animation->IsAnimating());
+
+  // Now simulate a navigation to an error page and check that the URL is
+  // unelided.
+  content::MockNavigationHandle navigation;
+  navigation.set_url(GURL(kSimplifiedDomainDisplayUrl));
+  navigation.set_is_error_page(true);
+  omnibox_view()->DidStartNavigation(&navigation);
+  omnibox_view()->DidFinishNavigation(&navigation);
+  ExpectUnelidedFromSimplifiedDomain(
+      omnibox_view()->GetRenderText(),
+      gfx::Range(kSimplifiedDomainDisplayUrlScheme.size(),
+                 kSimplifiedDomainDisplayUrl.size()));
+}
+
 // Tests that in the hide-on-interaction field trial, the URL is simplified on
 // cross-document main-frame navigations, but not on same-document navigations.
 TEST_P(OmniboxViewViewsHideOnInteractionTest, SameDocNavigations) {