🏠 Add expand button and flag to enable it

[Home] Add expand button and flag to enable it

This change adds an alternate UI for the Chrome Home bottom sheet.
Instead of being able to swipe up when the sheet is peeking, there is
the option to have a button that sits to the left of the omnibox that
expands the sheet when pressed. The feature name for this UI is
"ChromeHomeExpandButton".

This flag runs into the same problems as Chrome Home when first
being enabled -- native is not ready when UI is initialized. To
avoid yet another Chrome restart, the pull-handle version of the UI
will appear briefly before switching to the button.

BUG=716220
Review-Url: https://codereview.chromium.org/2853583002
Cr-Commit-Position: refs/heads/master@{#468781}
Committed: https://chromium.googlesource.com/chromium/src/+/d484eacb1a1016a1f2fda3bc57b8e309fd08df76

[mdjones, 2017-04-28 18:44:26]

Description was changed from

==========
[Home] Add expand button and flag to enable it

This change adds an alternate UI for the Chrome Home bottom sheet.
Instead of being able to swipe up when the sheet is peeking, there is
the option to have a button that sits to the left of the omnibox that
expands the sheet when pressed. The feature name for this UI is
"ChromeHomeExpandButton".

This flag runs into the same problems as Chrome Home when first
being enabled -- native is not ready when UI is initialized. To
avoid yet another Chrome restart, the pull-handle version of the UI
will appear briefly before switching to the button.

BUG=716220
==========

to

==========
[Home] Add expand button and flag to enable it

This change adds an alternate UI for the Chrome Home bottom sheet.
Instead of being able to swipe up when the sheet is peeking, there is
the option to have a button that sits to the left of the omnibox that
expands the sheet when pressed. The feature name for this UI is
"ChromeHomeExpandButton".

This flag runs into the same problems as Chrome Home when first
being enabled -- native is not ready when UI is initialized. To
avoid yet another Chrome restart, the pull-handle version of the UI
will appear briefly before switching to the button.

BUG=716220
==========

[mdjones, 2017-04-28 19:00:46]

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

[mdjones, 2017-04-28 19:00:47]

Screenshots in bug; ptal

[Theresa, 2017-04-28 21:50:32]

lgtm

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:272:
int topMarginForControls = getExtraTopMargin();
Should we return early if getExtraTopMargin() == 0?

[mdjones, 2017-04-28 22:15:30]

Did a better job centering the arrow. ptal

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:272:
int topMarginForControls = getExtraTopMargin();
On 2017/04/28 21:50:32, Theresa wrote:
> Should we return early if getExtraTopMargin() == 0?

Yep, done.

[Theresa, 2017-04-28 22:16:56]

still lgtm

[mdjones, 2017-04-28 22:16:58]

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

[commit-bot: I haz the power, 2017-04-28 22:17:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mdjones, 2017-04-28 23:24:03]

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

[mdjones, 2017-04-28 23:24:03]

+jwd for histograms

[mdjones, 2017-04-28 23:24:53]

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

[mdjones, 2017-04-28 23:24:53]

+dfalcantara for owners

[gone, 2017-04-30 01:56:00]

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:225:
if (!mUseToolbarHandle && mExpandButton != null && mExpandButton.getVisibility()
!= GONE) {
You constantly use !mUseToolbarHandle && mExpandButton != null.  Pull it out
into another function to keep your checks consistent.

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:299:
// The toolbar handle is part of the control container so it can draw on top of
the
why?

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:347:
if (mUseToolbarHandle) {
You treat the toolbar handle's non-existence as an assumption that the expand
button exists, but in other places you explicitly check for the expand button to
not be null.  Why?

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:406:
float previousAlpha = 0.f;
Where do you actually use this?

[mdjones, 2017-05-01 20:46:08]

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:272:
int topMarginForControls = getExtraTopMargin();
On 2017/04/28 21:50:32, Theresa wrote:
> Should we return early if getExtraTopMargin() == 0?

Turns out we can't do this. Since the toolbar is initialized with a non-zero
number before the button is setup, this needs to run with 0 in order to
correctly reset the views.

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:225:
if (!mUseToolbarHandle && mExpandButton != null && mExpandButton.getVisibility()
!= GONE) {
On 2017/04/30 01:56:00, slow (dfalcantara) wrote:
> You constantly use !mUseToolbarHandle && mExpandButton != null.  Pull it out
> into another function to keep your checks consistent.

Removed the null checks.

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:299:
// The toolbar handle is part of the control container so it can draw on top of
the
On 2017/04/30 01:56:00, slow (dfalcantara) wrote:
> why?

Reasons.

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:347:
if (mUseToolbarHandle) {
On 2017/04/30 01:56:00, slow (dfalcantara) wrote:
> You treat the toolbar handle's non-existence as an assumption that the expand
> button exists, but in other places you explicitly check for the expand button
to
> not be null.  Why?

The expand button waits for the native library to be ready to check if it's
needed before setting it. I moved the expand button to always be set in
onFinishInflate and removed the checks.

https://codereview.chromium.org/2853583002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:406:
float previousAlpha = 0.f;
On 2017/04/30 01:56:00, slow (dfalcantara) wrote:
> Where do you actually use this?

I don't any more. Removed.

[jwd, 2017-05-01 20:59:11]

Are there existing metrics for when home is activated? Is there value in having
something specifically logged when this button is pressed?

[mdjones, 2017-05-01 21:21:49]

On 2017/05/01 20:59:11, jwd wrote:
> Are there existing metrics for when home is activated? Is there value in
having
> something specifically logged when this button is pressed?

This mostly exists as a UX toy, but I added an extra open reason in our metrics
to track its usage.

[jwd, 2017-05-01 21:33:56]

Thanks!

Metrics LGTM

[gone, 2017-05-02 17:25:07]

lgtm

https://codereview.chromium.org/2853583002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:322:
mBottomSheet.onExpandButtonPressed();
if (mBottomSheet != null) mBottomSheet.onExpandButtonPressed();?

[mdjones, 2017-05-02 18:46:10]

https://codereview.chromium.org/2853583002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2853583002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:322:
mBottomSheet.onExpandButtonPressed();
On 2017/05/02 17:25:07, slow (dfalcantara) wrote:
> if (mBottomSheet != null) mBottomSheet.onExpandButtonPressed();?

Done.

[mdjones, 2017-05-02 18:46:16]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-05-02 18:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, jwd@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2853583002/#ps200001
(title: "nit")

[commit-bot: I haz the power, 2017-05-02 18:47:32]

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-05-02 19:51:56]

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

[commit-bot: I haz the power, 2017-05-02 19:51:57]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:19
  Falling back to three-way merge...
  Applied patch to
'chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java'
with conflicts.
  U
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
index
818de37bf0b0c850204a2f1651b167cc8835981d..0f1a08e4cdc31b5597742621a32898d37807bcb6
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
@@ -5,10 +5,12 @@
 package org.chromium.chrome.browser.toolbar;
 
 import android.content.Context;
+import android.content.res.ColorStateList;
 import android.graphics.Canvas;
 import android.graphics.Rect;
 import android.graphics.drawable.Drawable;
 import android.os.Build;
+import android.os.SystemClock;
 import android.support.v7.widget.Toolbar;
 import android.util.AttributeSet;
 import android.view.View;
@@ -19,6 +21,8 @@ import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.SysUtils;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.util.ColorUtils;
+import org.chromium.chrome.browser.util.FeatureUtilities;
+import org.chromium.chrome.browser.widget.TintedImageButton;
 import org.chromium.chrome.browser.widget.bottomsheet.BottomSheet;
 import org.chromium.chrome.browser.widget.bottomsheet.BottomSheetMetrics;
 import org.chromium.chrome.browser.widget.bottomsheet.BottomSheetObserver;
@@ -88,6 +92,9 @@ public class BottomToolbarPhone extends ToolbarPhone {
     /** A handle to the bottom sheet. */
     private BottomSheet mBottomSheet;
 
+    /** A handle to the expand button that Chrome Home may or may not use. */
+    private TintedImageButton mExpandButton;
+
     /**
      * Whether the end toolbar buttons should be hidden regardless of whether
the URL bar is
      * focused.
@@ -105,6 +112,9 @@ public class BottomToolbarPhone extends ToolbarPhone {
     /** Whether accessibility is enabled. */
     private boolean mAccessibilityEnabled;
 
+    /** Whether or not the toolbar handle should be used. */
+    private boolean mUseToolbarHandle;
+
     /**
      * Constructs a BottomToolbarPhone object.
      * @param context The Context in which this View object is created.
@@ -119,6 +129,7 @@ public class BottomToolbarPhone extends ToolbarPhone {
                 context.getResources(), R.drawable.toolbar_handle_light);
         mLocationBarVerticalMargin =
                
getResources().getDimensionPixelOffset(R.dimen.bottom_location_bar_vertical_margin);
+        mUseToolbarHandle = true;
     }
 
     /**
@@ -126,6 +137,7 @@ public class BottomToolbarPhone extends ToolbarPhone {
      * @param useLightDrawable If the handle color should be light.
      */
     public void updateHandleTint(boolean useLightDrawable) {
+        if (!mUseToolbarHandle) return;
         mToolbarHandleView.setImageDrawable(useLightDrawable ? mHandleLight :
mHandleDark);
     }
 
@@ -210,13 +222,43 @@ public class BottomToolbarPhone extends ToolbarPhone {
      *         correctly offset them from the handle that sits above them.
      */
     private int getExtraTopMargin() {
+        if (!mUseToolbarHandle) return 0;
         return
getResources().getDimensionPixelSize(R.dimen.bottom_toolbar_top_margin);
     }
 
     @Override
+    protected int getBoundsAfterAccountingForLeftButton() {
+        int padding = super.getBoundsAfterAccountingForLeftButton();
+        if (!mUseToolbarHandle && mExpandButton.getVisibility() != GONE) {
+            padding = mExpandButton.getMeasuredWidth();
+        }
+        return padding;
+    }
+
+    @Override
+    public void updateButtonVisibility() {
+        super.updateButtonVisibility();
+        if (!mUseToolbarHandle) {
+            mExpandButton.setVisibility(
+                    urlHasFocus() || isTabSwitcherAnimationRunning() ?
INVISIBLE : VISIBLE);
+        }
+    }
+
+    @Override
+    protected void updateUrlExpansionAnimation() {
+        super.updateUrlExpansionAnimation();
+
+        if (!mUseToolbarHandle) {
+            mExpandButton.setVisibility(mShouldHideEndToolbarButtons ?
View.GONE : View.VISIBLE);
+        }
+    }
+
+    @Override
     public void onFinishInflate() {
         super.onFinishInflate();
 
+        mExpandButton = (TintedImageButton)
findViewById(R.id.expand_sheet_button);
+
         // Add extra top margin to the URL bar to compensate for the change to
location bar's
         // vertical margin in the constructor.
         ((MarginLayoutParams)
mLocationBar.findViewById(R.id.url_bar).getLayoutParams()).topMargin =
@@ -226,6 +268,14 @@ public class BottomToolbarPhone extends ToolbarPhone {
         // visibility from changing during transitions.
         mBrowsingModeViews.remove(mLocationBar);
 
+        updateToolbarTopMargin();
+    }
+
+    /**
+     * Update the top margin of all the components inside the toolbar. If the
toolbar handle is
+     * being used, extra margin is added.
+     */
+    private void updateToolbarTopMargin() {
         // Programmatically apply a top margin to all the children of the
toolbar container. This
         // is done so the view hierarchy does not need to be changed.
         int topMarginForControls = getExtraTopMargin();
@@ -247,24 +297,61 @@ public class BottomToolbarPhone extends ToolbarPhone {
     public void onAttachedToWindow() {
         super.onAttachedToWindow();
 
-        // The toolbar handle is part of the control container so it can draw
on top of the other
-        // toolbar views. Get the root view and search for the handle.
+        // The toolbar handle is part of the control container so it can draw
on top of the
+        // other toolbar views, this way there is only a single handle instead
of each having its
+        // own. Get the root view and search for the handle.
         mToolbarHandleView = (ImageView)
getRootView().findViewById(R.id.toolbar_handle);
         mToolbarHandleView.setImageDrawable(mHandleDark);
     }
 
     @Override
+    public void onNativeLibraryReady() {
+        super.onNativeLibraryReady();
+
+        mUseToolbarHandle =
!FeatureUtilities.isChromeHomeExpandButtonEnabled();
+
+        if (!mUseToolbarHandle) initExpandButton();
+    }
+
+    /**
+     * Initialize the "expand" button if it is being used.
+     */
+    private void initExpandButton() {
+        mLocationBarVerticalMargin =
+               
getResources().getDimensionPixelOffset(R.dimen.location_bar_vertical_margin);
+
+        mToolbarHandleView.setVisibility(View.GONE);
+
+        mExpandButton.setOnClickListener(new OnClickListener() {
+            @Override
+            public void onClick(View v) {
+                if (mBottomSheet != null) mBottomSheet.onExpandButtonPressed();
+            }
+        });
+
+        mExpandButton.setVisibility(View.VISIBLE);
+        mBrowsingModeViews.add(mExpandButton);
+
+        updateToolbarTopMargin();
+    }
+
+    @Override
     protected void updateVisualsForToolbarState() {
         super.updateVisualsForToolbarState();
 
         // TODO(mdjones): Creating a new tab from the tab switcher skips the
         // drawTabSwitcherFadeAnimation which would otherwise make this line
unnecessary.
-        if (mTabSwitcherState == STATIC_TAB) mToolbarHandleView.setAlpha(1f);
+        if (mTabSwitcherState == STATIC_TAB && mUseToolbarHandle)
mToolbarHandleView.setAlpha(1f);
 
         // The tab switcher's background color should not affect the toolbar
handle; it should only
         // switch color based on the static tab's theme color. This is done so
fade in/out looks
         // correct.
-        mToolbarHandleView.setImageDrawable(isLightTheme() ? mHandleDark :
mHandleLight);
+        if (mUseToolbarHandle) {
+            mToolbarHandleView.setImageDrawable(isLightTheme() ? mHandleDark :
mHandleLight);
+        } else {
+            ColorStateList tint = mUseLightToolbarDrawables ? mLightModeTint :
mDarkModeTint;
+            mExpandButton.setTint(tint);
+        }
     }
 
     @Override
@@ -285,7 +372,7 @@ public class BottomToolbarPhone extends ToolbarPhone {
         mNewTabButton.setAlpha(progress);
 
         mLocationBar.setAlpha(1f - progress);
-        mToolbarHandleView.setAlpha(1f - progress);
+        if (mUseToolbarHandle) mToolbarHandleView.setAlpha(1f - progress);
 
         int tabSwitcherThemeColor =
getToolbarColorForVisualState(VisualState.TAB_SWITCHER_NORMAL);
 
@@ -316,7 +403,12 @@ public class BottomToolbarPhone extends ToolbarPhone {
     protected void drawTabSwitcherAnimationOverlay(Canvas canvas, float
animationProgress) {
         // Intentionally overridden to block everything but the compositor
screen shot. Otherwise
         // the toolbar in Chrome Home does not have an animation overlay
component.
-        if (mTextureCaptureMode) super.drawTabSwitcherAnimationOverlay(canvas,
0f);
+        if (mTextureCaptureMode) {
+            super.drawTabSwitcherAnimationOverlay(canvas, 0f);
+            if (!mUseToolbarHandle && mExpandButton.getVisibility() !=
View.GONE) {
+                drawChild(canvas, mExpandButton, SystemClock.uptimeMillis());
+            }
+        }
     }
 
     @Override

[mdjones, 2017-05-02 20:31:16]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-05-02 20:31:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, jwd@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2853583002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-02 20:31:37]

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-05-02 21:34:50]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493757076005840,
"parent_rev": "989c0769083cd9dd898eb5f3c7ba9993d3877cd9", "commit_rev":
"d484eacb1a1016a1f2fda3bc57b8e309fd08df76"}

[commit-bot: I haz the power, 2017-05-02 21:35:02]

Description was changed from

==========
[Home] Add expand button and flag to enable it

This change adds an alternate UI for the Chrome Home bottom sheet.
Instead of being able to swipe up when the sheet is peeking, there is
the option to have a button that sits to the left of the omnibox that
expands the sheet when pressed. The feature name for this UI is
"ChromeHomeExpandButton".

This flag runs into the same problems as Chrome Home when first
being enabled -- native is not ready when UI is initialized. To
avoid yet another Chrome restart, the pull-handle version of the UI
will appear briefly before switching to the button.

BUG=716220
==========

to

==========
[Home] Add expand button and flag to enable it

This change adds an alternate UI for the Chrome Home bottom sheet.
Instead of being able to swipe up when the sheet is peeking, there is
the option to have a button that sits to the left of the omnibox that
expands the sheet when pressed. The feature name for this UI is
"ChromeHomeExpandButton".

This flag runs into the same problems as Chrome Home when first
being enabled -- native is not ready when UI is initialized. To
avoid yet another Chrome restart, the pull-handle version of the UI
will appear briefly before switching to the button.

BUG=716220

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

[commit-bot: I haz the power, 2017-05-02 21:35:03]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/d484eacb1a1016a1f2fda3bc57b8...