Delete panel metrics and define insets in terms of distance metrics.

Renamed INSETS_PANEL to INSETS_DIALOG_CONTENTS. Renamed
INSETS_DIALOG_BUTTON to INSETS_DIALOG_BUTTON_ROW, and made it have a 0
top inset in Harmony to fix the double-wide margin between the
content and button row.

Defined most insets metrics in terms of distance metrics, and removed
their overrides from HarmonyLayoutProvider since they would now be
redundant.

Renamed DISTANCE_DIALOG_BUTTON_MARGIN to
DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN. Some callsites were using this
metric to mean "dialog content left/right margin" and those were
changed to use a new metric, DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.

Split DISTANCE_PANEL_CONTENT_MARGIN into three different metrics
depending on what the callsite actually meant (though all three have the
same ultimate value):
* DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN
* DISTANCE_BUBBLE_CONTENTS_VERTICAL_MARGIN
* DISTANCE_DIALOG_CONTENTS_VERTICAL_MARGIN

Opportunistically changed some usages of layout_constants.cc constants
to LayoutProvider metrics.

BUG=716222
Review-Url: https://codereview.chromium.org/2888563004
Cr-Commit-Position: refs/heads/master@{#474135}
Committed: https://chromium.googlesource.com/chromium/src/+/dd45e5f7692fe435bfaa61981ecf56724ab4e31a

[Bret, 2017-05-17 01:16:20]

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

[commit-bot: I haz the power, 2017-05-17 01:17:58]

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-05-17 01:55:25]

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

[commit-bot: I haz the power, 2017-05-17 01:55:25]

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

[Bret, 2017-05-17 19:01:59]

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

[commit-bot: I haz the power, 2017-05-17 19:05:29]

Dry run: CQ is trying da patch.

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

[Bret, 2017-05-17 21:25:30]

Description was changed from

==========
TODO: better description

BUG=716222
==========

to

==========
Renamed INSETS_PANEL to INSETS_DIALOG_CONTENTS. Renamed
INSETS_DIALOG_BUTTON to INSETS_DIALOG_BUTTON_ROW, and made it have a 0
top inset in Harmony to fix the double-wide margin between the
content and button row.

Defined most insets metrics in terms of distance metrics, and removed
their overrides from HarmonyLayoutProvider since they would now be
redundant.

Renamed DISTANCE_DIALOG_BUTTON_MARGIN to
DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN. Some callsites were using this
metric to mean "dialog content left/right margin" and those were
changed to use a new metric, DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.

Split DISTANCE_PANEL_CONTENT_MARGIN into three different metrics
depending on what the callsite actually meant (though all three have the
same ultimate value):
* DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN
* DISTANCE_BUBBLE_CONTENTS_VERTICAL_MARGIN
* DISTANCE_DIALOG_CONTENTS_VERTICAL_MARGIN

Opportunistically changed some usages of layout_constants.cc constants
to LayoutProvider metrics.

BUG=716222
==========

[Bret, 2017-05-17 21:25:30]

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

[Bret, 2017-05-17 22:11:04]

bsep@chromium.org changed reviewers:
+ sky@chromium.org

[Bret, 2017-05-17 22:11:41]

pkasting: Harmony changes
sky: ui/views OWNERS

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:286:
provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL) +
From what I can tell, this is the distance between the title/ratings and the
extension icon. I couldn't find any reason this should be
DISTANCE_PANEL_CONTENT_MARGIN specifically, so I changed it to this. It's
ultimately a 1-pixel difference.

[Peter Kasting, 2017-05-18 00:00:07]

LGTM

https://codereview.chromium.org/2888563004/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/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:142:
views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN),
One of a number of places in the codebase where it would probably make sense to
have a helper function in BoxLayout a la GridLayout::CreatePanel() that
constructs some new box layout and puts the correct dialog margins around the
edge.

Doesn't need to be done in this CL.

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW);
I was pondering how here you use the button row insets but elsewhere you use
DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.  It seems like it's inconsistent,
and they have different consequences in terms of whether they assume the two
side values are symmetric.

Here's a thought for a path forward:
(1) Provide INSETS_DIALOG, which is the dialog's outer insets. 
INSETS_DIALOG_XXX for the sub-pieces can stick around for convenience if we
want, and are computed by taking INSETS_DIALOG and then setting the top or
bottom or whatever to zero.  (To do this, it would be convenient if Insets had
setters fro individual sides.  I've wanted those a lot in the past weeks.)
(2) Make BoxLayout take an Insets arg, instead of the horizontal and vertical
constants it has now.  This is more powerful and simpler to specify, and we can
replace callsites like the ones in this CL with just passing INSETS_DIALOG.
(3) Once this is done, look to eliminate direct usage of
DISTANCE_DIALOG_CONTENTS_XXX_MARGIN in favor of having people use the insets
metrics above.  This aims to force callers not to assume that the left and right
sides match, for example.
(4) Once that's done, make DISTANCE_DIALOG_CONTENTS_XXX_MARGIN be protected in
layout_provider.h instead of public, so they become "implementation detail"
metrics.

WDYT?  In my head, this makes the system clearer and simpler, yet also slightly
more powerful than before.

This is probably all post-this-CL, but I wanted your thoughts while you're
adding this stuff.

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:75:
provider->GetDistanceMetric(DISTANCE_RELATED_CONTROL_HORIZONTAL_SMALL)));
This doesn't seem right, you've gone from a vertical constant to a horizontal
one (in a vertical context, which seems wrong).

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:48:
provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS).left() +
This looks like a dialog that rolls its own buttons, rather than a true bubble. 
Maybe it should be using the dialog insets instead of the bubble ones?

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/task_manager_view.cc:343: content_insets.top(),
content_insets.left(), 0, content_insets.right()));
The 0 here looks odd.  Why are we not just using the normal content insets?

https://codereview.chromium.org/2888563004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2888563004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:344: // TODO(tapted): Remove this under
Harmony.
Nit: Remove TODO?

[Bret, 2017-05-18 00:50:06]

https://codereview.chromium.org/2888563004/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/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:142:
views::DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN),
On 2017/05/18 00:00:06, Peter Kasting wrote:
> One of a number of places in the codebase where it would probably make sense
to
> have a helper function in BoxLayout a la GridLayout::CreatePanel() that
> constructs some new box layout and puts the correct dialog margins around the
> edge.
> 
> Doesn't need to be done in this CL.

Yes, that seems like a good idea. It'll be even easier when we remove a bunch of
these weird distinctions we're making just because they're arbitrarily different
pre-Harmony.

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW);
On 2017/05/18 00:00:06, Peter Kasting wrote:
> I was pondering how here you use the button row insets but elsewhere you use
> DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.  It seems like it's inconsistent,
> and they have different consequences in terms of whether they assume the two
> side values are symmetric.
> 
> Here's a thought for a path forward:
> (1) Provide INSETS_DIALOG, which is the dialog's outer insets. 
> INSETS_DIALOG_XXX for the sub-pieces can stick around for convenience if we
> want, and are computed by taking INSETS_DIALOG and then setting the top or
> bottom or whatever to zero.  (To do this, it would be convenient if Insets had
> setters fro individual sides.  I've wanted those a lot in the past weeks.)
> (2) Make BoxLayout take an Insets arg, instead of the horizontal and vertical
> constants it has now.  This is more powerful and simpler to specify, and we
can
> replace callsites like the ones in this CL with just passing INSETS_DIALOG.
> (3) Once this is done, look to eliminate direct usage of
> DISTANCE_DIALOG_CONTENTS_XXX_MARGIN in favor of having people use the insets
> metrics above.  This aims to force callers not to assume that the left and
right
> sides match, for example.
> (4) Once that's done, make DISTANCE_DIALOG_CONTENTS_XXX_MARGIN be protected in
> layout_provider.h instead of public, so they become "implementation detail"
> metrics.
> 
> WDYT?  In my head, this makes the system clearer and simpler, yet also
slightly
> more powerful than before.
> 
> This is probably all post-this-CL, but I wanted your thoughts while you're
> adding this stuff.

I tried to only used the HORIZONTAL/VERTICAL metrics when I had to (i.e. for
BoxLayout). I think your way forward is solid. I've also run into several places
where an Insets#set_left or whatever would have been really nice, so I might add
that.

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:75:
provider->GetDistanceMetric(DISTANCE_RELATED_CONTROL_HORIZONTAL_SMALL)));
On 2017/05/18 00:00:06, Peter Kasting wrote:
> This doesn't seem right, you've gone from a vertical constant to a horizontal
> one (in a vertical context, which seems wrong).

Oops, done.

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:48:
provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS).left() +
On 2017/05/18 00:00:06, Peter Kasting wrote:
> This looks like a dialog that rolls its own buttons, rather than a true
bubble. 
> Maybe it should be using the dialog insets instead of the bubble ones?

This way the ultimate value doesn't change pre-harmony (from 13; dialog insets
have 20 on the horizontal). I didn't really interrogate it further than that,
since I'm not even sure we'll want to keep a bubble/dialog distinction
post-Harmony.

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/task_manager_view.cc:343: content_insets.top(),
content_insets.left(), 0, content_insets.right()));
On 2017/05/18 00:00:06, Peter Kasting wrote:
> The 0 here looks odd.  Why are we not just using the normal content insets?

I guess someone wanted the "End Process" button closer to the process table?
Changed it to do the correct thing in Harmony at least.

https://codereview.chromium.org/2888563004/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2888563004/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:344: // TODO(tapted): Remove this under
Harmony.
On 2017/05/18 00:00:07, Peter Kasting wrote:
> Nit: Remove TODO?

Done.

[Peter Kasting, 2017-05-18 00:56:29]

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW);
On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> On 2017/05/18 00:00:06, Peter Kasting wrote:
> > I was pondering how here you use the button row insets but elsewhere you use
> > DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.  It seems like it's
inconsistent,
> > and they have different consequences in terms of whether they assume the two
> > side values are symmetric.
> > 
> > Here's a thought for a path forward:
> > (1) Provide INSETS_DIALOG, which is the dialog's outer insets. 
> > INSETS_DIALOG_XXX for the sub-pieces can stick around for convenience if we
> > want, and are computed by taking INSETS_DIALOG and then setting the top or
> > bottom or whatever to zero.  (To do this, it would be convenient if Insets
had
> > setters fro individual sides.  I've wanted those a lot in the past weeks.)
> > (2) Make BoxLayout take an Insets arg, instead of the horizontal and
vertical
> > constants it has now.  This is more powerful and simpler to specify, and we
> can
> > replace callsites like the ones in this CL with just passing INSETS_DIALOG.
> > (3) Once this is done, look to eliminate direct usage of
> > DISTANCE_DIALOG_CONTENTS_XXX_MARGIN in favor of having people use the insets
> > metrics above.  This aims to force callers not to assume that the left and
> right
> > sides match, for example.
> > (4) Once that's done, make DISTANCE_DIALOG_CONTENTS_XXX_MARGIN be protected
in
> > layout_provider.h instead of public, so they become "implementation detail"
> > metrics.
> > 
> > WDYT?  In my head, this makes the system clearer and simpler, yet also
> slightly
> > more powerful than before.
> > 
> > This is probably all post-this-CL, but I wanted your thoughts while you're
> > adding this stuff.
> 
> I tried to only used the HORIZONTAL/VERTICAL metrics when I had to (i.e. for
> BoxLayout). I think your way forward is solid. I've also run into several
places
> where an Insets#set_left or whatever would have been really nice, so I might
add
> that.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=723924 on this
stuff.  You're welcome to take it if it would make your life easier.  It seems
like a nice-to-have rather than a must-have for Harmony phase 1, so I'm not
assigning it.

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:48:
provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS).left() +
On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> On 2017/05/18 00:00:06, Peter Kasting wrote:
> > This looks like a dialog that rolls its own buttons, rather than a true
> bubble. 
> > Maybe it should be using the dialog insets instead of the bubble ones?
> 
> This way the ultimate value doesn't change pre-harmony (from 13; dialog insets
> have 20 on the horizontal). I didn't really interrogate it further than that,
> since I'm not even sure we'll want to keep a bubble/dialog distinction
> post-Harmony.

OK... I'd always prefer fixing stuff, even if it changes behavior, since I think
the bubble/dialog distinction will likely persist for some time, but I don't
know, and I don't care enough to argue more.

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/task_manager_view.cc:343: content_insets.top(),
content_insets.left(), 0, content_insets.right()));
On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> On 2017/05/18 00:00:06, Peter Kasting wrote:
> > The 0 here looks odd.  Why are we not just using the normal content insets?
> 
> I guess someone wanted the "End Process" button closer to the process table?
> Changed it to do the correct thing in Harmony at least.

Mrph.  Can we just Do The Correct Thing all the time?  I'm OK with behavioral
changes to pre-Harmony when it seems correct and simpler.

[Bret, 2017-05-18 01:20:31]

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON_ROW);
On 2017/05/18 00:56:28, Peter Kasting wrote:
> On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> > On 2017/05/18 00:00:06, Peter Kasting wrote:
> > > I was pondering how here you use the button row insets but elsewhere you
use
> > > DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.  It seems like it's
> inconsistent,
> > > and they have different consequences in terms of whether they assume the
two
> > > side values are symmetric.
> > > 
> > > Here's a thought for a path forward:
> > > (1) Provide INSETS_DIALOG, which is the dialog's outer insets. 
> > > INSETS_DIALOG_XXX for the sub-pieces can stick around for convenience if
we
> > > want, and are computed by taking INSETS_DIALOG and then setting the top or
> > > bottom or whatever to zero.  (To do this, it would be convenient if Insets
> had
> > > setters fro individual sides.  I've wanted those a lot in the past weeks.)
> > > (2) Make BoxLayout take an Insets arg, instead of the horizontal and
> vertical
> > > constants it has now.  This is more powerful and simpler to specify, and
we
> > can
> > > replace callsites like the ones in this CL with just passing
INSETS_DIALOG.
> > > (3) Once this is done, look to eliminate direct usage of
> > > DISTANCE_DIALOG_CONTENTS_XXX_MARGIN in favor of having people use the
insets
> > > metrics above.  This aims to force callers not to assume that the left and
> > right
> > > sides match, for example.
> > > (4) Once that's done, make DISTANCE_DIALOG_CONTENTS_XXX_MARGIN be
protected
> in
> > > layout_provider.h instead of public, so they become "implementation
detail"
> > > metrics.
> > > 
> > > WDYT?  In my head, this makes the system clearer and simpler, yet also
> > slightly
> > > more powerful than before.
> > > 
> > > This is probably all post-this-CL, but I wanted your thoughts while you're
> > > adding this stuff.
> > 
> > I tried to only used the HORIZONTAL/VERTICAL metrics when I had to (i.e. for
> > BoxLayout). I think your way forward is solid. I've also run into several
> places
> > where an Insets#set_left or whatever would have been really nice, so I might
> add
> > that.
> 
> I filed https://bugs.chromium.org/p/chromium/issues/detail?id=723924 on this
> stuff.  You're welcome to take it if it would make your life easier.  It seems
> like a nice-to-have rather than a must-have for Harmony phase 1, so I'm not
> assigning it.

I'll probably do it. I'll assign it to myself if I do.

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:48:
provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS).left() +
On 2017/05/18 00:56:28, Peter Kasting wrote:
> On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> > On 2017/05/18 00:00:06, Peter Kasting wrote:
> > > This looks like a dialog that rolls its own buttons, rather than a true
> > bubble. 
> > > Maybe it should be using the dialog insets instead of the bubble ones?
> > 
> > This way the ultimate value doesn't change pre-harmony (from 13; dialog
insets
> > have 20 on the horizontal). I didn't really interrogate it further than
that,
> > since I'm not even sure we'll want to keep a bubble/dialog distinction
> > post-Harmony.
> 
> OK... I'd always prefer fixing stuff, even if it changes behavior, since I
think
> the bubble/dialog distinction will likely persist for some time, but I don't
> know, and I don't care enough to argue more.

I double-checked and it's to align the body text with the dialog icon, so it'll
look wonky if it's changed. If it makes you feel better, this whole SetBorder
will ideally go away when I refactor dialog title/icons.

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

https://codereview.chromium.org/2888563004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/task_manager_view.cc:343: content_insets.top(),
content_insets.left(), 0, content_insets.right()));
On 2017/05/18 00:56:28, Peter Kasting wrote:
> On 2017/05/18 00:50:05, Bret Sepulveda wrote:
> > On 2017/05/18 00:00:06, Peter Kasting wrote:
> > > The 0 here looks odd.  Why are we not just using the normal content
insets?
> > 
> > I guess someone wanted the "End Process" button closer to the process table?
> > Changed it to do the correct thing in Harmony at least.
> 
> Mrph.  Can we just Do The Correct Thing all the time?  I'm OK with behavioral
> changes to pre-Harmony when it seems correct and simpler.

Okay, done.

[sky, 2017-05-18 16:39:24]

LGTM

https://codereview.chromium.org/2888563004/diff/80001/ui/views/layout/grid_la...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/2888563004/diff/80001/ui/views/layout/grid_la...
ui/views/layout/grid_layout.cc:654: GridLayout* GridLayout::CreatePanel(View*
host) {
I'm ok with this change, but WDYT of removing this function in favor of callers
creating an explicit function.

[Peter Kasting, 2017-05-18 16:46:46]

https://codereview.chromium.org/2888563004/diff/80001/ui/views/layout/grid_la...
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/2888563004/diff/80001/ui/views/layout/grid_la...
ui/views/layout/grid_layout.cc:654: GridLayout* GridLayout::CreatePanel(View*
host) {
On 2017/05/18 16:39:24, sky wrote:
> I'm ok with this change, but WDYT of removing this function in favor of
callers
> creating an explicit function.

What does "callers creating an explicit function" mean?

Like, a helper at each callsite?  We have a lot.  In fact the lack of a global
convenience function like this for fill/boxlayout is something we've been
tripping over and may want to fix.  Not sure the best way to do it.

[sky, 2017-05-18 17:00:42]

My observation is that it isn't particularly clear what inset CreatePanel()
uses. Especially given the function name and the type enum
 (INSETS_DIALOG_CONTENTS) no longer match. I was suggesting removing
CreatePanel() entirely and having the sites that use it create a GridLayout
and set the border using the appropriate enum for the site. But, if you
think it's common to use this function, then I suggest renaming it to
better match the enum. Perhaps CreateWithDialogContentsInset().

   -Scott

On Thu, May 18, 2017 at 9:46 AM, <pkasting@chromium.org> wrote:

>
> https://codereview.chromium.org/2888563004/diff/80001/ui/
> views/layout/grid_layout.cc
> File ui/views/layout/grid_layout.cc (right):
>
> https://codereview.chromium.org/2888563004/diff/80001/ui/
> views/layout/grid_layout.cc#newcode654
> ui/views/layout/grid_layout.cc:654: GridLayout*
> GridLayout::CreatePanel(View* host) {
> On 2017/05/18 16:39:24, sky wrote:
> > I'm ok with this change, but WDYT of removing this function in favor
> of callers
> > creating an explicit function.
>
> What does "callers creating an explicit function" mean?
>
> Like, a helper at each callsite? We have a lot. In fact the lack of a
> global convenience function like this for fill/boxlayout is something
> we've been tripping over and may want to fix. Not sure the best way to
> do it.
>
> https://codereview.chromium.org/2888563004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2017-05-18 17:07:50]

On 2017/05/18 17:00:42, sky wrote:
> My observation is that it isn't particularly clear what inset CreatePanel()
> uses. Especially given the function name and the type enum
>  (INSETS_DIALOG_CONTENTS) no longer match.

Yeah, I agree.

> I was suggesting removing
> CreatePanel() entirely and having the sites that use it create a GridLayout
> and set the border using the appropriate enum for the site. But, if you
> think it's common to use this function, then I suggest renaming it to
> better match the enum. Perhaps CreateWithDialogContentsInset().

I might bikeshed over the name (CreateForDialog()?) but I'm in favor of renaming
this.  I didn't think through how INSETS_PANEL being renamed means this function
should also be renamed, but it probably should.

I'd be OK with doing that separately just because of how many places it will
(trivially) touch.

[Bret, 2017-05-18 18:40:07]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-05-18 18:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2888563004/#ps80001
(title: "change task manager insets")

[commit-bot: I haz the power, 2017-05-18 18:41:19]

CQ is trying da patch.

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

[Bret, 2017-05-18 18:49:12]

I'm planning on redoing CreatePanel, I just didn't want to do it in this patch
since like you said it touches a lot of files. I also want to make dialogs have
a INSETS_DIALOG_CONTENTS margin by default. Once that happens a lot of the
callsites will go away, so I'm undecided on simply renaming the function versus
making the remaining callsites manually add the border.

[commit-bot: I haz the power, 2017-05-18 18:55:06]

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

[commit-bot: I haz the power, 2017-05-18 18:55:07]

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

[sky, 2017-05-18 19:45:29]

Renaming in a separate patch SGTM.

On Thu, May 18, 2017 at 10:07 AM, <pkasting@chromium.org> wrote:

> On 2017/05/18 17:00:42, sky wrote:
> > My observation is that it isn't particularly clear what inset
> CreatePanel()
> > uses. Especially given the function name and the type enum
> > (INSETS_DIALOG_CONTENTS) no longer match.
>
> Yeah, I agree.
>
> > I was suggesting removing
> > CreatePanel() entirely and having the sites that use it create a
> GridLayout
> > and set the border using the appropriate enum for the site. But, if you
> > think it's common to use this function, then I suggest renaming it to
> > better match the enum. Perhaps CreateWithDialogContentsInset().
>
> I might bikeshed over the name (CreateForDialog()?) but I'm in favor of
> renaming
> this. I didn't think through how INSETS_PANEL being renamed means this
> function
> should also be renamed, but it probably should.
>
> I'd be OK with doing that separately just because of how many places it
> will
> (trivially) touch.
>
> https://codereview.chromium.org/2888563004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bret, 2017-05-24 00:51:13]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-05-24 00:51:14]

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

[commit-bot: I haz the power, 2017-05-24 00:52:46]

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-05-24 02:45:06]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495587073629410,
"parent_rev": "f01e9fac8ee174ad2dc5829c069c4cd78c5850b4", "commit_rev":
"dd45e5f7692fe435bfaa61981ecf56724ab4e31a"}

[commit-bot: I haz the power, 2017-05-24 02:45:17]

Description was changed from

==========
Renamed INSETS_PANEL to INSETS_DIALOG_CONTENTS. Renamed
INSETS_DIALOG_BUTTON to INSETS_DIALOG_BUTTON_ROW, and made it have a 0
top inset in Harmony to fix the double-wide margin between the
content and button row.

Defined most insets metrics in terms of distance metrics, and removed
their overrides from HarmonyLayoutProvider since they would now be
redundant.

Renamed DISTANCE_DIALOG_BUTTON_MARGIN to
DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN. Some callsites were using this
metric to mean "dialog content left/right margin" and those were
changed to use a new metric, DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.

Split DISTANCE_PANEL_CONTENT_MARGIN into three different metrics
depending on what the callsite actually meant (though all three have the
same ultimate value):
* DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN
* DISTANCE_BUBBLE_CONTENTS_VERTICAL_MARGIN
* DISTANCE_DIALOG_CONTENTS_VERTICAL_MARGIN

Opportunistically changed some usages of layout_constants.cc constants
to LayoutProvider metrics.

BUG=716222
==========

to

==========
Renamed INSETS_PANEL to INSETS_DIALOG_CONTENTS. Renamed
INSETS_DIALOG_BUTTON to INSETS_DIALOG_BUTTON_ROW, and made it have a 0
top inset in Harmony to fix the double-wide margin between the
content and button row.

Defined most insets metrics in terms of distance metrics, and removed
their overrides from HarmonyLayoutProvider since they would now be
redundant.

Renamed DISTANCE_DIALOG_BUTTON_MARGIN to
DISTANCE_DIALOG_BUTTON_BOTTOM_MARGIN. Some callsites were using this
metric to mean "dialog content left/right margin" and those were
changed to use a new metric, DISTANCE_DIALOG_CONTENTS_HORIZONTAL_MARGIN.

Split DISTANCE_PANEL_CONTENT_MARGIN into three different metrics
depending on what the callsite actually meant (though all three have the
same ultimate value):
* DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN
* DISTANCE_BUBBLE_CONTENTS_VERTICAL_MARGIN
* DISTANCE_DIALOG_CONTENTS_VERTICAL_MARGIN

Opportunistically changed some usages of layout_constants.cc constants
to LayoutProvider metrics.

BUG=716222

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

[commit-bot: I haz the power, 2017-05-24 02:45:23]

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