Simplify NonClientView Layout slightly.

The old code assumes children might need layout even when not invalid.  This was
originally added to fix http://crbug.com/8511 .  This seems like a bogus fix; if
the metrics of something in the nonclient area change in such a way that we
actually notice and make it to NonClientView::Layout(), we should have had an
opportunity to invalid child layouts as needed.  I suspect this doesn't actually
happen anymore (some of the rest of that bugfix is already gone), but if it does
we should fix in a different way.

This also reorders definitions to match declaration order, removes a bunch of
section-break comments that are atypical style nowadays, and adds some TODOs for
further refactoring.


Bug: none
Change-Id: Ib386921c85c43ee05473acf1ac1b660a68d28809
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1823642
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712323}

ash/frame/non_client_frame_view_ash.cc

@@ -212,6 +212,10 @@ NonClientFrameViewAsh::NonClientFrameViewAsh(views::Widget* frame)
   window_util::InstallResizeHandleWindowTargeterForWindow(frame_window);
   // |header_view_| is set as the non client view's overlay view so that it can
   // overlay the web contents in immersive fullscreen.
+  // TODO(pkasting): Consider having something like NonClientViewAsh, which
+  // would avoid the need to expose an "overlay view" concept on the
+  // cross-platform class, and might allow for simpler creation/ownership/
+  // plumbing.
   frame->non_client_view()->SetOverlayView(overlay_view_);
 
   // A delegate may be set which takes over the responsibilities of the

ui/views/window/non_client_view.cc

@@ -22,16 +22,72 @@
 
 namespace views {
 
+namespace {
+
 // The frame view and the client view are always at these specific indices,
 // because the RootView message dispatch sends messages to items higher in the
 // z-order first and we always want the client view to have first crack at
 // handling mouse messages.
-static constexpr int kFrameViewIndex = 0;
-static constexpr int kClientViewIndex = 1;
+constexpr int kFrameViewIndex = 0;
+constexpr int kClientViewIndex = 1;
 // The overlay view is always on top (view == children().back()).
 
-////////////////////////////////////////////////////////////////////////////////
-// NonClientFrameView, default implementations:
+}  // namespace
+
+NonClientFrameView::~NonClientFrameView() = default;
+
+bool NonClientFrameView::ShouldPaintAsActive() const {
+  return GetWidget()->ShouldPaintAsActive();
+}
+
+int NonClientFrameView::GetHTComponentForFrame(const gfx::Point& point,
+                                               int top_resize_border_height,
+                                               int resize_border_thickness,
+                                               int top_resize_corner_height,
+                                               int resize_corner_width,
+                                               bool can_resize) {
+  // Tricky: In XP, native behavior is to return HTTOPLEFT and HTTOPRIGHT for
+  // a |resize_corner_size|-length strip of both the side and top borders, but
+  // only to return HTBOTTOMLEFT/HTBOTTOMRIGHT along the bottom border + corner
+  // (not the side border).  Vista goes further and doesn't return these on any
+  // of the side borders.  We allow callers to match either behavior.
+  int component;
+  if (point.x() < resize_border_thickness) {
+    if (point.y() < top_resize_corner_height)
+      component = HTTOPLEFT;
+    else if (point.y() >= (height() - resize_border_thickness))
+      component = HTBOTTOMLEFT;
+    else
+      component = HTLEFT;
+  } else if (point.x() >= (width() - resize_border_thickness)) {
+    if (point.y() < top_resize_corner_height)
+      component = HTTOPRIGHT;
+    else if (point.y() >= (height() - resize_border_thickness))
+      component = HTBOTTOMRIGHT;
+    else
+      component = HTRIGHT;
+  } else if (point.y() < top_resize_border_height) {
+    if (point.x() < resize_corner_width)
+      component = HTTOPLEFT;
+    else if (point.x() >= (width() - resize_corner_width))
+      component = HTTOPRIGHT;
+    else
+      component = HTTOP;
+  } else if (point.y() >= (height() - resize_border_thickness)) {
+    if (point.x() < resize_corner_width)
+      component = HTBOTTOMLEFT;
+    else if (point.x() >= (width() - resize_corner_width))
+      component = HTBOTTOMRIGHT;
+    else
+      component = HTBOTTOM;
+  } else {
+    return HTNOWHERE;
+  }
+
+  // If the window can't be resized, there are no resize boundaries, just
+  // window borders.
+  return can_resize ? component : HTBORDER;
+}
 
 bool NonClientFrameView::GetClientMask(const gfx::Size& size,
                                        SkPath* mask) const {
@@ -49,8 +105,39 @@ gfx::Point NonClientFrameView::GetSystemMenuScreenPixelLocation() const {
 }
 #endif
 
-////////////////////////////////////////////////////////////////////////////////
-// NonClientView, public:
+void NonClientFrameView::PaintAsActiveChanged(bool active) {}
+
+void NonClientFrameView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  node_data->role = ax::mojom::Role::kClient;
+}
+
+void NonClientFrameView::OnThemeChanged() {
+  SchedulePaint();
+}
+
+NonClientFrameView::NonClientFrameView() {
+  SetEventTargeter(std::make_unique<views::ViewTargeter>(this));
+}
+
+// ViewTargeterDelegate:
+bool NonClientFrameView::DoesIntersectRect(const View* target,
+                                           const gfx::Rect& rect) const {
+  CHECK_EQ(target, this);
+
+  // For the default case, we assume the non-client frame view never overlaps
+  // the client view.
+  return !GetWidget()->client_view()->bounds().Intersects(rect);
+}
+
+#if defined(OS_WIN)
+int NonClientFrameView::GetSystemMenuY() const {
+  return GetBoundsForClientView().y();
+}
+#endif
+
+BEGIN_METADATA(NonClientFrameView)
+METADATA_PARENT_CLASS(View)
+END_METADATA()
 
 NonClientView::NonClientView() {
   SetEventTargeter(std::make_unique<views::ViewTargeter>(this));
@@ -130,26 +217,10 @@ void NonClientView::SizeConstraintsChanged() {
   frame_view_->SizeConstraintsChanged();
 }
 
-void NonClientView::LayoutFrameView() {
-  // First layout the NonClientFrameView, which determines the size of the
-  // ClientView...
-  gfx::Rect new_frame_bounds = GetLocalBounds();
-  if (frame_view_->bounds() == new_frame_bounds) {
-    // SetBoundsRect does a |needs_layout_| check if the bounds aren't actually
-    // changing before triggering a layout. Ensure we do a layout either way.
-    frame_view_->Layout();
-  } else {
-    frame_view_->SetBoundsRect(new_frame_bounds);
-  }
-}
-
 void NonClientView::SetAccessibleName(const base::string16& name) {
   accessible_name_ = name;
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// NonClientView, View overrides:
-
 gfx::Size NonClientView::CalculatePreferredSize() const {
   // TODO(pkasting): This should probably be made to look similar to
   // GetMinimumSize() below.  This will require implementing GetPreferredSize()
@@ -167,45 +238,32 @@ gfx::Size NonClientView::GetMaximumSize() const {
 }
 
 void NonClientView::Layout() {
-  LayoutFrameView();
-
-  // Then layout the ClientView, using those bounds.
-  gfx::Rect client_bounds = frame_view_->GetBoundsForClientView();
-
-
-  if (client_bounds != client_view_->bounds()) {
-    client_view_->SetBoundsRect(client_bounds);
-  } else {
-    client_view_->Layout();
-  }
+  // TODO(pkasting): The frame view should have the client view as a child and
+  // lay it out directly + set its clip path.  Done correctly, this should let
+  // us use a FillLayout on this class that holds |frame_view_| and
+  // |overlay_view_|, and eliminate CalculatePreferredSize()/GetMinimumSize()/
+  // GetMaximumSize()/Layout().  The frame view and client view were originally
+  // siblings because "many Views make the assumption they are only inserted
+  // into a View hierarchy once" ( http://codereview.chromium.org/27317 ), but
+  // where that is still the case it should simply be fixed.
+  frame_view_->SetBoundsRect(GetLocalBounds());
+  client_view_->SetBoundsRect(frame_view_->GetBoundsForClientView());
 
   SkPath client_clip;
   if (frame_view_->GetClientMask(client_view_->size(), &client_clip))
     client_view_->set_clip_path(client_clip);
 
-  if (overlay_view_ && overlay_view_->GetVisible())
+  if (overlay_view_)
     overlay_view_->SetBoundsRect(GetLocalBounds());
 }
 
-void NonClientView::ViewHierarchyChanged(
-    const ViewHierarchyChangedDetails& details) {
-  // Add our child views here as we are added to the Widget so that if we are
-  // subsequently resized all the parent-child relationships are established.
-  if (details.is_add && GetWidget() && details.child == this) {
-    AddChildViewAt(frame_view_.get(), kFrameViewIndex);
-    AddChildViewAt(client_view_, kClientViewIndex);
-    if (overlay_view_)
-      AddChildView(overlay_view_);
-  }
-}
-
 void NonClientView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kClient;
   node_data->SetName(accessible_name_);
 }
 
 View* NonClientView::GetTooltipHandlerForPoint(const gfx::Point& point) {
-  // The same logic as for |TargetForRect()| applies here.
+  // The same logic as for TargetForRect() applies here.
   if (frame_view_->parent() == this) {
     // During the reset of the frame_view_ it's possible to be in this code
     // after it's been removed from the view hierarchy but before it's been
@@ -221,6 +279,21 @@ View* NonClientView::GetTooltipHandlerForPoint(const gfx::Point& point) {
   return View::GetTooltipHandlerForPoint(point);
 }
 
+void NonClientView::ViewHierarchyChanged(
+    const ViewHierarchyChangedDetails& details) {
+  // Add our child views here as we are added to the Widget so that if we are
+  // subsequently resized all the parent-child relationships are established.
+  // TODO(pkasting): The above comment makes no sense to me.  Try to eliminate
+  // the various setters, and create and add children directly in the
+  // constructor.
+  if (details.is_add && GetWidget() && details.child == this) {
+    AddChildViewAt(frame_view_.get(), kFrameViewIndex);
+    AddChildViewAt(client_view_, kClientViewIndex);
+    if (overlay_view_)
+      AddChildView(overlay_view_);
+  }
+}
+
 View* NonClientView::TargetForRect(View* root, const gfx::Rect& rect) {
   CHECK_EQ(root, this);
 
@@ -253,102 +326,4 @@ BEGIN_METADATA(NonClientView)
 METADATA_PARENT_CLASS(View)
 END_METADATA()
 
-////////////////////////////////////////////////////////////////////////////////
-// NonClientFrameView, public:
-
-NonClientFrameView::~NonClientFrameView() = default;
-
-bool NonClientFrameView::ShouldPaintAsActive() const {
-  return GetWidget()->ShouldPaintAsActive();
-}
-
-int NonClientFrameView::GetHTComponentForFrame(const gfx::Point& point,
-                                               int top_resize_border_height,
-                                               int resize_border_thickness,
-                                               int top_resize_corner_height,
-                                               int resize_corner_width,
-                                               bool can_resize) {
-  // Tricky: In XP, native behavior is to return HTTOPLEFT and HTTOPRIGHT for
-  // a |resize_corner_size|-length strip of both the side and top borders, but
-  // only to return HTBOTTOMLEFT/HTBOTTOMRIGHT along the bottom border + corner
-  // (not the side border).  Vista goes further and doesn't return these on any
-  // of the side borders.  We allow callers to match either behavior.
-  int component;
-  if (point.x() < resize_border_thickness) {
-    if (point.y() < top_resize_corner_height)
-      component = HTTOPLEFT;
-    else if (point.y() >= (height() - resize_border_thickness))
-      component = HTBOTTOMLEFT;
-    else
-      component = HTLEFT;
-  } else if (point.x() >= (width() - resize_border_thickness)) {
-    if (point.y() < top_resize_corner_height)
-      component = HTTOPRIGHT;
-    else if (point.y() >= (height() - resize_border_thickness))
-      component = HTBOTTOMRIGHT;
-    else
-      component = HTRIGHT;
-  } else if (point.y() < top_resize_border_height) {
-    if (point.x() < resize_corner_width)
-      component = HTTOPLEFT;
-    else if (point.x() >= (width() - resize_corner_width))
-      component = HTTOPRIGHT;
-    else
-      component = HTTOP;
-  } else if (point.y() >= (height() - resize_border_thickness)) {
-    if (point.x() < resize_corner_width)
-      component = HTBOTTOMLEFT;
-    else if (point.x() >= (width() - resize_corner_width))
-      component = HTBOTTOMRIGHT;
-    else
-      component = HTBOTTOM;
-  } else {
-    return HTNOWHERE;
-  }
-
-  // If the window can't be resized, there are no resize boundaries, just
-  // window borders.
-  return can_resize ? component : HTBORDER;
-}
-
-void NonClientFrameView::PaintAsActiveChanged(bool active) {}
-
-void NonClientFrameView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  node_data->role = ax::mojom::Role::kClient;
-}
-
-void NonClientFrameView::OnThemeChanged() {
-  SchedulePaint();
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// NonClientFrameView, protected:
-
-NonClientFrameView::NonClientFrameView() {
-  SetEventTargeter(std::make_unique<views::ViewTargeter>(this));
-}
-
-// ViewTargeterDelegate:
-bool NonClientFrameView::DoesIntersectRect(const View* target,
-                                           const gfx::Rect& rect) const {
-  CHECK_EQ(target, this);
-
-  // For the default case, we assume the non-client frame view never overlaps
-  // the client view.
-  return !GetWidget()->client_view()->bounds().Intersects(rect);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// NonClientFrameView, private:
-
-#if defined(OS_WIN)
-int NonClientFrameView::GetSystemMenuY() const {
-  return GetBoundsForClientView().y();
-}
-#endif
-
-BEGIN_METADATA(NonClientFrameView)
-METADATA_PARENT_CLASS(View)
-END_METADATA()
-
 }  // namespace views

ui/views/window/non_client_view.h

@@ -214,11 +214,6 @@ class VIEWS_EXPORT NonClientView : public View, public ViewTargeterDelegate {
     client_view_ = client_view;
   }
 
-  // Layout just the frame view. This is necessary on Windows when non-client
-  // metrics such as the position of the window controls changes independently
-  // of a window resize message.
-  void LayoutFrameView();
-
   // Set the accessible name of this view.
   void SetAccessibleName(const base::string16& name);
 
@@ -228,7 +223,6 @@ class VIEWS_EXPORT NonClientView : public View, public ViewTargeterDelegate {
   gfx::Size GetMaximumSize() const override;
   void Layout() override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
-
   views::View* GetTooltipHandlerForPoint(const gfx::Point& point) override;
 
  protected:

ui/views/window/non_client_view_unittest.cc

@@ -70,6 +70,7 @@ TEST_F(NonClientViewTest, OnlyLayoutChildViewsOnce) {
   widget.Init(std::move(params));
 
   NonClientView* non_client_view = widget.non_client_view();
+  non_client_view->Layout();
 
   auto* frame_view =
       static_cast<NonClientFrameTestView*>(non_client_view->frame_view());
@@ -79,20 +80,15 @@ TEST_F(NonClientViewTest, OnlyLayoutChildViewsOnce) {
   int initial_frame_view_layouts = frame_view->layout_count();
   int initial_client_view_layouts = client_view->layout_count();
 
+  // Make sure it does no layout when nothing has changed.
   non_client_view->Layout();
-  EXPECT_EQ(frame_view->layout_count(), initial_frame_view_layouts + 1);
-  EXPECT_EQ(client_view->layout_count(), initial_client_view_layouts + 1);
-
-  // One more time to make sure it does the layout
-  // even though nothing has changed.
-  non_client_view->Layout();
-  EXPECT_EQ(frame_view->layout_count(), initial_frame_view_layouts + 2);
-  EXPECT_EQ(client_view->layout_count(), initial_client_view_layouts + 2);
+  EXPECT_EQ(frame_view->layout_count(), initial_frame_view_layouts);
+  EXPECT_EQ(client_view->layout_count(), initial_client_view_layouts);
 
   // Ensure changing bounds triggers a (single) layout.
   widget.SetBounds(gfx::Rect(0, 0, 161, 100));
-  EXPECT_EQ(frame_view->layout_count(), initial_frame_view_layouts + 3);
-  EXPECT_EQ(client_view->layout_count(), initial_client_view_layouts + 3);
+  EXPECT_EQ(frame_view->layout_count(), initial_frame_view_layouts + 1);
+  EXPECT_EQ(client_view->layout_count(), initial_client_view_layouts + 1);
 }
 
 }  // namespace test