New issue
Advanced search Search tips

Issue 916674 link

Starred by 2 users
Status: Verified
Owner:
knollr@chromium.org
Closed: Jan 7
Cc:
dfried@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 915224



Sign in to add a comment

FlexLayout calls view::SetVisible every time

Project Member Reported by knollr@chromium.org, Dec 19 
Chrome Version: 73.0.3646.0
OS: Any

FlexLayout calls view::SetVisible during the layout phase which could be an issue for custom views that have custom logic in that method.

Instead of calling it in every layout phase, we could just check if the visibility needs to be updated and call it conditionally.

Comment 1 by dfried@chromium.org, Dec 19 

As I wrote on the CL, I'm not sure we actually want View::SetVisible() to be virtual, but if it has to be, then this is probably a good change, and certainly does no real harm.
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bab14ff14a7c2be182cc3812252293ebdc387500

commit bab14ff14a7c2be182cc3812252293ebdc387500
Author: Richard Knoll <knollr@chromium.org>
Date: Thu Dec 20 10:23:16 2018

fix: only call view::SetVisible when we actually need to

Although the default implementation is a no-op if the visibility did not
change, other subclasses of |View| may not expect this. This will check
the current value of view::visible() to see if we need to update it.

Bug:  916674 
Change-Id: If1dbb8d011844d30cfe0ee13644bc9552327736d
Reviewed-on: https://chromium-review.googlesource.com/c/1384290
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618158}
[modify] https://crrev.com/bab14ff14a7c2be182cc3812252293ebdc387500/ui/views/layout/flex_layout.cc
[modify] https://crrev.com/bab14ff14a7c2be182cc3812252293ebdc387500/ui/views/layout/flex_layout_unittest.cc

Comment 3 by dfried@chromium.org, Jan 3 

Is this fixed?

Comment 4 by knollr@google.com, Jan 7

Status: Verified (was: Untriaged)
Yes! Sorry, missed to update the issue :-)
►

Sign in to add a comment