[Custom Tabs] Implement Bottombar

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java

Index: chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
new file mode 100644
index 0000000000000000000000000000000000000000..0a9431d104a9c5c54461a9804361f12b14e963dc
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParams.java
@@ -0,0 +1,246 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.customtabs;
+
+import android.app.PendingIntent;
+import android.app.PendingIntent.CanceledException;
+import android.content.Context;
+import android.content.Intent;
+import android.content.res.Resources;
+import android.graphics.Bitmap;
+import android.graphics.drawable.BitmapDrawable;
+import android.graphics.drawable.Drawable;
+import android.os.Bundle;
+import android.support.annotation.NonNull;
+import android.support.customtabs.CustomTabsIntent;
+import android.text.TextUtils;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.ImageButton;
+
+import org.chromium.base.Log;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.util.IntentUtils;
+import org.chromium.chrome.browser.widget.TintedDrawable;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Container for all parameters related to creating a customizable button.
+ */
+class CustomButtonParams {
+    private static final String TAG = "CustomTabs";
+
+    private final PendingIntent mPendingIntent;
+    private int mId;
+    private Bitmap mIcon;
+    private String mDescription;
+    private boolean mShouldTint;
+    private boolean mOnToolbar;

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

nit: mIsOnToolbar?

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

Done.

+
+    private CustomButtonParams(int id, @NonNull Bitmap icon, @NonNull String description,

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

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

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

Good suggestion. Done.

+            PendingIntent pendingIntent, boolean tinted, boolean onToolbar) {
+        mId = id;
+        mIcon = icon;
+        mDescription = description;
+        mPendingIntent = pendingIntent;
+        mShouldTint = tinted;
+        mOnToolbar = onToolbar;
+    }
+
+    /**
+     * Replaces the current icon and description with new ones.
+     */
+    void update(@NonNull Bitmap icon, @NonNull String description) {
+        mIcon = icon;
+        mDescription = description;
+    }
+
+    /**
+     * @return Whether this button should be shown on the toolbar.
+     */
+    boolean showOnToolbar() {
+        return mOnToolbar;
+    }
+
+    /**
+     * @return The id associated with this button. The custom button on the toolbar always uses
+     *         {@link CustomTabsIntent#TOOLBAR_ACTION_BUTTON_ID} as id.
+     */
+    int getId() {
+        return mId;
+    }
+
+    /**
+     * @return The drawable for the customized button. If the button should be tinted, returns a
+     *         {@link TintedDrawable}

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

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.

[Ian Wen, 2016/01/08 05:54:45]

Done.

+     */
+    Drawable getIcon(Resources res) {
+        if (mShouldTint) {
+            return TintedDrawable.constructTintedDrawable(res, mIcon);
+        } else {
+            return new BitmapDrawable(res, mIcon);
+        }
+    }
+
+    /**
+     * @return The content description for the customized button.
+     */
+    String getDescription() {
+        return mDescription;
+    }
+
+    /**
+    * @return The {@link PendingIntent} that will be sent when user clicks the customized button.
+    */
+    PendingIntent getPendingIntent() {
+        return mPendingIntent;
+    }
+
+    /**
+     * Converts the params to an {@link ImageButton} on the bottom bar.
+     * @param parent The parent that the inflated {@link ImageButton}.
+     * @return The inflated {@link ImageButton}. Returns null if this button is shown in toolbar.
+     */
+    ImageButton toBottomBarButton(Context context, ViewGroup parent) {

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

Not a fan of the function name.  You're not really converting parameters --
you're building a button from them.

buildBottomBarButton()?
buildButtonForBottomBar()?

[Ian Wen, 2016/01/08 05:54:45]

Done.

+        if (mOnToolbar) return null;
+
+        ImageButton button = (ImageButton) LayoutInflater.from(context)
+                .inflate(R.layout.custom_tabs_bottombar_item, parent, false);
+        button.setImageBitmap(mIcon);
+        button.setContentDescription(mDescription);
+        if (mPendingIntent == null) {
+            button.setEnabled(false);
+        } else {
+            // TODO(ianwen): add UMA for button clicking.
+            button.setOnClickListener(new View.OnClickListener() {
+                @Override
+                public void onClick(View v) {
+                    try {
+                        mPendingIntent.send();
+                    } catch (CanceledException e) {
+                        Log.e(TAG, "CanceledException while sending pending intent in custom tab");
+                    }
+                }
+            });
+        }
+        return button;
+    }
+
+    /**
+     * Parses a list of {@link CustomButtonParams} from the intent sent by clients.
+     * @param intent The intent sent by the client.
+     * @return A list of parsed {@link CustomButtonParams}. Return an empty list if input is invalid

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

maybe:

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

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

Done.

+     */
+    static List<CustomButtonParams> fromIntent(Context context, Intent intent) {
+        List<CustomButtonParams> paramsList = new ArrayList<>(1);
+        if (intent == null) return paramsList;
+
+        Bundle singleBundle = IntentUtils.safeGetBundleExtra(intent,
+                CustomTabsIntent.EXTRA_ACTION_BUTTON_BUNDLE);
+        ArrayList<Bundle> bundleList = IntentUtils.getParcelableArrayListExtra(intent,
+                CustomTabsIntent.EXTRA_ACTION_BUTTON_BUNDLE);
+        boolean tinted = IntentUtils.safeGetBooleanExtra(intent,
+                CustomTabsIntent.EXTRA_TINT_ACTION_BUTTON, false);
+        if (singleBundle != null) {
+            CustomButtonParams params = fromBundle(context, singleBundle, tinted, false);
+            paramsList.add(params);
+        } else if (bundleList != null) {
+            Set<Integer> ids = new HashSet<>();
+            for (Bundle bundle : bundleList) {
+                CustomButtonParams params = fromBundle(context, bundle, tinted, true);
+                if (params == null) {
+                    continue;
+                } else if (ids.contains(params.getId())) {
+                    Log.e(TAG, "Bottom bar items contain duplicate id: " + params.getId());
+                    continue;
+                }
+                ids.add(params.getId());
+                paramsList.add(params);
+            }
+        }
+        return paramsList;
+    }
+
+    /**
+     * Parses params out of a bundle. Note if a custom button contains a bitmap that does not fit
+     * into toolbar, it will be put to bottom bar.

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

fit into the toolbar

put into the bottom bar.

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

Done.

+     * @param fromList Whether the bundle is contained in a list or it is the single bundle that
+     *                 directly comes from the intent.
+     */
+    private static CustomButtonParams fromBundle(Context context, Bundle bundle, boolean tinted,
+            boolean fromList) {
+        if (bundle == null) return null;
+
+        if (fromList && !bundle.containsKey(CustomTabsIntent.KEY_ID)) return null;
+        int id = IntentUtils.safeGetInt(bundle, CustomTabsIntent.KEY_ID,
+                CustomTabsIntent.TOOLBAR_ACTION_BUTTON_ID);
+
+        Bitmap bitmap = tryParseBitmapFromBundle(bundle);
+        if (bitmap == null) {
+            Log.e(TAG, "Invalid action button: bitmap not present in bundle!");
+            return null;
+        }
+
+        String description = tryParseDescriptionFromBundle(bundle);
+        if (TextUtils.isEmpty(description)) {
+            Log.e(TAG, "Invalid action button: content description not present in bundle!");
+            bitmap.recycle();
+            return null;
+        }
+
+        boolean onToolbar = id == CustomTabsIntent.TOOLBAR_ACTION_BUTTON_ID;
+        if (onToolbar && !checkCustomButtonIconWithinBounds(context, bitmap)) {
+            onToolbar = false;
+            Log.w(TAG, "Button's icon not suitable for toolbar, putting it to bottom bar instead."
+                    + "See: https://developer.android.com/reference/android/support/customtabs/"
+                    + "CustomTabsIntent.html#KEY_ICON");
+        }
+
+        PendingIntent pi = IntentUtils.safeGetParcelable(bundle,
+                CustomTabsIntent.KEY_PENDING_INTENT);
+        // PendingIntent is a must for buttons on the toolbar, but it's optional for bottombar.
+        if (onToolbar && pi == null) {
+            Log.w(TAG, "Invalid action button on toolbar: pending intent not present in bundle!");
+            bitmap.recycle();
+            return null;
+        }
+
+        return new CustomButtonParams(id, bitmap, description, pi, tinted, onToolbar);
+    }
+
+    /**
+     * @return The bitmap contained in the given {@link Bundle}. Will return null if input is
+     *         invalid.
+     */
+    static Bitmap tryParseBitmapFromBundle(Bundle bundle) {

[gone, 2016/01/07 20:16:32]

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.

[Ian Wen, 2016/01/08 05:54:45]

Done.

+        if (bundle == null) return null;
+        Bitmap bitmap = IntentUtils.safeGetParcelable(bundle, CustomTabsIntent.KEY_ICON);
+        if (bitmap == null) return null;
+        return bitmap;
+    }
+
+    /**
+     * @return The content description contained in the given {@link Bundle}. Will return null if
+     *         input is invalid.
+     */
+    static String tryParseDescriptionFromBundle(Bundle bundle) {

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

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?

[Ian Wen, 2016/01/08 05:54:45]

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

+        String description = IntentUtils.safeGetString(bundle, CustomTabsIntent.KEY_DESCRIPTION);
+        if (TextUtils.isEmpty(description)) return null;
+        return description;
+    }
+
+    private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap bitmap) {

[gone, 2016/01/07 20:16:32]

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.

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

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

+        int height = context.getResources().getDimensionPixelSize(R.dimen.toolbar_icon_height);
+        if (bitmap.getHeight() < height) return false;
+        int scaledWidth = bitmap.getWidth() / bitmap.getHeight() * height;
+        if (scaledWidth > 2 * height) return false;
+        return true;
+    }
+}