Fixed collected cookies view info bar visibility

Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053
Committed: https://crrev.com/11f088e941f3264c42b2cb3265c2cc6d48397712
Cr-Commit-Position: refs/heads/master@{#435249}

[kylix_rd, 2016-11-14 21:50:57]

Description was changed from

==========
Fixed collected cookies view info bar visibility

BUG=
==========

to

==========
Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053
==========

[kylix_rd, 2016-11-14 21:50:57]

kylixrd@chromium.org changed reviewers:
+ pkasting@chromium.org

[kylix_rd, 2016-11-14 21:54:30]

While working on this view to convert to using a different layout manager, I
discovered that the InfobarView is never actually made visible. The grid row on
which the InfobarView instance is placed isn't included in the preferred size
calculation since it is initially invisible. Once it is made visible, the widget
& root view sizes need to be recalculated.

[Peter Kasting, 2016-11-14 22:38:51]

On 2016/11/14 21:54:30, kylix_rd wrote:
> While working on this view to convert to using a different layout manager, I
> discovered that the InfobarView is never actually made visible. The grid row
on
> which the InfobarView instance is placed isn't included in the preferred size
> calculation since it is initially invisible. Once it is made visible, the
widget
> & root view sizes need to be recalculated.

Having to muck directly with the Widget and RootView here sets off alarm bells
for me.  It seems like simply firing PreferredSizeChanged() ought to do the
right thing, perhaps in conjunction with handling ChildPreferredSizeChanged() in
the layout manager or dialog root or something?

[kylix_rd, 2016-11-15 14:45:48]

On 2016/11/14 22:38:51, Peter Kasting wrote:
> On 2016/11/14 21:54:30, kylix_rd wrote:
> > While working on this view to convert to using a different layout manager, I
> > discovered that the InfobarView is never actually made visible. The grid row
> on
> > which the InfobarView instance is placed isn't included in the preferred
size
> > calculation since it is initially invisible. Once it is made visible, the
> widget
> > & root view sizes need to be recalculated.
> 
> Having to muck directly with the Widget and RootView here sets off alarm bells
> for me.  It seems like simply firing PreferredSizeChanged() ought to do the
> right thing, perhaps in conjunction with handling ChildPreferredSizeChanged()
in
> the layout manager or dialog root or something?

Did not realize there was a PreferredSizeChanged() event. That is certainly a
better approach.

[kylix_rd, 2016-11-15 14:51:41]

On 2016/11/15 14:45:48, kylix_rd wrote:
> On 2016/11/14 22:38:51, Peter Kasting wrote:
> > On 2016/11/14 21:54:30, kylix_rd wrote:
> > > While working on this view to convert to using a different layout manager,
I
> > > discovered that the InfobarView is never actually made visible. The grid
row
> > on
> > > which the InfobarView instance is placed isn't included in the preferred
> size
> > > calculation since it is initially invisible. Once it is made visible, the
> > widget
> > > & root view sizes need to be recalculated.
> > 
> > Having to muck directly with the Widget and RootView here sets off alarm
bells
> > for me.  It seems like simply firing PreferredSizeChanged() ought to do the
> > right thing, perhaps in conjunction with handling
ChildPreferredSizeChanged()
> in
> > the layout manager or dialog root or something?
> 
> Did not realize there was a PreferredSizeChanged() event. That is certainly a
> better approach.

It's better only if it works. It doesn't look like it's properly wired up.
PreferredSizeChanged() propagates up to the parent by calling
ChildPreferredSizeChanged() and stop there. I'd have to override
ChildPreferredSizeChanged() all the way up the hierarchy to make this work. It
seems to me that something like this should have been been ubiquitous and work
consistently.

I'll look into what it would take to leverage this facility of the framework.
Right now it feels a little incomplete in the implementation.

[kylix_rd, 2016-11-15 22:08:15]

On 2016/11/15 14:51:41, kylix_rd wrote:
> On 2016/11/15 14:45:48, kylix_rd wrote:
> > On 2016/11/14 22:38:51, Peter Kasting wrote:
> > > On 2016/11/14 21:54:30, kylix_rd wrote:
> > > > While working on this view to convert to using a different layout
manager,
> I
> > > > discovered that the InfobarView is never actually made visible. The grid
> row
> > > on
> > > > which the InfobarView instance is placed isn't included in the preferred
> > size
> > > > calculation since it is initially invisible. Once it is made visible,
the
> > > widget
> > > > & root view sizes need to be recalculated.
> > > 
> > > Having to muck directly with the Widget and RootView here sets off alarm
> bells
> > > for me.  It seems like simply firing PreferredSizeChanged() ought to do
the
> > > right thing, perhaps in conjunction with handling
> ChildPreferredSizeChanged()
> > in
> > > the layout manager or dialog root or something?
> > 
> > Did not realize there was a PreferredSizeChanged() event. That is certainly
a
> > better approach.
> 
> It's better only if it works. It doesn't look like it's properly wired up.
> PreferredSizeChanged() propagates up to the parent by calling
> ChildPreferredSizeChanged() and stop there. I'd have to override
> ChildPreferredSizeChanged() all the way up the hierarchy to make this work. It
> seems to me that something like this should have been been ubiquitous and work
> consistently.
> 
> I'll look into what it would take to leverage this facility of the framework.
> Right now it feels a little incomplete in the implementation.

PreferredSizeChanged() simply doesn't propagate up the parent view chain unless
all intervening views have overridden ChildPreferredSizeChanged(). IMO, There
should have been a set of non-virtual methods which would propagate the event up
the view chain. These methods would also call the virtual methods,
PreferredSizeChanged() & ChildPreferredSizeChanged(). As it stands now, both are
virtual and unless the view overriding one or both of them also calls the
ancestor method, the event just stops.

[Peter Kasting, 2016-11-15 22:16:57]

On 2016/11/15 22:08:15, kylix_rd wrote:
> On 2016/11/15 14:51:41, kylix_rd wrote:
> > On 2016/11/15 14:45:48, kylix_rd wrote:
> > > On 2016/11/14 22:38:51, Peter Kasting wrote:
> > > > On 2016/11/14 21:54:30, kylix_rd wrote:
> > > > > While working on this view to convert to using a different layout
> manager,
> > I
> > > > > discovered that the InfobarView is never actually made visible. The
grid
> > row
> > > > on
> > > > > which the InfobarView instance is placed isn't included in the
preferred
> > > size
> > > > > calculation since it is initially invisible. Once it is made visible,
> the
> > > > widget
> > > > > & root view sizes need to be recalculated.
> > > > 
> > > > Having to muck directly with the Widget and RootView here sets off alarm
> > bells
> > > > for me.  It seems like simply firing PreferredSizeChanged() ought to do
> the
> > > > right thing, perhaps in conjunction with handling
> > ChildPreferredSizeChanged()
> > > in
> > > > the layout manager or dialog root or something?
> > > 
> > > Did not realize there was a PreferredSizeChanged() event. That is
certainly
> a
> > > better approach.
> > 
> > It's better only if it works. It doesn't look like it's properly wired up.
> > PreferredSizeChanged() propagates up to the parent by calling
> > ChildPreferredSizeChanged() and stop there. I'd have to override
> > ChildPreferredSizeChanged() all the way up the hierarchy to make this work.
It
> > seems to me that something like this should have been been ubiquitous and
work
> > consistently.
> > 
> > I'll look into what it would take to leverage this facility of the
framework.
> > Right now it feels a little incomplete in the implementation.
> 
> PreferredSizeChanged() simply doesn't propagate up the parent view chain
unless
> all intervening views have overridden ChildPreferredSizeChanged(). IMO, There
> should have been a set of non-virtual methods which would propagate the event
up
> the view chain. These methods would also call the virtual methods,
> PreferredSizeChanged() & ChildPreferredSizeChanged(). As it stands now, both
are
> virtual and unless the view overriding one or both of them also calls the
> ancestor method, the event just stops.

This would probably be a good time to loop in sky for an opinion about the
overall architecture and how best to do what you want.

I think the most tricky conceptual question is: if a particular child view's
preferred size changes, does that necessarily mean the preferred size of every
ancestor changes?  In practice, it clearly doesn't always mean that (in fact, it
might _not_ mean that more than it does).

[kylix_rd, 2016-11-15 22:31:27]

On 2016/11/15 22:16:57, Peter Kasting wrote:
> On 2016/11/15 22:08:15, kylix_rd wrote:
> > On 2016/11/15 14:51:41, kylix_rd wrote:
> > > On 2016/11/15 14:45:48, kylix_rd wrote:
> > > > On 2016/11/14 22:38:51, Peter Kasting wrote:
> > > > > On 2016/11/14 21:54:30, kylix_rd wrote:
> > > > > > While working on this view to convert to using a different layout
> > manager,
> > > I
> > > > > > discovered that the InfobarView is never actually made visible. The
> grid
> > > row
> > > > > on
> > > > > > which the InfobarView instance is placed isn't included in the
> preferred
> > > > size
> > > > > > calculation since it is initially invisible. Once it is made
visible,
> > the
> > > > > widget
> > > > > > & root view sizes need to be recalculated.
> > > > > 
> > > > > Having to muck directly with the Widget and RootView here sets off
alarm
> > > bells
> > > > > for me.  It seems like simply firing PreferredSizeChanged() ought to
do
> > the
> > > > > right thing, perhaps in conjunction with handling
> > > ChildPreferredSizeChanged()
> > > > in
> > > > > the layout manager or dialog root or something?
> > > > 
> > > > Did not realize there was a PreferredSizeChanged() event. That is
> certainly
> > a
> > > > better approach.
> > > 
> > > It's better only if it works. It doesn't look like it's properly wired up.
> > > PreferredSizeChanged() propagates up to the parent by calling
> > > ChildPreferredSizeChanged() and stop there. I'd have to override
> > > ChildPreferredSizeChanged() all the way up the hierarchy to make this
work.
> It
> > > seems to me that something like this should have been been ubiquitous and
> work
> > > consistently.
> > > 
> > > I'll look into what it would take to leverage this facility of the
> framework.
> > > Right now it feels a little incomplete in the implementation.
> > 
> > PreferredSizeChanged() simply doesn't propagate up the parent view chain
> unless
> > all intervening views have overridden ChildPreferredSizeChanged(). IMO,
There
> > should have been a set of non-virtual methods which would propagate the
event
> up
> > the view chain. These methods would also call the virtual methods,
> > PreferredSizeChanged() & ChildPreferredSizeChanged(). As it stands now, both
> are
> > virtual and unless the view overriding one or both of them also calls the
> > ancestor method, the event just stops.
> 
> This would probably be a good time to loop in sky for an opinion about the
> overall architecture and how best to do what you want.
> 
> I think the most tricky conceptual question is: if a particular child view's
> preferred size changes, does that necessarily mean the preferred size of every
> ancestor changes?  In practice, it clearly doesn't always mean that (in fact,
it
> might _not_ mean that more than it does).

If that's the case, then it is wholly unsuitable for what is needed here. Being
more explicit like I have is the necessary solution. Ultimately the top-level
widget's size needs to change. Another approach would be to reserve the physical
space and only show the infobar when needed. It would, however, leave some empty
whitespace on the dialog.

[Peter Kasting, 2016-11-15 23:30:27]

On 2016/11/15 22:31:27, kylix_rd wrote:
> On 2016/11/15 22:16:57, Peter Kasting wrote:
> > I think the most tricky conceptual question is: if a particular child view's
> > preferred size changes, does that necessarily mean the preferred size of
every
> > ancestor changes?  In practice, it clearly doesn't always mean that (in
fact,
> it
> > might _not_ mean that more than it does).
> 
> If that's the case, then it is wholly unsuitable for what is needed here.
Being
> more explicit like I have is the necessary solution. Ultimately the top-level
> widget's size needs to change. Another approach would be to reserve the
physical
> space and only show the infobar when needed. It would, however, leave some
empty
> whitespace on the dialog.

Another approach would be to float whatever notification this is atop stuff
below it and make it transient in time or easily closed by further user action.

But even in terms of implementing a change to the overall widget size, I'm still
wondering whether using something like PreferredSizeChanged(), and plumbing it
through your ancestry as needed, makes sense.  Should we resize the widget if
the sizes of other children change?  How many layers of ancestry are there? 
"Yes" and "few" are answers which would argue more strongly for something like
this route.

[kylix_rd, 2016-11-21 17:53:43]

On 2016/11/15 23:30:27, Peter Kasting wrote:
> On 2016/11/15 22:31:27, kylix_rd wrote:
> > On 2016/11/15 22:16:57, Peter Kasting wrote:
> > > I think the most tricky conceptual question is: if a particular child
view's
> > > preferred size changes, does that necessarily mean the preferred size of
> every
> > > ancestor changes?  In practice, it clearly doesn't always mean that (in
> fact,
> > it
> > > might _not_ mean that more than it does).
> > 
> > If that's the case, then it is wholly unsuitable for what is needed here.
> Being
> > more explicit like I have is the necessary solution. Ultimately the
top-level
> > widget's size needs to change. Another approach would be to reserve the
> physical
> > space and only show the infobar when needed. It would, however, leave some
> empty
> > whitespace on the dialog.
> 
> Another approach would be to float whatever notification this is atop stuff
> below it and make it transient in time or easily closed by further user
action.
> 
> But even in terms of implementing a change to the overall widget size, I'm
still
> wondering whether using something like PreferredSizeChanged(), and plumbing it
> through your ancestry as needed, makes sense.  Should we resize the widget if
> the sizes of other children change?  How many layers of ancestry are there? 
> "Yes" and "few" are answers which would argue more strongly for something like
> this route.

There seems to be about 4-5 layers of containers, several of which are commonly
used types. Specialized descendant versions would need to be created for each of
them.

As for whether or not the size should change, I cannot really say for sure.
However, based on how it is using the GridLayout, it seems that the intent was
to either reserve the space needed for the info-bar or to grow the size
necessary to show the info-bar. I did go back into the history of the view and
it's not clear that the info-bar was ever visible. A change in how the
GridLayout deals with invisible views may explain this as the info-bar view
isn't initially made visible.

[Peter Kasting, 2016-11-21 19:48:13]

On 2016/11/21 17:53:43, kylix_rd wrote:
> On 2016/11/15 23:30:27, Peter Kasting wrote:
> > On 2016/11/15 22:31:27, kylix_rd wrote:
> > > On 2016/11/15 22:16:57, Peter Kasting wrote:
> > > > I think the most tricky conceptual question is: if a particular child
> view's
> > > > preferred size changes, does that necessarily mean the preferred size of
> > every
> > > > ancestor changes?  In practice, it clearly doesn't always mean that (in
> > fact,
> > > it
> > > > might _not_ mean that more than it does).
> > > 
> > > If that's the case, then it is wholly unsuitable for what is needed here.
> > Being
> > > more explicit like I have is the necessary solution. Ultimately the
> top-level
> > > widget's size needs to change. Another approach would be to reserve the
> > physical
> > > space and only show the infobar when needed. It would, however, leave some
> > empty
> > > whitespace on the dialog.
> > 
> > Another approach would be to float whatever notification this is atop stuff
> > below it and make it transient in time or easily closed by further user
> action.
> > 
> > But even in terms of implementing a change to the overall widget size, I'm
> still
> > wondering whether using something like PreferredSizeChanged(), and plumbing
it
> > through your ancestry as needed, makes sense.  Should we resize the widget
if
> > the sizes of other children change?  How many layers of ancestry are there? 
> > "Yes" and "few" are answers which would argue more strongly for something
like
> > this route.
> 
> There seems to be about 4-5 layers of containers, several of which are
commonly
> used types. Specialized descendant versions would need to be created for each
of
> them.

...if we wanted to limit the behavior change to this dialog, right?  Of course,
that goes back to the core question of whether this is something we only want
here, or something in need of more systematic change.

> As for whether or not the size should change, I cannot really say for sure.
> However, based on how it is using the GridLayout, it seems that the intent was
> to either reserve the space needed for the info-bar or to grow the size
> necessary to show the info-bar. I did go back into the history of the view and
> it's not clear that the info-bar was ever visible. A change in how the
> GridLayout deals with invisible views may explain this as the info-bar view
> isn't initially made visible.

Perhaps we assumed that a change in child visibility would cause an
auto-relayout of the layout manager?  I don't know that that's ever been the
case, though.

I feel out of my depth here.  I'd really like to hear sky's input, at least on
the larger architectural questions.

[kylix_rd, 2016-11-23 17:44:43]

On 2016/11/21 19:48:13, Peter Kasting wrote:
> On 2016/11/21 17:53:43, kylix_rd wrote:
> > On 2016/11/15 23:30:27, Peter Kasting wrote:
> > > On 2016/11/15 22:31:27, kylix_rd wrote:
> > > > On 2016/11/15 22:16:57, Peter Kasting wrote:
> > > > > I think the most tricky conceptual question is: if a particular child
> > view's
> > > > > preferred size changes, does that necessarily mean the preferred size
of
> > > every
> > > > > ancestor changes?  In practice, it clearly doesn't always mean that
(in
> > > fact,
> > > > it
> > > > > might _not_ mean that more than it does).
> > > > 
> > > > If that's the case, then it is wholly unsuitable for what is needed
here.
> > > Being
> > > > more explicit like I have is the necessary solution. Ultimately the
> > top-level
> > > > widget's size needs to change. Another approach would be to reserve the
> > > physical
> > > > space and only show the infobar when needed. It would, however, leave
some
> > > empty
> > > > whitespace on the dialog.
> > > 
> > > Another approach would be to float whatever notification this is atop
stuff
> > > below it and make it transient in time or easily closed by further user
> > action.
> > > 
> > > But even in terms of implementing a change to the overall widget size, I'm
> > still
> > > wondering whether using something like PreferredSizeChanged(), and
plumbing
> it
> > > through your ancestry as needed, makes sense.  Should we resize the widget
> if
> > > the sizes of other children change?  How many layers of ancestry are
there? 
> > > "Yes" and "few" are answers which would argue more strongly for something
> like
> > > this route.
> > 
> > There seems to be about 4-5 layers of containers, several of which are
> commonly
> > used types. Specialized descendant versions would need to be created for
each
> of
> > them.
> 
> ...if we wanted to limit the behavior change to this dialog, right?  Of
course,
> that goes back to the core question of whether this is something we only want
> here, or something in need of more systematic change.
> 
> > As for whether or not the size should change, I cannot really say for sure.
> > However, based on how it is using the GridLayout, it seems that the intent
was
> > to either reserve the space needed for the info-bar or to grow the size
> > necessary to show the info-bar. I did go back into the history of the view
and
> > it's not clear that the info-bar was ever visible. A change in how the
> > GridLayout deals with invisible views may explain this as the info-bar view
> > isn't initially made visible.
> 
> Perhaps we assumed that a change in child visibility would cause an
> auto-relayout of the layout manager?  I don't know that that's ever been the
> case, though.

The visibility change does cause an auto-relayout of the layout manager. The
problem is that the initial space allocated to the view isn't enough to display
the just-made-visible view; it ends up being clipped. The top-level Widget
doesn't changed size to account for the extra size.
 
> I feel out of my depth here.  I'd really like to hear sky's input, at least on
> the larger architectural questions.

In this new patch, I made the InfobarView visible, but the content_ view
invisible. This has the effect of causing the GridLayout to reserve the space
for the InfobarView at the expense of a little more whitespace just below the
cookie_info_view_(CookieInfoView). It also required fewer changes.

[kylix_rd, 2016-11-23 17:45:17]

kylixrd@chromium.org changed reviewers:
+ sky@chromium.org

[kylix_rd, 2016-11-23 17:51:17]

Added sky@ to review per implied request from pkasting@

[kylix_rd, 2016-11-23 17:52:49]

On 2016/11/23 17:51:17, kylix_rd wrote:
> Added sky@ to review per implied request from pkasting@

Collected Cookies View without visible InfobarView:
https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNR3BxOGI3U19NZk0/vie...
Collected Cookies View with visible InfobarView:
https://drive.google.com/a/google.com/file/d/0B6LQg8CWo5bNN1NuUl9MY0JQX0U/vie...

[Peter Kasting, 2016-11-23 17:52:57]

On 2016/11/23 17:44:43, kylix_rd wrote:
> On 2016/11/21 19:48:13, Peter Kasting wrote:
> > On 2016/11/21 17:53:43, kylix_rd wrote:
> > > On 2016/11/15 23:30:27, Peter Kasting wrote:
> > > > On 2016/11/15 22:31:27, kylix_rd wrote:
> > > > > On 2016/11/15 22:16:57, Peter Kasting wrote:
> > > > > > I think the most tricky conceptual question is: if a particular
child
> > > view's
> > > > > > preferred size changes, does that necessarily mean the preferred
size
> of
> > > > every
> > > > > > ancestor changes?  In practice, it clearly doesn't always mean that
> (in
> > > > fact,
> > > > > it
> > > > > > might _not_ mean that more than it does).
> > > > > 
> > > > > If that's the case, then it is wholly unsuitable for what is needed
> here.
> > > > Being
> > > > > more explicit like I have is the necessary solution. Ultimately the
> > > top-level
> > > > > widget's size needs to change. Another approach would be to reserve
the
> > > > physical
> > > > > space and only show the infobar when needed. It would, however, leave
> some
> > > > empty
> > > > > whitespace on the dialog.
> > > > 
> > > > Another approach would be to float whatever notification this is atop
> stuff
> > > > below it and make it transient in time or easily closed by further user
> > > action.
> > > > 
> > > > But even in terms of implementing a change to the overall widget size,
I'm
> > > still
> > > > wondering whether using something like PreferredSizeChanged(), and
> plumbing
> > it
> > > > through your ancestry as needed, makes sense.  Should we resize the
widget
> > if
> > > > the sizes of other children change?  How many layers of ancestry are
> there? 
> > > > "Yes" and "few" are answers which would argue more strongly for
something
> > like
> > > > this route.
> > > 
> > > There seems to be about 4-5 layers of containers, several of which are
> > commonly
> > > used types. Specialized descendant versions would need to be created for
> each
> > of
> > > them.
> > 
> > ...if we wanted to limit the behavior change to this dialog, right?  Of
> course,
> > that goes back to the core question of whether this is something we only
want
> > here, or something in need of more systematic change.
> > 
> > > As for whether or not the size should change, I cannot really say for
sure.
> > > However, based on how it is using the GridLayout, it seems that the intent
> was
> > > to either reserve the space needed for the info-bar or to grow the size
> > > necessary to show the info-bar. I did go back into the history of the view
> and
> > > it's not clear that the info-bar was ever visible. A change in how the
> > > GridLayout deals with invisible views may explain this as the info-bar
view
> > > isn't initially made visible.
> > 
> > Perhaps we assumed that a change in child visibility would cause an
> > auto-relayout of the layout manager?  I don't know that that's ever been the
> > case, though.
> 
> The visibility change does cause an auto-relayout of the layout manager. The
> problem is that the initial space allocated to the view isn't enough to
display
> the just-made-visible view; it ends up being clipped. The top-level Widget
> doesn't changed size to account for the extra size.

Right.  If we want this behavior, we need to resize the top-level widget in
response to something.  My hope was that the top-level dialog could notice that
the layout manager's preferred size changed, and resize the overall widget in
response.

> > I feel out of my depth here.  I'd really like to hear sky's input, at least
on
> > the larger architectural questions.
> 
> In this new patch, I made the InfobarView visible, but the content_ view
> invisible. This has the effect of causing the GridLayout to reserve the space
> for the InfobarView at the expense of a little more whitespace just below the
> cookie_info_view_(CookieInfoView). It also required fewer changes.

This works too, and I'm fine with this technically, it's just a question of what
behavior we want.  There are arguments for both.

If we go the current route, I would add comments about why we need to do
visibility this way.

[sky, 2016-11-23 20:42:14]

Seems like there is a couple of questions here:

PreferredSizeChanged() & ChildPreferredSizeChanged():

These were added for a particular use case: direct children changing
their preferred size and having the parent be able to detect this and
layout. This is pretty narrow though, and these functions should
either be made more general or nuked. I have ideas here and am happy
to discuss if interested.

How should layout managers treat invisible views? Only the consumer of
the layoutmanager can make the decision as to what the right answer
is. What GridLayout does has come up in the past. See 235299 for
details and what I think should be done.

As to the proposed fix it sure looks like the dialog has way too much
whitespace, but I favor that over potentially resizing the dialog when
the infobar has to be shown.

  -Scott

On Wed, Nov 23, 2016 at 9:52 AM,  <pkasting@chromium.org> wrote:
> On 2016/11/23 17:44:43, kylix_rd wrote:
>> On 2016/11/21 19:48:13, Peter Kasting wrote:
>> > On 2016/11/21 17:53:43, kylix_rd wrote:
>> > > On 2016/11/15 23:30:27, Peter Kasting wrote:
>> > > > On 2016/11/15 22:31:27, kylix_rd wrote:
>> > > > > On 2016/11/15 22:16:57, Peter Kasting wrote:
>> > > > > > I think the most tricky conceptual question is: if a particular
> child
>> > > view's
>> > > > > > preferred size changes, does that necessarily mean the preferred
> size
>> of
>> > > > every
>> > > > > > ancestor changes? In practice, it clearly doesn't always mean
>> > > > > > that
>> (in
>> > > > fact,
>> > > > > it
>> > > > > > might _not_ mean that more than it does).
>> > > > >
>> > > > > If that's the case, then it is wholly unsuitable for what is
>> > > > > needed
>> here.
>> > > > Being
>> > > > > more explicit like I have is the necessary solution. Ultimately
>> > > > > the
>> > > top-level
>> > > > > widget's size needs to change. Another approach would be to
>> > > > > reserve
> the
>> > > > physical
>> > > > > space and only show the infobar when needed. It would, however,
>> > > > > leave
>> some
>> > > > empty
>> > > > > whitespace on the dialog.
>> > > >
>> > > > Another approach would be to float whatever notification this is
>> > > > atop
>> stuff
>> > > > below it and make it transient in time or easily closed by further
>> > > > user
>> > > action.
>> > > >
>> > > > But even in terms of implementing a change to the overall widget
>> > > > size,
> I'm
>> > > still
>> > > > wondering whether using something like PreferredSizeChanged(), and
>> plumbing
>> > it
>> > > > through your ancestry as needed, makes sense. Should we resize the
> widget
>> > if
>> > > > the sizes of other children change? How many layers of ancestry are
>> there?
>> > > > "Yes" and "few" are answers which would argue more strongly for
> something
>> > like
>> > > > this route.
>> > >
>> > > There seems to be about 4-5 layers of containers, several of which are
>> > commonly
>> > > used types. Specialized descendant versions would need to be created
>> > > for
>> each
>> > of
>> > > them.
>> >
>> > ...if we wanted to limit the behavior change to this dialog, right? Of
>> course,
>> > that goes back to the core question of whether this is something we only
> want
>> > here, or something in need of more systematic change.
>> >
>> > > As for whether or not the size should change, I cannot really say for
> sure.
>> > > However, based on how it is using the GridLayout, it seems that the
>> > > intent
>> was
>> > > to either reserve the space needed for the info-bar or to grow the
>> > > size
>> > > necessary to show the info-bar. I did go back into the history of the
>> > > view
>> and
>> > > it's not clear that the info-bar was ever visible. A change in how the
>> > > GridLayout deals with invisible views may explain this as the info-bar
> view
>> > > isn't initially made visible.
>> >
>> > Perhaps we assumed that a change in child visibility would cause an
>> > auto-relayout of the layout manager? I don't know that that's ever been
>> > the
>> > case, though.
>>
>> The visibility change does cause an auto-relayout of the layout manager.
>> The
>> problem is that the initial space allocated to the view isn't enough to
> display
>> the just-made-visible view; it ends up being clipped. The top-level Widget
>> doesn't changed size to account for the extra size.
>
> Right. If we want this behavior, we need to resize the top-level widget in
> response to something. My hope was that the top-level dialog could notice
> that
> the layout manager's preferred size changed, and resize the overall widget
> in
> response.
>
>> > I feel out of my depth here. I'd really like to hear sky's input, at
>> > least
> on
>> > the larger architectural questions.
>>
>> In this new patch, I made the InfobarView visible, but the content_ view
>> invisible. This has the effect of causing the GridLayout to reserve the
>> space
>> for the InfobarView at the expense of a little more whitespace just below
>> the
>> cookie_info_view_(CookieInfoView). It also required fewer changes.
>
> This works too, and I'm fine with this technically, it's just a question of
> what
> behavior we want. There are arguments for both.
>
> If we go the current route, I would add comments about why we need to do
> visibility this way.
>
> https://codereview.chromium.org/2503643002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylix_rd, 2016-11-23 20:55:47]

On 2016/11/23 20:42:14, sky wrote:
> Seems like there is a couple of questions here:
> 
> PreferredSizeChanged() & ChildPreferredSizeChanged():
> 
> These were added for a particular use case: direct children changing
> their preferred size and having the parent be able to detect this and
> layout. This is pretty narrow though, and these functions should
> either be made more general or nuked. I have ideas here and am happy
> to discuss if interested.

I'd like to have that discussion in a separate venue. While related to this
issue, making changes there may have a much wider impact beyond the scope of
what I'm trying to fix here.
 
> How should layout managers treat invisible views? Only the consumer of
> the layoutmanager can make the decision as to what the right answer
> is. What GridLayout does has come up in the past. See 235299 for
> details and what I think should be done.

For a grid layout, the presence of a given row or column should be somewhat
independent of it's content. In this instance, the row/column containing the
InfobarView is still present; it just has a vertical size of 0. In that, I agree
with the statement in 235299.
 
> As to the proposed fix it sure looks like the dialog has way too much
> whitespace, but I favor that over potentially resizing the dialog when
> the infobar has to be shown.

Short of a much wider disruption to the system, I also favor this approach.
 
>   -Scott
> 
> On Wed, Nov 23, 2016 at 9:52 AM,  <mailto:pkasting@chromium.org> wrote:
> > On 2016/11/23 17:44:43, kylix_rd wrote:
> >> On 2016/11/21 19:48:13, Peter Kasting wrote:
> >> > On 2016/11/21 17:53:43, kylix_rd wrote:
> >> > > On 2016/11/15 23:30:27, Peter Kasting wrote:
> >> > > > On 2016/11/15 22:31:27, kylix_rd wrote:
> >> > > > > On 2016/11/15 22:16:57, Peter Kasting wrote:
> >> > > > > > I think the most tricky conceptual question is: if a particular
> > child
> >> > > view's
> >> > > > > > preferred size changes, does that necessarily mean the preferred
> > size
> >> of
> >> > > > every
> >> > > > > > ancestor changes? In practice, it clearly doesn't always mean
> >> > > > > > that
> >> (in
> >> > > > fact,
> >> > > > > it
> >> > > > > > might _not_ mean that more than it does).
> >> > > > >
> >> > > > > If that's the case, then it is wholly unsuitable for what is
> >> > > > > needed
> >> here.
> >> > > > Being
> >> > > > > more explicit like I have is the necessary solution. Ultimately
> >> > > > > the
> >> > > top-level
> >> > > > > widget's size needs to change. Another approach would be to
> >> > > > > reserve
> > the
> >> > > > physical
> >> > > > > space and only show the infobar when needed. It would, however,
> >> > > > > leave
> >> some
> >> > > > empty
> >> > > > > whitespace on the dialog.
> >> > > >
> >> > > > Another approach would be to float whatever notification this is
> >> > > > atop
> >> stuff
> >> > > > below it and make it transient in time or easily closed by further
> >> > > > user
> >> > > action.
> >> > > >
> >> > > > But even in terms of implementing a change to the overall widget
> >> > > > size,
> > I'm
> >> > > still
> >> > > > wondering whether using something like PreferredSizeChanged(), and
> >> plumbing
> >> > it
> >> > > > through your ancestry as needed, makes sense. Should we resize the
> > widget
> >> > if
> >> > > > the sizes of other children change? How many layers of ancestry are
> >> there?
> >> > > > "Yes" and "few" are answers which would argue more strongly for
> > something
> >> > like
> >> > > > this route.
> >> > >
> >> > > There seems to be about 4-5 layers of containers, several of which are
> >> > commonly
> >> > > used types. Specialized descendant versions would need to be created
> >> > > for
> >> each
> >> > of
> >> > > them.
> >> >
> >> > ...if we wanted to limit the behavior change to this dialog, right? Of
> >> course,
> >> > that goes back to the core question of whether this is something we only
> > want
> >> > here, or something in need of more systematic change.
> >> >
> >> > > As for whether or not the size should change, I cannot really say for
> > sure.
> >> > > However, based on how it is using the GridLayout, it seems that the
> >> > > intent
> >> was
> >> > > to either reserve the space needed for the info-bar or to grow the
> >> > > size
> >> > > necessary to show the info-bar. I did go back into the history of the
> >> > > view
> >> and
> >> > > it's not clear that the info-bar was ever visible. A change in how the
> >> > > GridLayout deals with invisible views may explain this as the info-bar
> > view
> >> > > isn't initially made visible.
> >> >
> >> > Perhaps we assumed that a change in child visibility would cause an
> >> > auto-relayout of the layout manager? I don't know that that's ever been
> >> > the
> >> > case, though.
> >>
> >> The visibility change does cause an auto-relayout of the layout manager.
> >> The
> >> problem is that the initial space allocated to the view isn't enough to
> > display
> >> the just-made-visible view; it ends up being clipped. The top-level Widget
> >> doesn't changed size to account for the extra size.
> >
> > Right. If we want this behavior, we need to resize the top-level widget in
> > response to something. My hope was that the top-level dialog could notice
> > that
> > the layout manager's preferred size changed, and resize the overall widget
> > in
> > response.
> >
> >> > I feel out of my depth here. I'd really like to hear sky's input, at
> >> > least
> > on
> >> > the larger architectural questions.
> >>
> >> In this new patch, I made the InfobarView visible, but the content_ view
> >> invisible. This has the effect of causing the GridLayout to reserve the
> >> space
> >> for the InfobarView at the expense of a little more whitespace just below
> >> the
> >> cookie_info_view_(CookieInfoView). It also required fewer changes.
> >
> > This works too, and I'm fine with this technically, it's just a question of
> > what
> > behavior we want. There are arguments for both.
> >
> > If we go the current route, I would add comments about why we need to do
> > visibility this way.
> >
> > https://codereview.chromium.org/2503643002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Peter Kasting, 2016-11-23 21:00:01]

It seems like we're converging toward doing your current patch set.  Is there a
screenshot of this with/without the infobar?  I'm just wondering whether we
should file a bug for a better design in the future, e.g. replace the infobar
with a time-limited floating "bubble" atop other content, which could avoid both
the "too much whitespace" and "resize dialog" issues.

[kylix_rd, 2016-11-23 21:51:27]

On 2016/11/23 21:00:01, Peter Kasting wrote:
> It seems like we're converging toward doing your current patch set.  Is there
a
> screenshot of this with/without the infobar?  I'm just wondering whether we
> should file a bug for a better design in the future, e.g. replace the infobar
> with a time-limited floating "bubble" atop other content, which could avoid
both
> the "too much whitespace" and "resize dialog" issues.

See the links in comment #16.

[Peter Kasting, 2016-11-23 23:50:37]

On 2016/11/23 21:51:27, kylix_rd wrote:
> On 2016/11/23 21:00:01, Peter Kasting wrote:
> > It seems like we're converging toward doing your current patch set.  Is
there
> a
> > screenshot of this with/without the infobar?  I'm just wondering whether we
> > should file a bug for a better design in the future, e.g. replace the
infobar
> > with a time-limited floating "bubble" atop other content, which could avoid
> both
> > the "too much whitespace" and "resize dialog" issues.
> 
> See the links in comment #16.

Ah, thanks, missed those.

The infobar is not the most elegant UI in the universe, but I think it's good
enough that there's no need for followup here.  LGTM for behavior and
implementation, although bonus points if you are able to add those clarifying
comments about why we muck with visibility the way we do.

I think the discussion points raised re: size changed notifications, as well as
how GridLayout should deal with child visibility, probably deserve followup in
another forum, as you suggested.  It seems like one or both of you would be more
able to drive those discussions than I would, so I'll leave that in your hands?

[sky, 2016-11-23 23:53:53]

I'll file a bug on where I think PreferredSizeChanged() &
ChildPreferredSizeChanged() should go. There is already a bug on
GridLayout, and it just needs someone with time;)

  -Scott

On Wed, Nov 23, 2016 at 3:50 PM,  <pkasting@chromium.org> wrote:
> On 2016/11/23 21:51:27, kylix_rd wrote:
>> On 2016/11/23 21:00:01, Peter Kasting wrote:
>> > It seems like we're converging toward doing your current patch set. Is
> there
>> a
>> > screenshot of this with/without the infobar? I'm just wondering whether
>> > we
>> > should file a bug for a better design in the future, e.g. replace the
> infobar
>> > with a time-limited floating "bubble" atop other content, which could
>> > avoid
>> both
>> > the "too much whitespace" and "resize dialog" issues.
>>
>> See the links in comment #16.
>
> Ah, thanks, missed those.
>
> The infobar is not the most elegant UI in the universe, but I think it's
> good
> enough that there's no need for followup here. LGTM for behavior and
> implementation, although bonus points if you are able to add those
> clarifying
> comments about why we muck with visibility the way we do.
>
> I think the discussion points raised re: size changed notifications, as well
> as
> how GridLayout should deal with child visibility, probably deserve followup
> in
> another forum, as you suggested. It seems like one or both of you would be
> more
> able to drive those discussions than I would, so I'll leave that in your
> hands?
>
> https://codereview.chromium.org/2503643002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylix_rd, 2016-11-29 16:47:10]

This latest patch is, IMO, a better solution. This results in the same effect by
removing rather than adding/changing code. The GetPreferredSize() override in
the InfobarView class would return a zero(0) size based on it's own visibility.
Removing that check would allow it to always return the correct preferred size.

[kylix_rd, 2016-11-29 17:35:43]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 17:36:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 19:00:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 19:00:29]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2016-11-29 19:49:43]

Patchset #5 (id:80001) has been deleted

[kylix_rd, 2016-11-29 19:50:03]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 19:50:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2016-11-29 20:51:17]

LGTM

https://codereview.chromium.org/2503643002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/collected_cookies_views.cc (right):

https://codereview.chromium.org/2503643002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:157: // containing dialog
isn't large enough for it to ever become visible.
This comment makes sense in the context of this patch, but will be confusing in
the future for people without the context of either the bug or the old code. 
What about something like this:

Always return the preferred size, even if not currently visible.  This will
cause the layout manager to reserve space for this infobar so it can easily be
made visible later if necessary.  Otherwise, toggling the visibility of the
infobar would require resizing the entire dialog larger to make room, which is
undesirable from both a UX and a technical perspective.

[sky, 2016-11-29 21:08:59]

LGTM

[kylix_rd, 2016-11-29 22:23:17]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2016-11-29 22:23:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2503643002/#ps120001
(title: "Update comment per suggestion")

[commit-bot: I haz the power, 2016-11-29 22:23:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 00:27:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:27:01]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[kylix_rd, 2016-11-30 14:04:52]

The CQ bit was checked by kylixrd@chromium.org

[commit-bot: I haz the power, 2016-11-30 14:05:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 14:08:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480514692374070,
"parent_rev": "362bafdfccf543b5986894d5a0c857f461b06637", "commit_rev":
"605c8c2bd03266010a97fc72c2b6732ead4d61ed"}

[commit-bot: I haz the power, 2016-11-30 14:08:48]

Description was changed from

==========
Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053
==========

to

==========
Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053
==========

[commit-bot: I haz the power, 2016-11-30 14:08:49]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-30 14:13:04]

Description was changed from

==========
Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053
==========

to

==========
Fixed collected cookies view info bar visibility

Need to recalculate the dialog size once the InfobarView is made visible.

BUG=665053

Committed: https://crrev.com/11f088e941f3264c42b2cb3265c2cc6d48397712
Cr-Commit-Position: refs/heads/master@{#435249}
==========

[commit-bot: I haz the power, 2016-11-30 14:13:06]

Patchset 6 (id:??) landed as
https://crrev.com/11f088e941f3264c42b2cb3265c2cc6d48397712
Cr-Commit-Position: refs/heads/master@{#435249}