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..6c36b2765fc7236160242c94548bfbc555e4f4e4 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,11 @@ 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 -
+        (ui::MaterialDesignController::IsModeMaterial() ?
+            0 : kContentShadowHeight);
+  }
 
   return caption_buttons_bottom + kClientEdgeThickness;
 }
@@ -306,9 +310,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())
+  else
     PaintContentEdge(canvas);
 }
 
@@ -321,7 +326,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 +546,12 @@ void BrowserNonClientFrameViewAsh::PaintImmersiveLightbarStyleHeader(
       SK_ColorBLACK);
 }
 
+bool BrowserNonClientFrameViewAsh::IsToolbarVisible() const {
+  return browser_view()->IsToolbarVisible() &&
+         (ui::MaterialDesignController::IsModeMaterial() ?

[Peter Kasting, 2016/01/26 03:04:39]

Why do you need to do an MD check here?  The other frame views' versions of this
function don't.

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

Changed in next patch set.

+             !browser_view()->toolbar()->GetPreferredSize().IsEmpty() : true);
+}
+
 void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {

[Peter Kasting, 2016/01/26 03:04:39]

I'd like to see you refactor these next two functions to be of the same basic
structure/order as the current glass and opaque implementations.  In the process
of doing this, you may want to revert your change to
ToolbarView::PopupTopSpacing(); I haven't debugged locally on Windows yet
(checkout out of date) to be certain.

In particular you'll want to look at https://codereview.chromium.org/1455193003
, though that's not the only difference between this code and those files.

With these changes, the ordering in this function makes a check for "is the
tabstrip visible" first.  This results in not drawing a bunch of stuff for popup
windows, e.g. the toolbar background.  How much you'll need in PaintClientEdge()
isn't totally clear to me as I haven't looked closely at how those other
implementations do things; from glancing at your current implementation here, it
seems like you don't need the side and bottom edges, just the top, so maybe
something close to the simplicity of your current implementation works.  But
note that without doing any of this refactoring, the structure you have set up
in this implementation draws the top separator in both these two functions for
MD, which is a clear sign something isn't right. 

Doing this should fix the problem you're having with 2x popup windows in
customized themes, since those implementations explicitly check if the tabstrip
is visible, and if not, don't paint the toolbar background (among other things).

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

I have made an effort to do so in the next patch set, but some problems remain
(see my inline comments).

   gfx::Rect toolbar_bounds(browser_view()->GetToolbarBounds());
   if (toolbar_bounds.IsEmpty())
@@ -549,10 +560,10 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
   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 x = toolbar_bounds.x();
+  const int w = toolbar_bounds.width();
+  const int y = toolbar_bounds.y();
+  const int h = toolbar_bounds.height();
   const ui::ThemeProvider* tp = GetThemeProvider();
 
   if (ui::MaterialDesignController::IsModeMaterial()) {
@@ -571,22 +582,21 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
                        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.
+    // Draw the separator line atop the toolbar.
+    gfx::Rect separator_rect(
+        x, caption_button_container_->bounds().bottom(), w, 0);
+    gfx::ScopedCanvas scoped_canvas(canvas);

[Peter Kasting, 2016/01/26 03:04:39]

Why did you hoist this statement?

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

Changed in next patch set.

     if (browser_view()->IsTabStripVisible()) {
-      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);
     }
+    BrowserView::Paint1pxHorizontalLine(
+        canvas, tp->GetColor(ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR),
+        separator_rect, true);
 
     // Draw the content/toolbar separator.
     toolbar_bounds.Inset(kClientEdgeThickness, 0);
@@ -656,10 +666,18 @@ void BrowserNonClientFrameViewAsh::PaintToolbarBackground(gfx::Canvas* canvas) {
 }
 
 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 (ui::MaterialDesignController::IsModeMaterial()) {
+    gfx::Rect separator_rect(
+        0, caption_button_container_->bounds().bottom(), width(), 0);
+    BrowserView::Paint1pxHorizontalLine(
+        canvas, GetThemeProvider()->GetColor(
+            ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR),
+        separator_rect, true);
+  } else if (!UsePackagedAppHeaderStyle() && !UseWebAppHeaderStyle()) {
+    canvas->FillRect(
+        gfx::Rect(0, caption_button_container_->bounds().bottom(), width(),
+                  kClientEdgeThickness),
+        GetThemeProvider()->GetColor(
+            ThemeProperties::COLOR_TOOLBAR_TOP_SEPARATOR));
+  }
 }