DO NOT MERGE: Fix top_controls_manager to stay hidden

Fix top_controls_manager to stay hidden

Previously, top controls could show as height grows even if it was hidden.
But it should respect the permitted state and stay hidden.

BUG=430635

[Changwan Ryu, 2015-02-04 01:37:10]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, bokan@chromium.org, dtrainor@chromium.org

[Changwan Ryu, 2015-02-04 01:37:10]

This is required to refactor webapp activity using compositor view, as webapp
should allow top controls to stay hidden at the beginning. Please take a look.

[aelias_OOO_until_Jul13, 2015-02-04 02:21:22]

Hmm, I believe the reason the height change caused this bug is because of the
"top_controls_height_delta" in LayerTreeHost::FinishCommitOnImplThread and
LayerTreeImpl::PushPropertiesTo.  That logic is designed to maintain the top
controls in a showing state.  Now you're adding more logic later down the
pipeline to cancel that out, for the hidden case.

It seems to me it shouldn't be CC's responsibility to make the content_offset
react sanely to height changes.  Both of those values are supplied by client
code in upper levels of the renderer in the same commit, and that client code
should make them consistent with each other.

I'd suggest deleting the existing top_controls_height_delta subtraction logic,
and replacing the blink::WebWidget method setTopControlsLayoutHeight with a new
method "void setTopControlsHeight(float top_controls_height, bool
top_controls_shrink_layout_size)".  Then, the implementation in WebViewImpl
should adjust topControlsContentOffset to be proportional to the height change
(i.e. divide by the old height, multiply by the new height).

(I realize adding new plumbing to Blink is annoying but I was planning to
eventually make Blink aware of the distinction between height and layout height
anyway, because that information will be needed to allow main-thread top
controls scrolling.)

[aelias_OOO_until_Jul13, 2015-02-04 03:13:45]

I realized the proportionality algorithm I described won't work if the top
controls are going from 0 height to nonzero height and the top controls state is
SHOWN.  To avoid that, we should switch top_controls_content_offset_ to be
stored as a normalized value between 0 and 1, and multiply it by height at
time-of-use.  Then in general, we won't need to touch the offset at all when
height changes -- they'll be fully orthogonal.

Blink still needs to hear about top controls height in the normalized world
because it needs the absolute value for
FrameView::setTopControlsViewportAdjustment.

[aelias_OOO_until_Jul13, 2015-02-04 03:42:09]

I'm currently refactoring some top controls stuff anyway so let me prepare some
patches myself implementing what I described.

[Changwan Ryu, 2015-02-04 05:03:22]

On 2015/02/04 03:42:09, aelias wrote:
> I'm currently refactoring some top controls stuff anyway so let me prepare
some
> patches myself implementing what I described.

I thought of this more of a general inconsistency in the top controls manager
itself, though it can also be resolved by touching upper level code. One thing
clear is that top controls SHOULD NOT be shown when the height grows in
HIDDEN-only state while it SHOULD be shown in SHOWN-only state, which I am not
sure how the mentioned algorithm can address.

Does top_controls_height_delta part have to do with the way I changed the order
of the function calls? If so, I wonder if the refactoring can be done separately
on top of this.

[aelias_OOO_until_Jul13, 2015-02-04 05:21:33]

On 2015/02/04 at 05:03:22, changwan wrote:
> On 2015/02/04 03:42:09, aelias wrote:
> > I'm currently refactoring some top controls stuff anyway so let me prepare
some
> > patches myself implementing what I described.
> 
> I thought of this more of a general inconsistency in the top controls manager
itself, though it can also be resolved by touching upper level code. One thing
clear is that top controls SHOULD NOT be shown when the height grows in
HIDDEN-only state while it SHOULD be shown in SHOWN-only state, which I am not
sure how the mentioned algorithm can address.

I think it would be OK as such for the top controls manager to prevent the
offset from ever being set to an invalid value when in HIDDEN-only or SHOWN-only
mode.  But that wouldn't help when the top controls are free to move around
freely -- in that case, we still want to maintain the offset in its current
state (be that hidden or shown).  And because there's nothing outside
TopControlsManager actively changing the top controls offset, maintaining the
current state is sufficient to solve the entire problem currently. 

> 
> Does top_controls_height_delta part have to do with the way I changed the
order of the function calls? If so, I wonder if the refactoring can be done
separately on top of this.

No, that's unrelated -- the ordering sensitivity is due to the clamp to height
at the beginning of TopControlsManager::SetControlsTopOffset.

[Changwan Ryu, 2015-02-04 05:43:03]

On 2015/02/04 05:21:33, aelias wrote:
> On 2015/02/04 at 05:03:22, changwan wrote:
> > On 2015/02/04 03:42:09, aelias wrote:
> > > I'm currently refactoring some top controls stuff anyway so let me prepare
> some
> > > patches myself implementing what I described.
> > 
> > I thought of this more of a general inconsistency in the top controls
manager
> itself, though it can also be resolved by touching upper level code. One thing
> clear is that top controls SHOULD NOT be shown when the height grows in
> HIDDEN-only state while it SHOULD be shown in SHOWN-only state, which I am not
> sure how the mentioned algorithm can address.
> 
> I think it would be OK as such for the top controls manager to prevent the
> offset from ever being set to an invalid value when in HIDDEN-only or
SHOWN-only
> mode.  But that wouldn't help when the top controls are free to move around
> freely -- in that case, we still want to maintain the offset in its current
> state (be that hidden or shown).  And because there's nothing outside
> TopControlsManager actively changing the top controls offset, maintaining the
> current state is sufficient to solve the entire problem currently. 
> 
> > 
> > Does top_controls_height_delta part have to do with the way I changed the
> order of the function calls? If so, I wonder if the refactoring can be done
> separately on top of this.
> 
> No, that's unrelated -- the ordering sensitivity is due to the clamp to height
> at the beginning of TopControlsManager::SetControlsTopOffset.

Talked to aelias@ offline. I'll just wait for his patch.