[Home] Add a bottom navigation bar for the BottomSheet

[Home] Add a bottom navigation bar for the BottomSheet

This adds a basic navigation bar fixed to the bottom of the
BottomSheet. Changing BottomSheet content will be done in a
follow up CL(s).

BUG=699598
Review-Url: https://codereview.chromium.org/2738843003
Cr-Commit-Position: refs/heads/master@{#456833}
Committed: https://chromium.googlesource.com/chromium/src/+/4a2aaa37f0366b0a4673fa57867cda86f65118c9

[Theresa, 2017-03-08 18:02:49]

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

[commit-bot: I haz the power, 2017-03-08 18:03:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-08 18:05:23]

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

[Theresa, 2017-03-08 18:05:24]

ptal

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

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:596:
public float getMinOffset() {
nit: these should be moved up by the other public methods.

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java
(right):

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:36:
public void init(BottomSheet bottomSheet, int controlContainerHeightResId) {
mdjones@ -- what's the best way to get toolbar height these days?

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:49:
public void onTransitionPeekToHalf(float transitionFraction) {
nit: this should be located near the other BottomSheetObserver methods

[commit-bot: I haz the power, 2017-03-08 19:30:39]

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

[commit-bot: I haz the power, 2017-03-08 19:30:40]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-03-08 22:27:00]

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java
(right):

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:36:
public void init(BottomSheet bottomSheet, int controlContainerHeightResId) {
This method needs JavaDocs

[mdjones, 2017-03-09 16:52:10]

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

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/main.xml:74:
<org.chromium.chrome.browser.widget.BottomSheetBottomNav
Since this is on the main layout, should we conditionally inflate this or does
visibility="gone" prevent resource usage?

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java
(right):

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:36:
public void init(BottomSheet bottomSheet, int controlContainerHeightResId) {
On 2017/03/08 18:05:24, Theresa wrote:
> mdjones@ -- what's the best way to get toolbar height these days?

Ideally we can query the toolbar view directly or someone who knows about it. In
this case ChromeActivity has a handle to the control container view so we can
query that.

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:68:
Field shiftingMode = menuView.getClass().getDeclaredField("mShiftingMode");
Woo!

[Theresa, 2017-03-09 17:55:37]

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

[Theresa, 2017-03-09 17:55:45]

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

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/main.xml:74:
<org.chromium.chrome.browser.widget.BottomSheetBottomNav
On 2017/03/09 16:52:10, mdjones wrote:
> Since this is on the main layout, should we conditionally inflate this or does
> visibility="gone" prevent resource usage? 

Done. No, I think resources are still allocated even with visibility="gone"
(based on Android monitor).

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

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:596:
public float getMinOffset() {
On 2017/03/08 18:05:24, Theresa wrote:
> nit: these should be moved up by the other public methods.

Done.

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java
(right):

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:36:
public void init(BottomSheet bottomSheet, int controlContainerHeightResId) {
On 2017/03/09 16:52:10, mdjones wrote:
> On 2017/03/08 18:05:24, Theresa wrote:
> > mdjones@ -- what's the best way to get toolbar height these days?
> 
> Ideally we can query the toolbar view directly or someone who knows about it.
In
> this case ChromeActivity has a handle to the control container view so we can
> query that.

Done.

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:36:
public void init(BottomSheet bottomSheet, int controlContainerHeightResId) {
On 2017/03/08 22:27:00, Theresa wrote:
> This method needs JavaDocs

Done.

https://codereview.chromium.org/2738843003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:49:
public void onTransitionPeekToHalf(float transitionFraction) {
On 2017/03/08 18:05:24, Theresa wrote:
> nit: this should be located near the other BottomSheetObserver methods

Done.

[commit-bot: I haz the power, 2017-03-09 17:55:58]

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, 2017-03-09 18:53:13]

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

[commit-bot: I haz the power, 2017-03-09 18:53:14]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-03-09 19:13:54]

lgtm

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:373:
BottomSheetBottomNav bottomNav = (BottomSheetBottomNav)
findViewById(R.id.bottom_nav);
I believe bottom_nav is the view returned by the inflate call. Maybe too much
casting in one line though...

[Theresa, 2017-03-09 19:19:09]

twellington@chromium.org changed reviewers:
+ agrieve@chromium.org, dfalcantara@chromium.org

[Theresa, 2017-03-09 19:19:10]

+dfalcantara@ for ChromeActivity OWNERS
+agrieve@ for base/android and build/

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:373:
BottomSheetBottomNav bottomNav = (BottomSheetBottomNav)
findViewById(R.id.bottom_nav);
On 2017/03/09 19:13:54, mdjones wrote:
> I believe bottom_nav is the view returned by the inflate call. Maybe too much
> casting in one line though...

You are correct. I don't think it's too much casting -- I'll update it along w/
any comments from new reviewers.

[Theresa, 2017-03-09 19:36:37]

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

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:27:
public class BottomSheetBottomNav extends BottomNavigationView
Yesterday Matt and I discussed that this class will manage the
BottomSheetContent classes. I'm going to rename this
"BottomSheetContentController" as part of this patch.

[Theresa, 2017-03-09 23:10:07]

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:373:
BottomSheetBottomNav bottomNav = (BottomSheetBottomNav)
findViewById(R.id.bottom_nav);
On 2017/03/09 19:19:10, Theresa wrote:
> On 2017/03/09 19:13:54, mdjones wrote:
> > I believe bottom_nav is the view returned by the inflate call. Maybe too
much
> > casting in one line though...
> 
> You are correct. I don't think it's too much casting -- I'll update it along
w/
> any comments from new reviewers.

Done.

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

https://codereview.chromium.org/2738843003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetBottomNav.java:27:
public class BottomSheetBottomNav extends BottomNavigationView
On 2017/03/09 19:36:36, Theresa wrote:
> Yesterday Matt and I discussed that this class will manage the
> BottomSheetContent classes. I'm going to rename this
> "BottomSheetContentController" as part of this patch.

Done.

[gone, 2017-03-10 01:25:28]

lgtm on ChromeActivity

[Theresa, 2017-03-13 18:46:54]

agrieve@ - ptal. The patchset that's dependent on this is through review and
ready to land.

[agrieve, 2017-03-14 18:06:35]

proguard & build changes lgtm /w one nit.

Obligatory nag about using vector drawables rather than pngs (which I know
you're tracking, but these are really simple images...)

https://codereview.chromium.org/2738843003/diff/60001/base/android/proguard/c...
File base/android/proguard/chromium_apk.flags (right):

https://codereview.chromium.org/2738843003/diff/60001/base/android/proguard/c...
base/android/proguard/chromium_apk.flags:64: -keepclassmembers class
android.support.design.internal.BottomNavigationMenuView {
I think adding this is fine, but I think a better spot for it would be:
//chrome/android/java/proguard.flags

[Theresa, 2017-03-14 18:12:24]

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

[commit-bot: I haz the power, 2017-03-14 18:13:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-14 18:14:22]

I filed crbug.com/701447 to track potentially moving from PNGs to vector
drawables.

https://codereview.chromium.org/2738843003/diff/60001/base/android/proguard/c...
File base/android/proguard/chromium_apk.flags (right):

https://codereview.chromium.org/2738843003/diff/60001/base/android/proguard/c...
base/android/proguard/chromium_apk.flags:64: -keepclassmembers class
android.support.design.internal.BottomNavigationMenuView {
On 2017/03/14 18:06:34, agrieve wrote:
> I think adding this is fine, but I think a better spot for it would be:
> //chrome/android/java/proguard.flags

Done.

[commit-bot: I haz the power, 2017-03-14 19:51:48]

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

[commit-bot: I haz the power, 2017-03-14 19:51:49]

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

[Theresa, 2017-03-14 20:19:26]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-14 20:19:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dfalcantara@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2738843003/#ps80001
(title: "Move proguard exception to different file")

[commit-bot: I haz the power, 2017-03-14 20:20:16]

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-03-14 21:17:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489522766239010,
"parent_rev": "ec06dfef6be7f6c1dce6ecf841089863528fa05f", "commit_rev":
"4a2aaa37f0366b0a4673fa57867cda86f65118c9"}

[commit-bot: I haz the power, 2017-03-14 21:18:31]

Description was changed from

==========
[Home] Add a bottom navigation bar for the BottomSheet

This adds a basic navigation bar fixed to the bottom of the
BottomSheet. Changing BottomSheet content will be done in a
follow up CL(s).

BUG=699598
==========

to

==========
[Home] Add a bottom navigation bar for the BottomSheet

This adds a basic navigation bar fixed to the bottom of the
BottomSheet. Changing BottomSheet content will be done in a
follow up CL(s).

BUG=699598

Review-Url: https://codereview.chromium.org/2738843003
Cr-Commit-Position: refs/heads/master@{#456833}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a2aaa37f0366b0a4673fa57867c...
==========

[commit-bot: I haz the power, 2017-03-14 21:18:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4a2aaa37f0366b0a4673fa57867c...