Fix views::View::BoundsChanged

ui/views/view_unittest.cc

Index: ui/views/view_unittest.cc
diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
index 38683cc303a93c5be3645c0793ad4adea9bf0993..d398434826b0af19922e1bf75a2f833994de75bd 100644
--- a/ui/views/view_unittest.cc
+++ b/ui/views/view_unittest.cc
@@ -4099,4 +4099,79 @@ TEST_F(ViewTest, OnNativeThemeChanged) {
   widget->CloseNow();
 }
 
+class CustomView : public View {
+ public:
+  CustomView() {}
+  ~CustomView() override {}
+  void Layout() override {
+    layout_was_called = true;
+    if (!has_children())
+      return;
+    View* child = child_at(0);
+    child->SetBoundsRect(GetContentsBounds() +
+                         gfx::Vector2d(child_x_offset, 0));
+  }
+  using View::needs_layout;
+
+  bool layout_was_called = false;

[sky, 2015/11/17 18:37:26]

Make private (style guide says tests can use protected members, not public).
Also, end with _

+  int child_x_offset = 10;
+};

[sky, 2015/11/17 18:37:26]

private: DISALLOW...

+
+TEST_F(ViewTest, SimpleInvalidateLayout) {

[sky, 2015/11/17 18:37:26]

Please add description of what assertions your test is verifying. Especially
when the name is very generic. Same comment for test below.

+  scoped_ptr<CustomView> parent(new CustomView);

[sky, 2015/11/17 18:37:26]

Why use a scoped_ptr here?

+  CustomView* child = new CustomView;
+
+  parent->SetBounds(0, 0, 100, 100);
+
+  parent->layout_was_called = false;
+  parent->AddChildView(child);
+  // SetBounds must call layout, because AddChildView invalidates layout of
+  // |parent|.
+  parent->SetBounds(10, 0, 100, 100);

[flint, 2015/11/13 10:05:37]

Layout of parent is invalidated during parent->AddChildView(child) call:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s...
parent->SetBounds does not call its Layout because in the bounds of parent only
origin has changed. As a result, bounds of child will not be updated.

+
+  EXPECT_EQ(parent->child_x_offset, child->bounds().x());
+  EXPECT_TRUE(parent->layout_was_called);
+  EXPECT_FALSE(parent->needs_layout());
+
+  child->layout_was_called = false;
+  child->InvalidateLayout();
+  // SetBounds must call layout, because we invalidates layout of |child| and
+  // parent|.
+  parent->SetBounds(20, 0, 100, 100);

[flint, 2015/11/13 10:05:37]

The child view needs layout (i.e. its contents became bigger) and calls
InvalidateLayout. But parent's Layout is not called during parent->SetBounds
because only parent's bounds' origin changed. So child's bounds will not be
changed. And Layout of child will not be called too.

If a view invalidates layout, we have to call its Layout. Because of this, now
View::SetBoundsRect calls Layout even if bounds were not changed (e.g. when it
handles DragNDrop):
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s...
But it does not call Layout if the bounds' origin changed, and bounds' size did
not change.

+
+  EXPECT_EQ(parent->child_x_offset, child->bounds().x());
+  EXPECT_TRUE(parent->layout_was_called);
+  EXPECT_FALSE(parent->needs_layout());
+  EXPECT_FALSE(child->needs_layout());
+}
+
+TEST_F(ViewTest, InvalidateLayout) {
+  scoped_ptr<CustomView> parent(new CustomView);

[sky, 2015/11/17 18:37:26]

Same commeont about scoped_ptr.

+  CustomView* child = new CustomView;
+  CustomView* subchild = new CustomView;
+
+  parent->AddChildView(child);
+  parent->SetBounds(0, 0, 100, 100);
+
+  child->layout_was_called = false;
+  // AddChildView invalidates Layout of |child|.
+  child->AddChildView(subchild);
+  parent->child_x_offset += 100;
+  parent->Layout();
+  // But layout of child mist be called.
+  EXPECT_TRUE(child->layout_was_called);
+  EXPECT_EQ(10, subchild->bounds().x());
+  EXPECT_FALSE(child->needs_layout());
+
+  // If child origin changed Layout of child must call.
+  parent->child_x_offset += 100;
+  child->InvalidateLayout();
+  // Layout of |child| must be called because we invalidate it layout.
+  parent->Layout();
+
+  EXPECT_TRUE(child->layout_was_called);
+  EXPECT_EQ(10, subchild->bounds().x());
+  EXPECT_FALSE(child->needs_layout());
+}
+
 }  // namespace views