Implement MD specs for non-tabbed windows in Ash

chrome/browser/ui/views/toolbar/toolbar_view.cc

 // Copyright 2013 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/toolbar/toolbar_view.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/i18n/number_formatting.h"
@@ skipping 670 matching lines @@
 #if defined(OS_WIN)
       ConflictingModuleView::MaybeShow(browser_, app_menu_button_);
 #endif
     }
     incompatibility_badge_showing = true;
     return;
   }
 }
 
 int ToolbarView::PopupTopSpacing() const {
+  if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)

[Peter Kasting, 2016/02/10 07:23:40]

Oh, BTW, HOST_DESKTOP_TYPE_ASH is going away as we remove Metro mode Ash. 
Instead, use #if defined(OS_CHROMEOS) here.

It turns out I have to do this anyway:
https://codereview.chromium.org/1682373002

You should just modify my code so that instead of returning
kCLientEdgeThickness, you return 0.

[Peter Kasting, 2016/02/10 07:46:04]

(Which is in fact precisely what https://codereview.chromium.org/1685763004/
does.  However, I planned to let you land this patch before I landed that one,
since otherwise I'll break Ash between the two patches; instead, after you land,
I'll add the Ash changes necessary to that CR.)

[tdanderson, 2016/02/10 18:22:30]

Acknowledged.

+    return 0;

[Peter Kasting, 2016/02/10 09:12:57]

Actually, the more I think about this, the less sure I am that it's correct.

You do, in fact, reserve 2 px atop the toolbar, inside its bounds, for "shadow
above the location bar", which is what the reservation below here is for.  This
is also what's leading to your popup mode unittest size changing versus the
expectation.  Doesn't this cause the location bar itself to be 2 px too short? 
Why do we not need to reserve any space here?

[tdanderson, 2016/02/10 18:22:30]

In my understanding, we're not (and don't want to) reserve this extra padding
for a shadow in the case of a popup/app window (at least on Ash). The header,
toolbar, and contents should all be flush on top of one another, and adding in
the extra padding just creates a gap between the header and the toolbar.

I'm attaching screenshots on crbug.com/580302 to show you what each window type
looks like with no extra top spacing (like what is done here) and what it would
look like if I were to delete lines 691-692.

With the CL as-is, the toolbar (less borders) is the same height for tabbed
browser windows, popups, and app windows (25px for non-material, 26px for
material, and 30px for hybrid).

[Peter Kasting, 2016/02/10 23:46:21]

OK, reading your code again, I understand what's happening here. The gap here is
because of the code you deleted in BrowserNonClientFrameViewAsh::Layout().

We could use the 2 px gap here and restore the code there, then draw the 2 px of
shadow in the popup case.  But that would be inconsistent with how the other
frames handle popups, where the location bar doesn't have a shadow above it.  So
I think the changes you've made are correct.

This suggests that I need to refactor this code to do something similar for
opaque frames: don't add the 2 px here, and then in the opaque frame layout
code, ensure we expand the above-toolbar height by 2 px for popups.

[tdanderson, 2016/02/11 16:10:21]

Acknowledged.

+
   const int kAdditionalPopupTopSpacingNonGlass = 2;
   return views::NonClientFrameView::kClientEdgeThickness +
       (GetWidget()->ShouldWindowContentsBeTransparent() ?
           0 : kAdditionalPopupTopSpacingNonGlass);
 }
 
 gfx::Size ToolbarView::GetSizeInternal(
     gfx::Size (View::*get_size)() const) const {
   gfx::Size size((location_bar_->*get_size)());
   if (is_display_mode_normal()) {
@@ skipping 27 matching lines @@
       int content_height = std::max(back_->GetPreferredSize().height(),
                                     location_bar_->GetPreferredSize().height());
       int padding = GetLayoutInsets(TOOLBAR).height();
       size.SetToMax(gfx::Size(0, content_height + padding));
     } else {
       gfx::ImageSkia* normal_background =
           GetThemeProvider()->GetImageSkiaNamed(IDR_CONTENT_TOP_CENTER);
       size.SetToMax(
           gfx::Size(0, normal_background->height() - content_shadow_height()));
     }
-  } else if (size.height() == 0) {
-    // Location mode with a 0 height location bar. If on ash, expand by one
-    // pixel to show a border in the title bar, otherwise leave the size as zero
-    // height.
-    const int kAshBorderSpacing = 1;
-    if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
-      size.Enlarge(0, kAshBorderSpacing);
-  } else {
+  } else if (size.height() > 0) {
     size.Enlarge(
         0, PopupTopSpacing() + views::NonClientFrameView::kClientEdgeThickness);
   }
   return size;
 }
 
 void ToolbarView::LoadImages() {
   const ui::ThemeProvider* tp = GetThemeProvider();
 
   if (ui::MaterialDesignController::IsModeMaterial()) {
@@ skipping 55 matching lines @@
 void ToolbarView::OnShowHomeButtonChanged() {
   Layout();
   SchedulePaint();
 }
 
 int ToolbarView::content_shadow_height() const {
   return GetLayoutConstant(
       (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH) ?
           TOOLBAR_CONTENT_SHADOW_HEIGHT_ASH : TOOLBAR_CONTENT_SHADOW_HEIGHT);
 }