Revert "Use class properties for Ignored and can-be-visible state in a layout for a given view."

This reverts commit 5f44611e.

Reason for revert: Potential cause of recent crashes in toolbar layouts.

Original change's description:
> Use class properties for Ignored and can-be-visible state in a layout for a given view.
>
> This will remove the need for the child_infos_ field in LayoutManagerBase.
>
> Somewhat related to the work of integrating fill layout behavior into views::View.
>
> Bug: 1159116
> Change-Id: I968aa58fb3b29f78d96cd520e61ccb731c2e9ac9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2605567
> Commit-Queue: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#839620}

TBR=pkasting@chromium.org,kylixrd@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1159116
Change-Id: I9672a447b3b30530c049a9f6a89a8a1d42621bf6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611911Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840368}

ui/views/layout/layout_manager.h

@@ -91,7 +91,7 @@ class VIEWS_EXPORT LayoutManager {
 
  protected:
   // Sets the visibility of a view without triggering ViewVisibilitySet().
-  // During Layout(), use this method instead of View::SetVisible().
+  // During Layout(), use this method instead of View::SetVisibility().
   void SetViewVisibility(View* view, bool visible);
 
   // Gets the child views of the specified view in paint order (reverse

ui/views/layout/layout_manager_base.cc

@@ -9,7 +9,6 @@
 #include "base/auto_reset.h"
 #include "base/check_op.h"
 #include "ui/views/view.h"
-#include "ui/views/view_class_properties.h"
 
 namespace views {
 
@@ -101,7 +100,9 @@ ProposedLayout LayoutManagerBase::GetProposedLayout(
 
 void LayoutManagerBase::SetChildViewIgnoredByLayout(View* child_view,
                                                     bool ignored) {
-  if (child_view->GetProperty(kViewIgnoredByLayoutKey) == ignored)
+  auto it = child_infos_.find(child_view);
+  DCHECK(it != child_infos_.end());
+  if (it->second.ignored == ignored)
     return;
 
   base::AutoReset<bool> setter(&suppress_invalidate_, true);
@@ -111,7 +112,9 @@ void LayoutManagerBase::SetChildViewIgnoredByLayout(View* child_view,
 
 bool LayoutManagerBase::IsChildViewIgnoredByLayout(
     const View* child_view) const {
-  return child_view->GetProperty(kViewIgnoredByLayoutKey);
+  auto it = child_infos_.find(child_view);
+  DCHECK(it != child_infos_.end());
+  return it->second.ignored;
 }
 
 LayoutManagerBase::LayoutManagerBase() = default;
@@ -124,12 +127,27 @@ SizeBounds LayoutManagerBase::GetAvailableHostSize() const {
 
 bool LayoutManagerBase::IsChildIncludedInLayout(const View* child,
                                                 bool include_hidden) const {
-  return !child->GetProperty(kViewIgnoredByLayoutKey) &&
-         (include_hidden || child->GetProperty(kViewCanBeVisibleKey));
+  const auto it = child_infos_.find(child);
+
+  // During callbacks when a child is removed we can get in a state where a view
+  // in the child list of the host view is not in |child_infos_|. In that case,
+  // the view is being removed and is not part of the layout.
+  if (it == child_infos_.end())
+    return false;
+
+  return !it->second.ignored && (include_hidden || it->second.can_be_visible);
 }
 
 bool LayoutManagerBase::CanBeVisible(const View* child) const {
-  return child->GetProperty(kViewCanBeVisibleKey);
+  const auto it = child_infos_.find(child);
+
+  // During callbacks when a child is removed we can get in a state where a view
+  // in the child list of the host view is not in |child_infos_|. In that case,
+  // the view is being removed and is not part of the layout.
+  if (it == child_infos_.end())
+    return false;
+
+  return it->second.can_be_visible;
 }
 
 void LayoutManagerBase::LayoutImpl() {
@@ -235,6 +253,7 @@ void LayoutManagerBase::InvalidateLayout() {
 void LayoutManagerBase::Installed(View* host_view) {
   DCHECK(host_view);
   DCHECK(!host_view_);
+  DCHECK(child_infos_.empty());
 
   base::AutoReset<bool> setter(&suppress_invalidate_, true);
   PropagateInstalled(host_view);
@@ -242,6 +261,7 @@ void LayoutManagerBase::Installed(View* host_view) {
 
 void LayoutManagerBase::ViewAdded(View* host, View* view) {
   DCHECK_EQ(host_view_, host);
+  DCHECK(!base::Contains(child_infos_, view));
 
   base::AutoReset<bool> setter(&suppress_invalidate_, true);
   const bool invalidate = PropagateViewAdded(host, view);
@@ -251,8 +271,11 @@ void LayoutManagerBase::ViewAdded(View* host, View* view) {
 
 void LayoutManagerBase::ViewRemoved(View* host, View* view) {
   DCHECK_EQ(host_view_, host);
-  const bool removed_visible = view->GetProperty(kViewCanBeVisibleKey) &&
-                               !view->GetProperty(kViewIgnoredByLayoutKey);
+  DCHECK(base::Contains(child_infos_, view));
+
+  auto it = child_infos_.find(view);
+  DCHECK(it != child_infos_.end());
+  const bool removed_visible = it->second.can_be_visible && !it->second.ignored;
 
   base::AutoReset<bool> setter(&suppress_invalidate_, true);
   const bool invalidate = PropagateViewRemoved(host, view);
@@ -265,8 +288,10 @@ void LayoutManagerBase::ViewVisibilitySet(View* host,
                                           bool old_visibility,
                                           bool new_visibility) {
   DCHECK_EQ(host_view_, host);
-  const bool was_ignored = view->GetProperty(kViewIgnoredByLayoutKey);
-  if (view->GetProperty(kViewCanBeVisibleKey) == new_visibility)
+  auto it = child_infos_.find(view);
+  DCHECK(it != child_infos_.end());
+  const bool was_ignored = it->second.ignored;
+  if (it->second.can_be_visible == new_visibility)
     return;
 
   base::AutoReset<bool> setter(&suppress_invalidate_, true);
@@ -283,11 +308,11 @@ void LayoutManagerBase::AddOwnedLayoutInternal(
   if (host_view_) {
     owned_layout->Installed(host_view_);
     for (View* child_view : host_view_->children()) {
-      owned_layout->PropagateChildViewIgnoredByLayout(
-          child_view, child_view->GetProperty(kViewIgnoredByLayoutKey));
-      owned_layout->PropagateViewVisibilitySet(
-          host_view_, child_view,
-          child_view->GetProperty(kViewCanBeVisibleKey));
+      const ChildInfo& child_info = child_infos_.find(child_view)->second;
+      owned_layout->PropagateChildViewIgnoredByLayout(child_view,
+                                                      child_info.ignored);
+      owned_layout->PropagateViewVisibilitySet(host_view_, child_view,
+                                               child_info.can_be_visible);
     }
   }
   owned_layout->parent_layout_ = this;
@@ -303,7 +328,7 @@ LayoutManagerBase* LayoutManagerBase::GetRootLayoutManager() {
 
 bool LayoutManagerBase::PropagateChildViewIgnoredByLayout(View* child_view,
                                                           bool ignored) {
-  child_view->SetProperty(kViewIgnoredByLayoutKey, ignored);
+  child_infos_[child_view].ignored = ignored;
 
   bool result = false;
   for (auto& owned_layout : owned_layouts_) {
@@ -315,7 +340,7 @@ bool LayoutManagerBase::PropagateChildViewIgnoredByLayout(View* child_view,
 }
 
 bool LayoutManagerBase::PropagateViewAdded(View* host, View* view) {
-  view->SetProperty(kViewCanBeVisibleKey, view->GetVisible());
+  child_infos_.emplace(view, ChildInfo{view->GetVisible(), false});
 
   bool result = false;
 
@@ -327,8 +352,7 @@ bool LayoutManagerBase::PropagateViewAdded(View* host, View* view) {
 }
 
 bool LayoutManagerBase::PropagateViewRemoved(View* host, View* view) {
-  view->ClearProperty(kViewCanBeVisibleKey);
-  view->ClearProperty(kViewIgnoredByLayoutKey);
+  child_infos_.erase(view);
 
   bool result = false;
 
@@ -342,7 +366,7 @@ bool LayoutManagerBase::PropagateViewRemoved(View* host, View* view) {
 bool LayoutManagerBase::PropagateViewVisibilitySet(View* host,
                                                    View* view,
                                                    bool visible) {
-  view->SetProperty(kViewCanBeVisibleKey, visible);
+  child_infos_[view].can_be_visible = visible;
 
   bool result = false;
 
@@ -355,8 +379,9 @@ bool LayoutManagerBase::PropagateViewVisibilitySet(View* host,
 
 void LayoutManagerBase::PropagateInstalled(View* host) {
   host_view_ = host;
-  for (auto* child : host->children())
-    child->SetProperty(kViewCanBeVisibleKey, child->GetVisible());
+  for (auto* it : host->children()) {
+    child_infos_.emplace(it, ChildInfo{it->GetVisible(), false});
+  }
 
   for (auto& owned_layout : owned_layouts_)
     owned_layout->PropagateInstalled(host);

ui/views/layout/layout_manager_base.h

@@ -106,7 +106,7 @@ class VIEWS_EXPORT LayoutManagerBase : public LayoutManager {
 
   // Returns whether the specified child view can be visible. To be able to be
   // visible, |child| must be a child of the host view, and must have been
-  // visible when it was added or most recently had SetVisible(true) called on
+  // visible when it was added or most recently had GetVisible(true) called on
   // it by non-layout code.
   bool CanBeVisible(const View* child) const;
 
@@ -171,6 +171,13 @@ class VIEWS_EXPORT LayoutManagerBase : public LayoutManager {
  private:
   friend class LayoutManagerBaseAvailableSizeTest;
 
+  // Holds bookkeeping data used to determine inclusion of children in the
+  // layout.
+  struct ChildInfo {
+    bool can_be_visible = true;
+    bool ignored = false;
+  };
+
   // LayoutManager:
   void InvalidateLayout() final;
   void Installed(View* host) final;
@@ -196,6 +203,7 @@ class VIEWS_EXPORT LayoutManagerBase : public LayoutManager {
   void PropagateInvalidateLayout();
 
   View* host_view_ = nullptr;
+  std::map<const View*, ChildInfo> child_infos_;
   std::vector<std::unique_ptr<LayoutManagerBase>> owned_layouts_;
   LayoutManagerBase* parent_layout_ = nullptr;
 

ui/views/view.cc

@@ -56,7 +56,6 @@
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/drag_controller.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
-#include "ui/views/view_class_properties.h"
 #include "ui/views/view_observer.h"
 #include "ui/views/view_tracker.h"
 #include "ui/views/views_features.h"
@@ -3126,18 +3125,14 @@ View::DefaultFillLayout::~DefaultFillLayout() = default;
 
 void View::DefaultFillLayout::Layout(View* host) {
   const gfx::Rect contents_bounds = host->GetContentsBounds();
-  for (auto* child : host->children()) {
-    if (!child->GetProperty(kViewIgnoredByLayoutKey))
-      child->SetBoundsRect(contents_bounds);
-  }
+  for (auto* child : host->children())
+    child->SetBoundsRect(contents_bounds);
 }
 
 gfx::Size View::DefaultFillLayout::GetPreferredSize(const View* host) const {
   gfx::Size preferred_size;
-  for (auto* child : host->children()) {
-    if (!child->GetProperty(kViewIgnoredByLayoutKey))
-      preferred_size.SetToMax(child->GetPreferredSize());
-  }
+  for (auto* child : host->children())
+    preferred_size.SetToMax(child->GetPreferredSize());
   return preferred_size;
 }
 
@@ -3145,13 +3140,10 @@ int View::DefaultFillLayout::GetPreferredHeightForWidth(const View* host,
                                                         int width) const {
   const gfx::Insets insets = host->GetInsets();
   int preferred_height = 0;
-  for (auto* child : host->children()) {
-    if (!child->GetProperty(kViewIgnoredByLayoutKey)) {
-      preferred_height = std::max(
-          preferred_height,
-          child->GetHeightForWidth(width - insets.width()) + insets.height());
-    }
-  }
+  for (auto* child : host->children())
+    preferred_height = std::max(
+        preferred_height,
+        child->GetHeightForWidth(width - insets.width()) + insets.height());
   return preferred_height;
 }
 

ui/views/view_class_properties.cc

@@ -41,7 +41,5 @@ DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(FlexSpecification, kFlexBehaviorKey, nullptr)
 DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(LayoutAlignment,
                                    kCrossAxisAlignmentKey,
                                    nullptr)
-DEFINE_UI_CLASS_PROPERTY_KEY(bool, kViewIgnoredByLayoutKey, false)
-DEFINE_UI_CLASS_PROPERTY_KEY(bool, kViewCanBeVisibleKey, false)
 
 }  // namespace views

ui/views/view_class_properties.h

@@ -58,15 +58,6 @@ VIEWS_EXPORT extern const ui::ClassProperty<FlexSpecification*>* const
 VIEWS_EXPORT extern const ui::ClassProperty<LayoutAlignment*>* const
     kCrossAxisAlignmentKey;
 
-// Property indicating whether a view should be ignored by a layout. Supported
-// by LayoutManagerBase and View::DefaultFillLayout
-VIEWS_EXPORT extern const ui::ClassProperty<bool>* const
-    kViewIgnoredByLayoutKey;
-
-// Property indicating whether a view would can be visible based on the criteria
-// outlined in LayoutManagerBase::CanBeVisible().
-VIEWS_EXPORT extern const ui::ClassProperty<bool>* const kViewCanBeVisibleKey;
-
 }  // namespace views
 
 // Declaring the template specialization here to make sure that the

ui/views/widget/root_view.cc

@@ -22,8 +22,8 @@
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/drag_controller.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
-#include "ui/views/view_class_properties.h"
 #include "ui/views/view_targeter.h"
 #include "ui/views/widget/root_view_targeter.h"
 #include "ui/views/widget/widget.h"
@@ -210,7 +210,7 @@ void RootView::SetContentsView(View* contents_view) {
       << "Can't be called until after the native widget is created!";
   // The ContentsView must be set up _after_ the window is created so that its
   // Widget pointer is valid.
-  SetUseDefaultFillLayout(true);
+  SetLayoutManager(std::make_unique<FillLayout>());
   if (!children().empty())
     RemoveAllChildViews(true);
   AddChildView(contents_view);
@@ -268,7 +268,8 @@ void RootView::AnnounceText(const base::string16& text) {
   DCHECK(GetContentsView());
   if (!announce_view_) {
     announce_view_ = AddChildView(std::make_unique<AnnounceTextView>());
-    announce_view_->SetProperty(kViewIgnoredByLayoutKey, true);
+    static_cast<FillLayout*>(GetLayoutManager())
+        ->SetChildViewIgnoredByLayout(announce_view_, true);
   }
   announce_view_->Announce(text);
 #endif