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"
 
 namespace views {
 
+BoxLayout::ViewWrapper::ViewWrapper()
+    : view_(nullptr), layout_(nullptr), margins_(0, 0, 0, 0){};

[sky, 2017/04/25 17:28:58]

(0,0,0,0) is the default, so need to specify margins_ here.

[kylix_rd, 2017/04/26 17:41:33]

Done.

+
+BoxLayout::ViewWrapper::ViewWrapper(const BoxLayout* layout, const View* view)
+    : view_(view), layout_(layout), margins_(layout_->GetViewMargins(view)) {}
+
+BoxLayout::ViewWrapper::ViewWrapper(const ViewWrapper& wrapper)

[sky, 2017/04/25 17:28:58]

= default?

[kylix_rd, 2017/04/26 17:41:33]

Done.

+    : view_(wrapper.view_),
+      layout_(wrapper.layout_),
+      margins_(wrapper.margins_) {}
+
+BoxLayout::ViewWrapper::~ViewWrapper() {}
+
+gfx::Size BoxLayout::ViewWrapper::GetPreferredSize() const {
+  gfx::Size preferred_size = view_->GetPreferredSize();
+  preferred_size.Enlarge(margins_.width(), margins_.height());
+  return preferred_size;
+}
+
+void BoxLayout::ViewWrapper::SetBoundsRect(const gfx::Rect& bounds) {
+  const_cast<View*>(view_)->SetBounds(

[sky, 2017/04/25 17:28:58]

Why the const_cast here?

[kylix_rd, 2017/04/26 17:41:33]

The view_ field is declared as "const View* view_;" Most references to the view
are const. So I elected to cast the "const" away only at the one point that
modifies the view.

[sky, 2017/04/26 19:57:07]

If you're going to const_cast, why make the member const? Otherwise, what's the
point?

+      bounds.x() + margins_.left(), bounds.y() + margins_.top(),
+      bounds.width() - margins_.width(), bounds.height() - margins_.height());
+}
+
+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)
     : 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),
@@ skipping 16 matching lines @@
 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::ClearMarginsForView(const View* view) {
+  margin_map_.erase(view);
+}
+
+void BoxLayout::SetViewMargins(const View* view, const gfx::Insets& margins) {
+  margin_map_[view] = margins;
+}
+
 void BoxLayout::Layout(View* host) {
   DCHECK_EQ(host_, host);
-  gfx::Rect child_area(host->GetLocalBounds());
-  child_area.Inset(host->GetInsets());
+  gfx::Rect child_area(host->GetContentsBounds());
   child_area.Inset(inside_border_insets_);
 
   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 = ViewWrapper(this, host->child_at(i));
+    if (!child.visible())
       continue;
-    int flex = GetFlexForView(child);
-    int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
+    int flex = GetFlexForView(child.view());
+    int child_main_axis_size =
+        MainAxisSizeForView(child.view(), child_area.width());
     if (child_main_axis_size == 0 && flex == 0)
       continue;
     total_main_axis_size += child_main_axis_size + between_child_spacing_;
     ++num_visible;
     flex_sum += flex;
   }
 
   if (!num_visible)
     return;
 
@@ skipping 23 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 = ViewWrapper(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);
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
-      int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
+      int free_space =
+          CrossAxisSize(bounds) - CrossAxisSizeForView(child.view());
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
         position += free_space / 2;
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
         position += free_space;
       }
       SetCrossAxisPosition(position, &bounds);
-      SetCrossAxisSize(CrossAxisSizeForView(child), &bounds);
+      SetCrossAxisSize(CrossAxisSizeForView(child.view()), &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());
+    int child_main_axis_size =
+        MainAxisSizeForView(child.view(), child_area.width());
     SetMainAxisSize(child_main_axis_size + current_padding, &bounds);
-    if (MainAxisSize(bounds) > 0 || GetFlexForView(child) > 0)
+    if (MainAxisSize(bounds) > 0 || GetFlexForView(child.view()) > 0)
       main_position += MainAxisSize(bounds) + between_child_spacing_;
 
     // Clamp child view bounds to |child_area|.
     bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    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())
+      const ViewWrapper child = ViewWrapper(this, host->child_at(i));
+      if (!child.visible())
         continue;
 
-      width = std::max(width, child->GetPreferredSize().width());
+      width = std::max(width, child.GetPreferredSize().width());
     }
     width = std::max(width, 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);
+  ClearMarginsForView(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 48 matching lines @@
 }
 
 int BoxLayout::CrossAxisSizeForView(const View* 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());
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,

[sky, 2017/04/25 22:46:07]

It seems like what you're doing here is getting the max of the heights, which
includes the margins. Shouldn't you calculate the max of the margins and then
add that to that the max of the views? For example, if the alignment is
horizontal and you have two views, A and B then I believe you're calculating the
pref height as:

Max(a_wrapper.preferred_size().height(), b_wrapper.preferred_size().height());

where _wrapper.preferred_size() includes the margin.

Don't you want:
Max(A.margin.top, B.margin.top) +
Max(a.preferred_size.height(), b.preferred_size().height()) +
Max(A.margin.bottom, B.margin.bottom);
?
Similarly when you are positioning views I would assume you position them
relative to the max of the margins along the opposite axis.

[kylix_rd, 2017/04/26 17:41:33]

The next patch should resolve this confusion. The short answer is that the
width/height of each view always includes its margins. No collapsing is done at
this time.

                                                    int child_area_width) const {
   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())
+      const ViewWrapper child = ViewWrapper(this, host->child_at(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_;
     }
     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())
+      const ViewWrapper child = ViewWrapper(this, host->child_at(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);
+      int extra_height = MainAxisSizeForView(child.view(), 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_;
       height += extra_height;
     }
 
     child_area_bounds.set_width(child_area_width);
     child_area_bounds.set_height(height);
   }
 
   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());
 }
 
+gfx::Insets BoxLayout::GetViewMargins(const View* view) const {
+  MarginMap::const_iterator pos = margin_map_.find(view);
+  if (pos != margin_map_.cend())
+    return pos->second;
+  return gfx::Insets();
+}
+
 }  // namespace views