Attach NTP to Chrome Home bottom sheet

Attach NTP to Chrome Home bottom sheet

This change attaches the NTP to the bottom sheet as it's only form
of content. This is done when the user first scrolls up on the
toolbar. As a result, changes also needed to be made to the touch
event handling to allow the sheet and its content to be scrolled
correctly. The sheet's scrolling now uses the following rules:

- If the sheet is not in it's full state, any scroll will move the
  sheet.
- In the full state, if the user scrolls the sheet from the toolbar,
  the sheet will always move regardless of the state of the content.
- In the full state, if the sheet's content has 0 scroll, a scroll
  down on any part of the sheet will move the sheet down.
- In the full state, if the user scrolls the content, the content
  will move rather than the sheet.

BUG=671361
Review-Url: https://codereview.chromium.org/2640223002
Cr-Commit-Position: refs/heads/master@{#446133}
Committed: https://chromium.googlesource.com/chromium/src/+/f00c656f579d31ce878b8e472417cd163ddbee95

[mdjones, 2017-01-19 02:02:04]

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

[mdjones, 2017-01-19 02:02:05]

ptal

[Bernhard Bauer, 2017-01-19 10:13:04]

Description was changed from

==========
Attach NTP to Chrome Home bottom sheet

This change attaches the NTP to the bottom sheet as it's only form
of content. This is done when the user first scrolls up on the
toolbar. As a result, changes also needed to be made to the touch
event handling to allow the sheet and its content to be scrolled
correctly. The sheet's scrolling now uses the following rules:

- If the sheet is not in it's full state, any scroll will move the
  sheet.
- In the full state, if the user scrolls the sheet from the toolbar,
  the sheet will always move regardless of the state of the content.
- In the full state, if the sheet's content has 0 scroll, a scroll
  down on any part of the sheet will move the sheet down.
- In the full state, if the user scrolls the content, the content
  will move rather than the sheet.

BUG=671361
==========

to

==========
Attach NTP to Chrome Home bottom sheet

This change attaches the NTP to the bottom sheet as it's only form
of content. This is done when the user first scrolls up on the
toolbar. As a result, changes also needed to be made to the touch
event handling to allow the sheet and its content to be scrolled
correctly. The sheet's scrolling now uses the following rules:

- If the sheet is not in it's full state, any scroll will move the
  sheet.
- In the full state, if the user scrolls the sheet from the toolbar,
  the sheet will always move regardless of the state of the content.
- In the full state, if the sheet's content has 0 scroll, a scroll
  down on any part of the sheet will move the sheet down.
- In the full state, if the user scrolls the content, the content
  will move rather than the sheet.

BUG=671361
==========

[mdjones, 2017-01-20 21:22:25]

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

[mdjones, 2017-01-20 21:22:25]

+dfalcantara

[gone, 2017-01-23 19:48:40]

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:49:
android:layout_height="match_parent">
nit: space before > to match the rest of the file.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:69:
private static final float EPSILON = 0.0001f;
Can we just get a common version of this?  We've got a bunch of different
EPSILON definitions for different float equality checks that really shouldn't
all be different.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:97:
/** The width and height of the view that contains the bottom sheet. */
Separate the javadocs.  They're meant to apply to the statement immediately
after them, which confuses IDEs.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:120:
private final int[] mLocationArr;
1) Don't abbreviate Array.
2) Private finals up top, unless you have a good reason to keep this here.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:162:
// If the sheet is in the max position, don't move if the scroll is upward.
Comment's not terribly helpful; fix comment s'il vous plait.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:213:
mLocationArr = new int[2];
Might as well

private final int mLocationArray = new int[2];

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:333:
mLocationArr[0] = mLocationArr[1] = 0;
Shouldn't this thing straight up return false here because it's an invalid
state?  Granted, mToolbarHeight is set only when the sheet is initialized, so
it'll be zero in that case and it'll fall through, but it'd return false in that
case anyway.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:390:
mStateRatios[0] = mToolbarHeight / mContainerHeight;
Is this thing actually updating the ratio array?  I'd thought that mStateRatios
was supposed to be unchanging and final.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:392:
// Compute the height that the content section of the bottom sheet should be.
// Compute the height of the content section of the bottom sheet.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:402:
* necessary.
Yeah... this logic makes me sad.  Is this only until we have all the sheets
implemented as ScrollingViews?  It'd switch to an assert at that point, I
imagine?

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:406:
private ScrollingView findScrollingChild(View view) {
@Nullable

[mdjones, 2017-01-24 01:37:25]

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:49:
android:layout_height="match_parent">
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> nit: space before > to match the rest of the file.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:69:
private static final float EPSILON = 0.0001f;
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Can we just get a common version of this?  We've got a bunch of different
> EPSILON definitions for different float equality checks that really shouldn't
> all be different.

Added constant to MathUtils, I'll update other references later.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:97:
/** The width and height of the view that contains the bottom sheet. */
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Separate the javadocs.  They're meant to apply to the statement immediately
> after them, which confuses IDEs.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:120:
private final int[] mLocationArr;
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> 1) Don't abbreviate Array.
> 2) Private finals up top, unless you have a good reason to keep this here.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:162:
// If the sheet is in the max position, don't move if the scroll is upward.
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Comment's not terribly helpful; fix comment s'il vous plait.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:213:
mLocationArr = new int[2];
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Might as well
> 
> private final int mLocationArray = new int[2];

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:333:
mLocationArr[0] = mLocationArr[1] = 0;
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Shouldn't this thing straight up return false here because it's an invalid
> state?  Granted, mToolbarHeight is set only when the sheet is initialized, so
> it'll be zero in that case and it'll fall through, but it'd return false in
that
> case anyway.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:390:
mStateRatios[0] = mToolbarHeight / mContainerHeight;
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Is this thing actually updating the ratio array?  I'd thought that
mStateRatios
> was supposed to be unchanging and final.

Yeah, unfortunately we don't know the height of the toolbar or compositor view
until it is inflated (both are needed to compute this value).

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:392:
// Compute the height that the content section of the bottom sheet should be.
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> // Compute the height of the content section of the bottom sheet.

Done.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:402:
* necessary.
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> Yeah... this logic makes me sad.  Is this only until we have all the sheets
> implemented as ScrollingViews?  It'd switch to an assert at that point, I
> imagine?

Yeah, I'll remove after I switch away from using native pages for content. Each
content view will implement a limited interface and assert that the view
obtained from each is something that scrolls.

https://codereview.chromium.org/2640223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:406:
private ScrollingView findScrollingChild(View view) {
On 2017/01/23 19:48:39, dfalcantara (load balance plz) wrote:
> @Nullable

Done.

[PEConn, 2017-01-24 10:38:23]

peconn@chromium.org changed reviewers:
+ peconn@chromium.org

[PEConn, 2017-01-24 10:38:23]

https://codereview.chromium.org/2640223002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2640223002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:345:
private static boolean areFloatsEqual(float f1, float f2) {
Can this go into MathUtils too?

[mdjones, 2017-01-24 16:38:13]

https://codereview.chromium.org/2640223002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2640223002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:345:
private static boolean areFloatsEqual(float f1, float f2) {
On 2017/01/24 10:38:23, PEConn wrote:
> Can this go into MathUtils too?

Done.

[gone, 2017-01-25 19:19:10]

lgtm % comment

https://codereview.chromium.org/2640223002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2640223002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:164:
// Instead, allow the sheet's content to handle it if it needes to.
needles?

[Ian Wen, 2017-01-25 19:55:44]

lgtm!

https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/bottom_control_container.xml:53:
android:layout_width="match_parent"
SetWillDraw() can help you draw white background for FrameLayout.

[Ian Wen, 2017-01-25 19:56:05]

On 2017/01/25 19:55:44, Ian Wen wrote:
> lgtm!
> 
>
https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
> File chrome/android/java/res/layout/bottom_control_container.xml (right):
> 
>
https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
> chrome/android/java/res/layout/bottom_control_container.xml:53:
> android:layout_width="match_parent"
> SetWillDraw() can help you draw white background for FrameLayout.

Please ignore this comment. :)

[mdjones, 2017-01-25 19:57:25]

https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2640223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/bottom_control_container.xml:53:
android:layout_width="match_parent"
On 2017/01/25 19:55:44, Ian Wen wrote:
> SetWillDraw() can help you draw white background for FrameLayout.

Removed the need for this.

[mdjones, 2017-01-25 19:57:42]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-01-25 19:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2640223002/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2017-01-25 19:58:35]

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, 2017-01-25 21:58:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485374262778400,
"parent_rev": "18a4bda779e6b2d09a6dac1bdfe86a4732892e8a", "commit_rev":
"f00c656f579d31ce878b8e472417cd163ddbee95"}

[commit-bot: I haz the power, 2017-01-25 21:58:57]

Description was changed from

==========
Attach NTP to Chrome Home bottom sheet

This change attaches the NTP to the bottom sheet as it's only form
of content. This is done when the user first scrolls up on the
toolbar. As a result, changes also needed to be made to the touch
event handling to allow the sheet and its content to be scrolled
correctly. The sheet's scrolling now uses the following rules:

- If the sheet is not in it's full state, any scroll will move the
  sheet.
- In the full state, if the user scrolls the sheet from the toolbar,
  the sheet will always move regardless of the state of the content.
- In the full state, if the sheet's content has 0 scroll, a scroll
  down on any part of the sheet will move the sheet down.
- In the full state, if the user scrolls the content, the content
  will move rather than the sheet.

BUG=671361
==========

to

==========
Attach NTP to Chrome Home bottom sheet

This change attaches the NTP to the bottom sheet as it's only form
of content. This is done when the user first scrolls up on the
toolbar. As a result, changes also needed to be made to the touch
event handling to allow the sheet and its content to be scrolled
correctly. The sheet's scrolling now uses the following rules:

- If the sheet is not in it's full state, any scroll will move the
  sheet.
- In the full state, if the user scrolls the sheet from the toolbar,
  the sheet will always move regardless of the state of the content.
- In the full state, if the sheet's content has 0 scroll, a scroll
  down on any part of the sheet will move the sheet down.
- In the full state, if the user scrolls the content, the content
  will move rather than the sheet.

BUG=671361

Review-Url: https://codereview.chromium.org/2640223002
Cr-Commit-Position: refs/heads/master@{#446133}
Committed:
https://chromium.googlesource.com/chromium/src/+/f00c656f579d31ce878b8e472417...
==========

[commit-bot: I haz the power, 2017-01-25 21:58:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f00c656f579d31ce878b8e472417...