Made Blink aware of top controls offset (Chromium-side)

Made Blink aware of top controls offset

Added all the plumbing to make Blink aware of the top controls position
and how its viewport has been resized as a result. This CL adds all the
machinery and plumbing but does not yet connect main-thread scrolls to
the top controls. This is needed to support scrolling the top controls
during slow-scrolls as well as correct viewport behavior in the virtual
viewport pinch-to-zoom.

The machinery for top controls now includes two values:

top_controls_layout_height: The amount that the viewport was shrunk to
accommodate the top controls. This is updated only during a layout that
causes a viewport resize (i.e. RenderWidget::OnResize). The compositor
needs this to know how much the viewport layer has already been resized
by Blink.

top_controls_content_offset: The amount that the top controls are showing.
This will be the same as top_controls_layout_height right after a viewport
resize, but will diverge since the top controls will move without
immediately causing a RenderWidget resize.

Blink now keeps track of the top controls "content offset", which is the
distance the content is offset from the top of the screen due to top
controls. It receives updates to this value from the Impl thread during
a commit and echos them back to the compositor.

On the compositor side, the top controls layout height is moved into the
LayerTreeImpl. This is necessary since a commit will change the viewport
layer's size on the pending tree. Keeping it in LayerTreeHostImpl meant
that, until the pending tree was activated, the top controls layout height
corresponded to the viewport bounds in the pending tree. This was causing
flickering as the viewport container was the incorrect size for a short
time. The compositor also keeps track of the top controls content offset
value. The top controls offset uses the same model as page scale, keeping
an offset (the value as known by the main thread), a delta from the main
thread's value, and a sent_delta to keep track of what has been sent to
the main thread.

See https://codereview.chromium.org/513053003 for Blink-side patch.

BUG=333143
Committed: https://crrev.com/88eae010cdf470913c2575222185c6ab99067df4
Cr-Commit-Position: refs/heads/master@{#294003}

[bokan, 2014-08-28 18:42:45]

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

[bokan, 2014-08-28 18:42:45]

Hi Alexandre,

PTAL at this and the associated Blink patch. I've also renamed the
top_controls_layout_height members in other places but I've left them out of the
review for easier reading. I can merge those in when this passes muster or save
for a seperate CL. Thanks.

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:125: virtual void
setTopControlsContentOffset(int);
It feels strange that this class ends up with two of the same named methods
(with case diff) but they come from separate interfaces where it makes sense in
those contexts. WDYT?

[bokan, 2014-08-28 21:34:28]

Forgot to mention, still working on tests so ignore the lack thereof.

[aelias_OOO_until_Jul13, 2014-08-29 04:37:02]

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:35: virtual void
SetTopControlsContentOffset(int offset) = 0;
I think if you stop baking this value into FrameView size (mentioned below how
we can avoid it) we can use floats everywhere instead of ints.

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:143: int top_controls_content_offset;
scrolls and page scale updates in this struct are deltas, can this one be as
well for consistency?

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2197: active_tree_->set_top_controls_delta(
We end up holding a field with redundant information (can be fully inferred from
other existing fields), which I prefer to avoid.  How about making
top_controls_manager hold less state and instead call getters/setters that
manipulate the active tree?

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:125: virtual void
setTopControlsContentOffset(int);
On 2014/08/28 at 18:42:45, bokan wrote:
> It feels strange that this class ends up with two of the same named methods
(with case diff) but they come from separate interfaces where it makes sense in
those contexts. WDYT?

Could you instead add an additional parameter to applyScrollAndScale and rename
that to something else (applyImplThreadDeltas?).  Aside from this issue, I find
bundling related state into the same method can help avoid clamping-related bugs
(avoids problems like you first make the viewport smaller, scroll clamped, then
you adjust the scroll...)

https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:668:
compositor_->SetTopControlsContentOffset(top_controls_layout_height);
This isn't quite what I had in mind and it's impossible to build main-thread top
controls manipulation on top of this data model.  Blink should be pushing this
value to the compositor, not RenderWidget.  I think switching to that model will
also unblock pushing a delta instead of an absolute value in ScrollAndScaleSet.

My proposed change will raise the question of how does Blink know what direction
to resize the outer viewport in.  That's where the browser's
top_controls_layout_height remains relevant: it can be passed into Blink instead
of into CC.  I propose maintaining the current name "top_controls_layout_height"
for this particular purpose.  So in summary I propose:

- Blink receives top_controls_layout_height value from RenderWidget
- Blink receives top_controls_content_offset_delta from CC
- CC receives top_controls_content_offset from Blink
(also needed for main-thread-manipulation in future: Blink receives top control
size from RenderWidget so it can clamp properly)

In addition I think this means the sketchy FrameView resize you're currently
doing in the Blink-side patch becomes unnecessary since CC receives sufficient
outer viewport size delta information via top_controls_content_offset instead. 
(That is, Blink will *know* about top controls position increments but it won't
*do* anything with them aside from informing CC.)

[bokan, 2014-08-29 18:02:14]

Sorry, I think I may have deceived you by making a confusing call. See inline
comments.

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:35: virtual void
SetTopControlsContentOffset(int offset) = 0;
On 2014/08/29 04:37:01, aelias wrote:
> I think if you stop baking this value into FrameView size (mentioned below how
> we can avoid it) we can use floats everywhere instead of ints.

Assuming this can be done impl-side then yes. See my concerns about that in
comment in render_widget.

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:143: int top_controls_content_offset;
On 2014/08/29 04:37:01, aelias wrote:
> scrolls and page scale updates in this struct are deltas, can this one be as
> well for consistency?

Yup, though I think the value that gets sent to Blink will should still be
offset+delta (for the reason mentioned in my comment in render_widget)

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2197: active_tree_->set_top_controls_delta(
On 2014/08/29 04:37:01, aelias wrote:
> We end up holding a field with redundant information (can be fully inferred
from
> other existing fields), which I prefer to avoid.  How about making
> top_controls_manager hold less state and instead call getters/setters that
> manipulate the active tree?

sgtm

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:125: virtual void
setTopControlsContentOffset(int);
On 2014/08/29 04:37:01, aelias wrote:
> On 2014/08/28 at 18:42:45, bokan wrote:
> > It feels strange that this class ends up with two of the same named methods
> (with case diff) but they come from separate interfaces where it makes sense
in
> those contexts. WDYT?
> 
> Could you instead add an additional parameter to applyScrollAndScale and
rename
> that to something else (applyImplThreadDeltas?).  Aside from this issue, I
find
> bundling related state into the same method can help avoid clamping-related
bugs
> (avoids problems like you first make the viewport smaller, scroll clamped,
then
> you adjust the scroll...)

I had a FIXME to do just that. My concern was that the inner/outer viewport
scroll offsets are set before applyScrollAndScale and so I was worried about
clamping and I was a bit hesitant to change around the order these things happen
in for this CL. I do feel like I understand these paths better now so I'll take
another look here.

https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:668:
compositor_->SetTopControlsContentOffset(top_controls_layout_height);
On 2014/08/29 04:37:01, aelias wrote:
> This isn't quite what I had in mind and it's impossible to build main-thread
top
> controls manipulation on top of this data model.  Blink should be pushing this
> value to the compositor, not RenderWidget.  I think switching to that model
will
> also unblock pushing a delta instead of an absolute value in
ScrollAndScaleSet.

Ack...I think I introduced some confusion here by accidentally abusing the
compositor interface. This method is part of RenderWidgetCompositor's
LayerTreeHostClient interface so this is setting the offset in *Blink*. This
should actually be webwidget_->setTopControlsContentOffset(), which
(confusingly) gets called through compositor_->SetTopControlsContentOffset(). I
believe this is currently doing what you describe below but masquerading as
updating the compositor. That's a mistake on my part, I'll fix this to be clear.

> - Blink receives top_controls_content_offset_delta from CC

Maybe I'm missing something, but I think this still has the problem I described
in my earlier e-mail. There's a race between the Browser and the Compositor as
to who updates Blink first. The compositor sends its total TC offset (including
delta) to the Browser but the delta it would send to Blink is the delta from the
last committed offset. If the Browser decides to resize Blink before the
compositor commits then the delta would be applied twice since it's already
included in the Browser's offset.

> (also needed for main-thread-manipulation in future: Blink receives top
control
> size from RenderWidget so it can clamp properly)

Yup, will add.

> In addition I think this means the sketchy FrameView resize you're currently
> doing in the Blink-side patch becomes unnecessary since CC receives sufficient
> outer viewport size delta information via top_controls_content_offset instead.

> (That is, Blink will *know* about top controls position increments but it
won't
> *do* anything with them aside from informing CC.)

Could you elaborate on how this would work? The problem I see is that the
compositor can't apply the top_controls_content_offset against the viewport
container since the viewport bounds may or may not already account for it. In
the case of a RenderWidget::Resize, the viewport container's size is set so that
no adjustment is needed. When a compositor commit happens, only the TC offset is
changed so the viewport's bounds need an adjustment. The way I currently get
around this is making the viewport adjustment in Blink (it could also be done in
LayerTreeHost) and during RenderWidget::Resize the TopControls get resized prior
to the widget resize so it will clobber the viewport adjustment.

[aelias_OOO_until_Jul13, 2014-08-29 20:37:15]

On 2014/08/29 18:02:14, bokan wrote:
> Sorry, I think I may have deceived you by making a confusing call. See inline
> comments.
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_client.h (right):
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_client.h:35: virtual void
> SetTopControlsContentOffset(int offset) = 0;
> On 2014/08/29 04:37:01, aelias wrote:
> > I think if you stop baking this value into FrameView size (mentioned below
how
> > we can avoid it) we can use floats everywhere instead of ints.
> 
> Assuming this can be done impl-side then yes. See my concerns about that in
> comment in render_widget.
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common.h (right):
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common.h:143: int top_controls_content_offset;
> On 2014/08/29 04:37:01, aelias wrote:
> > scrolls and page scale updates in this struct are deltas, can this one be as
> > well for consistency?
> 
> Yup, though I think the value that gets sent to Blink will should still be
> offset+delta (for the reason mentioned in my comment in render_widget)
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:2197: active_tree_->set_top_controls_delta(
> On 2014/08/29 04:37:01, aelias wrote:
> > We end up holding a field with redundant information (can be fully inferred
> from
> > other existing fields), which I prefer to avoid.  How about making
> > top_controls_manager hold less state and instead call getters/setters that
> > manipulate the active tree?
> 
> sgtm
> 
>
https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
> File content/renderer/gpu/render_widget_compositor.h (right):
> 
>
https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
> content/renderer/gpu/render_widget_compositor.h:125: virtual void
> setTopControlsContentOffset(int);
> On 2014/08/29 04:37:01, aelias wrote:
> > On 2014/08/28 at 18:42:45, bokan wrote:
> > > It feels strange that this class ends up with two of the same named
methods
> > (with case diff) but they come from separate interfaces where it makes sense
> in
> > those contexts. WDYT?
> > 
> > Could you instead add an additional parameter to applyScrollAndScale and
> rename
> > that to something else (applyImplThreadDeltas?).  Aside from this issue, I
> find
> > bundling related state into the same method can help avoid clamping-related
> bugs
> > (avoids problems like you first make the viewport smaller, scroll clamped,
> then
> > you adjust the scroll...)
> 
> I had a FIXME to do just that. My concern was that the inner/outer viewport
> scroll offsets are set before applyScrollAndScale and so I was worried about
> clamping and I was a bit hesitant to change around the order these things
happen
> in for this CL. I do feel like I understand these paths better now so I'll
take
> another look here.
> 
>
https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/511253003/diff/40001/content/renderer/render_...
> content/renderer/render_widget.cc:668:
> compositor_->SetTopControlsContentOffset(top_controls_layout_height);
> On 2014/08/29 04:37:01, aelias wrote:
> > This isn't quite what I had in mind and it's impossible to build main-thread
> top
> > controls manipulation on top of this data model.  Blink should be pushing
this
> > value to the compositor, not RenderWidget.  I think switching to that model
> will
> > also unblock pushing a delta instead of an absolute value in
> ScrollAndScaleSet.
> 
> Ack...I think I introduced some confusion here by accidentally abusing the
> compositor interface. This method is part of RenderWidgetCompositor's
> LayerTreeHostClient interface so this is setting the offset in *Blink*. This
> should actually be webwidget_->setTopControlsContentOffset(), which
> (confusingly) gets called through compositor_->SetTopControlsContentOffset().
I
> believe this is currently doing what you describe below but masquerading as
> updating the compositor. That's a mistake on my part, I'll fix this to be
clear.
> 
> > - Blink receives top_controls_content_offset_delta from CC
> 
> Maybe I'm missing something, but I think this still has the problem I
described
> in my earlier e-mail. There's a race between the Browser and the Compositor as
> to who updates Blink first. The compositor sends its total TC offset
(including
> delta) to the Browser but the delta it would send to Blink is the delta from
the
> last committed offset. If the Browser decides to resize Blink before the
> compositor commits then the delta would be applied twice since it's already
> included in the Browser's offset.
> 
> > (also needed for main-thread-manipulation in future: Blink receives top
> control
> > size from RenderWidget so it can clamp properly)
> 
> Yup, will add.
> 
> > In addition I think this means the sketchy FrameView resize you're currently
> > doing in the Blink-side patch becomes unnecessary since CC receives
sufficient
> > outer viewport size delta information via top_controls_content_offset
instead.
> 
> > (That is, Blink will *know* about top controls position increments but it
> won't
> > *do* anything with them aside from informing CC.)
> 
> Could you elaborate on how this would work? The problem I see is that the
> compositor can't apply the top_controls_content_offset against the viewport
> container since the viewport bounds may or may not already account for it. In
> the case of a RenderWidget::Resize, the viewport container's size is set so
that
> no adjustment is needed. When a compositor commit happens, only the TC offset
is
> changed so the viewport's bounds need an adjustment. The way I currently get
> around this is making the viewport adjustment in Blink (it could also be done
in
> LayerTreeHost) and during RenderWidget::Resize the TopControls get resized
prior
> to the widget resize so it will clobber the viewport adjustment.

I was suggesting that Blink shouldn't do any adjustment to outer viewport size
in reaction to top controls information.  It would rely on CC to make that
adjustment even when Blink would be capable of doing so.

Alternatively, I guess Blink could make the adjustment and push an adjusted
top_controls_layout_height (in addition to all the current value) to inform CC
of what happened.

[aelias_OOO_until_Jul13, 2014-08-29 21:00:37]

Basically, the point of top_controls_layout_height is precisely to
eliminate the confusion about by how much a given viewport size has already
been adjusted or not.  So I think both your issues could be solved by
plumbing top_controls_layout_height to more places (for your first problem,
Blink could make smarter decisions and avoid the weird resize clobber if it
heard about it for RenderWidget, and for the second, CC could also hear a
version of it reflecting the adjustment already made to outer viewport).
On Aug 29, 2014 11:02 AM, <bokan@chromium.org> wrote:

> Reviewers: aelias,
>
> Message:
> Sorry, I think I may have deceived you by making a confusing call. See
> inline
> comments.
>
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_client.h
> File cc/trees/layer_tree_host_client.h (right):
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_client.h#newcode35
> cc/trees/layer_tree_host_client.h:35: virtual void
> SetTopControlsContentOffset(int offset) = 0;
> On 2014/08/29 04:37:01, aelias wrote:
>
>> I think if you stop baking this value into FrameView size (mentioned
>>
> below how
>
>> we can avoid it) we can use floats everywhere instead of ints.
>>
>
> Assuming this can be done impl-side then yes. See my concerns about that
> in comment in render_widget.
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_common.h
> File cc/trees/layer_tree_host_common.h (right):
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_common.h#newcode143
> cc/trees/layer_tree_host_common.h:143: int top_controls_content_offset;
> On 2014/08/29 04:37:01, aelias wrote:
>
>> scrolls and page scale updates in this struct are deltas, can this one
>>
> be as
>
>> well for consistency?
>>
>
> Yup, though I think the value that gets sent to Blink will should still
> be offset+delta (for the reason mentioned in my comment in
> render_widget)
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_impl.cc
> File cc/trees/layer_tree_host_impl.cc (right):
>
> https://codereview.chromium.org/511253003/diff/40001/cc/
> trees/layer_tree_host_impl.cc#newcode2197
> cc/trees/layer_tree_host_impl.cc:2197:
> active_tree_->set_top_controls_delta(
> On 2014/08/29 04:37:01, aelias wrote:
>
>> We end up holding a field with redundant information (can be fully
>>
> inferred from
>
>> other existing fields), which I prefer to avoid.  How about making
>> top_controls_manager hold less state and instead call getters/setters
>>
> that
>
>> manipulate the active tree?
>>
>
> sgtm
>
> https://codereview.chromium.org/511253003/diff/40001/
> content/renderer/gpu/render_widget_compositor.h
> File content/renderer/gpu/render_widget_compositor.h (right):
>
> https://codereview.chromium.org/511253003/diff/40001/
> content/renderer/gpu/render_widget_compositor.h#newcode125
> content/renderer/gpu/render_widget_compositor.h:125: virtual void
> setTopControlsContentOffset(int);
> On 2014/08/29 04:37:01, aelias wrote:
>
>> On 2014/08/28 at 18:42:45, bokan wrote:
>> > It feels strange that this class ends up with two of the same named
>>
> methods
>
>> (with case diff) but they come from separate interfaces where it makes
>>
> sense in
>
>> those contexts. WDYT?
>>
>
>  Could you instead add an additional parameter to applyScrollAndScale
>>
> and rename
>
>> that to something else (applyImplThreadDeltas?).  Aside from this
>>
> issue, I find
>
>> bundling related state into the same method can help avoid
>>
> clamping-related bugs
>
>> (avoids problems like you first make the viewport smaller, scroll
>>
> clamped, then
>
>> you adjust the scroll...)
>>
>
> I had a FIXME to do just that. My concern was that the inner/outer
> viewport scroll offsets are set before applyScrollAndScale and so I was
> worried about clamping and I was a bit hesitant to change around the
> order these things happen in for this CL. I do feel like I understand
> these paths better now so I'll take another look here.
>
> https://codereview.chromium.org/511253003/diff/40001/
> content/renderer/render_widget.cc
> File content/renderer/render_widget.cc (right):
>
> https://codereview.chromium.org/511253003/diff/40001/
> content/renderer/render_widget.cc#newcode668
> content/renderer/render_widget.cc:668:
> compositor_->SetTopControlsContentOffset(top_controls_layout_height);
> On 2014/08/29 04:37:01, aelias wrote:
>
>> This isn't quite what I had in mind and it's impossible to build
>>
> main-thread top
>
>> controls manipulation on top of this data model.  Blink should be
>>
> pushing this
>
>> value to the compositor, not RenderWidget.  I think switching to that
>>
> model will
>
>> also unblock pushing a delta instead of an absolute value in
>>
> ScrollAndScaleSet.
>
> Ack...I think I introduced some confusion here by accidentally abusing
> the compositor interface. This method is part of
> RenderWidgetCompositor's LayerTreeHostClient interface so this is
> setting the offset in *Blink*. This should actually be
> webwidget_->setTopControlsContentOffset(), which (confusingly) gets
> called through compositor_->SetTopControlsContentOffset(). I believe
> this is currently doing what you describe below but masquerading as
> updating the compositor. That's a mistake on my part, I'll fix this to
> be clear.
>
>  - Blink receives top_controls_content_offset_delta from CC
>>
>
> Maybe I'm missing something, but I think this still has the problem I
> described in my earlier e-mail. There's a race between the Browser and
> the Compositor as to who updates Blink first. The compositor sends its
> total TC offset (including delta) to the Browser but the delta it would
> send to Blink is the delta from the last committed offset. If the
> Browser decides to resize Blink before the compositor commits then the
> delta would be applied twice since it's already included in the
> Browser's offset.
>
>  (also needed for main-thread-manipulation in future: Blink receives
>>
> top control
>
>> size from RenderWidget so it can clamp properly)
>>
>
> Yup, will add.
>
>  In addition I think this means the sketchy FrameView resize you're
>>
> currently
>
>> doing in the Blink-side patch becomes unnecessary since CC receives
>>
> sufficient
>
>> outer viewport size delta information via top_controls_content_offset
>>
> instead.
>
>> (That is, Blink will *know* about top controls position increments but
>>
> it won't
>
>> *do* anything with them aside from informing CC.)
>>
>
> Could you elaborate on how this would work? The problem I see is that
> the compositor can't apply the top_controls_content_offset against the
> viewport container since the viewport bounds may or may not already
> account for it. In the case of a RenderWidget::Resize, the viewport
> container's size is set so that no adjustment is needed. When a
> compositor commit happens, only the TC offset is changed so the
> viewport's bounds need an adjustment. The way I currently get around
> this is making the viewport adjustment in Blink (it could also be done
> in LayerTreeHost) and during RenderWidget::Resize the TopControls get
> resized prior to the widget resize so it will clobber the viewport
> adjustment.
>
> Description:
> Made Blink aware of top controls offset
>
> Added all the plumbing to make Blink aware of the top controls position
> and how its viewport has been resized as a result. This CL adds all the
> machinery and plumbing but does not yet connect main-thread scrolls to
> the top controls. This is needed to support scrolling the top controls
> during slow-scrolls as well as correct viewport behavior in the virtual
> viewport pinch-to-zoom.
>
> Blink now keeps track of the top controls "content offset", which is the
> distance the content is offset from the top of the screen. It receives
> updates to this value from (a) the Impl thread during a commit and from
> (b) the Browser process during a resize. In (a), Blink resizes the
> FrameView to match the viewport layer in the CC, however, this does not
> cause a layout. A layout only occurs during (b) and will clobber the
> FrameView resize caused by the top controls.
>
> On the compositor side, the top controls offset is moved into the
> LayerTreeImpl. This is necessary since a commit will change the viewport
> layer's size on the pending tree. Keeping the offset in LayerTreeHostImpl
> meant that, until the pending tree was activated, the top controls offset
> corresponded to the viewport bounds in the pending tree. This was causing
> flickering as the viewport container was the incorrect size for a short
> time. The top controls offset uses the same model as page scale, keeping
> an offset (the value as known by the main thread), a delta from the main
> thread's value, and a sent_delta to keep track of what has been sent to
> the main thread.
>
> Finally, the recently added top_controls_layout_height gets renamed to
> top_controls_content_offset since it no longer corresponds to a layout
> in Blink.
>
> See https://codereview.chromium.org/513053003 for Blink-side patch.
>
> BUG=333143
>
> Please review this at https://codereview.chromium.org/511253003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+97, -41 lines):
>   M cc/input/top_controls_manager.h
>   M cc/trees/layer_tree_host.h
>   M cc/trees/layer_tree_host.cc
>   M cc/trees/layer_tree_host_client.h
>   M cc/trees/layer_tree_host_common.h
>   M cc/trees/layer_tree_host_impl.h
>   M cc/trees/layer_tree_host_impl.cc
>   M cc/trees/layer_tree_impl.h
>   M cc/trees/layer_tree_impl.cc
>   M content/browser/renderer_host/compositor_impl_android.h
>   M content/renderer/gpu/render_widget_compositor.h
>   M content/renderer/gpu/render_widget_compositor.cc
>   M content/renderer/render_widget.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bokan, 2014-09-02 18:54:15]

On 2014/08/29 21:00:37, aelias wrote:
> Basically, the point of top_controls_layout_height is precisely to
> eliminate the confusion about by how much a given viewport size has already
> been adjusted or not.

So what is the relationship between top_controls_layout_height and the
viewport bounds? My interpretation of top_controls_layout_height
beforehand was that it was the amount that the top controls were showing at
the time of the last layout in Blink (i.e. how much the FrameView was shrunk
because of top controls). This isn't the case anymore though because we update
the value in Blink during a commit but we don't do a layout. In my current
patch I make the assumption that
top_controls_layout_height + viewport_container_layer::bounds ==
device_viewport_size
is an invariant but this requires the "sketchy FrameView resize" to hold. 

> So I think both your issues could be solved by
> plumbing top_controls_layout_height to more places (for your first problem,
> Blink could make smarter decisions and avoid the weird resize clobber if it
> heard about it for RenderWidget, and for the second, CC could also hear a
> version of it reflecting the adjustment already made to outer viewport).

Here's a diagram of what the plumbing looks like in my current patch:
http://bokan.ca/tc.jpg
top_controls_layout_height is plumbed through RenderWidget and Blink already
but I'm confused about how this should help. Assuming I got rid of the FrameView
resize on WebViewImpl::setTopControlsLayoutHeight, there would be no clobber;
however, my invariant above would now break whenever CC committed so I think
my understanding of what top_controls_layout_height means is wrong. Could you
clear that up? I'm not sure what you mean by "Blink making smarter decisions"
or "version reflecting the adjustment already made", should Blink give CC a 
modified value of top_controls_layout_height it got from RenderWidget?

Thanks.

[bokan, 2014-09-02 18:57:16]

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:143: int top_controls_content_offset;
On 2014/08/29 04:37:01, aelias wrote:
> scrolls and page scale updates in this struct are deltas, can this one be as
> well for consistency?

Just realized doing this has the same problem with RenderWidget::Resize causing
a double application of the sent delta, I'll wait until we clear that up.

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/511253003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2197: active_tree_->set_top_controls_delta(
On 2014/08/29 18:02:14, bokan wrote:
> On 2014/08/29 04:37:01, aelias wrote:
> > We end up holding a field with redundant information (can be fully inferred
> from
> > other existing fields), which I prefer to avoid.  How about making
> > top_controls_manager hold less state and instead call getters/setters that
> > manipulate the active tree?
> 
> sgtm

Done.

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/511253003/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.h:125: virtual void
setTopControlsContentOffset(int);
On 2014/08/29 04:37:01, aelias wrote:
> On 2014/08/28 at 18:42:45, bokan wrote:
> > It feels strange that this class ends up with two of the same named methods
> (with case diff) but they come from separate interfaces where it makes sense
in
> those contexts. WDYT?
> 
> Could you instead add an additional parameter to applyScrollAndScale and
rename
> that to something else (applyImplThreadDeltas?).  Aside from this issue, I
find
> bundling related state into the same method can help avoid clamping-related
bugs
> (avoids problems like you first make the viewport smaller, scroll clamped,
then
> you adjust the scroll...)

Done.

[aelias_OOO_until_Jul13, 2014-09-02 23:13:47]

On 2014/09/02 18:54:15, bokan wrote:
> On 2014/08/29 21:00:37, aelias wrote:
> > Basically, the point of top_controls_layout_height is precisely to
> > eliminate the confusion about by how much a given viewport size has already
> > been adjusted or not.
> 
> So what is the relationship between top_controls_layout_height and the
> viewport bounds? My interpretation of top_controls_layout_height
> beforehand was that it was the amount that the top controls were showing at
> the time of the last layout in Blink (i.e. how much the FrameView was shrunk
> because of top controls). This isn't the case anymore though because we update
> the value in Blink during a commit but we don't do a layout. In my current
> patch I make the assumption that
> top_controls_layout_height + viewport_container_layer::bounds ==
> device_viewport_size
> is an invariant but this requires the "sketchy FrameView resize" to hold. 

I think you can preserve your meaning by making sure that "CC committing should
have no effect on either top_controls_layout_height" in any circumstance, more
details below.

> 
> > So I think both your issues could be solved by
> > plumbing top_controls_layout_height to more places (for your first problem,
> > Blink could make smarter decisions and avoid the weird resize clobber if it
> > heard about it for RenderWidget, and for the second, CC could also hear a
> > version of it reflecting the adjustment already made to outer viewport).
> 
> Here's a diagram of what the plumbing looks like in my current patch:
> http://bokan.ca/tc.jpg

The most important thing I would change that diagram is to add another data
field in Blink called "top_controls_content_offset", in addition to
"top_controls_layout_height".  Currently you're setting Blink
top_controls_layout_height (and FrameView size) in two conflicting ways, from
the browser and from CC.  I think Blink needs to keep track of both what the
browser told it and what CC told it in different fields, since all of the
information remains relevant.  Blink would store "top_controls_layout_height"
and "top_controls_content_offset" and the FrameView would always remain as what
the browser told it in WebView::resize().

> top_controls_layout_height is plumbed through RenderWidget and Blink already
> but I'm confused about how this should help. Assuming I got rid of the
FrameView
> resize on WebViewImpl::setTopControlsLayoutHeight, there would be no clobber;
> however, my invariant above would now break whenever CC committed so I think
> my understanding of what top_controls_layout_height means is wrong.

I think the easiest way to solve this is that CC committing should have no
effect on either top_controls_layout_height, FrameView size or outer viewport
size.  It should just passively write a value to top_controls_content_offset
that would only be used to be echoed to CC later.  That's the minimum possible
behavior change and is enough to solve both your original problem and allow top
controls movement during main-thread scrolling.

(The second reasonable way in my view is for CC committing to have no effect on
top_controls_layout_height or FrameView size, but to affect outer viewport size.
 In this case CC would need to be provided with an adjusted value of
top_controls_layout_size representing the current effect on outer viewport size.
 This is a bit more confusing in terms of top controls-related values but might
make some things more elegant in CC -- I leave it up to you whether you prefer
this route.)

> Could you
> clear that up? I'm not sure what you mean by "Blink making smarter decisions"
> or "version reflecting the adjustment already made", should Blink give CC a 
> modified value of top_controls_layout_height it got from RenderWidget?

Yes if you take the "second reasonable way" I described above.  With the
"easiest way" you can just give raw top_controls_layout_height to CC.

> 
> Thanks.

No problem.  Seems discussing in writing has been not been very efficient for
this topic so let me know if you want to schedule a VC if you think the
questions are still not settled.  Note that I feel strongly about two things in
particular, 1) main-thread top controls scrolling should be easily possible
without further changes across the Blink API, 2) values in Blink such as the
FrameView's and the top controls shouldn't be "clobbered" according to two
different semantics.  I mentioned two ways to solve those problems that I think
are reasonable, let me know if you have other proposals though.

[bokan, 2014-09-03 18:41:53]

On 2014/09/02 23:13:47, aelias wrote:
> On 2014/09/02 18:54:15, bokan wrote:
> > On 2014/08/29 21:00:37, aelias wrote:
> > > Basically, the point of top_controls_layout_height is precisely to
> > > eliminate the confusion about by how much a given viewport size has
already
> > > been adjusted or not.
> > 
> > So what is the relationship between top_controls_layout_height and the
> > viewport bounds? My interpretation of top_controls_layout_height
> > beforehand was that it was the amount that the top controls were showing at
> > the time of the last layout in Blink (i.e. how much the FrameView was shrunk
> > because of top controls). This isn't the case anymore though because we
update
> > the value in Blink during a commit but we don't do a layout. In my current
> > patch I make the assumption that
> > top_controls_layout_height + viewport_container_layer::bounds ==
> > device_viewport_size
> > is an invariant but this requires the "sketchy FrameView resize" to hold. 
> 
> I think you can preserve your meaning by making sure that "CC committing
should
> have no effect on either top_controls_layout_height" in any circumstance, more
> details below.
> 
> > 
> > > So I think both your issues could be solved by
> > > plumbing top_controls_layout_height to more places (for your first
problem,
> > > Blink could make smarter decisions and avoid the weird resize clobber if
it
> > > heard about it for RenderWidget, and for the second, CC could also hear a
> > > version of it reflecting the adjustment already made to outer viewport).
> > 
> > Here's a diagram of what the plumbing looks like in my current patch:
> > http://bokan.ca/tc.jpg
> 
> The most important thing I would change that diagram is to add another data
> field in Blink called "top_controls_content_offset", in addition to
> "top_controls_layout_height".  Currently you're setting Blink
> top_controls_layout_height (and FrameView size) in two conflicting ways, from
> the browser and from CC.  I think Blink needs to keep track of both what the
> browser told it and what CC told it in different fields, since all of the
> information remains relevant.  Blink would store "top_controls_layout_height"
> and "top_controls_content_offset" and the FrameView would always remain as
what
> the browser told it in WebView::resize().
> 
> > top_controls_layout_height is plumbed through RenderWidget and Blink already
> > but I'm confused about how this should help. Assuming I got rid of the
> FrameView
> > resize on WebViewImpl::setTopControlsLayoutHeight, there would be no
clobber;
> > however, my invariant above would now break whenever CC committed so I think
> > my understanding of what top_controls_layout_height means is wrong.
> 
> I think the easiest way to solve this is that CC committing should have no
> effect on either top_controls_layout_height, FrameView size or outer viewport
> size.  It should just passively write a value to top_controls_content_offset
> that would only be used to be echoed to CC later.  That's the minimum possible
> behavior change and is enough to solve both your original problem and allow
top
> controls movement during main-thread scrolling.
> 
> (The second reasonable way in my view is for CC committing to have no effect
on
> top_controls_layout_height or FrameView size, but to affect outer viewport
size.
>  In this case CC would need to be provided with an adjusted value of
> top_controls_layout_size representing the current effect on outer viewport
size.
>  This is a bit more confusing in terms of top controls-related values but
might
> make some things more elegant in CC -- I leave it up to you whether you prefer
> this route.)
> 
> > Could you
> > clear that up? I'm not sure what you mean by "Blink making smarter
decisions"
> > or "version reflecting the adjustment already made", should Blink give CC a 
> > modified value of top_controls_layout_height it got from RenderWidget?
> 
> Yes if you take the "second reasonable way" I described above.  With the
> "easiest way" you can just give raw top_controls_layout_height to CC.
> 
> > 
> > Thanks.
> 
> No problem.  Seems discussing in writing has been not been very efficient for
> this topic so let me know if you want to schedule a VC if you think the
> questions are still not settled.  Note that I feel strongly about two things
in
> particular, 1) main-thread top controls scrolling should be easily possible
> without further changes across the Blink API, 2) values in Blink such as the
> FrameView's and the top controls shouldn't be "clobbered" according to two
> different semantics.  I mentioned two ways to solve those problems that I
think
> are reasonable, let me know if you have other proposals though.

Ok, I think I understand now. See bokan.ca/tc2.jpg for the updated view, is this
along
the lines you were thinking? The semantics are now:

top_controls_layout_height: The height that the viewport layer was shrunk by to
accommodate
the top controls. This is communicated directly to CC during
RenderWidget::OnResize and
CC doesn't change the value, it only gets updated. Blink itself doesn't see it.

top_controls_content_offset: The live top controls offset. This is a loop that
runs
between CC and Blink. The Impl thread sends the delta to Blink during a commit
and
it gets echoed back. The Impl thread also updates the Browser host using this
value.
In the future, slow-path scrolls could modify this value to scroll the top
controls.

This still has the problem of clamping. When the impl thread commits a positive
content_offset delta (i.e. the top controls came down), the max scroll offset
for the
viewport increased, but since we don't resize the FrameView in Blink on commit,
when the scroll
offsets are set they'll be clamped. The only situation I can see this in is if
the page
is fully scrolled and the top controls are animated in and out (by crossing the
top_controls_show_height threshold). I can repro this issue in existing versions
of Chrome
so it's not specific to this CL. Its not really noticeable now but I expect it
will be more so
with the virtual viewport turned on so I think we'll need some sort of resize or
scroll fixup
shortly, but I'd be inclined to do that in a separate patch.

[aelias_OOO_until_Jul13, 2014-09-03 19:38:14]

Looks good modulo nits, just needs tests.

https://codereview.chromium.org/511253003/diff/80001/cc/input/top_controls_ma...
File cc/input/top_controls_manager_client.h (right):

https://codereview.chromium.org/511253003/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager_client.h:14: virtual void
SetControlsTopOffset(float offsetj) = 0;
Typo: "offsetj"

https://codereview.chromium.org/511253003/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/511253003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:30: virtual void ApplyViewportProperties(const
gfx::Vector2d& scroll_delta,
ApplyViewportDeltas

[aelias_OOO_until_Jul13, 2014-09-03 19:38:48]

On 2014/09/03 at 18:41:53, bokan wrote:
> This still has the problem of clamping. When the impl thread commits a
positive
> content_offset delta (i.e. the top controls came down), the max scroll offset
for the
> viewport increased, but since we don't resize the FrameView in Blink on
commit, when the scroll
> offsets are set they'll be clamped. The only situation I can see this in is if
the page
> is fully scrolled and the top controls are animated in and out (by crossing
the
> top_controls_show_height threshold). I can repro this issue in existing
versions of Chrome
> so it's not specific to this CL. Its not really noticeable now but I expect it
will be more so
> with the virtual viewport turned on so I think we'll need some sort of resize
or scroll fixup
> shortly, but I'd be inclined to do that in a separate patch.

Sounds fine to address this in a future patch.

[bokan, 2014-09-04 23:09:52]

PTAL, everything's cleaned up and tests added.

https://codereview.chromium.org/511253003/diff/80001/cc/input/top_controls_ma...
File cc/input/top_controls_manager_client.h (right):

https://codereview.chromium.org/511253003/diff/80001/cc/input/top_controls_ma...
cc/input/top_controls_manager_client.h:14: virtual void
SetControlsTopOffset(float offsetj) = 0;
On 2014/09/03 19:38:14, aelias wrote:
> Typo: "offsetj"

Done.

https://codereview.chromium.org/511253003/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/511253003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_client.h:30: virtual void ApplyViewportProperties(const
gfx::Vector2d& scroll_delta,
On 2014/09/03 19:38:14, aelias wrote:
> ApplyViewportDeltas

Done.

[aelias_OOO_until_Jul13, 2014-09-05 00:32:57]

cc/ lgtm

[bokan, 2014-09-05 01:09:00]

bokan@chromium.org changed reviewers:
+ mkosiba@chromium.org, piman@chromium.org

[bokan, 2014-09-05 01:09:01]

+piman@ for content/
+mkosiba@ for android_webview/

[bokan, 2014-09-05 01:09:44]

On 2014/09/05 01:09:01, bokan wrote:
> +piman@ for content/
...and ui/compositor/

[piman, 2014-09-05 01:14:49]

lgtm

[mkosiba (inactive), 2014-09-05 09:58:42]

android_webview/ LGTM

[bokan, 2014-09-09 02:40:05]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-09-09 02:55:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bokan@chromium.org/511253003/120001

[commit-bot: I haz the power, 2014-09-09 08:21:46]

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

[commit-bot: I haz the power, 2014-09-09 08:21:48]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2014-09-09 12:41:29]

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

[bokan, 2014-09-09 12:41:30]

+jamesr@ for build fix in mojo example

[jamesr, 2014-09-09 18:30:04]

mojo lgtm

[bokan, 2014-09-09 18:30:27]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-09-09 18:39:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bokan@chromium.org/511253003/140001

[commit-bot: I haz the power, 2014-09-09 20:45:39]

Committed patchset #8 (id:140001) as 72d0df85d72d51a904415537ce16e269006be193

[commit-bot: I haz the power, 2014-09-10 03:55:25]

Patchset 8 (id:??) landed as
https://crrev.com/88eae010cdf470913c2575222185c6ab99067df4
Cr-Commit-Position: refs/heads/master@{#294003}