Photo Picker dialog: Add UMA statistics.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java
index 24bdc226e65f2ba107dbd0408fd2e8f48bb92f06..ef612c012a0d6c0b8e26171ebc02cfab7814d6f7 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java
@@ -19,7 +19,9 @@ 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;
@@ -35,7 +37,14 @@ import java.util.List;
 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;
@@ -145,7 +154,7 @@ public class PickerCategoryView extends RelativeLayout
         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);
@@ -195,6 +204,7 @@ public class PickerCategoryView extends RelativeLayout
         mDialog.setOnCancelListener(new DialogInterface.OnCancelListener() {
             @Override
             public void onCancel(DialogInterface dialog) {
+                recordFinalUmaStats(ACTION_CANCEL);
                 mListener.onPickerUserAction(PhotoPickerListener.Action.CANCEL, null);
             }
         });
@@ -232,8 +242,10 @@ public class PickerCategoryView extends RelativeLayout
     @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.

         }
 
@@ -284,6 +296,7 @@ public class PickerCategoryView extends RelativeLayout
      * Notifies the listener that the user selected to launch the gallery.
      */
     public void showGallery() {
+        recordFinalUmaStats(ACTION_BROWSE);
         mListener.onPickerUserAction(PhotoPickerListener.Action.LAUNCH_GALLERY, null);
     }
 
@@ -291,6 +304,7 @@ public class PickerCategoryView extends RelativeLayout
      * 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);
     }
 
@@ -383,6 +397,19 @@ public class PickerCategoryView extends RelativeLayout
         }
     }
 
+    /**
+     * 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) {