Introduce bottom controls to CC and let it respond to scrolling

Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

Committed: https://crrev.com/e5fc57843cf8d21d9bf51e389821657f3f768317
Cr-Commit-Position: refs/heads/master@{#412738}

[Ian Wen, 2016-06-29 18:07:25]

Description was changed from

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. top_controls_manager_unittest should be updated.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

BUG=581227
==========

to

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. top_controls_manager_unittest should be updated.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

[Ian Wen, 2016-06-29 18:08:29]

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

[Ian Wen, 2016-06-29 18:08:30]

PTAL

[aelias_OOO_until_Jul13, 2016-06-29 18:52:15]

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

[aelias_OOO_until_Jul13, 2016-06-29 18:52:50]

Seems reasonable enough, adding bokan@ for primary review since he maintains
this code now.

[bokan, 2016-06-30 12:31:21]

Code looks mostly fine. If we do go this route, we should rename top controls
manager to be less "top" specific.

That said, I do have reservations about reusing the top controls infrastructure
for more UI. The way top controls work has been a source of pain for interop and
web platform features. My suggestion would be to have bottom UI bars resize the
visual viewport instead. This is how we'd like the on screen keyboard to work
and you can see it by flipping the --enable-osk-overscroll flag and following
the code in https://codereview.chromium.org/1386403003.

The advantage is that resizes don't affect layout so showing a bar won't cause
the content to relayout and reshuffle or jank but it still allows content to
scroll and the change will be web exposed once
https://github.com/WICG/ViewportAPI ships. This may also let us do resizes at
60fps if we want the position to depend on scroll.

It should also be easier to implement since the plumbing is already all there.
All you'd need to do is manage the UI on the browser side and have it calculate
the visible rect in RWHV::GetVisibleViewportSize. WDYT?

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
cc/input/top_controls_manager.cc:80: return BottomControlsHeight() -
BottomControlsOffset();
This feels odd to me. When shown ratio is 0, BottomContentOffset will be
BottomControlsHeight. What does this method actually represent?

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
File cc/input/top_controls_manager.h (right):

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
cc/input/top_controls_manager.h:44: float BottomControlsOffset() const;
Please match the exiting convention:

BottomControlsOffset -> ControlsBottomOffset
BottomContentOffset -> ContentBottomOffset

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2459:
getContentViewClient().onBottomOffsetChanged(bottomOffsetYCss * deviceScale,
Shouldn't we check if the offsets actually changed here?

[aelias_OOO_until_Jul13, 2016-06-30 19:17:12]

On 2016/06/30 at 12:31:21, bokan wrote:
> My suggestion would be to have bottom UI bars resize the visual viewport
instead.

Just like with the top bar, we don't want the bottom bar to cover fixed-position
elements.  That's why it affects the layout viewport.

[Ian Wen, 2016-07-01 02:51:44]

Thanks for the review! :)

I agree what aelias@ said previously... Bottom controls need to play nicely with
fixed items on webpage and re-layout seems the only applicable approach.

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
cc/input/top_controls_manager.cc:80: return BottomControlsHeight() -
BottomControlsOffset();
On 2016/06/30 12:31:21, bokan wrote:
> This feels odd to me. When shown ratio is 0, BottomContentOffset will be
> BottomControlsHeight. What does this method actually represent?

Hmmm you are right. I should swap the value content-offset and controls-offset
here.

content-offset should mean "how much this bar takes on the screen now", and
controls-offset means "how much should I translate this bar". Actually I renamed
the values in the IPC message and browser side so that they make more sense.

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
File cc/input/top_controls_manager.h (right):

https://codereview.chromium.org/2106753004/diff/1/cc/input/top_controls_manag...
cc/input/top_controls_manager.h:44: float BottomControlsOffset() const;
On 2016/06/30 12:31:21, bokan wrote:
> Please match the exiting convention:
> 
> BottomControlsOffset -> ControlsBottomOffset
> BottomContentOffset -> ContentBottomOffset

Done.

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2459:
getContentViewClient().onBottomOffsetChanged(bottomOffsetYCss * deviceScale,
On 2016/06/30 12:31:21, bokan wrote:
> Shouldn't we check if the offsets actually changed here?

onOffsetsForFullscreenChanged() is the counterpart where we notifies the UI that
the top control has changed. Since it does not check if the values have actually
changed, I think to make it symmetric, we shouldn't check for the bottom as
well.

[bokan, 2016-07-01 17:17:50]

On 2016/06/30 19:17:12, aelias wrote:
> On 2016/06/30 at 12:31:21, bokan wrote:
> > My suggestion would be to have bottom UI bars resize the visual viewport
> instead.
> 
> Just like with the top bar, we don't want the bottom bar to cover
fixed-position
> elements.  That's why it affects the layout viewport.

But most of bottom bars I see are already an overlay - just checking contextual
search and reader mode, neither resizes the widget. My arguments against making
them resize the widget would be:

1) Some (admittedly not all) pages perform poorly with vertical resizes. This
will cause these "low friction" bottom bars to appear clunky and cause a poor
user experience in addition to using more battery power.

2) Pushing up position: fixed elements means we might overlay the content the
user is looking at/interacting with. Scrolling won't help in this case since
it's position: fixed.

3) Bottom fixed elements are rarely useful. Anecdotally, the only bottom fixed
elements I ever see are either: ads, "open in app", or "accept our TOS/cookie
policy"

4) We're throwing something on the user's screen. They already expect that it's
occluding their content.

Bringing up contextual search is smooth and painless every time, opening the OSK
(a resizing bottom "bar") on some pages feels like going back in time a few
years.

[bokan, 2016-07-01 17:19:14]

A few more nits but code is lgtm from me (non-OWNER though). I've stated my
arguments against using this for most bottom bars but I won't block if y'all
disagree :).

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2459:
getContentViewClient().onBottomOffsetChanged(bottomOffsetYCss * deviceScale,
On 2016/07/01 02:51:44, Ian Wen wrote:
> On 2016/06/30 12:31:21, bokan wrote:
> > Shouldn't we check if the offsets actually changed here?
> 
> onOffsetsForFullscreenChanged() is the counterpart where we notifies the UI
that
> the top control has changed. Since it does not check if the values have
actually
> changed, I think to make it symmetric, we shouldn't check for the bottom as
> well.

The documentation for that method also states that it's called when there's a
change. At the least, I would either change the names and comments on both these
methods to make it clear the values aren't necessarily changing or actually
check for changes before calling them. Anyway, this is well outside my domain,
Alexandre may have better guidance here.

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
File cc/input/top_controls_manager.h (right):

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
cc/input/top_controls_manager.h:39: // The amount of offset to translate the top
controls. 0 or negative value.
Thanks for adding comments.

Suggested change here, how about "The offset from the window top to the top edge
of the controls. Runs from 0 (controls fully shown) to negative values (down is
positive)." It's a bit ambiguous unless you know which edges you're measuring
and where 0 is.

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
cc/input/top_controls_manager.h:45: float TopControlsHeight() const;
Please comment that this is a static value. The actual offset is calculated
using the shown ratio.

https://codereview.chromium.org/2106753004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2106753004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:394: public
void onTopControlsChanged(float topControlsOffsetY, float topControlsShownY) {
Did we drop `Fullscreen` in the name intentionally here?

[aelias_OOO_until_Jul13, 2016-07-01 19:14:43]

Currently, this is only intended for use with the CCT bottom bar which is more
persistent than the other kinds.  (We are also considering using it for other
potential, more persistent bottom bar UIs, but not any of the existing transient
ones.)  You can see how it looks today by kicking off a CCT from the test app
"ninja -C out/Debug custom_tabs_client_example_apk".  Also, Google Spaces has
shipped use of them.  As you can see, it is very persistent just like the
URL-bar, so taking a similar approach makes sense.

[bokan, 2016-07-01 19:16:18]

On 2016/07/01 19:14:43, aelias wrote:
> Currently, this is only intended for use with the CCT bottom bar which is more
> persistent than the other kinds.  (We are also considering using it for other
> potential, more persistent bottom bar UIs, but not any of the existing
transient
> ones.)  You can see how it looks today by kicking off a CCT from the test app
> "ninja -C out/Debug custom_tabs_client_example_apk".  Also, Google Spaces has
> shipped use of them.  As you can see, it is very persistent just like the
> URL-bar, so taking a similar approach makes sense.

Chatted briefly with ianwen@ about this. Makes sense for these kinds of
scenarios. The linked bug had all sorts of other bars that I think wouldn't
benefit from this so as long as we're not converting all those cases I'm in
agreement.

[aelias_OOO_until_Jul13, 2016-07-01 22:23:59]

Still looks mostly alright, but ianwen@ mentioned offline that he was planning
to get rid of redundant denormalized values (which sounds fine to me), and there
are some unapplied nits.  Please ping again when it's ready for final review.

[Ian Wen, 2016-07-06 20:53:44]

PTAL.

I talked to tedchoc@ offline about the necessity of having two values (offset
and translation) in frame metadata. The original design was that we wanted to
always have sufficient info to describe a single frame. If I remove one of the
two values, we will rely on the assumption that the top/bottom controls max
height will never change, which is not a always-true assumption. Therefore in
the updated patchset I still kept the two values. The names of the values were
changed though, to make them make more sense in current codebase.

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2106753004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2459:
getContentViewClient().onBottomOffsetChanged(bottomOffsetYCss * deviceScale,
On 2016/07/01 17:19:13, bokan wrote:
> On 2016/07/01 02:51:44, Ian Wen wrote:
> > On 2016/06/30 12:31:21, bokan wrote:
> > > Shouldn't we check if the offsets actually changed here?
> > 
> > onOffsetsForFullscreenChanged() is the counterpart where we notifies the UI
> that
> > the top control has changed. Since it does not check if the values have
> actually
> > changed, I think to make it symmetric, we shouldn't check for the bottom as
> > well.
> 
> The documentation for that method also states that it's called when there's a
> change. At the least, I would either change the names and comments on both
these
> methods to make it clear the values aren't necessarily changing or actually
> check for changes before calling them. Anyway, this is well outside my domain,
> Alexandre may have better guidance here.

I changed the comments of the two methods so that they don't specifically
require the values to "change". Done.

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
File cc/input/top_controls_manager.h (right):

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
cc/input/top_controls_manager.h:39: // The amount of offset to translate the top
controls. 0 or negative value.
On 2016/07/01 17:19:13, bokan wrote:
> Thanks for adding comments.
> 
> Suggested change here, how about "The offset from the window top to the top
edge
> of the controls. Runs from 0 (controls fully shown) to negative values (down
is
> positive)." It's a bit ambiguous unless you know which edges you're measuring
> and where 0 is.

done.

https://codereview.chromium.org/2106753004/diff/20001/cc/input/top_controls_m...
cc/input/top_controls_manager.h:45: float TopControlsHeight() const;
On 2016/07/01 17:19:14, bokan wrote:
> Please comment that this is a static value. The actual offset is calculated
> using the shown ratio.

Done.

https://codereview.chromium.org/2106753004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2106753004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:394: public
void onTopControlsChanged(float topControlsOffsetY, float topControlsShownY) {
On 2016/07/01 17:19:14, bokan wrote:
> Did we drop `Fullscreen` in the name intentionally here?

Yes. The term "fullscreen" is, IMO, out dated and overloaded. It was first
introduced for the project to hide the top controls, later video/html5
fullscreen also utilized part of the code. What I am trying to do here is to
slowly remove the term "fullscreen" from the top controls code.

[aelias_OOO_until_Jul13, 2016-07-06 22:36:59]

https://codereview.chromium.org/2106753004/diff/40001/cc/output/compositor_fr...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2106753004/diff/40001/cc/output/compositor_fr...
cc/output/compositor_frame_metadata.h:52: gfx::Vector2dF top_bar_translate;
I agree we need two values, but how about standardizing on "height" and "0-to-1
ratio" instead?

There's some difficulties with the two-offset representation which led us to
move away from it in CC.  If "height" is zero, then there is no way to
distinguish between "shown" and "hidden".  So if you send two messages in
sequence, the first one instructing the state to change to "shown", and the
second one setting the height to nonzero, the information in the first message
is effectively lost.  The ratio representation solves this, as well as allowing
exact comparisons to 0 and 1 without worrying about possible floating-point
errors.

[Ian Wen, 2016-07-08 18:24:00]

Hi alieas@, I refactored the plumbing code so that it now passes height and
ratio. In the process I realized in some places we must use a concrete "offset".
These places include window_android, RenderCoordinates, page_handler. Ideally
ChromeFullScreenManager should be refactored to utilize the ratio/height pairs.
Yet it seems to be another CL's scope.

I have another ongoing effort to refactor the fullscreen manager in Android
Chrome, and the plan was drafted here:
https://docs.google.com/a/google.com/document/d/1kuDavsbninih3DCyvcAuikzRBoqi...

[aelias_OOO_until_Jul13, 2016-07-08 23:46:33]

Could you add one or two tests to top_controls_manager_unittest as part of this
patch?  Usually, we prefer not to punt testing to separate patches.

https://codereview.chromium.org/2106753004/diff/60001/cc/output/compositor_fr...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2106753004/diff/60001/cc/output/compositor_fr...
cc/output/compositor_frame_metadata.h:52: gfx::Vector2dF top_controls_height;
Hmm, I don't see a reasonable interpretation of 2 dimensions for this, so please
make the heights just floats as they are elsewhere.

https://codereview.chromium.org/2106753004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2106753004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2677: private
void onOffsetsChanged(float topControlsTranslate, float topControlsShown,
Hmm, I don't find these names clearer than the previous ones.  Unfortunately,
it's quite difficult to come up with good names for these values.  Another idea
I have is topControlsStartOffset and topControlsEndOffset.  I'd suggest just
leaving the name change out of this patch.

[Ian Wen, 2016-08-13 00:05:27]

Hi aelias@,

All comments are addressed. PTAL. :)

https://codereview.chromium.org/2106753004/diff/60001/cc/output/compositor_fr...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2106753004/diff/60001/cc/output/compositor_fr...
cc/output/compositor_frame_metadata.h:52: gfx::Vector2dF top_controls_height;
On 2016/07/08 23:46:33, aelias wrote:
> Hmm, I don't see a reasonable interpretation of 2 dimensions for this, so
please
> make the heights just floats as they are elsewhere.

Done.

https://codereview.chromium.org/2106753004/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2106753004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2677: private
void onOffsetsChanged(float topControlsTranslate, float topControlsShown,
On 2016/07/08 23:46:33, aelias wrote:
> Hmm, I don't find these names clearer than the previous ones.  Unfortunately,
> it's quite difficult to come up with good names for these values.  Another
idea
> I have is topControlsStartOffset and topControlsEndOffset.  I'd suggest just
> leaving the name change out of this patch.

Done.

[aelias_OOO_until_Jul13, 2016-08-13 01:04:42]

Description was changed from

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. top_controls_manager_unittest should be updated.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

to

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

[aelias_OOO_until_Jul13, 2016-08-13 01:05:25]

lgtm, feel free to add remaining OWNERS.

[Ian Wen, 2016-08-15 17:48:34]

ianwen@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org, wfh@chromium.org

[Ian Wen, 2016-08-15 17:48:35]

tedchoc@chromium.org: Please review changes in everything? ;) If you feel lazy
today, Tab.java it is.

sievers@chromium.org: Please review changes in cc.

wfh@chromium.org: Please review changes in ipc.

[Ian Wen, 2016-08-15 17:48:51]

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

[commit-bot: I haz the power, 2016-08-15 17:49:24]

Dry run: 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-08-15 17:49:25]

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

[commit-bot: I haz the power, 2016-08-15 17:49:26]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[Will Harris, 2016-08-15 17:56:44]

cc/ipc/* and content/common/view_messages.h lgtm

[Ian Wen, 2016-08-15 18:24:48]

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

[commit-bot: I haz the power, 2016-08-15 18:25:07]

Dry run: 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-08-15 18:25:08]

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

[commit-bot: I haz the power, 2016-08-15 18:25:09]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[no sievers, 2016-08-16 20:29:16]

content lgtm w/1 comment

https://chromiumcodereview.appspot.com/2106753004/diff/100001/content/browser...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/2106753004/diff/100001/content/browser...
content/browser/renderer_host/render_widget_host_view_android.cc:1004:
bottom_bar_shown_ratio_ != frame.metadata.bottom_controls_shown_ratio ||
This is for pixel-perfect scrolling where tearing down the Surface causes a
browser-composite dependency (for repositioning the omnibox *texture* layer
correctly). But your cl description says you don't need this.

The layer bounds are already getting correctly updated from the call site.

[Ian Wen, 2016-08-17 00:20:45]

ianwen@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Ian Wen, 2016-08-17 00:20:46]

dfalcantara@ PTAL the changes in tab.java. :)

https://codereview.chromium.org/2106753004/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2106753004/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1004:
bottom_bar_shown_ratio_ != frame.metadata.bottom_controls_shown_ratio ||
On 2016/08/16 20:29:16, sievers wrote:
> This is for pixel-perfect scrolling where tearing down the Surface causes a
> browser-composite dependency (for repositioning the omnibox *texture* layer
> correctly). But your cl description says you don't need this.
> 
> The layer bounds are already getting correctly updated from the call site.

Done. I removed #1004, but kept #1017 as is.

[gone, 2016-08-17 00:22:15]

Tab.java lgtm

[Ian Wen, 2016-08-17 00:22:47]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-17 00:22:48]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, sievers@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2106753004/#ps120001
(title: "sievers@'s comment")

[commit-bot: I haz the power, 2016-08-17 00:23:18]

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-08-17 00:23:19]

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

[commit-bot: I haz the power, 2016-08-17 00:23:20]

Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[Ian Wen, 2016-08-17 00:24:13]

Description was changed from

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

to

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

BUG=581227
==========

[Ian Wen, 2016-08-17 00:24:23]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2016-08-17 00:25:05]

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-08-17 00:28:04]

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

[commit-bot: I haz the power, 2016-08-17 00:28:06]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ian Wen, 2016-08-17 03:24:45]

Description was changed from

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

BUG=581227
==========

to

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

[Ian Wen, 2016-08-17 03:25:40]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-17 03:25:41]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
bokan@chromium.org, sievers@chromium.org, dfalcantara@chromium.org,
wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2106753004/#ps140001 (title: "rebase")

[commit-bot: I haz the power, 2016-08-17 03:25:57]

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-08-17 03:27:46]

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

[commit-bot: I haz the power, 2016-08-17 03:27:48]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Khushal, 2016-08-17 04:45:35]

https://codereview.chromium.org/2106753004/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2106753004/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:519:
sync_tree->set_bottom_controls_height(bottom_controls_height_);
The commit step now happens in LayerTree::PushPropertiesTo.

https://codereview.chromium.org/2106753004/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2106753004/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:253: void setBottomControlHeight(float height);
Sorry about the rebase pain. :(

You can move this to LayerTree since all tree specific settings live there now.

https://codereview.chromium.org/2106753004/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:544: float bottom_controls_height_;
Move to LayerTree.

https://codereview.chromium.org/2106753004/diff/140001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2106753004/diff/140001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:117:
frame.metadata.top_controls_shown_ratio ||
Should this check for a change in height too?

https://codereview.chromium.org/2106753004/diff/140001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:130:
current_frame_->top_controls_shown_ratio =
Need to update the top and bottom height also here.

https://codereview.chromium.org/2106753004/diff/140001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:241: content_size_in_dip.height() -
top_bar_shown - bottom_bar_shown);
The top and bottom bar should be added. This will give us the total viewport
size for this render frame, and we decide to show the background based on
whether the container size matches this.

[Ian Wen, 2016-08-17 05:44:51]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-17 05:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
bokan@chromium.org, sievers@chromium.org, dfalcantara@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2106753004/#ps160001
(title: "rebase again")

[commit-bot: I haz the power, 2016-08-17 05:45:15]

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-08-17 05:51:47]

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

[commit-bot: I haz the power, 2016-08-17 05:51:49]

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...)

[Ted C, 2016-08-17 20:54:47]

On 2016/08/17 05:51:49, commit-bot: I haz the power wrote:
> 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...)

ui/android - lgtm

[Ian Wen, 2016-08-17 22:26:37]

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

[commit-bot: I haz the power, 2016-08-17 22:27:10]

Dry run: 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-08-17 23:50:59]

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

[commit-bot: I haz the power, 2016-08-17 23:51:01]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Ian Wen, 2016-08-17 23:55:58]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-17 23:55:58]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
bokan@chromium.org, tedchoc@chromium.org, sievers@chromium.org,
dfalcantara@chromium.org, wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2106753004/#ps220001 (title: "findbug")

[commit-bot: I haz the power, 2016-08-17 23:56:25]

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-08-18 01:16:25]

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

[commit-bot: I haz the power, 2016-08-18 01:16:27]

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

[Ian Wen, 2016-08-18 03:41:15]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2016-08-18 03:41:38]

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-08-18 04:05:33]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-18 04:08:34]

Description was changed from

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

to

==========
Introduce bottom controls to CC and let it respond to scrolling

This CL introduces bottom controls to compositor. Once the height of the
bottom controls is specified, a resize signal will be sent to cc and
renderer with a shortened view port height. Upon user scrolling, the
height of the bottom bar is computed by applying the shown ratio of the
top controls to the bottom controls. Since bottom control is less
sensitive, it will react to scrolling even if top controls is
constrained. Also please note that the shrinking and expanding of the
bottom area do not need to be pixel perfect, as such perfection cannot
be observed by human vision.

TODOs:
1. Implement main thread scrolling, like what we did for top controls.
2. Investigate to conduct standalone scrolling animation if the top
controls is totally absent.

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

Committed: https://crrev.com/e5fc57843cf8d21d9bf51e389821657f3f768317
Cr-Commit-Position: refs/heads/master@{#412738}
==========

[commit-bot: I haz the power, 2016-08-18 04:08:36]

Patchset 12 (id:??) landed as
https://crrev.com/e5fc57843cf8d21d9bf51e389821657f3f768317
Cr-Commit-Position: refs/heads/master@{#412738}