New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 916394 link

Starred by 3 users
Status: Assigned
Owner:
alancutter@chromium.org
Cc:
sky@chromium.org
dfried@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Migrate HostedAppButtonContainer to use FlexLayout

Project Member Reported by alancutter@chromium.org, Dec 19 
HostedAppButtonContainer currently uses BoxLayout. This bug is to track refactoring it to use FlexLayout instead.

This is issue split from https://bugs.chromium.org/p/chromium/issues/detail?id=898632#c15.
There seems to be a cyclic interaction between FlexLayout and PreferredSizeChanged() probably due to the following code:
void BrowserNonClientFrameView::ChildPreferredSizeChanged(views::View* child) {
  if (browser_view()->initialized() && child == hosted_app_button_container_)
    Layout();
}

HostedAppButtonContainer::LayoutInContainer() calls itself recursively as deep as 6 levels before crashing with the following stack trace:

#0  SkISize::width() const at ui/gfx/geometry/point.h:48
#1  gfx::operator==(gfx::Point const&, gfx::Point const&) at ui/gfx/geometry/point.h:99
#2  gfx::operator==(gfx::Rect const&, gfx::Rect const&) at ui/gfx/geometry/rect.h:254
#3  views::View::SetBoundsRect(gfx::Rect const&) at ui/views/view.cc:224
#4  DoLayout() at ui/views/layout/flex_layout.cc:438
#5  Layout() at ui/views/layout/flex_layout.cc:1030
#6  Layout() at ui/views/view.cc:520
#7  views::View::SetBoundsRect(gfx::Rect const&) at ui/views/view.cc:229
#8  SetBounds() at ui/views/view.cc:220
#9  LayoutInContainer() at chrome/browser/ui/views/frame/hosted_app_button_container.cc:303
#10 LayoutTitleBar() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:330
#11 Layout() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:572
#12 Layout() at ui/views/view.cc:520
#13 BrowserNonClientFrameView::ChildPreferredSizeChanged(views::View*) at chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:334
#14 views::View::PreferredSizeChanged() at ui/views/view.cc:1442
#15 HostedAppButtonContainer::ChildVisibilityChanged(views::View*) at chrome/browser/ui/views/frame/hosted_app_button_container.cc:442
#16 views::View::SetVisible(bool) at ui/views/view.cc:381
#17 views::LayoutManager::SetViewVisibility(views::View*, bool) at ui/views/layout/layout_manager.cc:48
#18 DoLayout() at ui/views/layout/flex_layout.cc:433
#19 Layout() at ui/views/layout/flex_layout.cc:1030
#20 Layout() at ui/views/view.cc:520
#21 views::View::SetBoundsRect(gfx::Rect const&) at ui/views/view.cc:229
#22 SetBounds() at ui/views/view.cc:220
#23 LayoutInContainer() at chrome/browser/ui/views/frame/hosted_app_button_container.cc:303
#24 LayoutTitleBar() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:330
#25 Layout() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:572
#26 Layout() at ui/views/view.cc:520
#27 BrowserNonClientFrameView::ChildPreferredSizeChanged(views::View*) at chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:334
#28 views::View::PreferredSizeChanged() at ui/views/view.cc:1442
#29 HostedAppButtonContainer::ChildVisibilityChanged(views::View*) at chrome/browser/ui/views/frame/hosted_app_button_container.cc:442
#30 views::View::SetVisible(bool) at ui/views/view.cc:381
#31 views::LayoutManager::SetViewVisibility(views::View*, bool) at ui/views/layout/layout_manager.cc:48
#32 DoLayout() at ui/views/layout/flex_layout.cc:433
#33 Layout() at ui/views/layout/flex_layout.cc:1030
#34 Layout() at ui/views/view.cc:520
#35 BoundsChanged() at ui/views/view.cc:2186
#36 views::View::SetBoundsRect(gfx::Rect const&) at ui/views/view.cc:243
#37 SetBounds() at ui/views/view.cc:220
#38 LayoutInContainer() at chrome/browser/ui/views/frame/hosted_app_button_container.cc:303
#39 LayoutTitleBar() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:330
#40 Layout() at chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:572
#41 Layout() at ui/views/view.cc:520
#42 OpaqueBrowserFrameView::UpdateWindowTitle() at chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:327
#43 UpdateWindowTitle() at ui/views/window/non_client_view.cc:119
#44 views::Widget::UpdateWindowTitle() at ui/views/widget/widget.cc:850
#45 Init() at ui/views/widget/widget.cc:361
#46 InitBrowserFrame() at chrome/browser/ui/views/frame/browser_frame.cc:89

The main difference between FlexLayout and BoxLayout causing seems to be that FlexLayout calls SetViewVisibility() while BoxLayout does not.

Comment 2 by alancutter@chromium.org, Dec 19 

I feel like a View's visibility should be an input to Layout and not an output. Maintaining this would enable invoking Layout() on visibility changes.

Comment 3 by dfried@chromium.org, Dec 19 

This was a long discussion with sky@ over how it should work, and the problem is that visibility is both an input and an output and there's not much we can do about that with the existing way View works.

We need the layout manager to be able to drop views out when they're set to be able to flex to zero. It was suggested we might just size them to zero but there have been some bugs regarding that and it basically provides another code path for a view to NOT display, which is not ideal.

In general, we should not be actively forcing Layout() calls on visibility or bounds change. I think we need to audit how the layout system works; in my conversations with sky@ the biggest problem is that it doesn't really work the way you'd expect it to (i.e. like SchedulePaint() does).

Comment 4 by alancutter@chromium.org, Dec 20

Cc: sky@chromium.org
Can layout managers use a different visibility bool for what they need? Similar to CSS having width and actual width as separate concepts.

I'm curious to read up on the discussion, I'm not convinced that a value being both an input and output to layout is a good idea mainly because things don't work how one expects and I'm not seeing an obvious idiomatic alternative to re-invoking Layout() when an external event modifies a View's visibility.

Comment 5 by dfried@chromium.org, Dec 20 

Hrm...
I think flex_layout.cc:1007 might ought to read:

  host_->InvalidateLayout()

instead. That would make the appropriate request that the host view be laid out, which would result in the visibility change being evaluated.

Comment 6 by dfried@chromium.org, Dec 20 

Let me add that I'm glad you're experimenting with this and catching some cases we didn't think about. There was no way FlexLayout was going to be *perfect* on the first try :)

Comment 7 by benhenry@google.com, Jan 11

Status: Assigned (was: Untriaged)
This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this.
►

Sign in to add a comment