🏠 Add pull-handle to bottom toolbar

[Home] Add pull-handle to bottom toolbar

This change adds a pill-shaped handle to the bottom toolbar for Chrome
Home indicating that it can be pulled up. To avoid refactoring large
sections of code this is done in a relatively unorthodox way. The
height of the toolbar needs to increase but the view hierarchy is
uncondusive to change. So instead of wrapping the toolbar controls
in a bottom-aligned view, a top margin is programmatically added when
the views are inflated.

The handle itself is programatically drawn and exists as an immediate
child of the ViewResourceFrameLayout. This allows it to be captured in
the screenshot used by the compositor and for it to be drawn on top
of the other toolbar views (particularly the location bar). Both a
normal and incognito version of the handle are maintained by the
BottomToolbarPhone class.

BUG=692865
Review-Url: https://codereview.chromium.org/2744763005
Cr-Commit-Position: refs/heads/master@{#457299}
Committed: https://chromium.googlesource.com/chromium/src/+/0f3941c30bdb81dcb934d6a12d84859eedebff0e

[mdjones, 2017-03-13 20:14:43]

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

[mdjones, 2017-03-13 20:14:44]

Well, the good news is I have most of the code isolated in BottomToolbarPhone,
the bad news is that it isn't exactly pretty. ptal

[mdjones, 2017-03-13 20:57:52]

Description was changed from

==========
[Home] Add pull-handle to bottom toolbar

This change adds a pill-shaped handle to the bottom toolbar for Chrome
Home indicating that it can be pulled up. To avoid refactoring large
sections of code this is done in a relatively unorthodox way. The
height of the toolbar needs to increase but the view hierarchy is
uncondusive to change. So instead of wrapping the toolbar controls
in a bottom-aligned view, a top margin is programmatically added when
the views are inflated.

The handle itself is programatically drawn and exists as an immediate
child of the ViewResourceFrameLayout. This allows it to be captured in
the screenshot used by the compositor and for it to be drawn on top
of the other toolbar views (particularly the location bar). Both a
normal and incognito version of the handle are maintained by the
BottomToolbarPhone class.

BUG=692865
==========

to

==========
[Home] Add pull-handle to bottom toolbar

This change adds a pill-shaped handle to the bottom toolbar for Chrome
Home indicating that it can be pulled up. To avoid refactoring large
sections of code this is done in a relatively unorthodox way. The
height of the toolbar needs to increase but the view hierarchy is
uncondusive to change. So instead of wrapping the toolbar controls
in a bottom-aligned view, a top margin is programmatically added when
the views are inflated.

The handle itself is programatically drawn and exists as an immediate
child of the ViewResourceFrameLayout. This allows it to be captured in
the screenshot used by the compositor and for it to be drawn on top
of the other toolbar views (particularly the location bar). Both a
normal and incognito version of the handle are maintained by the
BottomToolbarPhone class.

BUG=692865
==========

[Theresa, 2017-03-13 23:08:42]

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:30: <color
name="toolbar_handle_color">#61000000</color>
Are the pull handle greys close enough to one of the other shades of grey
already defined or do we really need new colors?

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

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:147:
getResources().getDimensionPixelSize(R.dimen.toolbar_shadow_height);
Can toolbar_handle_margin_top be its current value + toolbar_shadow_height?

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:157:
mToolbarHandleView.setImageBitmap(mUseLightToolbarDrawables ? mHandleLight :
mHandleDark);
We'll also likely need to change the pull handle bitmap when selection is
enabled in the other toolbar views. I'm trying to get an answer from UX.. Not
sure that this patch needs to change, just something to consider.

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

https://codereview.chromium.org/2744763005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:35:
android:layout_marginTop="@dimen/toolbar_shadow_height" />
What happens if we adjust this margin rather than programatically push down all
of the toolbar items?

[mdjones, 2017-03-13 23:43:39]

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:30: <color
name="toolbar_handle_color">#61000000</color>
On 2017/03/13 23:08:42, Theresa wrote:
> Are the pull handle greys close enough to one of the other shades of grey
> already defined or do we really need new colors?

google_grey_700 would work but it lacks the alpha component. I'll ping UX.

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

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:147:
getResources().getDimensionPixelSize(R.dimen.toolbar_shadow_height);
On 2017/03/13 23:08:42, Theresa wrote:
> Can toolbar_handle_margin_top be its current value + toolbar_shadow_height?

Unfortunately no, the margin from the top of the toolbar background is used
elsewhere. I can add another unique dimension for it, but it would be
single-use. I'll do whichever you prefer.

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:157:
mToolbarHandleView.setImageBitmap(mUseLightToolbarDrawables ? mHandleLight :
mHandleDark);
On 2017/03/13 23:08:42, Theresa wrote:
> We'll also likely need to change the pull handle bitmap when selection is
> enabled in the other toolbar views. I'm trying to get an answer from UX.. Not
> sure that this patch needs to change, just something to consider.

When we need that functionality, we can add a method to manually set the handle
color. As long as updateVisualsForToolbarState isn't constantly being call it
should be pretty straight forward.

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

https://codereview.chromium.org/2744763005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:35:
android:layout_marginTop="@dimen/toolbar_shadow_height" />
On 2017/03/13 23:08:42, Theresa wrote:
> What happens if we adjust this margin rather than programatically push down
all
> of the toolbar items?

All the contents try to adjust to the new height. I would set the size of the
toolbar container to be smaller but then the background is too short.

[Theresa, 2017-03-14 00:01:45]

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:30: <color
name="toolbar_handle_color">#61000000</color>
On 2017/03/13 23:43:39, mdjones wrote:
> On 2017/03/13 23:08:42, Theresa wrote:
> > Are the pull handle greys close enough to one of the other shades of grey
> > already defined or do we really need new colors?
> 
> google_grey_700 would work but it lacks the alpha component. I'll ping UX.

Depending on what Chris says, maybe we can use an existing alpha + black e.g.
4D000000 is ~30%. If nothing existing is close enough, let's at least push for a
round alpha number that's more likely to be reused e.g. 40%

#80FFFFFF we use in a few other places (e.g. light_mode_tint.xml) and we'll need
it for selection mode too (just asked Chris), so I think we could define
something like semi_opaque_white and re-use the color.

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

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:147:
getResources().getDimensionPixelSize(R.dimen.toolbar_shadow_height);
On 2017/03/13 23:43:39, mdjones wrote:
> On 2017/03/13 23:08:42, Theresa wrote:
> > Can toolbar_handle_margin_top be its current value + toolbar_shadow_height?
> 
> Unfortunately no, the margin from the top of the toolbar background is used
> elsewhere. I can add another unique dimension for it, but it would be
> single-use. I'll do whichever you prefer.

This is setting the margin on the handle, which uses a new dimension
(toolbar_handle_margin_top), right? I was suggesting that we update that one. It
doesn't appear to be used anywhere else.

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

https://codereview.chromium.org/2744763005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:35:
android:layout_marginTop="@dimen/toolbar_shadow_height" />
On 2017/03/13 23:43:39, mdjones wrote:
> On 2017/03/13 23:08:42, Theresa wrote:
> > What happens if we adjust this margin rather than programatically push down
> all
> > of the toolbar items?
> 
> All the contents try to adjust to the new height. I would set the size of the
> toolbar container to be smaller but then the background is too short.

Okay -- so we may have to programatically adjust the margin on all of the
various toolbar's button? Maybe we can wrap the toolbar button and apply the
margin to one view/toolbar?

[mdjones, 2017-03-14 16:25:28]

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:30: <color
name="toolbar_handle_color">#61000000</color>
On 2017/03/14 00:01:44, Theresa wrote:
> On 2017/03/13 23:43:39, mdjones wrote:
> > On 2017/03/13 23:08:42, Theresa wrote:
> > > Are the pull handle greys close enough to one of the other shades of grey
> > > already defined or do we really need new colors?
> > 
> > google_grey_700 would work but it lacks the alpha component. I'll ping UX.
> 
> Depending on what Chris says, maybe we can use an existing alpha + black e.g.
> 4D000000 is ~30%. If nothing existing is close enough, let's at least push for
a
> round alpha number that's more likely to be reused e.g. 40%
> 
> #80FFFFFF we use in a few other places (e.g. light_mode_tint.xml) and we'll
need
> it for selection mode too (just asked Chris), so I think we could define
> something like semi_opaque_white and re-use the color.

Added semi_opaque_white and switched to using google_grey_500 for normal tabs.

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

https://codereview.chromium.org/2744763005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:147:
getResources().getDimensionPixelSize(R.dimen.toolbar_shadow_height);
On 2017/03/14 00:01:44, Theresa wrote:
> On 2017/03/13 23:43:39, mdjones wrote:
> > On 2017/03/13 23:08:42, Theresa wrote:
> > > Can toolbar_handle_margin_top be its current value +
toolbar_shadow_height?
> > 
> > Unfortunately no, the margin from the top of the toolbar background is used
> > elsewhere. I can add another unique dimension for it, but it would be
> > single-use. I'll do whichever you prefer.
> 
> This is setting the margin on the handle, which uses a new dimension
> (toolbar_handle_margin_top), right? I was suggesting that we update that one.
It
> doesn't appear to be used anywhere else.

Ah yes, you're right. I got one of the other dimensions confused. Done.

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

https://codereview.chromium.org/2744763005/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:35:
android:layout_marginTop="@dimen/toolbar_shadow_height" />
On 2017/03/14 00:01:45, Theresa wrote:
> On 2017/03/13 23:43:39, mdjones wrote:
> > On 2017/03/13 23:08:42, Theresa wrote:
> > > What happens if we adjust this margin rather than programatically push
down
> > all
> > > of the toolbar items?
> > 
> > All the contents try to adjust to the new height. I would set the size of
the
> > toolbar container to be smaller but then the background is too short.
> 
> Okay -- so we may have to programatically adjust the margin on all of the
> various toolbar's button? Maybe we can wrap the toolbar button and apply the
> margin to one view/toolbar?

I can, but I would need to make two copies of what is now
toolbar_phone_common.xml; the buttons/location bar/background are all part of
the same layout.

[gone, 2017-03-14 20:02:36]

Screenshots?

[mdjones, 2017-03-14 20:36:43]

On 2017/03/14 20:02:36, dfalcantara (load balance plz) wrote:
> Screenshots?

https://bugs.chromium.org/p/chromium/issues/detail?id=692865#c5

[gone, 2017-03-14 22:38:41]

lgtm

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

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:129:
// Skip the shadow that sits at the top of the toolbar.
Add why.

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:174:

I guess you really like newlines, but please remove this one.

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

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:111:
if (mProgressBar == null) {
Does this really have to be here?  onLayout seems like an odd place to add a new
View to the hierarchy.

[Theresa, 2017-03-15 15:35:47]

lgtm

[mdjones, 2017-03-15 17:26:12]

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

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:129:
// Skip the shadow that sits at the top of the toolbar.
On 2017/03/14 22:38:40, dfalcantara (load balance plz) wrote:
> Add why.

Done.

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:174:

On 2017/03/14 22:38:40, dfalcantara (load balance plz) wrote:
> I guess you really like newlines, but please remove this one.

Done.

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

https://codereview.chromium.org/2744763005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java:111:
if (mProgressBar == null) {
On 2017/03/14 22:38:40, dfalcantara (load balance plz) wrote:
> Does this really have to be here?  onLayout seems like an odd place to add a
new
> View to the hierarchy.

This is the result of a bad rebase. The latest patch set excludes all these
changes.

[mdjones, 2017-03-15 22:35:40]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-03-15 22:35:40]

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

[commit-bot: I haz the power, 2017-03-15 22:36:07]

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-15 23:23:12]

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

[commit-bot: I haz the power, 2017-03-15 23:23:13]

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

[mdjones, 2017-03-16 00:07:39]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-03-16 00:07:40]

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

[commit-bot: I haz the power, 2017-03-16 00:08: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-03-16 01:21:36]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489622859870580,
"parent_rev": "81035a93c651f680b1bf0d6c8241128d947d8ac7", "commit_rev":
"0f3941c30bdb81dcb934d6a12d84859eedebff0e"}

[commit-bot: I haz the power, 2017-03-16 01:22:32]

Description was changed from

==========
[Home] Add pull-handle to bottom toolbar

This change adds a pill-shaped handle to the bottom toolbar for Chrome
Home indicating that it can be pulled up. To avoid refactoring large
sections of code this is done in a relatively unorthodox way. The
height of the toolbar needs to increase but the view hierarchy is
uncondusive to change. So instead of wrapping the toolbar controls
in a bottom-aligned view, a top margin is programmatically added when
the views are inflated.

The handle itself is programatically drawn and exists as an immediate
child of the ViewResourceFrameLayout. This allows it to be captured in
the screenshot used by the compositor and for it to be drawn on top
of the other toolbar views (particularly the location bar). Both a
normal and incognito version of the handle are maintained by the
BottomToolbarPhone class.

BUG=692865
==========

to

==========
[Home] Add pull-handle to bottom toolbar

This change adds a pill-shaped handle to the bottom toolbar for Chrome
Home indicating that it can be pulled up. To avoid refactoring large
sections of code this is done in a relatively unorthodox way. The
height of the toolbar needs to increase but the view hierarchy is
uncondusive to change. So instead of wrapping the toolbar controls
in a bottom-aligned view, a top margin is programmatically added when
the views are inflated.

The handle itself is programatically drawn and exists as an immediate
child of the ViewResourceFrameLayout. This allows it to be captured in
the screenshot used by the compositor and for it to be drawn on top
of the other toolbar views (particularly the location bar). Both a
normal and incognito version of the handle are maintained by the
BottomToolbarPhone class.

BUG=692865

Review-Url: https://codereview.chromium.org/2744763005
Cr-Commit-Position: refs/heads/master@{#457299}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f3941c30bdb81dcb934d6a12d84...
==========

[commit-bot: I haz the power, 2017-03-16 01:22:34]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0f3941c30bdb81dcb934d6a12d84...