Try to fix layout of scroll views in TrayDetailsViews.

Try to fix layout of scroll views in TrayDetailsViews.

This code is still going to be fairly fragile, but it does get rid of
nearly all code in FixedSizedScrollView (for MD), which will hopefully
make it easier to reason about layout or make changes.

Note that this collaterally fixes a bug where we reserved 1dp at the
bottom of the scroller such that elements at the bottom of the scroll
area would be 1dp above the bottom of the bubble. The specs show them
as full bleed, and this fixes that (plus it makes scroll view layout
even simpler because ClipHeightTo doesn't take into account insets).

BUG=663790
Committed: https://crrev.com/0ebb32cb3aefbc61483a1dd1cb81561fef0323e4
Cr-Commit-Position: refs/heads/master@{#431666}

[Evan Stade, 2016-11-11 04:59:34]

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

[commit-bot: I haz the power, 2016-11-11 04:59:49]

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

[Evan Stade, 2016-11-11 04:59:51]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[commit-bot: I haz the power, 2016-11-11 05:27:20]

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

[commit-bot: I haz the power, 2016-11-11 05:27:21]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-11 20:09:55]

Evan, one question below:

https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:395: scroller_->ClipHeightTo(0,
scroller_->height());
It seems this only ever be reached once during the lifetime of |scroller_|,
i.e., after the first call to ClipHeightTo() then is_bounded() will always
return true, so no further calls to ClipHeightTo() will be made in subsequent
layout passes. Wouldn't this be problematic as the height of the scroller
continues to grow?

[Evan Stade, 2016-11-11 22:07:25]

https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_details_view.cc:395: scroller_->ClipHeightTo(0,
scroller_->height());
On 2016/11/11 20:09:55, tdanderson wrote:
> It seems this only ever be reached once during the lifetime of |scroller_|,
> i.e., after the first call to ClipHeightTo() then is_bounded() will always
> return true, so no further calls to ClipHeightTo() will be made in subsequent
> layout passes. Wouldn't this be problematic as the height of the scroller
> continues to grow?

you mean if more rows are added, or if the scroller itself gets allotted a
different height? It's fine if rows are added or removed. I don't think that
detailed views ever change in height, but yes that would be a problem if it did
occur (hence my comment about fragility). The hard-coded-ness of the height here
is perhaps a clue as to why someone felt the need to invent a "FixedSized"
scroll view.

Note that we essentially /have/ to call ClipHeightTo at some point because so
much of ScrollView only works if it is_bounded() (not so sure why, but perhaps
for good reason?).

[tdanderson, 2016-11-11 22:10:29]

On 2016/11/11 22:07:25, Evan Stade wrote:
>
https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
> File ash/common/system/tray/tray_details_view.cc (right):
> 
>
https://codereview.chromium.org/2496873002/diff/1/ash/common/system/tray/tray...
> ash/common/system/tray/tray_details_view.cc:395: scroller_->ClipHeightTo(0,
> scroller_->height());
> On 2016/11/11 20:09:55, tdanderson wrote:
> > It seems this only ever be reached once during the lifetime of |scroller_|,
> > i.e., after the first call to ClipHeightTo() then is_bounded() will always
> > return true, so no further calls to ClipHeightTo() will be made in
subsequent
> > layout passes. Wouldn't this be problematic as the height of the scroller
> > continues to grow?
> 
> you mean if more rows are added, or if the scroller itself gets allotted a
> different height? It's fine if rows are added or removed. I don't think that
> detailed views ever change in height, but yes that would be a problem if it
did
> occur (hence my comment about fragility). The hard-coded-ness of the height
here
> is perhaps a clue as to why someone felt the need to invent a "FixedSized"
> scroll view.

I meant if rows are added. Thanks for the reply. LGTM.

> 
> Note that we essentially /have/ to call ClipHeightTo at some point because so
> much of ScrollView only works if it is_bounded() (not so sure why, but perhaps
> for good reason?).

[Evan Stade, 2016-11-11 22:13:37]

The CQ bit was checked by estade@chromium.org

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

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-11 22:24:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-11 22:44:22]

Description was changed from

==========
Try to fix layout of scroll views in TrayDetailsViews.

This code is still going to be fairly fragile, but it does get rid of
nearly all code in FixedSizedScrollView (for MD), which will hopefully
make it easier to reason about layout or make changes.

Note that this collaterally fixes a bug where we reserved 1dp at the
bottom of the scroller such that elements at the bottom of the scroll
area would be 1dp above the bottom of the bubble. The specs show them
as full bleed, and this fixes that (plus it makes scroll view layout
even simpler because ClipHeightTo doesn't take into account insets).

BUG=663790
==========

to

==========
Try to fix layout of scroll views in TrayDetailsViews.

This code is still going to be fairly fragile, but it does get rid of
nearly all code in FixedSizedScrollView (for MD), which will hopefully
make it easier to reason about layout or make changes.

Note that this collaterally fixes a bug where we reserved 1dp at the
bottom of the scroller such that elements at the bottom of the scroll
area would be 1dp above the bottom of the bubble. The specs show them
as full bleed, and this fixes that (plus it makes scroll view layout
even simpler because ClipHeightTo doesn't take into account insets).

BUG=663790

Committed: https://crrev.com/0ebb32cb3aefbc61483a1dd1cb81561fef0323e4
Cr-Commit-Position: refs/heads/master@{#431666}
==========

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

Patchset 1 (id:??) landed as
https://crrev.com/0ebb32cb3aefbc61483a1dd1cb81561fef0323e4
Cr-Commit-Position: refs/heads/master@{#431666}