Fix RTL URL rendering in Omnibox (domain off screen on long URL).

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

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index 6add5dda5be118436629c2afa3534209146d6c72..5596603fc2a6c10cb0984d3281f0588ba5b1989a 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -783,26 +783,36 @@ void OmniboxViewViews::OnBlur() {
   // midst of running but hasn't yet opened the popup, it will be halted.
   // If we fully reverted in this case, we'd lose the cursor/highlight
   // information saved above.
-  if (!model()->user_input_in_progress() && model()->popup_model()->IsOpen() &&
-      text() != model()->PermanentText())
+  if (!model()->user_input_in_progress() && model()->popup_model() &&
+      model()->popup_model()->IsOpen() && text() != model()->PermanentText()) {

[Peter Kasting, 2017/05/18 18:27:18]

(1) How come you added a popup_model() null-check?  Is that necessary for the
test?  If so, I would explicitly say in a comment here that the popup model can
be null in tests (since it should never be in production).

(2) Omnibox code doesn't use {} in cases like this (one-line conditional bodies,
multi-line conditional); was this git cl format?

[Matt Giuca, 2017/05/19 05:55:32]

Yes, it null-crashed in the test. Added a comment.
No, it was "matt cl format" :)

Somewhere I picked up a rule that you need braces if either the conditional or
the body was multi-line. Style guide doesn't say this. I think it's a good rule
anyway but reverting.

[Peter Kasting, 2017/05/19 06:11:18]

Scott likes that rule.  I used to really hate it, now I'm more ambivalent.  In
this case I wouldn't have minded it, it's more a "consistency with other omnibox
code" thing than the change actually being bad.

[Matt Giuca, 2017/05/19 06:55:42]

Acknowledged.

     RevertAll();
-  else
+  } else {
     CloseOmniboxPopup();
+  }
 
   // Tell the model to reset itself.
   model()->OnKillFocus();
 
-  // Make sure the beginning of the text is visible.
-  SelectRange(gfx::Range(0));
-
-  GetRenderText()->SetElideBehavior(gfx::ELIDE_TAIL);
+  // When deselected, elide and reset scroll position. After eliding, the old
+  // scroll offset is meaningless (since the string is guaranteed to fit within
+  // the view). The scroll must be reset or the text may be rendered partly or
+  // wholly off-screen.
+  //
+  // Important: Since the URL can contain bidirectional text, it is important to
+  // set the display offset directly to 0 (not simply scroll to the start of the
+  // text, since the start of the text may not be at the left edge).
+  gfx::RenderText* render_text = GetRenderText();
+  render_text->SetElideBehavior(gfx::ELIDE_TAIL);
+  render_text->SetDisplayOffset(0);
 
   // Focus changes can affect the visibility of any keyword hint.
-  if (model()->is_keyword_hint())
-    location_bar_view_->Layout();
+  if (location_bar_view_) {

[Peter Kasting, 2017/05/18 18:27:18]

Again, if this null-check is needed for tests, say that; otherwise don't add it.

[Matt Giuca, 2017/05/19 05:55:32]

Done.

+    if (model()->is_keyword_hint())
+      location_bar_view_->Layout();
 
-  // The location bar needs to repaint without a focus ring.
-  location_bar_view_->SchedulePaint();
+    // The location bar needs to repaint without a focus ring.
+    location_bar_view_->SchedulePaint();
+  }
 }
 
 bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {