Implement MD specs for non-tabbed windows in Ash

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

 // Copyright 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.
 
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h"
 
 #include <algorithm>
 
 #include "ash/ash_layout_constants.h"
 #include "ash/frame/caption_buttons/frame_caption_button_container_view.h"
@@ skipping 11 matching lines @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/frame/browser_frame.h"
 #include "chrome/browser/ui/views/frame/browser_header_painter_ash.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
 #include "chrome/browser/ui/views/frame/web_app_left_header_view_ash.h"
 #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"
 #include "extensions/browser/extension_registry.h"
 #include "grit/theme_resources.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
 #include "ui/base/hit_test.h"
 #include "ui/base/layout.h"
@@ skipping 140 matching lines @@
     return ((frame()->IsMaximized() || frame()->IsFullscreen()) && !restored) ?
         kTabstripTopSpacingShort : kTabstripTopSpacingTall;
   }
 
   if (UsePackagedAppHeaderStyle() || UseWebAppHeaderStyle())
     return header_painter_->GetHeaderHeight();
 
   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;
 }
 
 int BrowserNonClientFrameViewAsh::GetThemeBackgroundXInset() const {
   return ash::HeaderPainterUtil::GetThemeBackgroundXInset();
 }
 
 void BrowserNonClientFrameViewAsh::UpdateThrobber(bool running) {
   if (window_icon_)
@@ skipping 95 matching lines @@
     return;
   }
 
   caption_button_container_->SetPaintAsActive(ShouldPaintAsActive());
   if (web_app_left_header_view_)
     web_app_left_header_view_->SetPaintAsActive(ShouldPaintAsActive());
 
   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() {
   // The header must be laid out before computing |painted_height| because the
   // computation of |painted_height| for app and popup windows depends on the
   // position of the window controls.
   header_painter_->LayoutHeader();
 
   int painted_height = GetTopInset(false);
-  if (browser_view()->IsTabStripVisible()) {
+  if (browser_view()->IsTabStripVisible())
     painted_height += browser_view()->tabstrip()->GetPreferredSize().height();
-  } else if (browser_view()->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;
-    painted_height += kToolbarTopEdgeExclusion;
-  }
+
   header_painter_->SetHeaderHeightForPainting(painted_height);
 
   if (avatar_button()) {
     LayoutAvatar();
     header_painter_->UpdateLeftViewXInset(avatar_button()->bounds().right());
   }
   header_painter_->UpdateLeftViewXInset(
       ash::HeaderPainterUtil::GetDefaultLeftViewXInset());
   BrowserNonClientFrameView::Layout();
 }
@@ skipping 194 matching lines @@
 }
 
 void BrowserNonClientFrameViewAsh::PaintImmersiveLightbarStyleHeader(
     gfx::Canvas* canvas) {
   // The light bar header is not themed because theming it does not look good.
   canvas->FillRect(
       gfx::Rect(width(), header_painter_->GetHeaderHeightForPainting()),
       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())
     return;
   gfx::Point toolbar_origin(toolbar_bounds.origin());
   View::ConvertPointToTarget(browser_view(), this, &toolbar_origin);
   toolbar_bounds.set_origin(toolbar_origin);
+  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);
 
-  int x = toolbar_bounds.x();
-  int w = toolbar_bounds.width();
-  int y = toolbar_bounds.y();
-  int h = toolbar_bounds.height();
-  const ui::ThemeProvider* tp = GetThemeProvider();
+  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 (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));
-    }
+    if (md) {
+      if (tp->HasCustomImage(IDR_THEME_TOOLBAR)) {
+        // Paint the main toolbar image.  Since this image is also used to draw

[Peter Kasting, 2016/02/03 01:09:23]

It would be nice to make this line up even more closely with the other frames'
implementations in terms of comments, variable names, etc., but I guess that
could be done as a separate pass.

[tdanderson, 2016/02/04 16:38:36]

I think the variable names here are already identical to the ones used for the
opaque frame class (at least for the MD case - I didn't make any additional
modifications to the pre-MD code).

+        // 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));
+      }
 
-    // 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()) {
+      // 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(
           GetBoundsForTabStrip(browser_view()->tabstrip()));
       tabstrip_bounds.set_x(GetMirroredXForRect(tabstrip_bounds));
       canvas->sk_canvas()->clipRect(gfx::RectToSkRect(tabstrip_bounds),
                                     SkRegion::kDifference_Op);
       separator_rect.set_y(tabstrip_bounds.bottom());
       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) {
+    // On devices having a device scale factor other than 1, a transparent gap
+    // will appear along the bottom edge of the location bar. Fill this region
+    // with white so it appears that the location bar extends to the bottom
+    // separator line.
+    // TODO(pkasting|tdanderson): Remove this hack.
+    canvas->FillRect(toolbar_bounds, SK_ColorWHITE);

[Peter Kasting, 2016/02/03 01:09:23]

Doesn't this look gruesomely bad in incognito?

I would probably just leave this out.  The transparent line is present on all
platforms and I'm fixing that, so working around it for now doesn't seem very
useful anyway.

[tdanderson, 2016/02/04 16:38:36]

OK, removed. Is there a bug for that?

+
+    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;
+
+  // The content separator is drawn by DefaultHeaderPainter in these cases.
+  if (UsePackagedAppHeaderStyle() || UseWebAppHeaderStyle())
+    return;
+
+  gfx::Rect separator_rect(
+      0, caption_button_container_->bounds().bottom(), width(), 0);
+  BrowserView::Paint1pxHorizontalLine(
+      canvas, GetThemeProvider()->GetColor(
+          ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR),
+      separator_rect, true);

[Peter Kasting, 2016/02/03 01:09:23]

It seems like this is drawing the same bottom separator that PaintToolbar() is
drawing.  One of these should be drawing the popup-mode top separator instead.

[tdanderson, 2016/02/04 16:38:36]

Thanks for catching that, fixed.

 }