Views/Harmony: Use LayoutDelegate::GetMetric for views::kPanelVertMargin.

chrome/browser/ui/views/sad_tab_view.cc

Index: chrome/browser/ui/views/sad_tab_view.cc
diff --git a/chrome/browser/ui/views/sad_tab_view.cc b/chrome/browser/ui/views/sad_tab_view.cc
index 3082cb97126a4d4c643ab7681b751588468cf737..3076feff51e7f1c598d38273dfa7b6d7f2092c93 100644
--- a/chrome/browser/ui/views/sad_tab_view.cc
+++ b/chrome/browser/ui/views/sad_tab_view.cc
@@ -9,6 +9,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "chrome/app/vector_icons/vector_icons.h"
+#include "chrome/browser/ui/views/harmony/layout_delegate.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -79,7 +80,10 @@ SadTabView::SadTabView(content::WebContents* web_contents,
   message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   message_->SetLineHeight(views::kPanelSubVerticalSpacing);
 
-  layout->StartRowWithPadding(0, column_set_id, 0, views::kPanelVertMargin);
+  layout->StartRowWithPadding(
+      0, column_set_id, 0,
+      LayoutDelegate::Get()->GetMetric(
+          LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));

[tapted, 2017/02/27 03:24:51]

I don't think it makes sense for SadTabView to be using panel constants (it's
not a panel/dialog). It's probably just coincidence that the value matches.. IMO
this should stay a constant, but it should be redefined up in sad_tab_view.cc's
anonymous namespace.

[Peter Kasting, 2017/02/28 02:21:39]

Hmm.

I agree this isn't a panel, but I kinda see "panel" in the layout constant name
here as potentially transitionary.  It seems like it might be OK to use
so-called "panel" constants for non-panel things if our hope is to eventually
not call them "panel" (as mine is).  Then again, maybe my hope is misguided.

In this case, using kPanelVertMargin at all, by either name, seems questionable.
 This is the vertical padding between "Aw, snap!" and "Something went wrong". 
Basically, it's the distance between a section title and its content.  I wonder
if it makes sense to space this one the way we space dialog titles vs. their
content (which in Harmony would be 8 DIPs).  Not sure how this is implemented
today.  RELATED_CONTROL_VERTICAL_SPACING?

[Patti Lor, 2017/03/03 08:24:39]

Had a look through all the metrics in LayoutDelegate and I don't think any fits
:/ SUBSECTION_HORIZONTAL_INDENT seems to suggest it would be more appropriately
named SUBSECTION_VERTICAL_SPACING or something like that (rather than something
with "CONTROL" in it) D: Since that doesn't exist, I've just done as Trent's
suggested, I think that's probably best.

Thanks both!

[Peter Kasting, 2017/03/04 02:10:55]

Well, "subsection" is supposed to refer to something like an indented list. 
This wouldn't be a "subsection", it'd simply be the body of content, below the
title.  I don't know what constant is used to space that out in dialogs -- I'm
having trouble tracing through the dialog code to figure out which inset/padding
value is used for this -- maybe Trent would know?  I'm wondering how I was so
confident in saying 8 DIPs before, now I can't even find that in the spec.

My worry about using anonymous constants is that, by the end of Harmony, we want
to have zero of these, or as nearly zero as possible.  So using one is basically
a way of punting this issue down the road instead of dealing with it, and making
it easier to miss it on future passes through here.

That doesn't really solve your problem, it's just an encouragement to try to
keep digging here.  Maybe this should be governed by the typography spec and the
amount of leading it prescribes?  Trent is the one working on that stuff so I'll
let him comment.

[tapted, 2017/03/06 02:20:32]

I think this would be derived from BubbleFrameView's constructor arguments:

  BubbleFrameView(const gfx::Insets& title_margins,
                  const gfx::Insets& content_margins);

padding between title/content would be title_margins.bottom() +
content_margins.top()


Which is...

NonClientFrameView* DialogDelegate::CreateDialogFrameView(
    Widget* widget,
    const gfx::Insets& content_margins) {
  BubbleFrameView* frame = new BubbleFrameView(
      ViewsDelegate::GetInstance()->GetDialogFrameViewInsets(),
      content_margins);

( Which is...

NonClientFrameView* DialogDelegate::CreateNonClientFrameView(Widget* widget) {
  if (ShouldUseCustomFrame())
    return CreateDialogFrameView(widget, gfx::Insets());

OR

views::NonClientFrameView* ChooserDialogView::CreateNonClientFrameView(
    views::Widget* widget) {
  // ChooserDialogView always has a parent, so ShouldUseCustomFrame() should
  // always be true.
  DCHECK(ShouldUseCustomFrame());
  return views::DialogDelegate::CreateDialogFrameView(
      widget, gfx::Insets(LayoutDelegate::Get()->GetMetric(
                  LayoutDelegate::Metric::PANEL_CONTENT_MARGIN)));

)

OR

NonClientFrameView* BubbleDialogDelegateView::CreateNonClientFrameView(
    Widget* widget) {
  BubbleFrameView* frame = new BubbleFrameView(title_margins_, margins_);

)

Which is...

  margins_ = views_delegate ? views_delegate->GetBubbleDialogMargins()
                            : gfx::Insets(kPanelVertMargin, kPanelHorizMargin);

  title_margins_ = views_delegate
                       ? views_delegate->GetDialogFrameViewInsets()
                       : gfx::Insets(kPanelVertMargin, kPanelHorizMargin, 0,
                                     kPanelHorizMargin);

OR

void BubbleDialogDelegateView::UseCompactMargins() {
  const int kCompactMargin = 6;
  margins_.Set(kCompactMargin, kCompactMargin, kCompactMargin, kCompactMargin);
}

OR calls to 

  void BubbleDialogDelegateView::set_margins(const gfx::Insets& margins) {
margins_ = margins; }



Let's assume views_delegate is non-null, and ignore ChooserDialogView.


non-bubble Dialogs then always have gfx::Insets() for content margins.
ViewsDelegate::GetDialogFrameViewInsets() is then used for title_margins. which
has

  // Titles are inset at the top and sides, but not at the bottom.
  return gfx::Insets(top, side, 0, side);

So. Zero?  I guess this relies on the contents setting an inset of some kind
each time.



for bubble Dialogs, title_margins_ always seems to have .bottom() == 0.

ViewsDelegate::GetBubbleDialogMargins() has

gfx::Insets ChromeViewsDelegate::GetBubbleDialogMargins() const {
  return gfx::Insets(LayoutDelegate::Get()->GetMetric(
      LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
}

So PANEL_CONTENT_MARGIN on all sides, which is `return
views::kPanelHorizMargin;` (13) or kHarmonyLayoutUnit (16)
The "Aw Snap!" title is doing  
title_->SetFontList(rb.GetFontList(ui::ResourceBundle::LargeFont)) 20pt which is
"Headline" in the spec which... doesn't have a "Leading" specified at
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...

I think we'll need to punt this for a specific UI review. There aren't too many
places in Chrome using ui::ResourceBundle::LargeFont `git grep` says there are
6:
 3 are ash only
 1 is just to draw a single letter on a placeholder extension icon
 1 is omnibox/answers in suggest
 1 is SadTabView

So it seems SadTabView is the only dialog-like thing that would be using
Headline. A constant here makes sense to me, until we get hear what the leading
should be from UX.

   layout->AddView(message_, 2, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);