Update property trees' bounds delta before updating viewport container sizes.

Update property trees' bounds delta before updating viewport container sizes.

Property trees store the bounds delta, caused by showing and hiding top
controls, separately from per-node data. This means that after a pending tree is
activated, these values need to be filled in from the LayerImpls. This was
previously happening after the call to UpdateViewportContainerSizes. This caused
a problem since UpdateViewportContainerSizes would indirectly clamp the layers
scroll offsets using LayerImpl::MaxScrollOffset, which gets the bounds_delta
from the property tree. This meant the bounds_delta would return a value of 0,
causing incorrect clamping.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7ad75edb50ed225455b994f4e310d6db86205aa2
Cr-Commit-Position: refs/heads/master@{#384054}

[bokan, 2016-03-30 01:31:16]

Description was changed from

==========
Update property trees' bounds delta before updating viewport container sizes.

Property trees store the bounds delta, caused by showing and hiding top
controls, separately from per-node data. This means that after a pending tree is
activated, these values need to be filled in from the LayerImpls. This was
previously happening after the call to UpdateViewportContainerSizes. This caused
a problem since UpdateViewportContainerSizes would indirectly clamp the layers
scroll offsets using LayerImpl::MaxScrollOffset, which gets the bounds_delta
from the property tree. This meant the bounds_delta would return a value of 0,
causing incorrect clamping.

BUG=597266
==========

to

==========
Update property trees' bounds delta before updating viewport container sizes.

Property trees store the bounds delta, caused by showing and hiding top
controls, separately from per-node data. This means that after a pending tree is
activated, these values need to be filled in from the LayerImpls. This was
previously happening after the call to UpdateViewportContainerSizes. This caused
a problem since UpdateViewportContainerSizes would indirectly clamp the layers
scroll offsets using LayerImpl::MaxScrollOffset, which gets the bounds_delta
from the property tree. This meant the bounds_delta would return a value of 0,
causing incorrect clamping.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-03-30 01:32:38]

bokan@chromium.org changed reviewers:
+ ajuma@chromium.org

[bokan, 2016-03-30 01:32:39]

Hey Ali, ptal. I think it's ok to move this call to only happen if we have a
pending tree right? Otherwise the layers won't have changed and we won't have to
update the bounds delta.

[bokan, 2016-03-30 11:18:30]

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

[commit-bot: I haz the power, 2016-03-30 11:18:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841783003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841783003/1

[commit-bot: I haz the power, 2016-03-30 12:18:35]

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

[commit-bot: I haz the power, 2016-03-30 12:18:36]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-03-30 14:21:24]

On 2016/03/30 01:32:39, bokan wrote:
> Hey Ali, ptal. I think it's ok to move this call to only happen if we have a
> pending tree right? Otherwise the layers won't have changed and we won't have
to
> update the bounds delta.

Bounds delta isn't computed at all when building property trees, so without the
update here the newly-pushed trees will have a bounds delta of 0 whenever we're
in single-thread mode. Is this correct (that is, does having non-zero bounds
delta depend on support for impl scrolling)?

[bokan, 2016-03-30 16:09:38]

On 2016/03/30 14:21:24, ajuma wrote:
> On 2016/03/30 01:32:39, bokan wrote:
> > Hey Ali, ptal. I think it's ok to move this call to only happen if we have a
> > pending tree right? Otherwise the layers won't have changed and we won't
have
> to
> > update the bounds delta.
> 
> Bounds delta isn't computed at all when building property trees, so without
the
> update here the newly-pushed trees will have a bounds delta of 0 whenever
we're
> in single-thread mode. Is this correct (that is, does having non-zero bounds
> delta depend on support for impl scrolling)?

by impl scrolling do you mean impl-side painting? I thought that's what having a
pending_tree means?

Technically, bounds delta should only be non-0 on Android since it's for top
controls and I think impl-side painting is always on for Android, right? Anyway,
this seems too brittle. A quick glance in ActivateAnimations shows it doesn't
rely in any obvious way on the layer tree. Do you know if that's the case? If
so, we could just move the UpdateViewportContainerSizes outside the if block
instead so that it happens after UpdatePropertyTreesForBounds. WDYT?

[ajuma, 2016-03-30 17:19:40]

On 2016/03/30 16:09:38, bokan wrote:
> On 2016/03/30 14:21:24, ajuma wrote:
> > On 2016/03/30 01:32:39, bokan wrote:
> > > Hey Ali, ptal. I think it's ok to move this call to only happen if we have
a
> > > pending tree right? Otherwise the layers won't have changed and we won't
> have
> > to
> > > update the bounds delta.
> > 
> > Bounds delta isn't computed at all when building property trees, so without
> the
> > update here the newly-pushed trees will have a bounds delta of 0 whenever
> we're
> > in single-thread mode. Is this correct (that is, does having non-zero bounds
> > delta depend on support for impl scrolling)?
> 
> by impl scrolling do you mean impl-side painting? I thought that's what having
a
> pending_tree means?

I just meant scrolling the impl tree, which we support only in threaded mode
(since we don't bother plumbing back scroll deltas from the impl side to the
main side in single-thread mode). 

> Technically, bounds delta should only be non-0 on Android since it's for top
> controls and I think impl-side painting is always on for Android, right? 

Yeah, the only case I'd worry about is layout tests (which mostly run in
single-thread mode), if these ever run with top controls enabled.

> Anyway,
> this seems too brittle. A quick glance in ActivateAnimations shows it doesn't
> rely in any obvious way on the layer tree. Do you know if that's the case?

Right, ActivateAnimations doesn't rely on bounds delta being up to date in
property trees.


> If so, we could just move the UpdateViewportContainerSizes outside the if
block
> instead so that it happens after UpdatePropertyTreesForBounds. WDYT?

Sounds good.

[bokan, 2016-03-30 17:57:11]

On 2016/03/30 17:19:40, ajuma wrote:
> On 2016/03/30 16:09:38, bokan wrote:
> > On 2016/03/30 14:21:24, ajuma wrote:
> > > On 2016/03/30 01:32:39, bokan wrote:
> > > > Hey Ali, ptal. I think it's ok to move this call to only happen if we
have
> a
> > > > pending tree right? Otherwise the layers won't have changed and we won't
> > have
> > > to
> > > > update the bounds delta.
> > > 
> > > Bounds delta isn't computed at all when building property trees, so
without
> > the
> > > update here the newly-pushed trees will have a bounds delta of 0 whenever
> > we're
> > > in single-thread mode. Is this correct (that is, does having non-zero
bounds
> > > delta depend on support for impl scrolling)?
> > 
> > by impl scrolling do you mean impl-side painting? I thought that's what
having
> a
> > pending_tree means?
> 
> I just meant scrolling the impl tree, which we support only in threaded mode
> (since we don't bother plumbing back scroll deltas from the impl side to the
> main side in single-thread mode). 
> 
> > Technically, bounds delta should only be non-0 on Android since it's for top
> > controls and I think impl-side painting is always on for Android, right? 
> 
> Yeah, the only case I'd worry about is layout tests (which mostly run in
> single-thread mode), if these ever run with top controls enabled.
> 
> > Anyway,
> > this seems too brittle. A quick glance in ActivateAnimations shows it
doesn't
> > rely in any obvious way on the layer tree. Do you know if that's the case?
> 
> Right, ActivateAnimations doesn't rely on bounds delta being up to date in
> property trees.
> 
> 
> > If so, we could just move the UpdateViewportContainerSizes outside the if
> block
> > instead so that it happens after UpdatePropertyTreesForBounds. WDYT?
> 
> Sounds good.

Ok, I moved UpdateViewportContainerSizes instead, ptal.

[ajuma, 2016-03-30 17:58:24]

Thanks, lgtm.

[bokan, 2016-03-30 17:58:54]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-03-30 17:59:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841783003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841783003/20001

[commit-bot: I haz the power, 2016-03-30 19:30:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-30 20:05:19]

Description was changed from

==========
Update property trees' bounds delta before updating viewport container sizes.

Property trees store the bounds delta, caused by showing and hiding top
controls, separately from per-node data. This means that after a pending tree is
activated, these values need to be filled in from the LayerImpls. This was
previously happening after the call to UpdateViewportContainerSizes. This caused
a problem since UpdateViewportContainerSizes would indirectly clamp the layers
scroll offsets using LayerImpl::MaxScrollOffset, which gets the bounds_delta
from the property tree. This meant the bounds_delta would return a value of 0,
causing incorrect clamping.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Update property trees' bounds delta before updating viewport container sizes.

Property trees store the bounds delta, caused by showing and hiding top
controls, separately from per-node data. This means that after a pending tree is
activated, these values need to be filled in from the LayerImpls. This was
previously happening after the call to UpdateViewportContainerSizes. This caused
a problem since UpdateViewportContainerSizes would indirectly clamp the layers
scroll offsets using LayerImpl::MaxScrollOffset, which gets the bounds_delta
from the property tree. This meant the bounds_delta would return a value of 0,
causing incorrect clamping.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7ad75edb50ed225455b994f4e310d6db86205aa2
Cr-Commit-Position: refs/heads/master@{#384054}
==========

[commit-bot: I haz the power, 2016-03-30 20:05:21]

Patchset 2 (id:??) landed as
https://crrev.com/7ad75edb50ed225455b994f4e310d6db86205aa2
Cr-Commit-Position: refs/heads/master@{#384054}