Simplified domain display: fix bug that bails out with narrow omnibox

This CL fixes a bug where we were incorrectly bailing out on
simplified domain elision all together when the whole URL doesn't fit
in the omnibox.

As background, note that the simplified domain field trials code sets
the steady-state omnibox's RenderText into NO_ELIDE mode. This is
because RenderText over-elides when a display offset has caused part
of the text to scroll offscreen. (I'm not sure if this RenderText
behavior is a bug or not, but it's tracked at crbug.com/1099078.) The
bug is that ElideURL() was bailing out early if the RenderText() didn't
fit the whole URL; instead we want to switch the RenderText into
NO_ELIDE mode and then elide in OmniboxViewViews as best we can.
(Currently, if the full domain doesn't fit, we elide from the right,
but a follow-up CL implements left-edge elision which is better from a
security perspective.)

This change also serves as a speculative fix for crbug.com/1107912,
which I can't reproduce. In that bug, the omnibox seems to somehow get
set back into ELIDE_TAIL mode when switching tabs, so that ElideURL()
early-returns when landing on a different tab instead of properly
eliding to the simplified domain. I'm not sure how the omnibox would
be getting set back into ELIDE_TAIL mode, but if this fix makes the
problem go away, then it confirms the theory that that is indeed
what's happening.

Bug: 1107912
Change-Id: I22ede3b875bac343a6d01fc8d5d8aeb25c2bb5e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2348156
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797148}

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

@@ -2499,20 +2499,24 @@ void OmniboxViewViews::ElideURL() {
   gfx::Range simplified_domain_bounds =
       GetSimplifiedDomainBounds(&ranges_surrounding_simplified_domain);
 
-  // The simplified domain string must be a substring of the current display
-  // text in order to elide to it.
-  if (GetRenderText()->GetDisplayText().find(GetText().substr(
-          simplified_domain_bounds.start(), simplified_domain_bounds.end())) ==
-      std::string::npos) {
-    return;
-  }
-  SetCursorEnabled(false);
   // Setting the elision behavior to anything other than NO_ELIDE would result
   // in the string getting cut off shorter the simplified domain, because
   // display offset isn't taken into account when RenderText elides the string.
-  // See https://crbug.com/1099078.
+  // See https://crbug.com/1099078. It's important to set to NO_ELIDE before
+  // starting to calculate simplified domain bounds with GetSubstringBounds(),
+  // because GetSubstringBounds() will fail if the simplified domain isn't
+  // visible due to RenderText elision.
   GetRenderText()->SetElideBehavior(gfx::NO_ELIDE);
 
+  // The simplified domain string must be a substring of the current display
+  // text in order to elide to it.
+  DCHECK_NE(
+      GetRenderText()->GetDisplayText().find(GetText().substr(
+          simplified_domain_bounds.start(), simplified_domain_bounds.end())),
+      std::string::npos);
+
+  SetCursorEnabled(false);
+
   gfx::Rect simplified_domain_rect;
   for (const auto& rect :
        GetRenderText()->GetSubstringBounds(simplified_domain_bounds)) {

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

@@ -193,6 +193,8 @@ class OmniboxViewViews : public OmniboxView,
   FRIEND_TEST_ALL_PREFIXES(
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       SchemeAndTrivialSubdomainElision);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
+                           SimplifiedDomainElisionWithNarrowOmnibox);
   FRIEND_TEST_ALL_PREFIXES(
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       HideOnInteractionAfterFocusAndBlur);

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

@@ -2912,6 +2912,35 @@ TEST_P(OmniboxViewViewsHideOnInteractionTest, SubframeNavigations) {
   }
 }
 
+// Tests that in the reveal-on-hover field trial variation, the RenderText does
+// not elide the domain if the omnibox is too narrow to fit it.
+TEST_P(OmniboxViewViewsRevealOnHoverTest,
+       SimplifiedDomainElisionWithNarrowOmnibox) {
+  const int kOmniboxWidth = 60;
+  gfx::RenderText* render_text = omnibox_view()->GetRenderText();
+  gfx::Rect current_bounds = omnibox_view()->GetLocalBounds();
+  gfx::Rect bounds(current_bounds.x(), current_bounds.y(), kOmniboxWidth,
+                   current_bounds.height());
+  omnibox_view()->SetBoundsRect(bounds);
+  SetUpSimplifiedDomainTest();
+
+  ASSERT_EQ(kSimplifiedDomainDisplayUrl, render_text->GetDisplayText());
+  ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
+      omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
+      kSimplifiedDomainDisplayUrlSubdomain,
+      kSimplifiedDomainDisplayUrlHostnameAndScheme,
+      kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
+
+  // The omnibox should contain a substring of the domain.
+  gfx::Rect hostname_bounds;
+  for (const auto& rect : render_text->GetSubstringBounds(
+           gfx::Range(kSimplifiedDomainDisplayUrlScheme.size(),
+                      kSimplifiedDomainDisplayUrlHostnameAndScheme.size()))) {
+    hostname_bounds.Union(rect);
+  }
+  EXPECT_FALSE(omnibox_view()->GetLocalBounds().Contains(hostname_bounds));
+}
+
 // Tests that in the hide-on-interaction field trial variation, the path is
 // faded out after omnibox focus and blur.
 TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,