Implement MD specs for non-tabbed windows in 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 789a37fa063738765bbec47ce6ef3dbaca24ce4e..0b72f6db6a1becda1247998827ce920a2f2ddb4f 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
@@ -29,6 +29,7 @@
 #include "chrome/browser/ui/views/profiles/avatar_menu_button.h"
 #include "chrome/browser/ui/views/tab_icon_view.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "content/public/browser/web_contents.h"
@@ -189,8 +190,8 @@ int BrowserNonClientFrameViewAsh::GetTopInset(bool restored) const {
   int caption_buttons_bottom = caption_button_container_->bounds().bottom();
 
   // The toolbar partially overlaps the caption buttons.
-  if (browser_view()->IsToolbarVisible())
-    return caption_buttons_bottom - kContentShadowHeight;
+  if (IsToolbarVisible())
+    return caption_buttons_bottom;
 
   return caption_buttons_bottom + kClientEdgeThickness;
 }
@@ -306,10 +307,10 @@ void BrowserNonClientFrameViewAsh::OnPaint(gfx::Canvas* canvas) {
   ash::HeaderPainter::Mode header_mode = ShouldPaintAsActive() ?
       ash::HeaderPainter::MODE_ACTIVE : ash::HeaderPainter::MODE_INACTIVE;
   header_painter_->PaintHeader(canvas, header_mode);
-  if (browser_view()->IsToolbarVisible())
+
+  if (IsToolbarVisible())
     PaintToolbarBackground(canvas);
-  else if (!UsePackagedAppHeaderStyle() && !UseWebAppHeaderStyle())
-    PaintContentEdge(canvas);
+  PaintContentEdge(canvas);
 }
 
 void BrowserNonClientFrameViewAsh::Layout() {
@@ -321,7 +322,7 @@ void BrowserNonClientFrameViewAsh::Layout() {
   int painted_height = GetTopInset(false);
   if (browser_view()->IsTabStripVisible()) {
     painted_height += browser_view()->tabstrip()->GetPreferredSize().height();
-  } else if (browser_view()->IsToolbarVisible()) {
+  } else if (IsToolbarVisible()) {
     // Paint the header so that it overlaps with the top few pixels of the
     // toolbar because the top few pixels of the toolbar are not opaque.
     const int kToolbarTopEdgeExclusion = 2;
@@ -541,6 +542,11 @@ void BrowserNonClientFrameViewAsh::PaintImmersiveLightbarStyleHeader(
       SK_ColorBLACK);
 }
 
+bool BrowserNonClientFrameViewAsh::IsToolbarVisible() const {
+  return browser_view()->IsToolbarVisible() &&
+      !browser_view()->toolbar()->GetPreferredSize().IsEmpty();
+}
+
 void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
   gfx::Rect toolbar_bounds(browser_view()->GetToolbarBounds());
   if (toolbar_bounds.IsEmpty())
@@ -548,33 +554,36 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
   gfx::Point toolbar_origin(toolbar_bounds.origin());
   View::ConvertPointToTarget(browser_view(), this, &toolbar_origin);
   toolbar_bounds.set_origin(toolbar_origin);
-
-  int x = toolbar_bounds.x();
-  int w = toolbar_bounds.width();
-  int y = toolbar_bounds.y();
-  int h = toolbar_bounds.height();
+  const int h = toolbar_bounds.height();
+  const bool md = ui::MaterialDesignController::IsModeMaterial();
   const ui::ThemeProvider* tp = GetThemeProvider();
+  const SkColor separator_color =
+      tp->GetColor(ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR);
 
-  if (ui::MaterialDesignController::IsModeMaterial()) {
-    if (tp->HasCustomImage(IDR_THEME_TOOLBAR)) {
-      // Paint the main toolbar image.  Since this image is also used to draw
-      // the tab background, we must use the tab strip offset to compute the
-      // image source y position.  If you have to debug this code use an image
-      // editor to paint a diagonal line through the toolbar image and ensure it
-      // lines up across the tab and toolbar.
-      gfx::ImageSkia* theme_toolbar = tp->GetImageSkiaNamed(IDR_THEME_TOOLBAR);
-      canvas->TileImageInt(*theme_toolbar, x + GetThemeBackgroundXInset(),
-                           y - GetTopInset(false), x, y, w,
-                           theme_toolbar->height());
-    } else {
-      canvas->FillRect(toolbar_bounds,
-                       tp->GetColor(ThemeProperties::COLOR_TOOLBAR));
-    }
-
-    // Draw the separator line atop the toolbar, on the left and right of the
-    // tabstrip.
-    // TODO(tdanderson): Draw the separator line for non-tabbed windows.
-    if (browser_view()->IsTabStripVisible()) {
+  if (browser_view()->IsTabStripVisible()) {
+    gfx::ImageSkia* bg = tp->GetImageSkiaNamed(IDR_THEME_TOOLBAR);
+    int x = toolbar_bounds.x();
+    const int y = toolbar_bounds.y();
+    const int bg_y =
+        GetTopInset(false) + Tab::GetYInsetForActiveTabBackground();
+    const int w = toolbar_bounds.width();
+
+    if (md) {
+      if (tp->HasCustomImage(IDR_THEME_TOOLBAR)) {
+        // Paint the main toolbar image.  Since this image is also used to draw
+        // the tab background, we must use the tab strip offset to compute the
+        // image source y position.  If you have to debug this code use an image
+        // editor to paint a diagonal line through the toolbar image and ensure
+        // it lines up across the tab and toolbar.
+        canvas->TileImageInt(*bg, x + GetThemeBackgroundXInset(), y - bg_y, x,
+                             y, w, h);
+      } else {
+        canvas->FillRect(toolbar_bounds,
+                         tp->GetColor(ThemeProperties::COLOR_TOOLBAR));
+      }
+
+      // Separator line above the toolbar, which coincides with the bottom edge
+      // of the tab strip.
       gfx::Rect separator_rect(x, y, w, 0);
       gfx::ScopedCanvas scoped_canvas(canvas);
       gfx::Rect tabstrip_bounds(
@@ -586,80 +595,80 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
       BrowserView::Paint1pxHorizontalLine(
           canvas, tp->GetColor(ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR),
           separator_rect, true);
+    } else {
+      // Gross hack: We split the toolbar images into two pieces, since
+      // sometimes (popup mode) the toolbar isn't tall enough to show the whole
+      // image.  The split happens between the top shadow section and the bottom
+      // gradient section so that we never break the gradient.
+      // NOTE(pkotwicz): If the computation for |bottom_y| is changed, Layout()
+      // must be changed as well.
+      int split_point = kFrameShadowThickness * 2;
+      int bottom_y = y + split_point;
+      int bottom_edge_height = h - split_point;
+
+      canvas->FillRect(gfx::Rect(x, bottom_y, w, bottom_edge_height),
+                       tp->GetColor(ThemeProperties::COLOR_TOOLBAR));
+
+      // Paint the main toolbar image.  Since this image is also used to draw
+      // the tab background, we must use the tab strip offset to compute the
+      // image source y position.  If you have to debug this code use an image
+      // editor to paint a diagonal line through the toolbar image and ensure it
+      // lines up across the tab and toolbar.
+      canvas->TileImageInt(
+          *bg,
+          x + GetThemeBackgroundXInset(),
+          bottom_y - bg_y,
+          x, bottom_y,
+          w, bg->height());
+
+      // 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(), bg->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(), bg->height());
     }
+  }
 
-    // Draw the content/toolbar separator.
-    toolbar_bounds.Inset(kClientEdgeThickness, 0);
-    BrowserView::Paint1pxHorizontalLine(
-        canvas,
-        tp->GetColor(ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR),
-        toolbar_bounds, true);
+  // Draw the toolbar/content separator.
+  toolbar_bounds.Inset(kClientEdgeThickness, h - kClientEdgeThickness,
+                       kClientEdgeThickness, 0);
+  if (md) {
+    canvas->FillRect(toolbar_bounds, SK_ColorWHITE);

[tdanderson, 2016/01/29 22:28:43]

Without line 656, at 2x device scale factor we will have a transparent row of
pixels between the toolbar and the separator line. By filling in with white here
it gives the illusion that the bottom of the toolbar is actually flush with the
separator line.

[Peter Kasting, 2016/01/30 02:53:37]

This is actually a bug on non-Ash as well; I must have regressed this.  Let me
fix this for other platforms, which should likely fix for Ash as well.

[tdanderson, 2016/02/01 22:17:51]

Thanks, I've added a TODO in the next patch set.

+    BrowserView::Paint1pxHorizontalLine(canvas, separator_color,
+                                        toolbar_bounds, true);
   } else {
-    // Gross hack: We split the toolbar images into two pieces, since sometimes
-    // (popup mode) the toolbar isn't tall enough to show the whole image.  The
-    // split happens between the top shadow section and the bottom gradient
-    // section so that we never break the gradient.
-    // NOTE(pkotwicz): If the computation for |bottom_y| is changed, Layout()
-    // must be changed as well.
-    int split_point = kFrameShadowThickness * 2;
-    int bottom_y = y + split_point;
-    int bottom_edge_height = h - split_point;
-
-    canvas->FillRect(gfx::Rect(x, bottom_y, w, bottom_edge_height),
-                     tp->GetColor(ThemeProperties::COLOR_TOOLBAR));
-
-    // Paint the main toolbar image.  Since this image is also used to draw the
-    // tab background, we must use the tab strip offset to compute the image
-    // source y position.  If you have to debug this code use an image editor
-    // to paint a diagonal line through the toolbar image and ensure it lines up
-    // across the tab and toolbar.
-    gfx::ImageSkia* theme_toolbar = tp->GetImageSkiaNamed(IDR_THEME_TOOLBAR);
-    canvas->TileImageInt(
-        *theme_toolbar,
-        x + GetThemeBackgroundXInset(),
-        bottom_y - GetTopInset(false),
-        x, bottom_y,
-        w, theme_toolbar->height());
-
-    // 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 content/toolbar separator.
-    canvas->FillRect(
-        gfx::Rect(x + kClientEdgeThickness,
-                  toolbar_bounds.bottom() - kClientEdgeThickness,
-                  w - (2 * kClientEdgeThickness), kClientEdgeThickness),
-        tp->GetColor(ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR));
+    canvas->FillRect(toolbar_bounds, separator_color);
   }
 }
 
 void BrowserNonClientFrameViewAsh::PaintContentEdge(gfx::Canvas* canvas) {
-  DCHECK(!UsePackagedAppHeaderStyle() && !UseWebAppHeaderStyle());
-  canvas->FillRect(
-      gfx::Rect(0, caption_button_container_->bounds().bottom(), width(),
-                kClientEdgeThickness),
-      GetThemeProvider()->GetColor(
-          ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR));
+  if (browser_view()->IsTabStripVisible())
+    return;
+
+  gfx::Rect separator_rect(
+      0, caption_button_container_->bounds().bottom(), width(), 0);
+  BrowserView::Paint1pxHorizontalLine(

[tdanderson, 2016/01/29 22:28:43]

DefaultHeaderPainter::PaintTitleBar() already paints this separator line
(DefaultHeaderPainter is used when UsePackagedAppHeaderStyle() ||
UseWebAppHeaderStyle() is true). So line 670 looks to be redundant in such
cases, but without it I am sometimes noticing incomplete rendering of the line
(until I force a repaint by maximizing/unmaximizing or resizing the window).

[Peter Kasting, 2016/01/30 02:53:37]

I would try to figure out why this isn't the case before your change, where that
was the only thing to render this line and there was no redundancy.

[tdanderson, 2016/02/01 22:17:51]

Fixed this in the next patch set.

+      canvas, GetThemeProvider()->GetColor(
+          ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR),
+      separator_rect, true);
 }