views: add layout delegates

views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392, 635173
Committed: https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d
Cr-Commit-Position: refs/heads/master@{#432611}

[Elly Fong-Jones, 2016-11-08 20:13:36]

Description was changed from

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173
==========

to

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173
==========

[Elly Fong-Jones, 2016-11-08 20:13:36]

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

[Elly Fong-Jones, 2016-11-08 20:14:06]

sky: ptal? https://bugs.chromium.org/p/chromium/issues/detail?id=602392#c15 has
before and after screenshots.

[sky, 2016-11-08 23:34:01]

https://codereview.chromium.org/2485083003/diff/1/ui/harmony/harmony_layout_d...
File ui/harmony/harmony_layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/1/ui/harmony/harmony_layout_d...
ui/harmony/harmony_layout_delegate.h:17: class HARMONY_EXPORT
HarmonyLayoutDelegate : public views::LayoutDelegate {
We are in the process of trying to cleanup the ui directory. The ui directory
should only contain the fundamental building blocks of UI components.  It should
not contain the UI features that are composed of those building blocks. I'm
inclined to think of harmony as not a fundamental building block, but something
that builds on views. As such I think this should be in components/harmony. If
only needed in chrome, then move there (but I suspect you'll want to use this in
ash code too, which means components/harmony).

https://codereview.chromium.org/2485083003/diff/1/ui/views/layout/layout_dele...
File ui/views/layout/layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/1/ui/views/layout/layout_dele...
ui/views/layout/layout_delegate.h:12: class VIEWS_EXPORT LayoutDelegate {
Layout spacing and alignment are best left to clients, and not in views itself.
From this patch you need two things from this class:

. GridLayout::CreatePanel. In hind site this shouldn't exist in views, it should
move to client code, probably harmony in this patch.

. The insets and padding for DialogClientView. I would rather there is explicit
functions on ViewsDelegate to get what you need. You can make these functions
take in a View for context (if needed). If you envision needing a bunch of
things, then I'm ok with a LayoutConstants class that is obtained from
ViewsDelegate.

[Elly Fong-Jones, 2016-11-14 19:35:37]

On 2016/11/08 23:34:01, sky wrote:
>
https://codereview.chromium.org/2485083003/diff/1/ui/harmony/harmony_layout_d...
> File ui/harmony/harmony_layout_delegate.h (right):
> 
>
https://codereview.chromium.org/2485083003/diff/1/ui/harmony/harmony_layout_d...
> ui/harmony/harmony_layout_delegate.h:17: class HARMONY_EXPORT
> HarmonyLayoutDelegate : public views::LayoutDelegate {
> We are in the process of trying to cleanup the ui directory. The ui directory
> should only contain the fundamental building blocks of UI components.  It
should
> not contain the UI features that are composed of those building blocks. I'm
> inclined to think of harmony as not a fundamental building block, but
something
> that builds on views. As such I think this should be in components/harmony. If
> only needed in chrome, then move there (but I suspect you'll want to use this
in
> ash code too, which means components/harmony).
> 
>
https://codereview.chromium.org/2485083003/diff/1/ui/views/layout/layout_dele...
> File ui/views/layout/layout_delegate.h (right):
> 
>
https://codereview.chromium.org/2485083003/diff/1/ui/views/layout/layout_dele...
> ui/views/layout/layout_delegate.h:12: class VIEWS_EXPORT LayoutDelegate {
> Layout spacing and alignment are best left to clients, and not in views
itself.
> From this patch you need two things from this class:
> 
> . GridLayout::CreatePanel. In hind site this shouldn't exist in views, it
should
> move to client code, probably harmony in this patch.
> 
> . The insets and padding for DialogClientView. I would rather there is
explicit
> functions on ViewsDelegate to get what you need. You can make these functions
> take in a View for context (if needed). If you envision needing a bunch of
> things, then I'm ok with a LayoutConstants class that is obtained from
> ViewsDelegate.

Okay, I think this is all done. ptal? :)

[sky, 2016-11-14 21:49:33]

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:9: static
HarmonyLayoutDelegate harmony_layout_delegate_;
Style guide says static should only be PODs.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:20: };
private: DISALLOW_... ?

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.cc:12: static LayoutDelegate
layout_delegate_;
Same comment about statics.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:14: enum LayoutDistanceType {
generally we're using enum class for new types.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:29: static LayoutDelegate*
GetActiveInstance();
It isn't clear from this description how the value differs from Get(). Without
looking at the code I expected the two to be the same.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_utils.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.cc:6: #include
"chrome/browser/ui/views/harmony/layout_delegate.h"
newline between lines 5 and 6.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.cc:13: views::GridLayout*
CreatePanelLayout(views::View* host) {
optional: Consider having this set the returned GridLayout as the layout manager
on host to save extra code. When I created the views variant of this function my
reviewer suggested otherwise, but I think it's just as clear to set the
LayoutManager and return it here to save code.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_utils.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.h:13: namespace chrome {
We generally don't use 'chrome' namespace for files like this. There are long
discussions on chromium-dev about namespaces. layout_utils is fine as it matches
the file name.

[Elly Fong-Jones, 2016-11-15 17:13:39]

sky: ptal? :)

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:9: static
HarmonyLayoutDelegate harmony_layout_delegate_;
On 2016/11/14 21:49:32, sky wrote:
> Style guide says static should only be PODs.

Ack. I misread the definition of POD in the C++ spec, so I thought this was POD,
but it has virtual methods :( oh well; switched to LazyInstance.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:20: };
On 2016/11/14 21:49:32, sky wrote:
> private: DISALLOW_... ?

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.cc:12: static LayoutDelegate
layout_delegate_;
On 2016/11/14 21:49:32, sky wrote:
> Same comment about statics.

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:14: enum LayoutDistanceType {
On 2016/11/14 21:49:32, sky wrote:
> generally we're using enum class for new types.

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:29: static LayoutDelegate*
GetActiveInstance();
On 2016/11/14 21:49:32, sky wrote:
> It isn't clear from this description how the value differs from Get(). Without
> looking at the code I expected the two to be the same.

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_utils.cc (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.cc:6: #include
"chrome/browser/ui/views/harmony/layout_delegate.h"
On 2016/11/14 21:49:32, sky wrote:
> newline between lines 5 and 6.

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.cc:13: views::GridLayout*
CreatePanelLayout(views::View* host) {
On 2016/11/14 21:49:32, sky wrote:
> optional: Consider having this set the returned GridLayout as the layout
manager
> on host to save extra code. When I created the views variant of this function
my
> reviewer suggested otherwise, but I think it's just as clear to set the
> LayoutManager and return it here to save code.

Done.

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/layout_utils.h (right):

https://codereview.chromium.org/2485083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/layout_utils.h:13: namespace chrome {
On 2016/11/14 21:49:32, sky wrote:
> We generally don't use 'chrome' namespace for files like this. There are long
> discussions on chromium-dev about namespaces. layout_utils is fine as it
matches
> the file name.

Done.

[sky, 2016-11-15 17:43:38]

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:27: static LayoutDelegate*
Get();
Thanks for the improved docs in GetActiveInstance. I still think having two
functions is error prone. It's all two easy to review code calling Get() and not
realize it's not the one that should be called. AFAICT Get() is only called from
GetActiveInstance(). In which case can Get() be removed and/or made private? I
would also rename GetActiveInstance() to Get().

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:53:
DISALLOW_COPY_AND_ASSIGN(LayoutDelegate);
I think you want DISALLOW_IMPLICIT_CONSTRUCTORS.

[Elly Fong-Jones, 2016-11-15 19:47:26]

sky: ptal? :)

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:27: static LayoutDelegate*
Get();
On 2016/11/15 17:43:38, sky wrote:
> Thanks for the improved docs in GetActiveInstance. I still think having two
> functions is error prone. It's all two easy to review code calling Get() and
not
> realize it's not the one that should be called. AFAICT Get() is only called
from
> GetActiveInstance(). In which case can Get() be removed and/or made private? I
> would also rename GetActiveInstance() to Get().

Done.

https://codereview.chromium.org/2485083003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:53:
DISALLOW_COPY_AND_ASSIGN(LayoutDelegate);
On 2016/11/15 17:43:38, sky wrote:
> I think you want DISALLOW_IMPLICIT_CONSTRUCTORS.

It has an explicitly-declared LayoutDelegate() above, though (which is needed
for LazyInstance to work, it seems).

[sky, 2016-11-15 20:48:06]

LGTM

[Elly Fong-Jones, 2016-11-16 13:51:06]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-11-16 13:51:29]

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, 2016-11-16 14:12:19]

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

[commit-bot: I haz the power, 2016-11-16 14:12:20]

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

[Elly Fong-Jones, 2016-11-16 19:06:41]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-11-16 19:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2485083003/#ps80001
(title: "fix msvc compile")

[commit-bot: I haz the power, 2016-11-16 19:08:10]

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, 2016-11-16 20:30:32]

Description was changed from

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173
==========

to

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173
==========

[commit-bot: I haz the power, 2016-11-16 20:30:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-16 20:55:19]

Description was changed from

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173
==========

to

==========
views: add layout delegates

This change:
1) Adds the concept of a "layout delegate" for a View, which is responsible for
   computing padding amounts and other minutiae of control positioning;
2) Adds a HarmonyLayoutDelegate which returns the Harmony specified values for
   some of the existing padding constants;
3) Changes GridLayout and DialogClientView to honor LayoutDelegate;
4) Makes chrome's LoginView use HarmonyLayoutDelegate when --secondary-ui-md.

BUG=602392,635173

Committed: https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d
Cr-Commit-Position: refs/heads/master@{#432611}
==========

[commit-bot: I haz the power, 2016-11-16 20:55:20]

Patchset 5 (id:??) landed as
https://crrev.com/964f7ce4df1d586722cf3819eac9cc814cd98e1d
Cr-Commit-Position: refs/heads/master@{#432611}