[Custom Tabs]Add API for updating action button in service

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

Index: chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
new file mode 100644
index 0000000000000000000000000000000000000000..9d27bc50f000b9cd9d0e8fd9f7c8d09600b02312
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/ActionButtonParams.java
@@ -0,0 +1,89 @@
+// 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.content.Context;
+import android.graphics.Bitmap;
+import android.os.Bundle;
+import android.support.annotation.NonNull;
+import android.support.customtabs.CustomTabsIntent;
+import android.text.TextUtils;
+
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.util.IntentUtils;
+
+/**
+ * Container for all parameters related to creating a custom action button.
+ */
+/* package */ class ActionButtonParams {
+    final Bitmap mIcon;

[Yusuf, 2015/08/21 00:07:36]

see below. I think we should use a single static way of constructing this and
also make this not final. Sp lets add getters for mIcon and mDescription.

[Ian Wen, 2015/08/21 17:01:23]

Done.

+    final String mDescription;
+    final PendingIntent mPendingIntent;
+
+    public ActionButtonParams(@NonNull Bitmap icon, @NonNull String description,

[Yusuf, 2015/08/21 00:07:36]

why not have a class method with update(Bitmap, String) for this? The static
call below makes it look like that is the only constructor. Then we have this,
which I think is a bit confusing
.

[Ian Wen, 2015/08/21 17:01:23]

Good idea. I was constrained by keeping them final. Done.

+            @NonNull PendingIntent pendingIntent) {
+        mIcon = icon;
+        mDescription = description;
+        mPendingIntent = pendingIntent;
+    }
+
+    /**
+     * Parses an {@link ActionButtonParams} from an action button bundle sent by clients.
+     * @param bundle The action button button specified by

[Yusuf, 2015/08/21 00:07:36]

action button bundle

[Ian Wen, 2015/08/21 17:01:23]

Done.

+     *               {@link CustomTabsIntent#EXTRA_ACTION_BUTTON_BUNDLE}
+     * @return The parsed {@link ActionButtonParams}. Return null if input is invalid.
+     */
+    static ActionButtonParams fromBundle(Context context, Bundle bundle) {
+        if (bundle == null) return null;
+
+        Bitmap bitmap = tryParseBitmapFromBundle(context, bundle);
+        if (bitmap == null) return null;
+
+        String description = tryParseDescriptionFromBundle(bundle);
+        if (TextUtils.isEmpty(description)) {
+            bitmap.recycle();
+            return null;
+        }
+
+        PendingIntent pi = IntentUtils.safeGetParcelable(bundle,
+                CustomTabsIntent.KEY_PENDING_INTENT);
+        if (pi == null) return null;
+        return new ActionButtonParams(bitmap, description, pi);
+    }
+
+    /**
+     * @return The bitmap contained in the given {@link Bundle}. Will return null if input is
+     *         invalid.
+     */
+    static Bitmap tryParseBitmapFromBundle(Context context, Bundle bundle) {

[Benoit L, 2015/08/21 11:31:14]

It seems that you are not using the tryParse* methods outside of a context where
you could have used fromBundle(), which already does the checks, and so you can
assume that whatever has been returned by fromBundle() is corrent, right?

If it's the case, why don't you make these methods private?

[Ian Wen, 2015/08/21 17:01:23]

I wish I could do what you suggested (had the same initial idea myself).

Yet in CustomTabConnection I can't construct a valid params because there will
be no PendingIntent. Also I don't want to just write a isValid() method that
creates a bitmap for verification purpose and discard it later (not memory
friendly)... After some thinking I decided to take Yusuf's suggestion, which is
to have an update() method in this class.

[Benoit L, 2015/08/21 17:06:27]

Alright, that's fine.
Thank you for explaining the reasoning.

+        if (bundle == null) return null;
+        Bitmap bitmap = IntentUtils.safeGetParcelable(bundle, CustomTabsIntent.KEY_ICON);
+        if (bitmap == null) return null;
+        if (!checkCustomButtonIconWithinBounds(context, bitmap)) {
+            bitmap.recycle();
+            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) {
+        String description = IntentUtils.safeGetString(bundle, CustomTabsIntent.KEY_DESCRIPTION);
+        if (TextUtils.isEmpty(description)) return null;
+        return description;
+    }
+
+    private static boolean checkCustomButtonIconWithinBounds(Context context, Bitmap bitmap) {
+        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;
+    }
+}