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 -
+        (ui::MaterialDesignController::IsModeMaterial() ?
+            0 : kContentShadowHeight);
+  }
 
   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())
+  else
     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()) {
     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;
     painted_height += kToolbarTopEdgeExclusion;
   }
   header_painter_->SetHeaderHeightForPainting(painted_height);
 
   if (avatar_button()) {
     LayoutAvatar();
     header_painter_->UpdateLeftViewXInset(avatar_button()->bounds().right());
@@ skipping 199 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() &&
+         (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())
     return;
   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 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()) {
     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.
+    // 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);
     BrowserView::Paint1pxHorizontalLine(
         canvas,
         tp->GetColor(ThemeProperties::COLOR_TOOLBAR_BOTTOM_SEPARATOR),
         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
@@ skipping 49 matching lines @@
     // 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));
   }
 }
 
 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));
+  }
 }