Controls offsets computed if either top or bottom are showing

Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/53c6c1cd141b834f90ac630305bdb7ba1a462fbb
Cr-Commit-Position: refs/heads/master@{#430476}

[mdjones, 2016-10-20 15:50:56]

Description was changed from

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. To do
this, uses of ContentTopOffset in the code were replaced with
ContentOffsetInternal which will use the bottom controls height if
the top is not showing.

BUG=652892
==========

to

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. To do
this, uses of ContentTopOffset in the code were replaced with
ContentOffsetInternal which will use the bottom controls height if
the top is not showing.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[mdjones, 2016-10-21 16:43:47]

mdjones@chromium.org changed reviewers:
+ ianwen@chromium.org, tedchoc@chromium.org

[mdjones, 2016-10-21 16:43:48]

This fix makes bottom controls work alone, but this class probably needs to be
refactored to compute the offsets independently. ptal

[mdjones, 2016-10-27 21:34:52]

Description was changed from

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. To do
this, uses of ContentTopOffset in the code were replaced with
ContentOffsetInternal which will use the bottom controls height if
the top is not showing.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Ian Wen, 2016-10-28 18:32:22]

Please make sure all three cases work:
1. top only
2. bottom only
3. top and bottom are both there. Ratio should depend on top.

As far as I can tell, the code lgtm. :)

[Ted C, 2016-10-31 20:17:39]

https://codereview.chromium.org/2442473002/diff/40001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager_unittest.cc (right):

https://codereview.chromium.org/2442473002/diff/40001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager_unittest.cc:522:
TEST(BrowserControlsManagerTest, ScrollThenRestoreBottomControlsNoTopControls) {
Unless my eyes fail me, this has the same test name as the one above.  does that
work?

[mdjones, 2016-10-31 21:11:41]

https://codereview.chromium.org/2442473002/diff/40001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager_unittest.cc (right):

https://codereview.chromium.org/2442473002/diff/40001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager_unittest.cc:522:
TEST(BrowserControlsManagerTest, ScrollThenRestoreBottomControlsNoTopControls) {
On 2016/10/31 20:17:38, Ted C wrote:
> Unless my eyes fail me, this has the same test name as the one above.  does
that
> work?

Oh boy... bad rebase. Looks like some of this code slipped in with the rename
patch... The suite name is different which is why this still compiled. Removed
duplicate.

[Ted C, 2016-10-31 21:15:07]

lgtm

[mdjones, 2016-11-01 18:21:55]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-01 18:21:56]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2442473002/#ps60001
(title: "fix results of bad rebase")

[commit-bot: I haz the power, 2016-11-01 18:22:19]

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-01 18:28:54]

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

[commit-bot: I haz the power, 2016-11-01 18:28:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mdjones, 2016-11-01 20:01:22]

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

[mdjones, 2016-11-01 20:01:23]

+aelias for owners

[aelias_OOO_until_Jul13, 2016-11-01 21:15:46]

https://codereview.chromium.org/2442473002/diff/60001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.cc (right):

https://codereview.chromium.org/2442473002/diff/60001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.cc:143: float baseline_content_offset =
TopControlsHeight()
I think this should be structured more symmetrically.  How about structuring
this in a way that you check BottomControlsHeight() and potentially act twice? 
Even if we're not planning to support the "both bottom and top" use case, I
think the code would be easier to understand that way (one less arbitrary
assumption baked in).

https://codereview.chromium.org/2442473002/diff/60001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager_unittest.cc (right):

https://codereview.chromium.org/2442473002/diff/60001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager_unittest.cc:504:
TEST(BrowserControlsOffsetManagerTest,
Could you add a new CC test covering the bug you're fixing in this patch?

https://codereview.chromium.org/2442473002/diff/60001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager_unittest.cc:505:
ScrollThenRestoreBottomControlsNoTopControls) {
Another rebase issue.

[mdjones, 2016-11-02 17:15:36]

Rebased to include full patch. Note that top controls drive these calculations
if they are showing.

[aelias_OOO_until_Jul13, 2016-11-04 04:24:50]

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.cc (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.cc:205: TopControlsHeight() ?
(ContentTopOffset() - old_offset) : 0.0f);
ContentTopOffset() method name is now a lie (it doesn't affect content top
offset at all when only bottom controls are present) and you have a bit of
duplicated logic here.

So I suggest introducing a new method called VisibleControlsHeight which has the
exact same implementation as ContentTopOffset() does today.  Then change
ContentTopOffset() implementation to be { TopControlsHeight() ?
VisibleControlsHeight() : 0.f }

[aelias_OOO_until_Jul13, 2016-11-04 04:35:09]

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.h (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.h:100: float
baseline_bottom_content_offset_;
I tried Chrome Home flag locally and observed the bug that bottom position fixed
elements are overlapped by the bottom bar.  Please introduce a new method
ContentBottomOffset() symmetric to ContentTopOffset(), and apply the adjustment
to outer viewport BoundsDelta.

[mdjones, 2016-11-05 00:17:26]

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.cc (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.cc:205: TopControlsHeight() ?
(ContentTopOffset() - old_offset) : 0.0f);
On 2016/11/04 04:24:50, aelias wrote:
> ContentTopOffset() method name is now a lie (it doesn't affect content top
> offset at all when only bottom controls are present) and you have a bit of
> duplicated logic here.
> 
> So I suggest introducing a new method called VisibleControlsHeight which has
the
> exact same implementation as ContentTopOffset() does today.  Then change
> ContentTopOffset() implementation to be { TopControlsHeight() ?
> VisibleControlsHeight() : 0.f }

I'm not sure I understand what you mean. In the case that only the bottom
controls are showing, ContentTopOffset will correctly return 0. The proposed
change would still result in me needing the logic for scroll_delta.

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.h (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.h:100: float
baseline_bottom_content_offset_;
On 2016/11/04 04:35:09, aelias wrote:
> I tried Chrome Home flag locally and observed the bug that bottom position
fixed
> elements are overlapped by the bottom bar.  Please introduce a new method
> ContentBottomOffset() symmetric to ContentTopOffset(), and apply the
adjustment
> to outer viewport BoundsDelta.

The issue with the bottom bar overlapping is addressed in follow up CLs where I
pass the bottom controls information to ChromeFullscreenManager.

ContentBottomOffset is already in this class but I'm not sure how that
information helps more than simply having the bottom control offset would.

[aelias_OOO_until_Jul13, 2016-11-05 00:47:16]

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.cc (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.cc:205: TopControlsHeight() ?
(ContentTopOffset() - old_offset) : 0.0f);
On 2016/11/05 at 00:17:26, mdjones wrote:
> I'm not sure I understand what you mean. In the case that only the bottom
controls are showing, ContentTopOffset will correctly return 0. The proposed
change would still result in me needing the logic for scroll_delta.

OK, right.  Well, the source of my confusion is that assumed you made the change
here in order to fix a bug and drew a logical conclusion without actually
looking again at ContentTopOffset() implementation :).  Maybe you only added the
extra unnecessary complexity for some principle of cleanliness -- if so, I don't
agree with that principle, I prefer code to have as few branches as possible.

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.h (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.h:100: float
baseline_bottom_content_offset_;
On 2016/11/05 at 00:17:26, mdjones wrote:
> On 2016/11/04 04:35:09, aelias wrote:
> > I tried Chrome Home flag locally and observed the bug that bottom position
fixed
> > elements are overlapped by the bottom bar.  Please introduce a new method
> > ContentBottomOffset() symmetric to ContentTopOffset(), and apply the
adjustment
> > to outer viewport BoundsDelta.
> 
> The issue with the bottom bar overlapping is addressed in follow up CLs where
I pass the bottom controls information to ChromeFullscreenManager.

This bug can't be fixed properly with a change to ChromeFullscreenManager,
because the code in the browser process runs asynchronously from the code that
positions and renders the fixed-position (in CC impl thread -- this is the
reason CC is involved in top-controls-hiding to begin with).  The only way to
get a pixel-perfect, jitter-free alignment of fixed-position element and the
URL-bar is via the approach I suggested.

You might be interested in this old slide deck about the URL-bar hiding design:
https://docs.google.com/presentation/d/1vHXijJqm011cfupbcNmQMI1HdA2lZ0ZjjirwR...

[mdjones, 2016-11-07 17:14:45]

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.cc (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.cc:205: TopControlsHeight() ?
(ContentTopOffset() - old_offset) : 0.0f);
On 2016/11/05 00:47:15, aelias wrote:
> On 2016/11/05 at 00:17:26, mdjones wrote:
> > I'm not sure I understand what you mean. In the case that only the bottom
> controls are showing, ContentTopOffset will correctly return 0. The proposed
> change would still result in me needing the logic for scroll_delta.
> 
> OK, right.  Well, the source of my confusion is that assumed you made the
change
> here in order to fix a bug and drew a logical conclusion without actually
> looking again at ContentTopOffset() implementation :).  Maybe you only added
the
> extra unnecessary complexity for some principle of cleanliness -- if so, I
don't
> agree with that principle, I prefer code to have as few branches as possible.

Done, added logic to ContentTopOffset and ContentBottomOffset to return 0 if
their respective heights are 0.

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.h (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.h:100: float
baseline_bottom_content_offset_;
On 2016/11/05 00:47:15, aelias wrote:
> On 2016/11/05 at 00:17:26, mdjones wrote:
> > On 2016/11/04 04:35:09, aelias wrote:
> > > I tried Chrome Home flag locally and observed the bug that bottom position
> fixed
> > > elements are overlapped by the bottom bar.  Please introduce a new method
> > > ContentBottomOffset() symmetric to ContentTopOffset(), and apply the
> adjustment
> > > to outer viewport BoundsDelta.
> > 
> > The issue with the bottom bar overlapping is addressed in follow up CLs
where
> I pass the bottom controls information to ChromeFullscreenManager.
> 
> This bug can't be fixed properly with a change to ChromeFullscreenManager,
> because the code in the browser process runs asynchronously from the code that
> positions and renders the fixed-position (in CC impl thread -- this is the
> reason CC is involved in top-controls-hiding to begin with).  The only way to
> get a pixel-perfect, jitter-free alignment of fixed-position element and the
> URL-bar is via the approach I suggested.
> 
> You might be interested in this old slide deck about the URL-bar hiding
design:
>
https://docs.google.com/presentation/d/1vHXijJqm011cfupbcNmQMI1HdA2lZ0ZjjirwR...

I think what you are talking about may have already been done with the addition
of the bottom controls logic. I have added you as a reviewer on the follow up
patches to put this change into context. We can discuss further if there are
still issues.

[aelias_OOO_until_Jul13, 2016-11-08 00:37:46]

lgtm

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
File cc/input/browser_controls_offset_manager.h (right):

https://codereview.chromium.org/2442473002/diff/80001/cc/input/browser_contro...
cc/input/browser_controls_offset_manager.h:100: float
baseline_bottom_content_offset_;
On 2016/11/07 at 17:14:45, mdjones wrote:
> I think what you are talking about may have already been done with the
addition of the bottom controls logic. I have added you as a reviewer on the
follow up patches to put this change into context. We can discuss further if
there are still issues.

Yeah, OK, I see the appropriate code exists in CC in
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?q=SetBo...
, and I suppose that if it I observed it doesn't work, it would be because the
Java side sends weird sizes down at the moment.

[mdjones, 2016-11-08 01:08:20]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-11-08 01:08:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2442473002/#ps100001
(title: "remove duped logic")

[commit-bot: I haz the power, 2016-11-08 01:09:04]

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-08 02:01:01]

Description was changed from

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-08 02:01:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-08 02:07:15]

Description was changed from

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Controls offsets computed if either top or bottom are showing

This change allows the TopControlsManager to correctly compute the
bottom controls offset regardless of the top controls state. This
is done by separately tracking the bottom controls content offset
and using it when the top controls have no height.

BUG=652892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/53c6c1cd141b834f90ac630305bdb7ba1a462fbb
Cr-Commit-Position: refs/heads/master@{#430476}
==========

[commit-bot: I haz the power, 2016-11-08 02:07:15]

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