BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/layout/box_layout.h"
 
 #include <algorithm>
 
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/view.h"
+#include "ui/views/view_properties.h"
 
 namespace views {
 
+namespace {
+
+// Returns the maximum of the given insets along the given |axis|.
+// NOTE: |axis| is different from |orientation_|; it specifies the actual
+// desired axis.
+enum Axis { HORIZONTAL_AXIS, VERTICAL_AXIS };
+
+gfx::Insets MaxAxisInsets(Axis axis,
+                          const gfx::Insets& leading1,
+                          const gfx::Insets& leading2,
+                          const gfx::Insets& trailing1,
+                          const gfx::Insets& trailing2) {
+  if (axis == HORIZONTAL_AXIS)

[sky, 2017/06/05 21:04:10]

use {}

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    return gfx::Insets(0, std::max(leading1.left(), leading2.left()), 0,
+                       std::max(trailing1.right(), trailing2.right()));
+  return gfx::Insets(std::max(leading1.top(), leading2.top()), 0,
+                     std::max(trailing1.bottom(), trailing2.bottom()), 0);
+}
+}

[sky, 2017/06/05 21:04:10]

Newline between 32/33 and // namespace (with two spaces before the '//'). See
last three lines in this file for what this should look like.

[kylix_rd, 2017/06/06 18:22:35]

Done.

+
+BoxLayout::ViewWrapper::ViewWrapper()
+    : view_(nullptr), layout_(nullptr), orientation_(kHorizontal) {}
+
+BoxLayout::ViewWrapper::ViewWrapper(const BoxLayout* layout, View* view)
+    : view_(view), layout_(layout), orientation_(layout_->orientation_) {
+  gfx::Insets* margins = view_ ? view_->GetProperty(kMarginsKey) : nullptr;
+  if (margins)
+    margins_ = *margins;
+}
+
+BoxLayout::ViewWrapper::~ViewWrapper() {}
+
+int BoxLayout::ViewWrapper::GetHeightForWidth(int width) const {
+  if (layout_->collapse_margins_spacing_)

[sky, 2017/06/05 21:04:10]

This is subtle and worth a comment. In particular *why* does this not include
the margins?

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    return view_->GetHeightForWidth(width);
+  if (orientation_ == Orientation::kHorizontal) {
+    return view_->GetHeightForWidth(std::max(0, width - margins_.width())) +
+           margins_.height();
+  } else {

[sky, 2017/06/05 21:04:10]

no else after early return.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    return view_->GetHeightForWidth(width) + margins_.height();
+  }
+}
+
+gfx::Size BoxLayout::ViewWrapper::GetPreferredSize() const {
+  gfx::Size preferred_size = view_->GetPreferredSize();
+  if (!layout_->collapse_margins_spacing_)
+    preferred_size.Enlarge(margins_.width(), margins_.height());
+  return preferred_size;
+}
+
+void BoxLayout::ViewWrapper::SetBoundsRect(const gfx::Rect& bounds) {
+  gfx::Rect new_bounds = bounds;
+  if (!layout_->collapse_margins_spacing_) {
+    if (orientation_ == Orientation::kHorizontal) {
+      new_bounds.set_x(bounds.x() + margins_.left());
+      new_bounds.set_width(std::max(0, bounds.width() - margins_.width()));
+    }
+    if (orientation_ == Orientation::kVertical) {

[sky, 2017/06/05 21:04:10]

Replace this if with an else.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+      new_bounds.set_y(bounds.y() + margins_.top());
+      new_bounds.set_height(std::max(0, bounds.height() - margins_.height()));
+    }
+  }
+  view_->SetBoundsRect(new_bounds);
+}
+
+bool BoxLayout::ViewWrapper::visible() const {
+  return view_->visible();
+}
+
 BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
                      int inside_border_horizontal_spacing,
                      int inside_border_vertical_spacing,
-                     int between_child_spacing)
+                     int between_child_spacing,
+                     bool collapse_margins_spacing)
     : orientation_(orientation),
       inside_border_insets_(inside_border_vertical_spacing,
                             inside_border_horizontal_spacing),
       between_child_spacing_(between_child_spacing),
       main_axis_alignment_(MAIN_AXIS_ALIGNMENT_START),
       cross_axis_alignment_(CROSS_AXIS_ALIGNMENT_STRETCH),
       default_flex_(0),
       minimum_cross_axis_size_(0),
-      host_(NULL) {
-}
+      collapse_margins_spacing_(collapse_margins_spacing),
+      host_(NULL) {}
 
 BoxLayout::~BoxLayout() {
 }
 
 void BoxLayout::SetFlexForView(const View* view, int flex_weight) {
   DCHECK(host_);
   DCHECK(view);
   DCHECK_EQ(host_, view->parent());
   DCHECK_GE(flex_weight, 0);
   flex_map_[view] = flex_weight;
 }
 
 void BoxLayout::ClearFlexForView(const View* view) {
   DCHECK(view);
   flex_map_.erase(view);
 }
 
 void BoxLayout::SetDefaultFlex(int default_flex) {
   DCHECK_GE(default_flex, 0);
   default_flex_ = default_flex;
 }
 
 void BoxLayout::Layout(View* host) {
   DCHECK_EQ(host_, host);
-  gfx::Rect child_area(host->GetLocalBounds());
-  child_area.Inset(host->GetInsets());
-  child_area.Inset(inside_border_insets_);
+  gfx::Rect child_area(host->GetContentsBounds());
+
+  AdjustMainAxisForMargin(&child_area);
+  gfx::Insets max_cross_axis_margin;

[sky, 2017/06/05 21:04:10]

Early out if child_area empty?

[kylix_rd, 2017/06/06 18:22:34]

Done.

+  if (!collapse_margins_spacing_) {
+    AdjustCrossAxisForInsets(&child_area);
+    max_cross_axis_margin = CrossAxisMaxViewMargin();
+  }
 
   int total_main_axis_size = 0;
   int num_visible = 0;
   int flex_sum = 0;
   // Calculate the total size of children in the main axis.
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    const ViewWrapper child(this, host->child_at(i));
+    if (!child.visible())
       continue;
-    int flex = GetFlexForView(child);
+    int flex = GetFlexForView(child.view());
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     if (child_main_axis_size == 0 && flex == 0)
       continue;
-    total_main_axis_size += child_main_axis_size + between_child_spacing_;
+    total_main_axis_size += child_main_axis_size +
+                            MainAxisMarginBetweenViews(
+                                child, ViewWrapper(this, NextVisibleView(i)));
     ++num_visible;
     flex_sum += flex;
   }
 
   if (!num_visible)
     return;
 
   total_main_axis_size -= between_child_spacing_;
   // Free space can be negative indicating that the views want to overflow.
   int main_free_space = MainAxisSize(child_area) - total_main_axis_size;
@@ skipping 20 matching lines @@
     gfx::Rect new_child_area(child_area);
     SetMainAxisPosition(position, &new_child_area);
     SetMainAxisSize(size, &new_child_area);
     child_area.Intersect(new_child_area);
   }
 
   int main_position = MainAxisPosition(child_area);
   int total_padding = 0;
   int current_flex = 0;
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    ViewWrapper child(this, host->child_at(i));
+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
     // the cross axis size because child Views may calculate their cross axis
     // size based on their main axis size. See https://crbug.com/682266.
 
     // Calculate cross axis size.
     gfx::Rect bounds(child_area);
+    gfx::Rect min_child_area(child_area);
+    gfx::Insets child_margins;
+    if (collapse_margins_spacing_)
+      child_margins = MaxAxisInsets(

[sky, 2017/06/05 21:04:10]

{}

[kylix_rd, 2017/06/06 18:22:35]

Done.

+          orientation_ == kVertical ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+          child.margins(), inside_border_insets_, child.margins(),
+          inside_border_insets_);
+    else
+      child_margins = child.margins();
+    if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_STRETCH ||
+        cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      InsetCrossAxis(&min_child_area, CrossAxisLeadingInset(child_margins),
+                     CrossAxisTrailingInset(child_margins));
+    }
+
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
-      int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
+      int cross_axis_margin_size = CrossAxisMarginSizeForView(child);
+      int view_cross_axis_size =
+          CrossAxisSizeForView(child) - cross_axis_margin_size;
+      int free_space = CrossAxisSize(bounds) - view_cross_axis_size;
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+        if (view_cross_axis_size > CrossAxisSize(min_child_area))
+          view_cross_axis_size = CrossAxisSize(min_child_area);
         position += free_space / 2;
+        position = std::max(position, CrossAxisLeadingEdge(min_child_area));
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
-        position += free_space;
+        position += free_space - CrossAxisTrailingInset(max_cross_axis_margin);
+        if (!collapse_margins_spacing_)
+          InsetCrossAxis(&min_child_area,
+                         CrossAxisLeadingInset(child.margins()),
+                         CrossAxisTrailingInset(max_cross_axis_margin));
+      } else {
+        position += CrossAxisLeadingInset(max_cross_axis_margin);
+        if (!collapse_margins_spacing_)
+          InsetCrossAxis(&min_child_area,
+                         CrossAxisLeadingInset(max_cross_axis_margin),
+                         CrossAxisTrailingInset(child.margins()));
       }
       SetCrossAxisPosition(position, &bounds);
-      SetCrossAxisSize(CrossAxisSizeForView(child), &bounds);
+      SetCrossAxisSize(view_cross_axis_size, &bounds);
     }
 
     // Calculate flex padding.
     int current_padding = 0;
-    if (GetFlexForView(child) > 0) {
-      current_flex += GetFlexForView(child);
+    if (GetFlexForView(child.view()) > 0) {
+      current_flex += GetFlexForView(child.view());
       int quot = (main_free_space * current_flex) / flex_sum;
       int rem = (main_free_space * current_flex) % flex_sum;
       current_padding = quot - total_padding;
       // Use the current remainder to round to the nearest pixel.
       if (std::abs(rem) * 2 >= flex_sum)
         current_padding += main_free_space > 0 ? 1 : -1;
       total_padding += current_padding;
     }
 
     // Set main axis size.
     // TODO(bruthig): Use the allocated width to determine the cross axis size.
     // See https://crbug.com/682266.
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     SetMainAxisSize(child_main_axis_size + current_padding, &bounds);
-    if (MainAxisSize(bounds) > 0 || GetFlexForView(child) > 0)
-      main_position += MainAxisSize(bounds) + between_child_spacing_;
+    if (MainAxisSize(bounds) > 0 || GetFlexForView(child.view()) > 0)
+      main_position += MainAxisSize(bounds) +
+                       MainAxisMarginBetweenViews(
+                           child, ViewWrapper(this, NextVisibleView(i)));
 
     // Clamp child view bounds to |child_area|.
-    bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    bounds.Intersect(min_child_area);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
   if (flex_sum)
     DCHECK_EQ(total_padding, main_free_space);
 }
 
 gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   DCHECK_EQ(host_, host);
   // Calculate the child views' preferred width.
   int width = 0;
   if (orientation_ == kVertical) {
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    int leading = 0;

[sky, 2017/06/05 21:04:10]

This whole block could use a comment.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    int trailing = 0;
+    gfx::Rect child_view_area;
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
-      width = std::max(width, child->GetPreferredSize().width());
+      gfx::Size child_size = child.view()->GetPreferredSize();

[sky, 2017/06/05 21:04:10]

This is worth a comment as to why you don't use the wrapper's GetPreferredSize
function.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+      gfx::Insets child_margins;
+      if (collapse_margins_spacing_)

[sky, 2017/06/05 21:04:10]

use {} as body spans multiple lines.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+        child_margins = MaxAxisInsets(HORIZONTAL_AXIS, child.margins(),
+                                      inside_border_insets_, child.margins(),
+                                      inside_border_insets_);
+      else
+        child_margins = child.margins();
+      if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_START) {
+        leading = std::max(leading, CrossAxisLeadingInset(child_margins));
+        width = std::max(
+            width, child_size.width() + CrossAxisTrailingInset(child_margins));
+      } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
+        trailing = std::max(trailing, CrossAxisTrailingInset(child_margins));
+        width = std::max(
+            width, child_size.width() + CrossAxisLeadingInset(child_margins));
+      } else {
+        gfx::Rect child_bounds =
+            gfx::Rect(-(child_size.width() / 2), 0, child_size.width(),

[sky, 2017/06/05 21:04:10]

Shouldn't you use the margins in this code path?

[kylix_rd, 2017/06/06 18:22:34]

No. This code is "centering" the view horizontally by using 0 as the
center-point. Since we don't have a rectangle which can be used to calculate a
common center point, a single known point (0) is used.

That's OK here, because we're not interested in the actual position of the view.
We're only interested in the overall width including the margins.

Comment added to that effect.

+                      child_size.height());
+        child_bounds.Inset(-child.margins().left(), 0, -child.margins().right(),
+                           0);
+        child_view_area.Union(child_bounds);
+        width = std::max(width, child_view_area.width());
+      }
     }
-    width = std::max(width, minimum_cross_axis_size_);
+    width = std::max(width + leading + trailing, minimum_cross_axis_size_);
   }
 
   return GetPreferredSizeForChildWidth(host, width);
 }
 
 int BoxLayout::GetPreferredHeightForWidth(const View* host, int width) const {
   DCHECK_EQ(host_, host);
   int child_width = width - NonChildSize(host).width();
   return GetPreferredSizeForChildWidth(host, child_width).height();
 }
 
 void BoxLayout::Installed(View* host) {
   DCHECK(!host_);
   host_ = host;
 }
 
 void BoxLayout::ViewRemoved(View* host, View* view) {
   ClearFlexForView(view);
 }
 
 int BoxLayout::GetFlexForView(const View* view) const {
-  std::map<const View*, int>::const_iterator it = flex_map_.find(view);
+  FlexMap::const_iterator it = flex_map_.find(view);
   if (it == flex_map_.end())
     return default_flex_;
 
   return it->second;
 }
 
 int BoxLayout::MainAxisSize(const gfx::Rect& rect) const {
   return orientation_ == kHorizontal ? rect.width() : rect.height();
 }
 
@@ skipping 30 matching lines @@
     rect->set_height(size);
 }
 
 void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
   if (orientation_ == kVertical)
     rect->set_x(position);
   else
     rect->set_y(position);
 }
 
-int BoxLayout::MainAxisSizeForView(const View* view,
+int BoxLayout::MainAxisSizeForView(const ViewWrapper& view,
                                    int child_area_width) const {
   return orientation_ == kHorizontal
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(cross_axis_alignment_ ==
-                                               CROSS_AXIS_ALIGNMENT_STRETCH
-                                           ? child_area_width
-                                           : view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(cross_axis_alignment_ ==
+                                              CROSS_AXIS_ALIGNMENT_STRETCH
+                                          ? child_area_width
+                                          : view.GetPreferredSize().width());
 }
 
-int BoxLayout::CrossAxisSizeForView(const View* view) const {
+int BoxLayout::MainAxisLeadingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.left() : insets.top();
+}
+
+int BoxLayout::MainAxisTrailingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.right() : insets.bottom();
+}
+
+int BoxLayout::CrossAxisLeadingEdge(const gfx::Rect& rect) const {
+  return orientation_ == kVertical ? rect.x() : rect.y();
+}
+
+int BoxLayout::CrossAxisLeadingInset(const gfx::Insets& insets) const {
+  return orientation_ == kVertical ? insets.left() : insets.top();
+}
+
+int BoxLayout::CrossAxisTrailingInset(const gfx::Insets& insets) const {
+  return orientation_ == kVertical ? insets.right() : insets.bottom();
+}
+
+int BoxLayout::MainAxisMarginBetweenViews(const ViewWrapper& leading,
+                                          const ViewWrapper& trailing) const {
+  if (!collapse_margins_spacing_ || !leading.view() || !trailing.view())
+    return between_child_spacing_;
+  return std::max(between_child_spacing_,
+                  std::max(MainAxisTrailingInset(leading.margins()),
+                           MainAxisLeadingInset(trailing.margins())));
+}
+
+gfx::Insets BoxLayout::MainAxisOuterMargin() const {
+  if (collapse_margins_spacing_) {
+    const ViewWrapper first(this, FirstVisibleView());
+    const ViewWrapper last(this, LastVisibleView());
+    return MaxAxisInsets(
+        orientation_ == kHorizontal ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+        inside_border_insets_, first.margins(), inside_border_insets_,
+        last.margins());
+  }
+  return MaxAxisInsets(
+      orientation_ == kHorizontal ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+      inside_border_insets_, gfx::Insets(), inside_border_insets_,
+      gfx::Insets());
+}
+
+gfx::Insets BoxLayout::CrossAxisMaxViewMargin() const {
+  int leading = 0;
+  int trailing = 0;
+  for (int i = 0; i < host_->child_count(); ++i) {
+    const ViewWrapper child(this, host_->child_at(i));
+    if (!child.visible())
+      continue;
+    leading = std::max(leading, CrossAxisLeadingInset(child.margins()));
+    trailing = std::max(trailing, CrossAxisTrailingInset(child.margins()));
+  }
+  if (orientation_ == Orientation::kVertical)
+    return gfx::Insets(0, leading, 0, trailing);
+  return gfx::Insets(leading, 0, trailing, 0);
+}
+
+void BoxLayout::AdjustMainAxisForMargin(gfx::Rect* rect) const {
+  rect->Inset(MainAxisOuterMargin());
+}
+
+void BoxLayout::AdjustCrossAxisForInsets(gfx::Rect* rect) const {
+  rect->Inset(orientation_ == Orientation::kVertical
+                  ? gfx::Insets(0, inside_border_insets_.left(), 0,
+                                inside_border_insets_.right())
+                  : gfx::Insets(inside_border_insets_.top(), 0,
+                                inside_border_insets_.bottom(), 0));
+}
+
+int BoxLayout::CrossAxisSizeForView(const ViewWrapper& view) const {
   // TODO(bruthig): For horizontal case use the available width and not the
   // preferred width. See https://crbug.com/682266.
   return orientation_ == kVertical
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(view.GetPreferredSize().width());
+}
+
+int BoxLayout::CrossAxisMarginSizeForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_
+             ? 0
+             : (orientation_ == kVertical ? view.margins().width()
+                                          : view.margins().height());
+}
+
+int BoxLayout::CrossAxisLeadingMarginForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_ ? 0 : CrossAxisLeadingInset(view.margins());
+}
+
+void BoxLayout::InsetCrossAxis(gfx::Rect* rect,
+                               int leading,
+                               int trailing) const {
+  if (orientation_ == kVertical)
+    rect->Inset(leading, 0, trailing, 0);
+  else
+    rect->Inset(0, leading, 0, trailing);
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
                                                    int child_area_width) const {
+  DCHECK_EQ(host, host_);
   gfx::Rect child_area_bounds;
 
   if (orientation_ == kHorizontal) {
     // Horizontal layouts ignore |child_area_width|, meaning they mimic the
     // default behavior of GridLayout::GetPreferredHeightForWidth().
     // TODO(estade|bruthig): Fix this See // https://crbug.com/682266.
     int position = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    gfx::Insets max_margins = CrossAxisMaxViewMargin();
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
-      gfx::Size size(child->GetPreferredSize());
+      gfx::Size size(child.view()->GetPreferredSize());
       if (size.IsEmpty())
         continue;
 
-      gfx::Rect child_bounds(position, 0, size.width(), size.height());
+      gfx::Rect child_bounds = gfx::Rect(
+          position, 0,
+          size.width() +
+              (!collapse_margins_spacing_ ? child.margins().width() : 0),
+          size.height());
+      gfx::Insets child_margins;
+      if (collapse_margins_spacing_)
+        child_margins =
+            MaxAxisInsets(VERTICAL_AXIS, child.margins(), inside_border_insets_,
+                          child.margins(), inside_border_insets_);
+      else
+        child_margins = child.margins();
+
+      if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_START) {
+        child_bounds.Inset(0, -CrossAxisLeadingInset(max_margins), 0,
+                           -child_margins.bottom());
+        child_bounds.set_origin(gfx::Point(position, 0));
+      } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
+        child_bounds.Inset(0, -child_margins.top(), 0,
+                           -CrossAxisTrailingInset(max_margins));
+        child_bounds.set_origin(gfx::Point(position, 0));
+      } else {
+        child_bounds.set_origin(
+            gfx::Point(position, -(child_bounds.height() / 2)));
+        child_bounds.Inset(0, -child_margins.top(), 0, -child_margins.bottom());
+      }
+
       child_area_bounds.Union(child_bounds);
-      position += size.width() + between_child_spacing_;
+      position += child_bounds.width() +
+                  MainAxisMarginBetweenViews(
+                      child, ViewWrapper(this, NextVisibleView(i)));

[sky, 2017/06/05 21:04:10]

optional: I have to admit I'm mildly bothered that NextVisibleView iterates, and
then the for loop iterates (this comment applies to other for loops that share
the same pattern).

[kylix_rd, 2017/06/06 18:22:34]

Understandable. It could be incorporated into the loop itself, but I fear it may
make the code more dense.

     }
     child_area_bounds.set_height(
         std::max(child_area_bounds.height(), minimum_cross_axis_size_));
   } else {
     int height = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
+      const ViewWrapper next(this, NextVisibleView(i));
       // Use the child area width for getting the height if the child is
       // supposed to stretch. Use its preferred size otherwise.
       int extra_height = MainAxisSizeForView(child, child_area_width);
       // Only add |between_child_spacing_| if this is not the only child.
-      if (height != 0 && extra_height > 0)
-        height += between_child_spacing_;
+      if (next.view() && extra_height > 0)
+        height += MainAxisMarginBetweenViews(child, next);
       height += extra_height;
     }
 
     child_area_bounds.set_width(child_area_width);
     child_area_bounds.set_height(height);
   }
 
-  gfx::Size non_child_size = NonChildSize(host);
+  gfx::Size non_child_size = NonChildSize(host_);
   return gfx::Size(child_area_bounds.width() + non_child_size.width(),
                    child_area_bounds.height() + non_child_size.height());
 }
 
 gfx::Size BoxLayout::NonChildSize(const View* host) const {
   gfx::Insets insets(host->GetInsets());
-  return gfx::Size(insets.width() + inside_border_insets_.width(),
-                   insets.height() + inside_border_insets_.height());
+  if (!collapse_margins_spacing_)
+    return gfx::Size(insets.width() + inside_border_insets_.width(),
+                     insets.height() + inside_border_insets_.height());
+  gfx::Insets main_axis = MainAxisOuterMargin();
+  gfx::Insets cross_axis = inside_border_insets_;
+  return gfx::Size(insets.width() + main_axis.width() + cross_axis.width(),
+                   insets.height() + main_axis.height() + cross_axis.height());
+}
+
+View* BoxLayout::NextVisibleView(int index) const {
+  for (int i = index + 1; i < host_->child_count(); ++i) {
+    View* result = host_->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
+}
+
+View* BoxLayout::FirstVisibleView() const {
+  return NextVisibleView(-1);
+}
+
+View* BoxLayout::LastVisibleView() const {
+  for (int i = host_->child_count() - 1; i >= 0; --i) {
+    View* result = host_->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
 }
 
 }  // namespace views