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

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

Replace direct references to views::kPanelHorizMargin under
chrome/browser/ui/views/* with a call to LayoutDelegate::GetMetric().

BUG=691897
Review-Url: https://codereview.chromium.org/2713803002
Cr-Commit-Position: refs/heads/master@{#455372}
Committed: https://chromium.googlesource.com/chromium/src/+/e05be844b2562f3c35ada23f782d2c1aea19adf0

[Patti Lor, 2017-02-23 06:53:38]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 06:53:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 07:06:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 07:06:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-02-23 23:11:51]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 23:12:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:49:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:49:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-02-24 02:06:02]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 02:06:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 02:47:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 02:47:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-02-24 03:11:31]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 03:12:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 03:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 03:26:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-02-24 06:00:24]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 06:00:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 07:28:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 07:28:06]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-26 23:15:30]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2017-02-26 23:15:31]

Hi Trent, PTAL. Again, I've missed stuff under
chrome/browser/{chromeos,extensions,ui/ash}/, and ui/views/.

Thanks!

[tapted, 2017-02-27 03:24:50]

tapted@chromium.org changed reviewers:
+ pkasting@chromium.org

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

Main thing from below, I think we should a `BoxLayout*
LayoutDelegate::CreatePanelBoxLayout(..)` method

looping in pkasting in case he has other ideas :) (you'll need more OWNERS in
any case)

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:139:
views::BoxLayout::kVertical, views::kButtonHEdgeMarginNew,
Does it make sense to move views::kButtonHEdgeMarginNew as well? (if [yes], I
think it's good to include on this CL). E.g. would it be PANEL_CONTENT_MARGIN?

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/importer/import_lock_dialog_view.cc:64:
bounds.Inset(views::kButtonHEdgeMargin,
This should probably be using the "new" edge margin (i.e. I don't think this
dialog is a special snowflake - it should use the same margins as everything
else.. also it should probably be using a LayoutManager, but that's orthogonal).

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/sad_tab_view.cc:86:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
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.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:87:
panel_margin, views::kRelatedControlVerticalSpacing);
Could we have a 

BoxLayout* LayoutDelegate::CreatePanelBoxLayout(..)

(i.e. similar to GridLayout::CreatePanel)

(we'll probably also want LayoutDelegate::CreatePanelGridLayout..)

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:216:
layout->AddPaddingRow(1,
This may have been coincidence too.. I think we should put the constant into
`kHeaderPaddingBottom` above so it's consistent with kHeaderMarginBottom

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:444:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
This only affects non-material, so I'm not sure it should change here (i.e. the
offset here really should be a compile-time constant, and this code should be
deleted when harmony is default)

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

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:139:
views::BoxLayout::kVertical, views::kButtonHEdgeMarginNew,
On 2017/02/27 03:24:51, tapted wrote:
> Does it make sense to move views::kButtonHEdgeMarginNew as well? (if [yes], I
> think it's good to include on this CL). E.g. would it be PANEL_CONTENT_MARGIN?

It's DIALOG_BUTTON_MARGIN.  The one below is UNRELATED_CONTROL_VERTICAL_SPACING.
 Many places in other touched files could also be converted.  I've been trying
to leave it up to Patti how to do this conversions and split them among CLs. 
It's fine by me to convert them in larger or smaller groups as desired.

I'd personally tend to convert directory-by-directory rather than
constant-by-constant, just because it makes it most obvious when things like the
LayoutDelegate itself should be pulled out to a temp.  But that preference need
not dictate this work.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/importer/import_lock_dialog_view.cc:64:
bounds.Inset(views::kButtonHEdgeMargin,
On 2017/02/27 03:24:51, tapted wrote:
> This should probably be using the "new" edge margin (i.e. I don't think this
> dialog is a special snowflake - it should use the same margins as everything
> else.. also it should probably be using a LayoutManager, but that's
orthogonal).

+1 to all this

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/sad_tab_view.cc:86:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
On 2017/02/27 03:24:51, tapted wrote:
> 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.

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?

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:87:
panel_margin, views::kRelatedControlVerticalSpacing);
On 2017/02/27 03:24:51, tapted wrote:
> Could we have a 
> 
> BoxLayout* LayoutDelegate::CreatePanelBoxLayout(..)
> 
> (i.e. similar to GridLayout::CreatePanel)
> 
> (we'll probably also want LayoutDelegate::CreatePanelGridLayout..)

https://bugs.chromium.org/p/chromium/issues/detail?id=687340 is about trying to
figure out the grid layout stuff for panels, let's not create a third place to
unify into there :).

My inclination is to do this as a BoxLayout member, if it's desirable.  It's
sort of "BoxLayout-specific" more than it is "LayoutDelegate-specific".

As to whether it's desirable, I don't know.  How often will it be used?  It
seems like a lot of places in this CL are each doing a one-off thing.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:216:
layout->AddPaddingRow(1,
On 2017/02/27 03:24:51, tapted wrote:
> This may have been coincidence too.. I think we should put the constant into
> `kHeaderPaddingBottom` above so it's consistent with kHeaderMarginBottom

I don't have a strong opinion on this one.  It seems like all this layout and
these constants will need rethinking for Harmony anyway, so whichever way we do
it we're likely to have to revisit this.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:444:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
On 2017/02/27 03:24:51, tapted wrote:
> This only affects non-material, so I'm not sure it should change here (i.e.
the
> offset here really should be a compile-time constant, and this code should be
> deleted when harmony is default)

I'm fine with changing this anyway, because the goal is to delete these views
constants entirely, and that may happen before we remove this code.

[Patti Lor, 2017-03-03 06:55:09]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 06:55:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-03-03 06:58:21]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-03 06:59:06]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Patti Lor, 2017-03-03 07:00:48]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 07:01:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2017-03-03 07:32:30]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 07:32:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 08:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 08:10:12]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-03-03 08:24:40]

Apologies for the delay - PTAL!

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:139:
views::BoxLayout::kVertical, views::kButtonHEdgeMarginNew,
On 2017/02/28 02:21:39, Peter Kasting wrote:
> On 2017/02/27 03:24:51, tapted wrote:
> > Does it make sense to move views::kButtonHEdgeMarginNew as well? (if [yes],
I
> > think it's good to include on this CL). E.g. would it be
PANEL_CONTENT_MARGIN?
> 
> It's DIALOG_BUTTON_MARGIN.  The one below is
UNRELATED_CONTROL_VERTICAL_SPACING.
>  Many places in other touched files could also be converted.  I've been trying
> to leave it up to Patti how to do this conversions and split them among CLs. 
> It's fine by me to convert them in larger or smaller groups as desired.
> 
> I'd personally tend to convert directory-by-directory rather than
> constant-by-constant, just because it makes it most obvious when things like
the
> LayoutDelegate itself should be pulled out to a temp.  But that preference
need
> not dictate this work.

Leaving as-is (at least for this CL) - but yeah directory by directory is
probably easier. I've just been running sed on everything under chrome/, but
with all the manual work that still needs to be put in for each replacement it's
probably the same amount of time or faster to do dir-by-dir. Thanks for the
advice!

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/importer/import_lock_dialog_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/importer/import_lock_dialog_view.cc:64:
bounds.Inset(views::kButtonHEdgeMargin,
On 2017/02/27 03:24:51, tapted wrote:
> This should probably be using the "new" edge margin (i.e. I don't think this
> dialog is a special snowflake - it should use the same margins as everything
> else.. also it should probably be using a LayoutManager, but that's
orthogonal).

Done.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/sad_tab_view.cc:86:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
On 2017/02/28 02:21:39, Peter Kasting wrote:
> On 2017/02/27 03:24:51, tapted wrote:
> > 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.
> 
> 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?

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!

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:87:
panel_margin, views::kRelatedControlVerticalSpacing);
On 2017/02/28 02:21:39, Peter Kasting wrote:
> On 2017/02/27 03:24:51, tapted wrote:
> > Could we have a 
> > 
> > BoxLayout* LayoutDelegate::CreatePanelBoxLayout(..)
> > 
> > (i.e. similar to GridLayout::CreatePanel)
> > 
> > (we'll probably also want LayoutDelegate::CreatePanelGridLayout..)
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=687340 is about trying
to
> figure out the grid layout stuff for panels, let's not create a third place to
> unify into there :).
> 
> My inclination is to do this as a BoxLayout member, if it's desirable.  It's
> sort of "BoxLayout-specific" more than it is "LayoutDelegate-specific".
> 
> As to whether it's desirable, I don't know.  How often will it be used?  It
> seems like a lot of places in this CL are each doing a one-off thing.

Did a quick grep for places creating new BoxLayouts - I think a lot of places
are creating them with 0s for all the spacing arguments, so not sure if that
would be used a lot in places that want default panel margins. Because of that
I've left it as is, so if either of you feels strongly about it let me know -
I'm happy to defer to you both on this.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:216:
layout->AddPaddingRow(1,
On 2017/02/27 03:24:51, tapted wrote:
> This may have been coincidence too.. I think we should put the constant into
> `kHeaderPaddingBottom` above so it's consistent with kHeaderMarginBottom

Done.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:216:
layout->AddPaddingRow(1,
On 2017/02/28 02:21:39, Peter Kasting wrote:
> On 2017/02/27 03:24:51, tapted wrote:
> > This may have been coincidence too.. I think we should put the constant into
> > `kHeaderPaddingBottom` above so it's consistent with kHeaderMarginBottom
> 
> I don't have a strong opinion on this one.  It seems like all this layout and
> these constants will need rethinking for Harmony anyway, so whichever way we
do
> it we're likely to have to revisit this.

Acknowledged.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:444:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN),
On 2017/02/28 02:21:39, Peter Kasting wrote:
> On 2017/02/27 03:24:51, tapted wrote:
> > This only affects non-material, so I'm not sure it should change here (i.e.
> the
> > offset here really should be a compile-time constant, and this code should
be
> > deleted when harmony is default)
> 
> I'm fine with changing this anyway, because the goal is to delete these views
> constants entirely, and that may happen before we remove this code.

Acknowledged, left as is.

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

LGTM with handwavey comment.

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/sad_tab_view.cc:86:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
On 2017/03/03 08:24:39, Patti Lor wrote:
> On 2017/02/28 02:21:39, Peter Kasting wrote:
> > On 2017/02/27 03:24:51, tapted wrote:
> > > 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.
> > 
> > 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?
> 
> 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!

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]

lgtm

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2713803002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/sad_tab_view.cc:86:
LayoutDelegate::Metric::PANEL_CONTENT_MARGIN));
On 2017/03/04 02:10:55, Peter Kasting wrote:
> On 2017/03/03 08:24:39, Patti Lor wrote:
> > On 2017/02/28 02:21:39, Peter Kasting wrote:
> > > On 2017/02/27 03:24:51, tapted wrote:
> > > > 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.
> > > 
> > > 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?
> > 
> > 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!
> 
> 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 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)


>  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.

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.

[Patti Lor, 2017-03-06 22:52:30]

Thanks Trent for doing all that investigation! I'll land it as is then :)

[Patti Lor, 2017-03-06 22:58:53]

The CQ bit was checked by patricialor@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:59:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 23:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 23:44:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Patti Lor, 2017-03-07 00:28:13]

The CQ bit was checked by patricialor@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:29:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 03:03:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 03:03:38]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Patti Lor, 2017-03-07 06:16:08]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-03-07 06:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2713803002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-07 06:17:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 08:20:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 08:20:41]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Patti Lor, 2017-03-08 02:04:37]

The CQ bit was checked by patricialor@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:05:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 03:27:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488938677208060,
"parent_rev": "ebadc5308f4a935c6e0ddec6fbb466e202913e30", "commit_rev":
"e05be844b2562f3c35ada23f782d2c1aea19adf0"}

[commit-bot: I haz the power, 2017-03-08 03:28:25]

Description was changed from

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

Replace direct references to views::kPanelHorizMargin under
chrome/browser/ui/views/* with a call to LayoutDelegate::GetMetric().

BUG=691897
==========

to

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

Replace direct references to views::kPanelHorizMargin under
chrome/browser/ui/views/* with a call to LayoutDelegate::GetMetric().

BUG=691897

Review-Url: https://codereview.chromium.org/2713803002
Cr-Commit-Position: refs/heads/master@{#455372}
Committed:
https://chromium.googlesource.com/chromium/src/+/e05be844b2562f3c35ada23f782d...
==========

[commit-bot: I haz the power, 2017-03-08 03:28:26]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e05be844b2562f3c35ada23f782d...