Add update menu item and app menu icon badge

chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java

Index: chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java b/chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
new file mode 100644
index 0000000000000000000000000000000000000000..f06ab5da7c31323d72e36f697cae9162f95296df
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
@@ -0,0 +1,265 @@
+// 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.omaha;
+
+import android.content.ActivityNotFoundException;
+import android.content.Context;
+import android.content.Intent;
+import android.net.Uri;
+import android.os.AsyncTask;
+import android.text.TextUtils;
+
+import org.chromium.base.CommandLine;
+import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
+import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.preferences.PrefServiceBridge;
+import org.chromium.components.variations.VariationsAssociatedData;
+
+/**
+ * Contains logic for whether the update menu item should be shown, whether the update toolbar badge
+ * should be shown, and UMA logging for the update menu item.
+ */
+public class UpdateMenuItemHelper {
+    private static UpdateMenuItemHelper sInstance;
+
+    private static final String TAG = "OmahaUpdateMenuItem";

[gone, 2015/12/08 22:24:23]

should be UpdateMenuItemHelper

[Theresa, 2015/12/10 03:53:18]

Done.

+
+    // Command line flag for testing.

[gone, 2015/12/08 22:24:24]

Move to ChromeSwitches.java

[Theresa, 2015/12/10 03:53:18]

Done.

+    private static final String FORCE_SHOW_UPDATE_MENU_ITEM = "force-show-update-menu-item";
+    private static final String MARKET_URL_FOR_TESTING = "market-url-for-testing";
+
+    // Finch configs
+    private static final String FIELD_TRIAL_NAME = "UpateMenuItem";

[gone, 2015/12/08 22:24:24]

Is "Upate" intentional?  Saw it in the histogram too.

[Theresa, 2015/12/10 03:53:18]

Nope. Fixed here and in fieldtrial_testing_config_android.json; double checked
the histograms and I think they are all correct.

+    private static final String ENABLED_VALUE = "true";
+    private static final String ENABLE_UPDATE_MENU_ITEM = "enable_update_menu_item";
+    private static final String ENABLE_UPDATE_BADGE = "enable_update_badge";
+    private static final String SHOW_SUMMARY = "show_summary";
+    private static final String CUSTOM_SUMMARY = "custom_summary";
+
+    // UMA constants for logging whether the menu item was clicked.
+    private static final int ITEM_CLICKED_INTENT_LAUNCHED = 0;
+    private static final int ITEM_CLICKED_INTENT_FAILED = 1;
+    private static final int ITEM_NOT_CLICKED = 2;
+    private static final int ITEM_CLICKED_BOUNDARY = 3;
+
+    // UMA constants for logging whether Chrome was updated after the menu item was clicked.
+    private static final int UPDATED = 0;
+    private static final int NOT_UPDATED = 1;
+    private static final int UPDATED_BOUNDARY = 2;
+
+    // Whether OmahaClient has already been checked for an update., and volunteer my time

[gone, 2015/12/08 22:24:24]

/methinks your email window focus was incorrect

[Theresa, 2015/12/10 03:53:18]

Definitely. How embarrassing - should have done another pass on this not at
9:30p, sorry!

[gone, 2015/12/10 21:45:09]

I've sent far worse ;)  Yours gives off a benevolent vibe!

+
+    private boolean mAlreadyCheckedForUpdates;
+
+    // Whether an update is available.
+    private boolean mUpdateAvailable;
+
+    // URL to direct the user to when Omaha detects a newer version available.
+    private String mUpdateUrl;
+
+    // Whether the menu item was clicked. This is used to log the click-through rate.
+    private boolean mMenuItemClicked;
+
+    /**
+     * @return The {@link UpdateMenuItemHelper} instance.
+     */
+    public static synchronized UpdateMenuItemHelper getInstance() {

[Theresa, 2015/12/08 17:59:37]

Fix this (findbugs error for making the method synchronized)

[gone, 2015/12/08 22:24:24]

Any reason why you're using a singleton?  Can it just be a member of the
ChromeApplication?

[Theresa, 2015/12/10 03:53:18]

Since this class is used in a lot of places that don't have a direct link to the
activity (and therefore a way to call getChromeApplication()), I prefer a
singleton to a bunch of calls to context.getApplicationContext() cast to
ChromeApplication. This isn't a strong preference, so I'm okay with moving it to
ChromeApplication if that makes more sense.

For background, aurimas@ has a TODO comment in ChromeApplication to replace
mUpdateInfoBarHelper w/ a static instance, so I figured I'd follow suit. It got
added during upstreaming/merging (I think because newt@ said to delete or inline
the function (probably because it's only used in ChromeActivity).

[gone, 2015/12/10 21:45:09]

Acknowledged.

+        if (sInstance == null) {
+            sInstance = new UpdateMenuItemHelper();
+            String testMarketUrl = getStringParamValue(MARKET_URL_FOR_TESTING);
+            if (!TextUtils.isEmpty(testMarketUrl)) {
+                sInstance.mUpdateUrl = testMarketUrl;
+            }
+        }
+        return sInstance;
+    }
+
+    /**
+     * Checks if the OmahaClient knows about an update.

[gone, 2015/12/08 22:24:24]

{@link OmahaClient}

[Theresa, 2015/12/10 03:53:18]

Done.

+     * @param activity The current {@link ChromeActivity}.
+     */
+    public void checkForUpdateOnBackgroundThread(final ChromeActivity activity) {
+        if (getBooleanParam(ENABLE_UPDATE_MENU_ITEM)) {

[gone, 2015/12/08 22:24:24]

early exit?

if (!getBooleanParam(ENABLE_UPDATE_MENU_ITEM) || mAlreadyCheckedForUpdates)
return;

[Theresa, 2015/12/10 03:53:18]

Done (mostly). The check for mAlreadyCheckedForUpdates is later.

[Theresa, 2015/12/10 20:40:47]

... is later because we still need to call activity.onCheckForUpdate() to
facilitate more than one DocumentActivity (see response to comment on that
class).

+            ThreadUtils.assertOnUiThread();
+
+            if (mAlreadyCheckedForUpdates) return;
+            mAlreadyCheckedForUpdates = true;
+
+            new AsyncTask<Void, Void, Void>() {
+                @Override
+                protected Void doInBackground(Void... params) {
+                    if (OmahaClient.isNewerVersionAvailable(activity)) {
+                        mUpdateUrl = OmahaClient.getMarketURL(activity);
+                        mUpdateAvailable = true;
+                    } else {
+                        mUpdateAvailable = false;
+                    }
+                    return null;
+                }
+
+                @Override
+                protected void onPostExecute(Void result) {
+                    if (activity.isActivityDestroyed()) return;
+                    activity.onCheckForUpdate(mUpdateAvailable);
+
+                    if (PrefServiceBridge.getInstance().getClickedUpdateMenuItem()) {
+                        recordUpdateHistogram();

[gone, 2015/12/08 22:24:23]

should recordUpdateHistogram() be the thing checking if
getClickedUpdateMenuItem() was called?  this is (mostly) duplicated with the
call below.

[Theresa, 2015/12/10 03:53:18]

Done.

+                    }
+                }
+            }.execute();
+        }
+    }
+
+    /**
+     * Logs whether an update was performed if the update menu item was clicked.
+     * Should be called from ChromeActivity#onStart().
+     */
+    public void onStart() {
+        if (mAlreadyCheckedForUpdates
+                && PrefServiceBridge.getInstance().getClickedUpdateMenuItem()) {
+            recordUpdateHistogram();
+        }
+    }
+
+    /**
+     * @param activity The current {@link ChromeActivity}.
+     * @return Whether the update menu item should be shown.
+     */
+    public boolean shouldShowMenuItem(ChromeActivity activity) {
+        if (getBooleanParam(FORCE_SHOW_UPDATE_MENU_ITEM)) {
+            return true;
+        }
+
+        if (!getBooleanParam(ENABLE_UPDATE_MENU_ITEM)) {
+            return false;
+        }
+
+        if (!mAlreadyCheckedForUpdates) {

[gone, 2015/12/08 22:24:24]

this last bit is exactly the same as shouldShowToolbarBadge's bottom half. 
consolidate?

[Theresa, 2015/12/10 03:53:18]

Done.

+            checkForUpdateOnBackgroundThread(activity);
+            return false;
+        }
+
+        return mUpdateAvailable;
+    }
+
+    /**
+     * @param context The current {@link Context}.
+     * @return The string to use for summary text or the empty string if no summary should be shown.
+     */
+    public String getMenuItemSummaryText(Context context) {
+        if (!getBooleanParam(SHOW_SUMMARY)) {
+            return "";
+        }
+
+        String customSummary = getStringParamValue(CUSTOM_SUMMARY);
+        if (!TextUtils.isEmpty(customSummary)) {
+            return customSummary;
+        }
+
+        return context.getResources().getString(R.string.menu_update_summary_default);
+    }
+
+    /**
+     * @param activity The current {@link ChromeActivity}.
+     * @return Whether the update badge should be shown in the toolbar.
+     */
+    public boolean shouldShowToolbarBadge(ChromeActivity activity) {
+        if (getBooleanParam(FORCE_SHOW_UPDATE_MENU_ITEM)) {
+            return true;
+        }
+
+        if (!getBooleanParam(ENABLE_UPDATE_BADGE)) {
+            return false;
+        }
+
+        if (!mAlreadyCheckedForUpdates) {
+            checkForUpdateOnBackgroundThread(activity);
+            return false;
+        }
+
+        return mUpdateAvailable;
+    }
+
+    /**
+     * Handles a click on the update menu item.
+     * @param activity The current {@link ChromeActivity}.
+     */
+    public void onMenuItemClicked(ChromeActivity activity) {

[gone, 2015/12/08 22:24:24]

early exit to avoid indentation?

if (mUpdateUrl == null) return;

[Theresa, 2015/12/10 03:53:18]

Done.

+        if (mUpdateUrl != null) {
+            // Fire an intent to open the URL.
+            try {
+                Intent launchIntent = new Intent(Intent.ACTION_VIEW, Uri.parse(mUpdateUrl));
+                activity.startActivity(launchIntent);
+                recordItemClickedHistogram(ITEM_CLICKED_INTENT_LAUNCHED);
+                PrefServiceBridge.getInstance().setClickedUpdateMenuItem(true);
+            } catch (ActivityNotFoundException e) {
+                Log.e(TAG, "Failed to launch Activity for: " + mUpdateUrl);
+                recordItemClickedHistogram(ITEM_CLICKED_INTENT_FAILED);
+            }
+        }
+    }
+
+    /**
+     * Should be called before the AppMenu is dismissed if the update menu item was clicked.
+     */
+    public void setMenuItemClicked() {
+        mMenuItemClicked = true;
+    }
+
+    /**
+     * Called when the AppMenu is dimissed. Logs a histogram immediately if the update menu item was
+     * not clicked. If it was clicked, logging is delayed until #onMenuItemClicked().
+     */
+    public void onAppMenuDismissed() {

[gone, 2015/12/08 22:24:24]

maybe onMenuDismissed for consistency with the calling function?

[Theresa, 2015/12/10 03:53:18]

Done.

+        if (!mMenuItemClicked) {
+            recordItemClickedHistogram(ITEM_NOT_CLICKED);
+        }
+        mMenuItemClicked = false;
+    }
+
+    private void recordItemClickedHistogram(int action) {
+        RecordHistogram.recordEnumeratedHistogram("GoogleUpdate.MenuItem.ActionTakenOnMenuOpen",
+                action, ITEM_CLICKED_BOUNDARY);
+    }
+
+    private void recordUpdateHistogram() {
+        RecordHistogram.recordEnumeratedHistogram(
+                "GoogleUpdate.MenuItem.ActionTakenAfterItemClicked",
+                mUpdateAvailable ? NOT_UPDATED : UPDATED, UPDATED_BOUNDARY);
+        PrefServiceBridge.getInstance().setClickedUpdateMenuItem(false);
+    }
+
+    /**
+     * Gets a boolean Finch parameter, assuming the <paramName>="true" format.  Also checks for a

[gone, 2015/12/08 22:24:23]

Don't know if we're supposed to be using "Finch".  Can you find the public name?

[Theresa, 2015/12/10 03:53:18]

I changed it to the class name (VariationsAssociatedData).

+     * command-line switch with the same name, for easy local testing.
+     * @param paramName The name of the Finch parameter (or command-line switch) to get a value for.
+     * @return Whether the Finch param is defined with a value "true", if there's a command-line
+     *         flag present with any value.
+     */
+    private static boolean getBooleanParam(String paramName) {
+        if (CommandLine.getInstance().hasSwitch(paramName)) {
+            return true;
+        }
+        return TextUtils.equals(ENABLED_VALUE,
+                VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, paramName));
+    }
+
+    /**
+     * Gets a String Finch parameter. Also checks for a command-line switch with the same name,
+     * for easy local testing.
+     * @param paramName The name of the Finch parameter (or command-line switch) to get a value for.
+     * @return The command-line flag value if present, or the Finch param is value if present.
+     */
+    private static String getStringParamValue(String paramName) {
+        String value = CommandLine.getInstance().getSwitchValue(paramName);
+        if (TextUtils.isEmpty(value)) {
+            value = VariationsAssociatedData.getVariationParamValue(FIELD_TRIAL_NAME, paramName);
+        }
+        return value;
+    }
+}