Change toolbar height for material design on Ash

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc

Index: chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
diff --git a/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc b/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
index a23f1b8baa9d3964ba69e599c4aa11af9f9b5a07..5077bbd54015baa070839ca7e5f131223d9827cc 100644
--- a/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
+++ b/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
@@ -661,7 +661,38 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
       x, bottom_y,
       w, theme_toolbar->height());
 
-  if (ui::MaterialDesignController::IsModeMaterial()) {
+  const bool mode_material = ui::MaterialDesignController::IsModeMaterial();
+
+  // The pre-material design content area line has a shadow that extends a
+  // couple of pixels above the toolbar bounds.
+  const int kContentShadowHeight = mode_material ? 0 : 2;
+  gfx::ImageSkia* toolbar_top =
+      tp->GetImageSkiaNamed(mode_material ?
+                            IDR_THEME_TOOLBAR : IDR_TOOLBAR_SHADE_TOP);

[Peter Kasting, 2015/09/08 19:38:33]

It's not clear to me why the previous material code was avoiding drawing these
but you changed it to draw them.  Is MD drawing the toolbar by just doing a
rectangular fill with the toolbar color?  If so, this code seems wrong; if not,
where was this previously happening?

Even if we wanted to draw these, we'd want to do separate material and
non-material blocks, since the material block would just draw once instead of
doing all these piecewise bits.  But reply to the above first.

[tdanderson, 2015/09/09 17:23:06]

The previous material code was just a quick hack to stop rendering the shade
assets along the edges of the toolbar, but this came at the cost of the toolbar
appearing shorter than it actually is.

We're rendering the toolbar background by tiling IDR_THEME_TOOLBAR. But as you
pointed out, for MD it's not necessary to split this up into pieces (see the
next patch set). I also don't think it's necessary for MD have the call to
FillRect() on line 648.
Agreed, this code is messy and a lot of it is not necessary for MD. In the next
patch set I have separated MD and non-MD.

+  canvas->TileImageInt(*toolbar_top,
+                       0, 0,
+                       x, y - kContentShadowHeight,
+                       w, split_point + kContentShadowHeight + 1);
+
+  // Draw the "lightening" shade line around the edges of the toolbar.
+  gfx::ImageSkia* toolbar_left =
+      tp->GetImageSkiaNamed(mode_material ?
+                            IDR_THEME_TOOLBAR : IDR_TOOLBAR_SHADE_LEFT);
+  canvas->TileImageInt(*toolbar_left,
+                       0, 0,
+                       x + kClientEdgeThickness,
+                       y + kClientEdgeThickness + kContentShadowHeight,
+                       toolbar_left->width(), theme_toolbar->height());
+  gfx::ImageSkia* toolbar_right =
+      tp->GetImageSkiaNamed(mode_material ?
+                            IDR_THEME_TOOLBAR : IDR_TOOLBAR_SHADE_RIGHT);
+  canvas->TileImageInt(*toolbar_right,
+                       0, 0,
+                       w - toolbar_right->width() - 2 * kClientEdgeThickness,
+                       y + kClientEdgeThickness + kContentShadowHeight,
+                       toolbar_right->width(), theme_toolbar->height());
+
+  if (mode_material) {
     // Draw the content/toolbar separator.
     toolbar_bounds.Inset(kClientEdgeThickness, 0);
     BrowserView::Paint1pxHorizontalLine(
@@ -669,32 +700,15 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
         ThemeProperties::GetDefaultColor(
             ThemeProperties::COLOR_TOOLBAR_SEPARATOR),
         toolbar_bounds);
-  } else {
-    // The pre-material design content area line has a shadow that extends a
-    // couple of pixels above the toolbar bounds.
-    const int kContentShadowHeight = 2;
-    gfx::ImageSkia* toolbar_top = tp->GetImageSkiaNamed(IDR_TOOLBAR_SHADE_TOP);
-    canvas->TileImageInt(*toolbar_top,
-                         0, 0,
-                         x, y - kContentShadowHeight,
-                         w, split_point + kContentShadowHeight + 1);
-
-    // Draw the "lightening" shade line around the edges of the toolbar.
-    gfx::ImageSkia* toolbar_left =
-        tp->GetImageSkiaNamed(IDR_TOOLBAR_SHADE_LEFT);
-    canvas->TileImageInt(*toolbar_left,
-                         0, 0,
-                         x + kClientEdgeThickness,
-                         y + kClientEdgeThickness + kContentShadowHeight,
-                         toolbar_left->width(), theme_toolbar->height());
-    gfx::ImageSkia* toolbar_right =
-        tp->GetImageSkiaNamed(IDR_TOOLBAR_SHADE_RIGHT);
-    canvas->TileImageInt(*toolbar_right,
-                         0, 0,
-                         w - toolbar_right->width() - 2 * kClientEdgeThickness,
-                         y + kClientEdgeThickness + kContentShadowHeight,
-                         toolbar_right->width(), theme_toolbar->height());
 
+    // Draw the tabstrip separator.

[Peter Kasting, 2015/09/08 19:38:33]

Just avoid drawing this at all, I'll take care of it.

[tdanderson, 2015/09/09 17:23:06]

I included this for the purposes of screenshots / agreement over where the
separator should be. I've removed it for each frame.

+    BrowserView::Paint1pxHorizontalLine(
+        canvas,
+        ThemeProperties::GetDefaultColor(
+            ThemeProperties::COLOR_TOOLBAR_SEPARATOR),
+        gfx::RectF(0, toolbar_bounds.y() - 1,
+                   toolbar_bounds.width(), kClientEdgeThickness));
+  } else {
     // Draw the content/toolbar separator.
     canvas->FillRect(
         gfx::Rect(x + kClientEdgeThickness,