Add main axis alignment for BoxLayout.

Add main axis alignment for BoxLayout.

This CL adds a MainAxisAlignment setting to BoxLayout which functions like the CSS
justify-content property. This property aligns the children items of a view
in the orientation axis of the BoxLayout.

set_spread_blank_space has been removed and replaced with
MAIN_AXIS_ALIGNMENT_FILL.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270934

[benwells, 2014-05-13 02:48:05]

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:55: padding = (child_area.width() - total) /
visible;
I think this code would be easier to follow at a glance if you added three a few
private functions:

int BoxLayout::MainAxisSize(child)
int BoxLayout::MainAxisPosition(child)
void BoxLayout::SetMainAxisSize(child, size)
void BoxLayout::SetMainAxisPosition(child, position)

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:53: void set_box_pack(BoxPack box_pack) { box_pack_
= box_pack; }
Comment what this is, and mention what the default is.

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout_unittest.cc (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout_unittest.cc:169: // Aligns children to the center
horizontally.
Before testing CENTER and END, add two more test cases, that default lays out as
start, and then set to start and test like that.

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout_unittest.cc:169: // Aligns children to the center
horizontally.
Also I think there should be some testing of when the children take up more
space than is available.

[calamity, 2014-05-13 04:29:29]

+sky for OWNERS.

I also added a couple of TODOs for additional capabilities that could be made to
BoxLayout. Let me know if you'd rather I remove them.

Also, it's come to my attention that box-pack is the old Flexbox name. The new
name is justify-content. Do you think that's a better name or should we try
something that's more obvious-sounding like MainAxisAlignment?

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:55: padding = (child_area.width() - total) /
visible;
On 2014/05/13 02:48:05, benwells wrote:
> I think this code would be easier to follow at a glance if you added three a
few
> private functions:
> 
> int BoxLayout::MainAxisSize(child)
> int BoxLayout::MainAxisPosition(child)
> void BoxLayout::SetMainAxisSize(child, size)
> void BoxLayout::SetMainAxisPosition(child, position)

Done.

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:53: void set_box_pack(BoxPack box_pack) { box_pack_
= box_pack; }
On 2014/05/13 02:48:05, benwells wrote:
> Comment what this is, and mention what the default is.

Is the member comment not enough?

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout_unittest.cc (right):

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout_unittest.cc:169: // Aligns children to the center
horizontally.
On 2014/05/13 02:48:05, benwells wrote:
> Also I think there should be some testing of when the children take up more
> space than is available.

Done.

https://codereview.chromium.org/284753002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout_unittest.cc:169: // Aligns children to the center
horizontally.
On 2014/05/13 02:48:05, benwells wrote:
> Before testing CENTER and END, add two more test cases, that default lays out
as
> start, and then set to start and test like that.

Done.

[benwells, 2014-05-13 04:44:21]

lgtm

https://codereview.chromium.org/284753002/diff/80001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/80001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:64: (MainAxisSize(new_child_area) -
total_main_axis_size) / 2);
Nit: a variable:

free_space = MainAxisSize(new_child_area) - total_main_axis_size

would make this clearer and would be used for BOX_PACK_END as well.

[calamity, 2014-05-13 05:04:06]

https://codereview.chromium.org/284753002/diff/80001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/80001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:64: (MainAxisSize(new_child_area) -
total_main_axis_size) / 2);
On 2014/05/13 04:44:21, benwells wrote:
> Nit: a variable:
> 
> free_space = MainAxisSize(new_child_area) - total_main_axis_size
> 
> would make this clearer and would be used for BOX_PACK_END as well.

Done.

[sky, 2014-05-13 16:24:58]

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:37: int visible = 0;
visible -> num_visible or visible_count

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:56: padding = free_space / visible;
This calculation isn't quite right.It has rounding areas. It may need to be
adjusted for the last view.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:120: int BoxLayout::MainAxisSize(const gfx::Rect&
child_area) {
const on this and the next.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:128: void BoxLayout::SetMainAxisSize(gfx::Rect*
child_area, int size) {
nit: style guide says out params should be last.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:34: enum BoxPack {
These names mean nothing to me. Please add a good description. You'll want to
update the class description too.

Also, it seems like this overlaps with spread_blank_space_. Shouldn't there be a
BOX_PACK_JUSTIFY that replaces spared_blank_space_?

[calamity, 2014-05-14 02:03:21]

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:37: int visible = 0;
On 2014/05/13 16:24:59, sky wrote:
> visible -> num_visible or visible_count

Done.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:56: padding = free_space / visible;
On 2014/05/13 16:24:59, sky wrote:
> This calculation isn't quite right.It has rounding areas. It may need to be
> adjusted for the last view.

I don't quite understand what the problem here is?

Is it that the last view won't fully stretch to the end of the host?

e.g Host = 100 wide; 30 children. Each of them would be 3 wide, except the last
which should be 13?

Also, this code isn't changing functionally here. If there's a problem, could we
log it as a separate bug and fix it in another patch?

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:120: int BoxLayout::MainAxisSize(const gfx::Rect&
child_area) {
On 2014/05/13 16:24:59, sky wrote:
> const on this and the next.

Done.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:128: void BoxLayout::SetMainAxisSize(gfx::Rect*
child_area, int size) {
On 2014/05/13 16:24:59, sky wrote:
> nit: style guide says out params should be last.

Done. This can be const too..

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:34: enum BoxPack {
On 2014/05/13 16:24:59, sky wrote:
> These names mean nothing to me. Please add a good description. You'll want to
> update the class description too.
> 
> Also, it seems like this overlaps with spread_blank_space_. Shouldn't there be
a
> BOX_PACK_JUSTIFY that replaces spared_blank_space_?

BOX_PACK_JUSTIFY would add space between fixed size children.

spread_blank_space_ increases the actual size of children and keeps  the
between_child_spacing_.

[sky, 2014-05-14 18:13:40]

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:56: padding = free_space / visible;
On 2014/05/14 02:03:27, calamity wrote:
> On 2014/05/13 16:24:59, sky wrote:
> > This calculation isn't quite right.It has rounding areas. It may need to be
> > adjusted for the last view.
> 
> I don't quite understand what the problem here is?
> 
> Is it that the last view won't fully stretch to the end of the host?
> 
> e.g Host = 100 wide; 30 children. Each of them would be 3 wide, except the
last
> which should be 13?
> 
> Also, this code isn't changing functionally here. If there's a problem, could
we
> log it as a separate bug and fix it in another patch?

Yes and sure.

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:34: enum BoxPack {
On 2014/05/14 02:03:27, calamity wrote:
> On 2014/05/13 16:24:59, sky wrote:
> > These names mean nothing to me. Please add a good description. You'll want
to
> > update the class description too.
> > 
> > Also, it seems like this overlaps with spread_blank_space_. Shouldn't there
be
> a
> > BOX_PACK_JUSTIFY that replaces spared_blank_space_?
> 
> BOX_PACK_JUSTIFY would add space between fixed size children.
> 
> spread_blank_space_ increases the actual size of children and keeps  the
> between_child_spacing_.

I see. My concern is that if you set spread_blank_space_ none of these apply.
For that reason set_spread_space should be folded into this structure. I think
what you want is:
enum MainAxisAlignment {
  MAIN_AXIS_ALIGN_START,
  MAIN_AXIS_ALIGN_CENTER,
  MAIN_AXIS_ALIGN_END,
  MAIN_AXIS_ALIGN_FILL,
  MAIN_AXIS_ALIGN_JUSTIFY,
};

[calamity, 2014-05-15 02:51:07]

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:56: padding = free_space / visible;
On 2014/05/14 18:13:40, sky wrote:
> On 2014/05/14 02:03:27, calamity wrote:
> > On 2014/05/13 16:24:59, sky wrote:
> > > This calculation isn't quite right.It has rounding areas. It may need to
be
> > > adjusted for the last view.
> > 
> > I don't quite understand what the problem here is?
> > 
> > Is it that the last view won't fully stretch to the end of the host?
> > 
> > e.g Host = 100 wide; 30 children. Each of them would be 3 wide, except the
> last
> > which should be 13?
> > 
> > Also, this code isn't changing functionally here. If there's a problem,
could
> we
> > log it as a separate bug and fix it in another patch?
> 
> Yes and sure.

Logged a bug at crbug.com/373571 .

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/284753002/diff/110001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:34: enum BoxPack {
On 2014/05/14 18:13:40, sky wrote:
> On 2014/05/14 02:03:27, calamity wrote:
> > On 2014/05/13 16:24:59, sky wrote:
> > > These names mean nothing to me. Please add a good description. You'll want
> to
> > > update the class description too.
> > > 
> > > Also, it seems like this overlaps with spread_blank_space_. Shouldn't
there
> be
> > a
> > > BOX_PACK_JUSTIFY that replaces spared_blank_space_?
> > 
> > BOX_PACK_JUSTIFY would add space between fixed size children.
> > 
> > spread_blank_space_ increases the actual size of children and keeps  the
> > between_child_spacing_.
> 
> I see. My concern is that if you set spread_blank_space_ none of these apply.
> For that reason set_spread_space should be folded into this structure. I think
> what you want is:
> enum MainAxisAlignment {
>   MAIN_AXIS_ALIGN_START,
>   MAIN_AXIS_ALIGN_CENTER,
>   MAIN_AXIS_ALIGN_END,
>   MAIN_AXIS_ALIGN_FILL,
>   MAIN_AXIS_ALIGN_JUSTIFY,
> };

Ah yes. Good point.

[sky, 2014-05-15 16:30:16]

LGTM

[calamity, 2014-05-16 01:52:23]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-16 01:53:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/284753002/170001

[commit-bot: I haz the power, 2014-05-16 05:21:18]

Change committed as 270934