BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

Index: ui/views/layout/box_layout.cc
diff --git a/ui/views/layout/box_layout.cc b/ui/views/layout/box_layout.cc
index 7e5abff4880af33a0b34583c3d0c7f127b7480c5..dd5edcad14eabe94b515add802de1451e6adc0c6 100644
--- a/ui/views/layout/box_layout.cc
+++ b/ui/views/layout/box_layout.cc
@@ -8,13 +8,66 @@
 
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/view.h"
+#include "ui/views/view_properties.h"
 
 namespace views {
 
+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_) {
+    return view_->GetHeightForWidth(width);
+  } else if (orientation_ == Orientation::kHorizontal) {

[sky, 2017/05/24 21:28:51]

style guide says no if after return.

[kylix_rd, 2017/05/25 14:32:17]

Done.

+    return view_->GetHeightForWidth(std::max(0, width - margins_.width())) +
+           margins_.height();
+  } else {
+    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 ||
+        layout_->cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      new_bounds.set_x(bounds.x() + margins_.left());
+      new_bounds.set_width(std::max(0, bounds.width() - margins_.width()));
+    }
+    if (orientation_ == Orientation::kVertical ||
+        layout_->cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      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),
@@ -23,8 +76,8 @@ BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
       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() {
 }
@@ -49,23 +102,27 @@ void BoxLayout::SetDefaultFlex(int 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);
+  AdjustCrossAxisForInsets(&child_area);
+  gfx::Insets 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())
+    ViewWrapper child(this, host->child_at(i));
+    ViewWrapper next(this, NextVisibleView(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, next);
     ++num_visible;
     flex_sum += flex;
   }
@@ -106,8 +163,9 @@ void BoxLayout::Layout(View* host) {
   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));
+    ViewWrapper next(this, NextVisibleView(i));
+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
@@ -118,21 +176,26 @@ void BoxLayout::Layout(View* host) {
     gfx::Rect bounds(child_area);
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
-      int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
+      int view_cross_axis_size = CrossAxisSizeForView(child);
+      int cross_axis_margin_width = CrossAxisMarginSizeForView(child);
+      int free_space = CrossAxisSize(bounds) - view_cross_axis_size;
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
-        position += free_space / 2;
+        position += ((free_space + cross_axis_margin_width) / 2) -
+                    CrossAxisMarginTopLeftForView(child);
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
-        position += free_space;
+        position += free_space + cross_axis_margin_width;
+      } else {
+        view_cross_axis_size -= cross_axis_margin_width;
       }
       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;
@@ -147,12 +210,19 @@ void BoxLayout::Layout(View* host) {
     // 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, next);
 
     // Clamp child view bounds to |child_area|.
-    bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    gfx::Rect min_child_area(child_area);
+    if (collapse_margins_spacing_ ||
+        cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_CENTER)
+      min_child_area.Inset(max_cross_axis_margin);
+    else
+      min_child_area.Inset(max_cross_axis_margin - child.margins());
+    bounds.Intersect(min_child_area);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
@@ -165,12 +235,13 @@ gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   // 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())
+    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.GetPreferredSize();
+      width = std::max(width, child_size.width());
     }
     width = std::max(width, minimum_cross_axis_size_);
   }
@@ -194,7 +265,7 @@ void BoxLayout::ViewRemoved(View* host, View* 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_;
 
@@ -245,26 +316,134 @@ void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
     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::MainAxisMarginBetweenViews(const ViewWrapper& left,
+                                          const ViewWrapper& right) const {
+  if (!collapse_margins_spacing_ || !left.view() || !right.view()) {
+    return between_child_spacing_;
+  } else if (orientation_ == Orientation::kHorizontal) {
+    return std::max(between_child_spacing_,
+                    std::max(left.margins().right(), right.margins().left()));

[sky, 2017/05/24 21:28:51]

I think this code would be easier to read if you isolate the conditionals based
on orientation to helper functions. e.g.

return std::max(between_child_spacing_, 
                std::max(MainAxisLeadingInset(left.margins()),
                         MainAxisTrailingOffset(right.margins())));

MainAxisLeadingInset could also be used on 348, and I suspect other places.
Easier to ensure the conditionals are right in one places vs a bunch of
functions.

You should extract similar functions for CrossAxisMargin calculation.

[kylix_rd, 2017/05/25 14:32:16]

Done.

+  } else {

[sky, 2017/05/24 21:28:51]

no else

[kylix_rd, 2017/05/25 14:32:17]

Done.

+    return std::max(between_child_spacing_,
+                    std::max(left.margins().bottom(), right.margins().top()));
+  }
+}
+
+gfx::Insets BoxLayout::MainAxisOuterMargin() const {
+  if (collapse_margins_spacing_) {
+    const ViewWrapper first(this, FirstVisibleView());
+    const ViewWrapper last(this, LastVisibleView());
+    if (orientation_ == Orientation::kHorizontal) {
+      return gfx::Insets(
+          0, std::max(inside_border_insets_.left(), first.margins().left()), 0,
+          std::max(inside_border_insets_.right(), last.margins().right()));
+    } else {

[sky, 2017/05/24 21:28:51]

no else

[kylix_rd, 2017/05/25 14:32:16]

Done.

+      return gfx::Insets(
+          std::max(inside_border_insets_.top(), first.margins().top()), 0,
+          std::max(inside_border_insets_.bottom(), last.margins().bottom()), 0);
+    }
+  } else {

[sky, 2017/05/24 21:28:51]

no else

[kylix_rd, 2017/05/25 14:32:17]

Done.

+    if (orientation_ == Orientation::kHorizontal) {
+      return gfx::Insets(0, inside_border_insets_.left(), 0,
+                         inside_border_insets_.right());
+    } else {

[sky, 2017/05/24 21:28:51]

no else

[kylix_rd, 2017/05/25 14:32:16]

Done.

+      return gfx::Insets(inside_border_insets_.top(), 0,
+                         inside_border_insets_.bottom(), 0);
+    }
+  }
+}
+
+gfx::Insets BoxLayout::CrossAxisMaxViewMargin() const {
+  bool is_vertical = orientation_ == Orientation::kVertical;
+  int top_or_left = 0;
+  int bottom_or_right = 0;
+  for (int i = 0; i < host_->child_count(); ++i) {
+    const ViewWrapper child(this, host_->child_at(i));
+    if (!child.visible())
+      continue;
+    top_or_left = std::max(top_or_left, is_vertical ? child.margins().left()
+                                                    : child.margins().top());
+    bottom_or_right =
+        std::max(bottom_or_right, is_vertical ? child.margins().right()
+                                              : child.margins().bottom());
+  }
+  if (is_vertical)
+    return gfx::Insets(0, top_or_left, 0, bottom_or_right);
+  else

[sky, 2017/05/24 21:28:51]

no else.

[kylix_rd, 2017/05/25 14:32:17]

Done.

+    return gfx::Insets(top_or_left, 0, bottom_or_right, 0);
+}
+
+gfx::Insets BoxLayout::CrossAxisOuterMargin() const {
+  gfx::Insets max_margin = CrossAxisMaxViewMargin();
+  bool is_vertical = orientation_ == Orientation::kVertical;
+  int top_or_left =
+      is_vertical ? inside_border_insets_.left() : inside_border_insets_.top();
+  int bottom_or_right = is_vertical ? inside_border_insets_.right()
+                                    : inside_border_insets_.bottom();
+  if (collapse_margins_spacing_) {
+    if (is_vertical)
+      return gfx::Insets(0, std::max(top_or_left, max_margin.left()), 0,
+                         std::max(bottom_or_right, max_margin.right()));
+    else
+      return gfx::Insets(std::max(top_or_left, max_margin.top()), 0,
+                         std::max(bottom_or_right, max_margin.bottom()), 0);
+  } else {
+    if (is_vertical)
+      return gfx::Insets(0, top_or_left + max_margin.left(), 0,
+                         bottom_or_right + max_margin.right());
+    else
+      return gfx::Insets(top_or_left + max_margin.top(), 0,
+                         bottom_or_right + max_margin.bottom(), 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 View* view) const {
+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::CrossAxisMarginTopLeftForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_
+             ? 0
+             : (orientation_ == kVertical ? view.margins().left()
+                                          : view.margins().top());
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
                                                    int child_area_width) const {
+  DCHECK_EQ(host, host_);
   gfx::Rect child_area_bounds;
 
   if (orientation_ == kHorizontal) {
@@ -272,34 +451,36 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     // 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())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      const ViewWrapper next(this, NextVisibleView(i));
+      if (!child.visible())
         continue;
 
-      gfx::Size size(child->GetPreferredSize());
+      gfx::Size size(child.GetPreferredSize());
       if (size.IsEmpty())
         continue;
 
       gfx::Rect child_bounds(position, 0, size.width(), size.height());
       child_area_bounds.Union(child_bounds);
-      position += size.width() + between_child_spacing_;
+      position += size.width() + MainAxisMarginBetweenViews(child, next);
     }
     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));
+      const ViewWrapper next(this, NextVisibleView(i));
+      if (!child.visible())
         continue;
 
       // 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;
     }
 
@@ -307,15 +488,45 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     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());
+  } else {

[sky, 2017/05/24 21:28:51]

no else

[kylix_rd, 2017/05/25 14:32:17]

Done.

+    gfx::Insets main_axis = MainAxisOuterMargin();
+    gfx::Insets cross_axis = CrossAxisOuterMargin();
+    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