Change width of rows in material design omnibox dropdown

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

 // Copyright (c) 2012 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.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
 #endif
@@ skipping 419 matching lines @@
         gfx::DIRECTIONALITY_FORCE_RTL : gfx::DIRECTIONALITY_FORCE_LTR);
     prefix_render_text->SetHorizontalAlignment(
           is_match_contents_rtl ? gfx::ALIGN_RIGHT : gfx::ALIGN_LEFT);
     prefix_render_text->SetDisplayRect(
         gfx::Rect(mirroring_context_->mirrored_left_coord(
                       prefix_x, prefix_x + prefix_width),
                   y, prefix_width, GetContentLineHeight()));
     prefix_render_text->Draw(canvas);
   }
 
+  // No text should be drawn within the right margin.

[Peter Kasting, 2015/08/18 19:55:37]

This isn't the right way to do this.  You need to change the code in
PaintMatch() so that it computes the contents and description widths based on
the correct available width, as we're making eliding and layout decisions that
rely on knowing precisely how much space is available.  What this code does
instead is lay out as if we have the full width, but then truncate the last
portion.

[tdanderson, 2015/08/19 17:22:35]

OK, please take a look at what I've done in Patch Set 2, which looks fine with
and without the MD flag on a local build. Though I'm a bit suspicious of the
fact that I am fiddling with the call to mirroring_context_->remaining_width(x)
in PaintMatch() but not the one in DrawRenderText().

+  if (ui::MaterialDesignController::IsModeMaterial())
+    right_x = std::min(right_x, width() - RightOffset());
+
   // Set the display rect to trigger eliding.
   render_text->SetDisplayRect(
       gfx::Rect(mirroring_context_->mirrored_left_coord(x, right_x), y,
                 right_x - x, GetContentLineHeight()));
   render_text->Draw(canvas);
   return right_x;
 }
 
 scoped_ptr<gfx::RenderText> OmniboxResultView::CreateRenderText(
     const base::string16& text) const {
@@ skipping 169 matching lines @@
   // |theme_provider| can be null when animations are running during shutdown,
   // after OmniboxResultView has been removed from the tree of Views.
   if (!theme_provider)
     return;
   const int horizontal_padding = theme_provider->GetDisplayProperty(
       ThemeProperties::PROPERTY_LOCATION_BAR_HORIZONTAL_PADDING);
   const int trailing_padding = theme_provider->GetDisplayProperty(
       ThemeProperties::PROPERTY_ICON_LABEL_VIEW_TRAILING_PADDING);
 
   icon_bounds_.SetRect(
-      horizontal_padding +
+      horizontal_padding + LeftOffset() +

[Peter Kasting, 2015/08/18 19:55:37]

Nit: If we create two more temps:

  const int start_x = LeftOffset() + horizontal_padding;
  const int end_x = width() - RightOffset() - horizontal_padding;

Then we can simplify the code here some:

  icon_bounds_.SetRect(
      start_x + ((icon.width() == default_icon_size_) ? 0 : trailing_padding),
      (GetContentLineHeight() - icon.height()) / 2,
      icon.width(), icon.height());

  const int text_x = start_x + default_icon_size_ + horizontal_padding_;
  int text_width = end_x - text_x;

  if (match_.associated_keyword) {
    const int max_kw_x = end_x - keyword_icon_->width();
    const int kw_x = animation_->CurrentValueBetween(max_kw_x, start_x);
    ...

[tdanderson, 2015/08/19 17:22:35]

Thanks, should've caught that. Done.

           ((icon.width() == default_icon_size_) ? 0 : trailing_padding),
       (GetContentLineHeight() - icon.height()) / 2, icon.width(),
       icon.height());
 
-  int text_x = (2 * horizontal_padding) + default_icon_size_;
-  int text_width = width() - text_x - horizontal_padding;
+  const int text_x =
+      LeftOffset() + (2 * horizontal_padding) + default_icon_size_;
+  int text_width = width() - text_x - horizontal_padding - RightOffset();
 
   if (match_.associated_keyword.get()) {
     const int kw_collapsed_size = keyword_icon_->width() + horizontal_padding;
-    const int max_kw_x = width() - kw_collapsed_size;
-    const int kw_x =
-        animation_->CurrentValueBetween(max_kw_x, horizontal_padding);
+    const int max_kw_x = width() - kw_collapsed_size - RightOffset();
+    const int kw_x = animation_->CurrentValueBetween(
+        max_kw_x, LeftOffset() + horizontal_padding);
+
     const int kw_text_x = kw_x + keyword_icon_->width() + horizontal_padding;
-
     text_width = kw_x - text_x - horizontal_padding;
     keyword_text_bounds_.SetRect(
         kw_text_x, 0, std::max(width() - kw_text_x - horizontal_padding, 0),
         height());
     keyword_icon_->SetPosition(
         gfx::Point(kw_x, (height() - keyword_icon_->height()) / 2));
   }
 
   text_bounds_.SetRect(text_x, 0, std::max(text_width, 0), height());
 }
 
 void OmniboxResultView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  animation_->SetSlideDuration(width() / 4);
+  animation_->SetSlideDuration((width() - LeftOffset() - RightOffset()) / 4);
 }
 
 void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
   const ResultViewState state = GetState();
   if (state != NORMAL)
     canvas->DrawColor(GetColor(state, BACKGROUND));
 
   // NOTE: While animating the keyword match, both matches may be visible.
 
   if (!ShowOnlyKeywordMatch()) {
@@ skipping 126 matching lines @@
   gfx::Range range(offset, offset + text.length());
   destination->AppendText(text);
   const TextStyle& text_style = GetTextStyle(text_type);
   // TODO(dschuyler): follow up on the problem of different font sizes within
   // one RenderText.  Maybe with destination->SetFontList(...).
   destination->ApplyStyle(gfx::BOLD, is_bold, range);
   destination->ApplyColor(
       GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
   destination->ApplyBaselineStyle(text_style.baseline, range);
 }
+
+int OmniboxResultView::LeftOffset() const {
+  if (ui::MaterialDesignController::IsModeMaterial())

[Peter Kasting, 2015/08/18 19:55:37]

Nit: Shorter:

  return ui::MaterialDesignController::IsModeMaterial() ?
      model_->left_margin() : 0;

(2 places)

[tdanderson, 2015/08/19 17:22:35]

Done.

+    return model_->left_margin();
+  return 0;
+}
+
+int OmniboxResultView::RightOffset() const {
+  if (ui::MaterialDesignController::IsModeMaterial())
+    return model_->right_margin();
+  return 0;
+}