[Custom Tabs] Implement Bottombar

[Custom Tabs] Implement Bottombar

This CL adds several extras to CustomTabsIntent so that clients are able
to customize a new UI component of Chrome Custom Tabs: bottombar. Adding
a button to bottombar is very similar to adding an action button, except
that an ID is required.

UMA stats, updateVisuals() related work will be added shortly.

BUG=573683
Committed: https://crrev.com/ab809fd870ffbed0e6b8e686bef61df30d0f8d9a
Cr-Commit-Position: refs/heads/master@{#368288}

[Ian Wen, 2016-01-02 14:20:39]

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

[Ian Wen, 2016-01-02 14:20:39]

PTAL. :)

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

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/main.xml:32: android:layout_marginTop="-2dp"
-2dp top margin prevents the shadow from consuming any space in the linear
layout. Similar techniques were also used in eb_add_edit_folder_activity.xml

A programmatic fading shadow is preferred against nine patches because this time
the shadow grows from bottom to top.

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/and...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/and...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:95: *
Extra bitmap that specifies the icon of the back button on the toolbar. If the
client chooses
These five constants are moved from CustomTabsIntentDataProvider.

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
(right):

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:23:
public class ActionButtonParams extends ButtonParams {
Some common attributes of ActionButtonParams are abstracted away to its base
class, ButtonParams.

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/BottombarItemParams.java
(right):

https://codereview.chromium.org/1557803002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/BottombarItemParams.java:32:
public class BottombarItemParams extends ButtonParams {
This should have been package protected. Fixed locally.

[Yusuf, 2016-01-04 22:28:42]

I will go through details soon, but I think there should be a corresponding API
side CL if only for debating over it. Maybe in the thirdparty/customtabsclient/
?

What API will add the new bundle extras? Will that be different from
setActionButton? How will the two APIs interact with each other? 

If I keep calling setActionButton, will that keep overriding the one button up
top or keep adding the other buttons to the bottom bar? Not really have strong
opinions about any of these yet, but I would like to hear your thoughts on them
:)

[Ian Wen, 2016-01-06 05:50:15]

Dear yusufo@, please take a look at the second patchset. Our side-by-side diff
behaves weirdly for ActionButtonParams. I recommend you use single column diff
for that file. :P

Corresponding client change: https://codereview.chromium.org/1568433003

[Yusuf, 2016-01-06 06:51:14]

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

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/main.xml:36: android:id="@+id/buttombar_stub"
bottombar

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
(right):

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:118:
// TODO(ianwen): add UMA for botton clicking.
button

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:139:
List<ActionButtonParams> result = new ArrayList<>(1);
paramsList

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:145:
CustomTabsIntent.EXTRA_ACTION_BUTTON_BUNDLE);
wrong extra here

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:178:
CustomTabsIntent.TOOLBAR_ACTION_BUTTON_ID);
This will misbehave if the client doesn't add any ID. We should set the
toolbar_action_button_id more explicitly and assign a dummy ID if there is none.
If there are no ID, there shouldnt be any buttons.

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:196:
Log.w(TAG, "Button's icon not suitable for toolbar, push it to bottom bar
instead."
s/push/putting

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1557803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:121:
private void retrieveActionButtons(Intent intent, Context context) {
lets say buttons instead. ActionButton is a remnant from CustomActionButton and
without Custom it is not meaningful. So lets say CustomButtonParams and
CustomButtons instead (except where it is already present in the public API)

[Yusuf, 2016-01-06 07:31:22]

Another question is what happens when the keyboard comes up with a bottom bar on
screen?

[Ian Wen, 2016-01-06 10:51:00]

Thanks for reading the CL in the late night!!

Comments are mostly addressed.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/main.xml (right):

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/res/layout/main.xml:36: android:id="@+id/buttombar_stub"
On 2016/01/06 06:51:14, Yusuf wrote:
> bottombar

In this file all stubs are named as "ABC_stub", and when they are inflated,
their id becomes "ABC". Maybe we should follow the conventions here? :)

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:118:
// TODO(ianwen): add UMA for botton clicking.
On 2016/01/06 06:51:14, Yusuf wrote:
> button

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:139:
List<ActionButtonParams> result = new ArrayList<>(1);
On 2016/01/06 06:51:14, Yusuf wrote:
> paramsList

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:145:
CustomTabsIntent.EXTRA_ACTION_BUTTON_BUNDLE);
On 2016/01/06 06:51:14, Yusuf wrote:
> wrong extra here

hmm this is intentional. The client now is able to pass either a single bundle
(the old way) or a list of bundles (the new way) through the same extra.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:178:
CustomTabsIntent.TOOLBAR_ACTION_BUTTON_ID);
On 2016/01/06 06:51:14, Yusuf wrote:
> This will misbehave if the client doesn't add any ID. We should set the
> toolbar_action_button_id more explicitly and assign a dummy ID if there is
none.
> If there are no ID, there shouldnt be any buttons.

Right. In the old API no ID was not included so we should support the old
usecase; in the new API every bundle should contain an id. I added a boolean in
this method to distinguish the two cases. Done.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java:196:
Log.w(TAG, "Button's icon not suitable for toolbar, push it to bottom bar
instead."
On 2016/01/06 06:51:14, Yusuf wrote:
> s/push/putting

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:121:
private void retrieveActionButtons(Intent intent, Context context) {
On 2016/01/06 06:51:14, Yusuf wrote:
> lets say buttons instead. ActionButton is a remnant from CustomActionButton
and
> without Custom it is not meaningful. So lets say CustomButtonParams and
> CustomButtons instead (except where it is already present in the public API)

Can't agree more. Done.

[Yusuf, 2016-01-06 23:02:55]

Another general question:

Right now although we are careful about the toolbar button being too wide, but
don't check the bottom bar's for exceeding the total width. I think now this
code will just crop from the sides, right? Probably we should just not show
after a certain point.

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/main.xml:36: android:id="@+id/buttombar_stub"
bottombar_stub

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

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:17: <color
name="toolbar_shadow_color">#1D000000</color>
Let's not try to combine these two and declare a new one instead. Toolbar is too
generic and it will be confusing in the long run. Keep the old one and declare a
new bottom_bar_shadow_color

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(left):

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:52:
"android.support.customtabs.extra.ACTION_BUTTON_BUNDLE";
I do understand why we are doing this, but will have to check this first. For
now, let's say this is OK.

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
(right):

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:41:
protected Bitmap mIcon;
protected static final
protected final
protected

private final
private

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:85:
if (mShouldTint) return TintedDrawable.constructTintedDrawable(res, mIcon);
if there is an else, there is always braces.

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:152:
} else if (bundleList != null) {
what if both are non null? This seems to be ignoring that case.

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:255:
// TODO(ianwen): support bottombar here.
so right now, we can't change icons on the bottom bar, is that right?

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1557803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:125:
if (params.showOnToolbar()) mToolbarButton = params;
if else with braces

[Ian Wen, 2016-01-07 03:09:50]

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/main.xml (right):

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/res/layout/main.xml:36: android:id="@+id/buttombar_stub"
On 2016/01/06 23:02:55, Yusuf wrote:
> bottombar_stub

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/res/values/colors.xml:17: <color
name="toolbar_shadow_color">#1D000000</color>
On 2016/01/06 23:02:55, Yusuf wrote:
> Let's not try to combine these two and declare a new one instead. Toolbar is
too
> generic and it will be confusing in the long run. Keep the old one and declare
a
> new bottom_bar_shadow_color

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:41:
protected Bitmap mIcon;
On 2016/01/06 23:02:55, Yusuf wrote:
> protected static final
> protected final
> protected
> 
> private final
> private

Done. Replaced protected with private and put final variables first.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:85:
if (mShouldTint) return TintedDrawable.constructTintedDrawable(res, mIcon);
On 2016/01/06 23:02:55, Yusuf wrote:
> if there is an else, there is always braces.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:152:
} else if (bundleList != null) {
On 2016/01/06 23:02:55, Yusuf wrote:
> what if both are non null? This seems to be ignoring that case.

If both are null this method returns an empty arraylist, it means the client
does not want to show any buttons. Same empty list will be returned by
IntentDataProvider#getAllCustomButtons() as well.

I removed two redundant return statements in #151 and #165 to make the logic
more clear.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:255:
// TODO(ianwen): support bottombar here.
On 2016/01/06 23:02:55, Yusuf wrote:
> so right now, we can't change icons on the bottom bar, is that right?

In this CL it is not implemented. Will upload a follow-up CL today to finish it.

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:125:
if (params.showOnToolbar()) mToolbarButton = params;
On 2016/01/06 23:02:55, Yusuf wrote:
> if else with braces

Done.

[Ian Wen, 2016-01-07 03:10:04]

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

[commit-bot: I haz the power, 2016-01-07 03:10:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1557803002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1557803002/100001

[commit-bot: I haz the power, 2016-01-07 03:47:29]

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

[commit-bot: I haz the power, 2016-01-07 03:47:30]

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

[Ian Wen, 2016-01-07 04:10:27]

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

[commit-bot: I haz the power, 2016-01-07 04:10:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1557803002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1557803002/120001

[commit-bot: I haz the power, 2016-01-07 04:54:26]

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

[commit-bot: I haz the power, 2016-01-07 04:54:27]

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

[Yusuf, 2016-01-07 07:55:24]

lgtm

[Yusuf, 2016-01-07 18:24:38]

onto dfalcantara@ for final approval

[gone, 2016-01-07 20:16:33]

Mostly nits.  Will lgtm to prevent another world rotation, but please address
the comments.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/res/values/colors.xml:171: <color
name="bottom_bar_shadow_color">#1D000000</color>
nit: lowercase

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:102: *
not to customize it, a default close button will be used.
Back button or close button?  You flip between the two.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:111:
public static final String EXTRA_TITLE_VISIBILITY_STATE =
TITLE_VISIBILITY_STATE or TITLE_VISIBILITY?  Inconsistent here.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:142:
public static final String KEY_ID =
"android.support.customtabs.customaction.ID";
The extras above have .extra.; does this need a .key. for clarity?

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:45:
private boolean mOnToolbar;
nit: mIsOnToolbar?

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:47:
private CustomButtonParams(int id, @NonNull Bitmap icon, @NonNull String
description,
Flip this to say that things are @Nullable instead.  Not only is it more
frequently used in the codebase, but it also makes it more clear which variables
should be null checked (e.g. if something is @Nullable => null check it VS if
something is NOT marked @NonNull => null check it).

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:82:
*         {@link TintedDrawable}
Is the TintedDrawable bit important for the callers to know?  It's still a
Drawable, so I'd just take out the second part of the comment.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:111:
ImageButton toBottomBarButton(Context context, ViewGroup parent) {
Not a fan of the function name.  You're not really converting parameters --
you're building a button from them.

buildBottomBarButton()?
buildButtonForBottomBar()?

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:139:
* @return A list of parsed {@link CustomButtonParams}. Return an empty list if
input is invalid
maybe:

@return Parsed list of {@link CustomButtonParams}, which is empty if the input
is invalid.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:173:
* into toolbar, it will be put to bottom bar.
fit into the toolbar

put into the bottom bar.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:222:
static Bitmap tryParseBitmapFromBundle(Bundle bundle) {
I think parseBitmapFromBundle() is clearer.  If it fails, then you're already
passing back null and tell people to worry about it in the JavaDoc.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:233:
static String tryParseDescriptionFromBundle(Bundle bundle) {
Same here.

Also, why aren't you null checking the bundle here?  You do it above.  Also,
does this need to be exposed, or can it be private static like above?

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:239:
private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap
bitmap) {
nit: isCustomButtonIconWithinBounds?

This function needs a bit more explanation on what it does.  Can't suggest a
logical name for the function because I'm not even sure how "bounds" are defined
here.  Case in point: you return false if the bitmap isn't at least as big as
"toolbar_icon_height", but there's no upper limit.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:470:
private void showBottombarIfNecessary() {
Strongly encouraging you to settle on a single capitalization scheme for
"bottombar" to avoid the webapp/WebApp/Webapp issues I've had to deal with for
years.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:476:
bottombarStub.setLayoutResource(R.layout.custom_tabs_bottombar);
Makes more sense to define the android:layout attribute in the XML instead of
doing it here, I think.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:211:
*         buttons, returns <code>null</code>.
Eh, you don't use <code> anywhere else null is mentioned.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:502:
final int itemNum = 3;
numItems?

[Ian Wen, 2016-01-08 05:54:46]

Except for changes in the public APIs in CustomTabsIntent.java, all comments
addressed. :)

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File chrome/android/java/res/values/colors.xml (right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/res/values/colors.xml:171: <color
name="bottom_bar_shadow_color">#1D000000</color>
On 2016/01/07 20:16:32, dfalcantara wrote:
> nit: lowercase

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:102: *
not to customize it, a default close button will be used.
On 2016/01/07 20:16:32, dfalcantara wrote:
> Back button or close button?  You flip between the two.

Ah sorry for the confusion. From EXTRA_CLOSE_BUTTON_ICON to
EXTRA_TINT_ACTION_BUTTON, these 5 constants are moved from
CustomTabIntentDataProvider to CustomTabsIntent for consistency with the support
library.

Since those are established APIs we probably should not change them. :) I only
changed their position in chromium. In support library all constants are living
in this file already.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:111:
public static final String EXTRA_TITLE_VISIBILITY_STATE =
On 2016/01/07 20:16:32, dfalcantara wrote:
> TITLE_VISIBILITY_STATE or TITLE_VISIBILITY?  Inconsistent here.

Same reason above. Old API. :)

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/android/support/customtabs/CustomTabsIntent.java:142:
public static final String KEY_ID =
"android.support.customtabs.customaction.ID";
On 2016/01/07 20:16:32, dfalcantara wrote:
> The extras above have .extra.; does this need a .key. for clarity?

Traditionally KEY_**, such KEY_ICON or KEY_PENDING_INTENT, do not have "key" in
their content. So KEY is a bit different than EXTRA here. :)

(We don't have final word of what the APIs will look like. It is possible that
during Android review people in support library will ask us to modify them.)

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:45:
private boolean mOnToolbar;
On 2016/01/07 20:16:33, dfalcantara wrote:
> nit: mIsOnToolbar?

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:47:
private CustomButtonParams(int id, @NonNull Bitmap icon, @NonNull String
description,
On 2016/01/07 20:16:33, dfalcantara wrote:
> Flip this to say that things are @Nullable instead.  Not only is it more
> frequently used in the codebase, but it also makes it more clear which
variables
> should be null checked (e.g. if something is @Nullable => null check it VS if
> something is NOT marked @NonNull => null check it).

Good suggestion. Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:82:
*         {@link TintedDrawable}
On 2016/01/07 20:16:33, dfalcantara wrote:
> Is the TintedDrawable bit important for the callers to know?  It's still a
> Drawable, so I'd just take out the second part of the comment.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:111:
ImageButton toBottomBarButton(Context context, ViewGroup parent) {
On 2016/01/07 20:16:33, dfalcantara wrote:
> Not a fan of the function name.  You're not really converting parameters --
> you're building a button from them.
> 
> buildBottomBarButton()?
> buildButtonForBottomBar()?

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:139:
* @return A list of parsed {@link CustomButtonParams}. Return an empty list if
input is invalid
On 2016/01/07 20:16:33, dfalcantara wrote:
> maybe:
> 
> @return Parsed list of {@link CustomButtonParams}, which is empty if the input
> is invalid.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:173:
* into toolbar, it will be put to bottom bar.
On 2016/01/07 20:16:33, dfalcantara wrote:
> fit into the toolbar
> 
> put into the bottom bar.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:222:
static Bitmap tryParseBitmapFromBundle(Bundle bundle) {
On 2016/01/07 20:16:32, dfalcantara wrote:
> I think parseBitmapFromBundle() is clearer.  If it fails, then you're already
> passing back null and tell people to worry about it in the JavaDoc.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:233:
static String tryParseDescriptionFromBundle(Bundle bundle) {
On 2016/01/07 20:16:33, dfalcantara wrote:
> Same here.
> 
> Also, why aren't you null checking the bundle here?  You do it above.  Also,
> does this need to be exposed, or can it be private static like above?

Renamed. This needs to be exposed to CustomTabsConnection, so it can't be
private...

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java:239:
private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap
bitmap) {
On 2016/01/07 20:16:32, dfalcantara wrote:
> nit: isCustomButtonIconWithinBounds?
> 
> This function needs a bit more explanation on what it does.  Can't suggest a
> logical name for the function because I'm not even sure how "bounds" are
defined
> here.  Case in point: you return false if the bitmap isn't at least as big as
> "toolbar_icon_height", but there's no upper limit.

Yeah I had the same feeling about the name. "Bound" is not clear here.

In a followup CL (https://codereview.chromium.org/1568813002) I renamed this
method to "checkIconFitsToolbar". Now it seems "check" is not the best verb
here. Inferring from  "is" you mentioned, I will rename it to
"doesIconFitsToolbar".

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:470:
private void showBottombarIfNecessary() {
On 2016/01/07 20:16:33, dfalcantara wrote:
> Strongly encouraging you to settle on a single capitalization scheme for
> "bottombar" to avoid the webapp/WebApp/Webapp issues I've had to deal with for
> years.

Renamed all Bottombar to either BottomBar or Bottom Bar. Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:476:
bottombarStub.setLayoutResource(R.layout.custom_tabs_bottombar);
On 2016/01/07 20:16:33, dfalcantara wrote:
> Makes more sense to define the android:layout attribute in the XML instead of
> doing it here, I think.

Acknowledged. All android:layout attributes (weight, width, height, margin) are
already defined in custom_tabs_bottombar_item.xml.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:211:
*         buttons, returns <code>null</code>.
On 2016/01/07 20:16:33, dfalcantara wrote:
> Eh, you don't use <code> anywhere else null is mentioned.

Done.

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://chromiumcodereview.appspot.com/1557803002/diff/120001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:502:
final int itemNum = 3;
On 2016/01/07 20:16:33, dfalcantara wrote:
> numItems?

Done.

[Ian Wen, 2016-01-08 05:57:32]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-01-08 05:57:33]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
dfalcantara@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1557803002/#ps140001 (title: "respond to
dfalcantara@'s comments")

[commit-bot: I haz the power, 2016-01-08 05:57:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1557803002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1557803002/140001

[commit-bot: I haz the power, 2016-01-08 06:56:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-08 06:57:12]

Description was changed from

==========
[Custom Tabs] Implement Bottombar

This CL adds several extras to CustomTabsIntent so that clients are able
to customize a new UI component of Chrome Custom Tabs: bottombar. Adding
a button to bottombar is very similar to adding an action button, except
that an ID is required.

UMA stats, updateVisuals() related work will be added shortly.

BUG=573683
==========

to

==========
[Custom Tabs] Implement Bottombar

This CL adds several extras to CustomTabsIntent so that clients are able
to customize a new UI component of Chrome Custom Tabs: bottombar. Adding
a button to bottombar is very similar to adding an action button, except
that an ID is required.

UMA stats, updateVisuals() related work will be added shortly.

BUG=573683

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

[commit-bot: I haz the power, 2016-01-08 06:57:13]

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