Commit cff5155b authored by Dana Fried's avatar Dana Fried Committed by Chromium LUCI CQ

Make LayoutManager::GetAvailableSize() more robust

LayoutManagerBase::GetAvailableSize(host, child) will now do the
following:
 - as before, if the cached layout is still valid since the last call to
   Layout(), it will return the available size of |child| in |host|'s
   layout
 - if the layout has been invalidated since the last call to Layout(), a
   new cached layout for |host| is calculated, which will be used in the
   next call to Layout() assuming the layout is not invalidated again

This gives us two guarantees we did not have before:
 1. instead of returning [no bounds], a correct size bound will almost
    always be returned
 2. a safeguard is in place to ensure that we don't get a stale value
    during |host|'s layout calculation - in fact, we no longer allow the
	method to be called during |host|'s layout calculation (it will
	DCHECK())

In general it is legal to call GetAvailableSize() during |child|'s
Layout() - which happens after |host|'s layout is committed - but not
during, for example, a FlexRule computation. It doesn't make sense for a
FlexRule (or similar) to call GetAvailableSize() because it already
receives a size bound from the caller.

This also fixes a potential issue (related to crbug.com/1041614) which
could result in a view calling layout code in response to certain types
of widget setup (among other possible situations) before the layout's
child data is updated, resulting in a DCHECK() or crash in the layout
manager. (For at least the short term an example crash can be found here:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8858322112097966752/+/steps/unit_tests__with_patch_/0/logs/Deterministic_failure:_PasswordSaveUpdateWithAccountStoreViewTest.HasTitleAndTwoButtons__status_CRASH_/0)

Change-Id: I3e491c3cb7a648867597ecf830fe52d11f483419
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622720
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842653}
parent fde17878
...@@ -67,7 +67,11 @@ class VIEWS_EXPORT LayoutManager { ...@@ -67,7 +67,11 @@ class VIEWS_EXPORT LayoutManager {
virtual int GetPreferredHeightForWidth(const View* host, int width) const; virtual int GetPreferredHeightForWidth(const View* host, int width) const;
// Returns the maximum space available in the layout for the specified child // Returns the maximum space available in the layout for the specified child
// view. Default is unbounded. // view. Default is unbounded. May result in a layout calculation for |host|
// if the layout is not valid, so while it can be called during the Layout()
// method for |view|, it should never be called during the actual computation
// of |host|'s layout (e.g. in a FlexLayout FlexRule calculation) to prevent
// an infinite loop.
virtual SizeBounds GetAvailableSize(const View* host, const View* view) const; virtual SizeBounds GetAvailableSize(const View* host, const View* view) const;
// Called when a View is added as a child of the View the LayoutManager has // Called when a View is added as a child of the View the LayoutManager has
......
...@@ -63,6 +63,8 @@ int LayoutManagerBase::GetPreferredHeightForWidth(const View* host, ...@@ -63,6 +63,8 @@ int LayoutManagerBase::GetPreferredHeightForWidth(const View* host,
SizeBounds LayoutManagerBase::GetAvailableSize(const View* host, SizeBounds LayoutManagerBase::GetAvailableSize(const View* host,
const View* view) const { const View* view) const {
DCHECK_EQ(host_view_, host); DCHECK_EQ(host_view_, host);
if (!cached_layout_size_)
GetProposedLayout(host->size());
if (cached_layout_size_) { if (cached_layout_size_) {
for (const auto& child_layout : cached_layout_.child_layouts) for (const auto& child_layout : cached_layout_.child_layouts)
if (child_layout.child_view == view) { if (child_layout.child_view == view) {
...@@ -92,6 +94,11 @@ std::vector<View*> LayoutManagerBase::GetChildViewsInPaintOrder( ...@@ -92,6 +94,11 @@ std::vector<View*> LayoutManagerBase::GetChildViewsInPaintOrder(
ProposedLayout LayoutManagerBase::GetProposedLayout( ProposedLayout LayoutManagerBase::GetProposedLayout(
const gfx::Size& host_size) const { const gfx::Size& host_size) const {
if (cached_layout_size_ != host_size) { if (cached_layout_size_ != host_size) {
#if (DCHECK_IS_ON())
// This calculation must not be re-entrant.
DCHECK(!calculating_layout_);
base::AutoReset<bool> calculating_layout(&calculating_layout_, true);
#endif
cached_layout_size_ = host_size; cached_layout_size_ = host_size;
cached_layout_ = CalculateProposedLayout(SizeBounds(host_size)); cached_layout_ = CalculateProposedLayout(SizeBounds(host_size));
} }
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include <utility> #include <utility>
#include <vector> #include <vector>
#include "base/dcheck_is_on.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/optional.h" #include "base/optional.h"
#include "ui/gfx/geometry/insets.h" #include "ui/gfx/geometry/insets.h"
...@@ -216,6 +217,11 @@ class VIEWS_EXPORT LayoutManagerBase : public LayoutManager { ...@@ -216,6 +217,11 @@ class VIEWS_EXPORT LayoutManagerBase : public LayoutManager {
// whether their bounds have changed. // whether their bounds have changed.
SizeBounds cached_available_size_; SizeBounds cached_available_size_;
#if (DCHECK_IS_ON())
// Used to prevent GetProposedLayout() from being re-entrant.
mutable bool calculating_layout_ = false;
#endif
// Do some really simple caching because layout generation can cost as much // Do some really simple caching because layout generation can cost as much
// as 1ms or more for complex views. // as 1ms or more for complex views.
mutable base::Optional<gfx::Size> cached_minimum_size_; mutable base::Optional<gfx::Size> cached_minimum_size_;
......
...@@ -2477,17 +2477,17 @@ void View::AddChildViewAtImpl(View* view, int index) { ...@@ -2477,17 +2477,17 @@ void View::AddChildViewAtImpl(View* view, int index) {
// inherit the visibility of the owner View. // inherit the visibility of the owner View.
view->UpdateLayerVisibility(); view->UpdateLayerVisibility();
// Need to notify the layout manager because one of the callbacks below might
// want to know the view's new preferred size, minimum size, etc.
if (HasLayoutManager())
GetLayoutManager()->ViewAdded(this, view);
if (widget) { if (widget) {
const ui::NativeTheme* new_theme = view->GetNativeTheme(); const ui::NativeTheme* new_theme = view->GetNativeTheme();
if (new_theme != old_theme) if (new_theme != old_theme)
view->PropagateThemeChanged(); view->PropagateThemeChanged();
} }
// Need to notify the layout manager because one of the callbacks below might
// want to know the view's new preferred size, minimum size, etc.
if (HasLayoutManager())
GetLayoutManager()->ViewAdded(this, view);
ViewHierarchyChangedDetails details(true, this, view, parent); ViewHierarchyChangedDetails details(true, this, view, parent);
for (View* v = this; v; v = v->parent_) for (View* v = this; v; v = v->parent_)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment