Photo Picker dialog: Add UMA statistics.

chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java

 // Copyright 2017 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.photo_picker;
 
 import android.app.Activity;
 import android.content.Context;
 import android.content.DialogInterface;
 import android.content.res.Configuration;
 import android.graphics.Bitmap;
 import android.graphics.Rect;
 import android.support.v7.widget.GridLayoutManager;
 import android.support.v7.widget.RecyclerView;
 import android.util.LruCache;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.widget.Button;
 import android.widget.RelativeLayout;
 
 import org.chromium.base.VisibleForTesting;
+import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ConversionUtils;
 import org.chromium.chrome.browser.widget.selection.SelectableListLayout;
 import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
 import org.chromium.ui.PhotoPickerListener;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
 /**
  * A class for keeping track of common data associated with showing photos in
  * the photo picker, for example the RecyclerView and the bitmap caches.
  */
 public class PickerCategoryView extends RelativeLayout
         implements FileEnumWorkerTask.FilesEnumeratedCallback, RecyclerView.RecyclerListener,
                    DecoderServiceHost.ServiceReadyCallback, View.OnClickListener {
-    private static final int KILOBYTE = 1024;
+    // Note: these values must match the PhotoPickerDialogAction enum values in histograms.xml.
+    // Only add new values at the end, right before ACTIONS. We depend on these specific values in
+    // UMA histograms.

[Mark P, 2017/06/02 17:17:57]

We use a comment more like the best practices one that make it clearer you
cannot change the values of items or delete them.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

(The current comment mainly emphasizes "keep in sync", which is too weak an
assertion.)

[Finnur, 2017/06/06 14:23:16]

Done.

+    private static final int ACTION_CANCEL = 0;
+    private static final int ACTION_PHOTO_PICKED = 1;
+    private static final int ACTION_NEW_PHOTO = 2;
+    private static final int ACTION_BROWSE = 3;
+    private static final int ACTION_BOUNDARY = 4;
 
     // The dialog that owns us.
     private PhotoPickerDialog mDialog;
 
     // The view containing the RecyclerView and the toolbar, etc.
     private SelectableListLayout<PickerBitmap> mSelectableListLayout;
 
     // Our context.
     private Context mContext;
 
@@ skipping 89 matching lines @@
         doneButton.setOnClickListener(this);
 
         calculateGridMetrics();
 
         mLayoutManager = new GridLayoutManager(mContext, mColumns);
         mRecyclerView.setHasFixedSize(true);
         mRecyclerView.setLayoutManager(mLayoutManager);
         mSpacingDecoration = new GridSpacingItemDecoration(mColumns, mPadding);
         mRecyclerView.addItemDecoration(mSpacingDecoration);
 
-        final int maxMemory = (int) (Runtime.getRuntime().maxMemory() / KILOBYTE);
+        final int maxMemory = ConversionUtils.bytesToKilobytes(Runtime.getRuntime().maxMemory());
         final int cacheSizeLarge = maxMemory / 2; // 1/2 of the available memory.
         final int cacheSizeSmall = maxMemory / 8; // 1/8th of the available memory.
         mLowResBitmaps = new LruCache<String, Bitmap>(cacheSizeSmall);
         mHighResBitmaps = new LruCache<String, Bitmap>(cacheSizeLarge);
     }
 
     @Override
     public void onConfigurationChanged(Configuration newConfig) {
         super.onConfigurationChanged(newConfig);
 
@@ skipping 29 matching lines @@
             PhotoPickerDialog dialog, PhotoPickerListener listener, boolean multiSelectionAllowed) {
         if (!multiSelectionAllowed) mSelectionDelegate.setSingleSelectionMode();
 
         mDialog = dialog;
         mMultiSelectionAllowed = multiSelectionAllowed;
         mListener = listener;
 
         mDialog.setOnCancelListener(new DialogInterface.OnCancelListener() {
             @Override
             public void onCancel(DialogInterface dialog) {
+                recordFinalUmaStats(ACTION_CANCEL);
                 mListener.onPickerUserAction(PhotoPickerListener.Action.CANCEL, null);
             }
         });
     }
 
     // FileEnumWorkerTask.FilesEnumeratedCallback:
 
     @Override
     public void filesEnumeratedCallback(List<PickerBitmap> files) {
         mPickerBitmaps = files;
@@ skipping 17 matching lines @@
         if (filePath != null) {
             getDecoderServiceHost().cancelDecodeImage(filePath);
         }
     }
 
     // OnClickListener:
 
     @Override
     public void onClick(View view) {
         if (view.getId() == R.id.done) {
+            recordFinalUmaStats(ACTION_PHOTO_PICKED);
             notifyPhotosSelected();
         } else {
+            recordFinalUmaStats(ACTION_CANCEL);
             mListener.onPickerUserAction(PhotoPickerListener.Action.CANCEL, null);

[Mark P, 2017/06/02 17:17:58]

nit: if this triggers a cancel listener, does that mean the cancel action gets
recorded again?  (I see a cancel listener on this class also records a CANCEL
action.)

(I'm always nervous about the same thing being logged in multiple places, per
bullet 5 on this slide:
https://docs.google.com/presentation/d/10iBwg3s8iiw5uMTz9ZR1lRLCdSB9OKyDsuUok...
)

[Finnur, 2017/06/06 14:23:16]

This is listening for the user clicking the X to close the dialog. The other
handler is for listening for the user pressing Back. I tried both and there's no
overlap (no over-counting).

[Mark P, 2017/06/07 17:19:30]

Acknowledged, thanks for checking.

         }
 
         mDialog.dismiss();
     }
 
     /**
      * Start loading of bitmaps, once files have been enumerated and service is
      * ready to decode.
      */
     private void processBitmaps() {
@@ skipping 29 matching lines @@
     }
 
     public boolean isMultiSelectAllowed() {
         return mMultiSelectionAllowed;
     }
 
     /**
      * Notifies the listener that the user selected to launch the gallery.
      */
     public void showGallery() {
+        recordFinalUmaStats(ACTION_BROWSE);
         mListener.onPickerUserAction(PhotoPickerListener.Action.LAUNCH_GALLERY, null);
     }
 
     /**
      * Notifies the listener that the user selected to launch the camera intent.
      */
     public void showCamera() {
+        recordFinalUmaStats(ACTION_NEW_PHOTO);
         mListener.onPickerUserAction(PhotoPickerListener.Action.LAUNCH_CAMERA, null);
     }
 
     /**
      * Calculates image size and how many columns can fit on-screen.
      */
     private void calculateGridMetrics() {
         Rect appRect = new Rect();
         ((Activity) mContext).getWindow().getDecorView().getWindowVisibleDisplayFrame(appRect);
 
@@ skipping 72 matching lines @@
                 if (position < mSpanCount) {
                     top = mSpacing;
                 }
                 bottom = mSpacing;
             }
 
             outRect.set(left, top, right, bottom);
         }
     }
 
+    /**
+     * Record UMA statistics (what action was taken in the dialog and other performance stats).
+     * @param action The action the user took in the dialog.
+     */
+    private void recordFinalUmaStats(int action) {
+        RecordHistogram.recordEnumeratedHistogram(
+                "Android.PhotoPicker.DialogAction", action, ACTION_BOUNDARY);

[Mark P, 2017/06/02 17:17:57]

optional nit: Are you interested in what actions happens in what orders?  If so,
you may want to log user actions too.

[Finnur, 2017/06/06 14:23:16]

Thanks for the tip, but the order is not noteworthy in this case. All of these
are final actions, as far as I'm concerned, and mark an end in my interest of
what happens (as far as metrics goes).

[Mark P, 2017/06/07 17:19:30]

Acknowledged, though I was more curious whether you wanted to know what people
did *before* they closed/canceled the dialog.

[Finnur, 2017/06/08 10:39:31]

There's little left to track; it is a very simple dialog. The only remaining
action (that doesn't result in ending the dialog) that is not tracked at the
moment is clicking on photos to select them. I'm not sure it is interesting to
have statistics on how and/or what they select, but I guess it can always be
added later.

+        RecordHistogram.recordCountHistogram(
+                "Android.PhotoPicker.DecodeRequests", mPickerAdapter.getDecodeRequestCount());
+        RecordHistogram.recordCountHistogram(
+                "Android.PhotoPicker.CacheHits", mPickerAdapter.getCacheHitCount());
+    }
+
     /** Sets a list of files to use as data for the dialog. For testing use only. */
     @VisibleForTesting
     public static void setTestFiles(List<PickerBitmap> testFiles) {
         sTestFiles = new ArrayList<>(testFiles);
     }
 
     @VisibleForTesting
     public SelectionDelegate<PickerBitmap> getSelectionDelegateForTesting() {
         return mSelectionDelegate;
     }
 }