views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This change:
1) Introduces a new type of InsetsMetric in ViewsDelegate: PANEL
2) Changes GridLayout::CreatePanel to use that metric instead of hardcoded insets
3) Changes GridLayout::CreatePanel to always install itself as the layout manager of the provided host View
4) Moves all callers of layout_utils::CreatePanelLayout over to GridLayout::CreatePanel
5) Deletes layout_utils

BUG=687340
Review-Url: https://codereview.chromium.org/2779973003
Cr-Commit-Position: refs/heads/master@{#461745}
Committed: https://chromium.googlesource.com/chromium/src/+/08376b4003fdea077ec6df37f88305a65751101c

[Elly Fong-Jones, 2017-03-28 19:06:44]

Description was changed from

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This causes a visible UI change: GridLayout::CreatePanel uses the panel margins
now, while the old version of CreatePanel used kPanelVertMargin and
kButtonHEdgeMarginNew, and CreatePanelLayout used metrics from LayoutDelegate.

BUG=687340
==========

to

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This causes a visible UI change: GridLayout::CreatePanel uses the panel margins
now, while the old version of CreatePanel used kPanelVertMargin and
kButtonHEdgeMarginNew, and CreatePanelLayout used metrics from LayoutDelegate.

BUG=687340
==========

[Elly Fong-Jones, 2017-03-28 19:06:44]

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

[Elly Fong-Jones, 2017-03-28 19:06:51]

pkasting: ptal? :)

[Peter Kasting, 2017-03-29 03:39:11]

https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout.cc
File ui/views/layout/grid_layout.cc (right):

https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
ui/views/layout/grid_layout.cc:656: InsetsMetric::BUBBLE_DIALOG));
Did you choose to use BUBBLE_DIALOG because it was close to the desired
behavior, and the previous behavior is hard to duplicate with existing metrics?

I'm worried about the visible effects of this behavior change.  Can you take a
sample screenshot of a place this would affect?

Unless this is a concrete visual improvement, I'm inclined to suggest we add the
necessary plumbing to ViewsDelegate (e.g. another inset enum value) to duplicate
the old behavior.

https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
ui/views/layout/grid_layout.cc:657: host->SetLayoutManager(layout);
Can you also audit the existing CreatePanel() calls and remove all the
SetLayoutManager() calls that existed because this wasn't here?

(Were any places NOT doing SetLayoutManager()?)

[Elly Fong-Jones, 2017-03-29 15:33:45]

On 2017/03/29 03:39:11, Peter Kasting wrote:
>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout.cc
> File ui/views/layout/grid_layout.cc (right):
> 
>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
> ui/views/layout/grid_layout.cc:656: InsetsMetric::BUBBLE_DIALOG));
> Did you choose to use BUBBLE_DIALOG because it was close to the desired
> behavior, and the previous behavior is hard to duplicate with existing
metrics?

Partly, yeah, but also because I don't know what a "panel" is, it isn't a thing
in the Harmony spec (that I can find) and I do not want to gratuitously
introduce the concept to ViewsDelegate. That said, it probably should be a
concept, at least until we can figure out whether to do away with it. Right now,
we have BUBBLE_DIALOG, which seems like it *means* "panel" based on its insets
being the panel insets, but the thing currently called "CreatePanel" uses a
different inset, which is not the panel inset :(. I might call this something
visibly deprecated like PANEL_OLD when I add it.
 
> I'm worried about the visible effects of this behavior change.  Can you take a
> sample screenshot of a place this would affect?

I'll redo this so it has no visible behavior changes.

> Unless this is a concrete visual improvement, I'm inclined to suggest we add
the
> necessary plumbing to ViewsDelegate (e.g. another inset enum value) to
duplicate
> the old behavior.

Alright, got it.

>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
> ui/views/layout/grid_layout.cc:657: host->SetLayoutManager(layout);
> Can you also audit the existing CreatePanel() calls and remove all the
> SetLayoutManager() calls that existed because this wasn't here?
> 
> (Were any places NOT doing SetLayoutManager()?)

Can do.

[Elly Fong-Jones, 2017-03-29 16:49:29]

On 2017/03/29 15:33:45, Elly Fong-Jones wrote:
> On 2017/03/29 03:39:11, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout.cc
> > File ui/views/layout/grid_layout.cc (right):
> > 
> >
>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
> > ui/views/layout/grid_layout.cc:656: InsetsMetric::BUBBLE_DIALOG));
> > Did you choose to use BUBBLE_DIALOG because it was close to the desired
> > behavior, and the previous behavior is hard to duplicate with existing
> metrics?
> 
> Partly, yeah, but also because I don't know what a "panel" is, it isn't a
thing
> in the Harmony spec (that I can find) and I do not want to gratuitously
> introduce the concept to ViewsDelegate. That said, it probably should be a
> concept, at least until we can figure out whether to do away with it. Right
now,
> we have BUBBLE_DIALOG, which seems like it *means* "panel" based on its insets
> being the panel insets, but the thing currently called "CreatePanel" uses a
> different inset, which is not the panel inset :(. I might call this something
> visibly deprecated like PANEL_OLD when I add it.
>  
> > I'm worried about the visible effects of this behavior change.  Can you take
a
> > sample screenshot of a place this would affect?
> 
> I'll redo this so it has no visible behavior changes.
> 
> > Unless this is a concrete visual improvement, I'm inclined to suggest we add
> the
> > necessary plumbing to ViewsDelegate (e.g. another inset enum value) to
> duplicate
> > the old behavior.
> 
> Alright, got it.

This is done now.

> 
> >
>
https://codereview.chromium.org/2779973003/diff/1/ui/views/layout/grid_layout...
> > ui/views/layout/grid_layout.cc:657: host->SetLayoutManager(layout);
> > Can you also audit the existing CreatePanel() calls and remove all the
> > SetLayoutManager() calls that existed because this wasn't here?
> > 
> > (Were any places NOT doing SetLayoutManager()?)
> 
> Can do.

An audit reveals no places where SetLayoutManager() was being skipped, phew :)

[Elly Fong-Jones, 2017-03-29 16:50:17]

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

[commit-bot: I haz the power, 2017-03-29 16:50:43]

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-29 17:09:14]

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

[commit-bot: I haz the power, 2017-03-29 17:09:15]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elly Fong-Jones, 2017-03-29 20:36:36]

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

[commit-bot: I haz the power, 2017-03-29 20:37:09]

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-29 20:59:33]

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

[commit-bot: I haz the power, 2017-03-29 20:59:34]

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

[Peter Kasting, 2017-03-29 22:46:16]

LGTM, but please update CL description to reflect your changes

[Peter Kasting, 2017-03-30 00:44:42]

https://codereview.chromium.org/2779973003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/options/vpn_config_view.cc (left):

https://codereview.chromium.org/2779973003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/options/vpn_config_view.cc:18: #include
"chrome/browser/ui/views/layout_utils.h"
Oh, I forgot.  Please remove this file from the DEPS from this folder now.

[Elly Fong-Jones, 2017-03-30 15:16:46]

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

[commit-bot: I haz the power, 2017-03-30 15:17:20]

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-30 16:33:40]

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

[commit-bot: I haz the power, 2017-03-30 16:33:41]

Dry run: This issue passed the CQ dry run.

[Elly Fong-Jones, 2017-03-31 14:33:26]

ellyjones@chromium.org changed reviewers:
+ derat@chromium.org, gab@chromium.org

[Elly Fong-Jones, 2017-03-31 14:33:46]

derat: ptal as chrome/browser/chromeos owner?
gab: ptal as chrome/browser/first_run owner?

[Daniel Erat, 2017-03-31 14:47:33]

rubber-stamp lgtm for c/b/chromeos

[Elly Fong-Jones, 2017-04-03 13:19:47]

On 2017/03/31 14:47:33, Daniel Erat wrote:
> rubber-stamp lgtm for c/b/chromeos

gab: ping? :)

[gab, 2017-04-03 16:50:15]

On 2017/03/31 14:33:46, Elly Fong-Jones wrote:
> derat: ptal as chrome/browser/chromeos owner?
> gab: ptal as chrome/browser/first_run owner?

rubber-stamp lgtm (I have no idea what removing
root_view->SetLayoutManager(layout); does but given PK approves of this in other
places I guess it's fine :))

[Elly Fong-Jones, 2017-04-03 16:56:11]

On 2017/04/03 16:50:15, gab wrote:
> On 2017/03/31 14:33:46, Elly Fong-Jones wrote:
> > derat: ptal as chrome/browser/chromeos owner?
> > gab: ptal as chrome/browser/first_run owner?
> 
> rubber-stamp lgtm (I have no idea what removing
> root_view->SetLayoutManager(layout); does but given PK approves of this in
other
> places I guess it's fine :))

Yup, this CL is making that call unnecessary :)

[Elly Fong-Jones, 2017-04-03 16:56:19]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-04-03 16:56:20]

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/2779973003/#ps100001
(title: "remove old DEPS entry")

[commit-bot: I haz the power, 2017-04-03 16:57: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-04-03 17:08:40]

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

[commit-bot: I haz the power, 2017-04-03 17:08:42]

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

[Elly Fong-Jones, 2017-04-03 17:42:25]

Description was changed from

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This causes a visible UI change: GridLayout::CreatePanel uses the panel margins
now, while the old version of CreatePanel used kPanelVertMargin and
kButtonHEdgeMarginNew, and CreatePanelLayout used metrics from LayoutDelegate.

BUG=687340
==========

to

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This change:
1) Introduces a new type of InsetsMetric in ViewsDelegate: PANEL
2) Changes GridLayout::CreatePanel to use that metric instead of hardcoded
insets
3) Changes GridLayout::CreatePanel to always install itself as the layout
manager of the provided host View
4) Moves all callers of layout_utils::CreatePanelLayout over to
GridLayout::CreatePanel
5) Deletes layout_utils

BUG=687340
==========

[Elly Fong-Jones, 2017-04-03 17:42:25]

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

[Elly Fong-Jones, 2017-04-03 17:42:39]

sky: ptal as /ui/views OWNER? :)

[sky, 2017-04-04 16:25:29]

LGTM

[Elly Fong-Jones, 2017-04-04 16:27:48]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2017-04-04 16:28:32]

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-04-04 16:34:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491323268609140,
"parent_rev": "5173eeca89c25e7ad01efd687879936ae474253f", "commit_rev":
"a52dbb1ea7f33732718d1996deb85a07bbad0d26"}

[commit-bot: I haz the power, 2017-04-04 16:40:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491323268609140,
"parent_rev": "803fdf80f842803c7c91dca2a87419afe1b3d60c", "commit_rev":
"08376b4003fdea077ec6df37f88305a65751101c"}

[commit-bot: I haz the power, 2017-04-04 16:41:15]

Description was changed from

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This change:
1) Introduces a new type of InsetsMetric in ViewsDelegate: PANEL
2) Changes GridLayout::CreatePanel to use that metric instead of hardcoded
insets
3) Changes GridLayout::CreatePanel to always install itself as the layout
manager of the provided host View
4) Moves all callers of layout_utils::CreatePanelLayout over to
GridLayout::CreatePanel
5) Deletes layout_utils

BUG=687340
==========

to

==========
views: fold layout_utils::CreatePanelLayout into GridLayout::CreatePanel

This change:
1) Introduces a new type of InsetsMetric in ViewsDelegate: PANEL
2) Changes GridLayout::CreatePanel to use that metric instead of hardcoded
insets
3) Changes GridLayout::CreatePanel to always install itself as the layout
manager of the provided host View
4) Moves all callers of layout_utils::CreatePanelLayout over to
GridLayout::CreatePanel
5) Deletes layout_utils

BUG=687340

Review-Url: https://codereview.chromium.org/2779973003
Cr-Commit-Position: refs/heads/master@{#461745}
Committed:
https://chromium.googlesource.com/chromium/src/+/08376b4003fdea077ec6df37f883...
==========

[commit-bot: I haz the power, 2017-04-04 16:41:20]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/08376b4003fdea077ec6df37f883...