[Android] Histogram for Android restore

chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
index 6e400876ab6b4c6d49b82dce9a742e47fae24a6a..47f867ac89c1626f86f95a49d99bcd7c31ba6653 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
@@ -10,6 +10,7 @@ import android.app.backup.BackupDataOutput;
 import android.content.Context;
 import android.content.SharedPreferences;
 import android.os.ParcelFileDescriptor;
+import android.support.annotation.IntDef;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
@@ -18,6 +19,7 @@ import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.library_loader.ProcessInitException;
+import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.firstrun.FirstRunGlueImpl;
 import org.chromium.chrome.browser.firstrun.FirstRunSignInProcessor;
 import org.chromium.chrome.browser.firstrun.FirstRunStatus;
@@ -32,6 +34,8 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.concurrent.Callable;
@@ -43,11 +47,36 @@ import java.util.concurrent.TimeUnit;
  */
 
 public class ChromeBackupAgent extends BackupAgent {
+    /**
+     *

[Bernhard Bauer, 2017/03/16 11:36:36]

:)

[aberent, 2017/03/16 12:16:50]

Done.

+     */
     private static final String ANDROID_DEFAULT_PREFIX = "AndroidDefault.";
     private static final String NATIVE_PREF_PREFIX = "native.";
 
     private static final String TAG = "ChromeBackupAgent";
 
+    @VisibleForTesting
+    static final String HISTOGRAM_ANDROID_RESTORE_RESULT = "Android.RestoreResult";
+    // Restore status is used to pass the result of any restore to Chrome's first run, so that

[Bernhard Bauer, 2017/03/16 11:36:36]

Can you leave empty lines before comments?

[aberent, 2017/03/16 12:16:50]

Done here, and where I think it helps readability elsewhere in the file (most
but not every case).

Note, however, that git cl format is taking out blank lines at the start of
methods, so I can't do this everywhere.

[Bernhard Bauer, 2017/03/16 14:34:01]

That's fine, I wouldn't add them there either (or at the beginning of a block in
general). 

+    // it can be recorded as a histogram.
+    @Retention(RetentionPolicy.SOURCE)
+    @IntDef({NO_RESTORE, RESTORE_COMPLETED, RESTORE_AFTER_FIRST_RUN, BROWSER_STARTUP_FAILED,
+            NOT_SIGNED_IN, RESTORE_STATUS_RECORDED})
+    public @interface RestoreStatus {}
+
+    // Values must match those in histogram.xml AndroidRestoreResult.
+    static final int NO_RESTORE = 0;
+    static final int RESTORE_COMPLETED = 1;
+    static final int RESTORE_AFTER_FIRST_RUN = 2;
+    static final int BROWSER_STARTUP_FAILED = 3;
+    static final int NOT_SIGNED_IN = 4;
+    static final int RESTORE_HISTOGRAM_BOUNDARY = 5;
+    // Set RESTORE_STATUS_RECORDED when the histogram has been recorded; so that it is only recorded
+    // once.
+    public static final int RESTORE_STATUS_RECORDED = 5;
+
+    private static final String RESTORE_STATUS = "Android_restore_status";

[Bernhard Bauer, 2017/03/16 11:36:36]

Nit: The combination of uppercase initial and underscores looks a bit weird :)
Lowercase everything?

[aberent, 2017/03/16 12:16:50]

Done.

+
     // Lists of preferences that should be restored unchanged.
 
     static final String[] BACKUP_ANDROID_BOOL_PREFS = {
@@ -217,9 +246,9 @@ public class ChromeBackupAgent extends BackupAgent {
         // Check that the user hasn't already seen FRE (not sure if this can ever happen, but if it
         // does then restoring the backup will overwrite the user's choices).
         SharedPreferences sharedPrefs = ContextUtils.getAppSharedPreferences();
-        if (sharedPrefs.getBoolean(FirstRunStatus.FIRST_RUN_FLOW_COMPLETE, false)
-                || sharedPrefs.getBoolean(
-                           FirstRunStatus.LIGHTWEIGHT_FIRST_RUN_FLOW_COMPLETE, false)) {
+        if (FirstRunStatus.getFirstRunFlowComplete()
+                || FirstRunStatus.getLightweightFirstRunFlowComplete()) {
+            setRestoreStatus(RESTORE_AFTER_FIRST_RUN);
             Log.w(TAG, "Restore attempted after first run");
             return;
         }
@@ -280,10 +309,12 @@ public class ChromeBackupAgent extends BackupAgent {
                 });
         if (!browserStarted) {
             // Something went wrong starting Chrome, skip the restore.
+            setRestoreStatus(BROWSER_STARTUP_FAILED);
             return;
         }
         // If the user hasn't signed in, or can't sign in, then don't restore anything.
         if (restoredUserName == null || !accountExistsOnDevice(restoredUserName)) {
+            setRestoreStatus(NOT_SIGNED_IN);
             Log.i(TAG, "Chrome was not signed in with a known account name, not restoring");
             return;
         }
@@ -331,6 +362,7 @@ public class ChromeBackupAgent extends BackupAgent {
 
         // The silent first run will change things, so there is no point in trying to prevent
         // additional backups at this stage. Don't write anything to |newState|.
+        setRestoreStatus(RESTORE_COMPLETED);
         Log.i(TAG, "Restore complete");
     }
 
@@ -352,6 +384,40 @@ public class ChromeBackupAgent extends BackupAgent {
         };
     }
 
+    /**
+     * Get the saved result of any restore that may have happened.
+     *
+     * @return the restore status, a RestoreStatus value.
+     */
+    @VisibleForTesting
+    @RestoreStatus
+    static int getRestoreStatus() {
+        return ContextUtils.getAppSharedPreferences().getInt(RESTORE_STATUS, NO_RESTORE);
+    }
+
+    /**
+     * Set the restore status

[Bernhard Bauer, 2017/03/16 11:36:36]

Nit: I don't think this comments adds any value.

[aberent, 2017/03/16 12:16:50]

Updated comment.

+     *
+     * @param status the status.
+     */
+    @VisibleForTesting
+    static void setRestoreStatus(@RestoreStatus int status) {
+        ContextUtils.getAppSharedPreferences().edit().putInt(RESTORE_STATUS, status).apply();
+    }
+
+    /**
+     * Record the restore histogram. To be called from Chrome itself once it is running.
+     */
+    static void recordRestoreHistogram() {
+        int restoreStatus = getRestoreStatus();
+        // Ensure restore status is only recorded once
+        if (restoreStatus != RESTORE_STATUS_RECORDED) {
+            RecordHistogram.recordEnumeratedHistogram(
+                    HISTOGRAM_ANDROID_RESTORE_RESULT, restoreStatus, RESTORE_HISTOGRAM_BOUNDARY);
+            setRestoreStatus(RESTORE_STATUS_RECORDED);
+        }
+    }
+
     @VisibleForTesting
     protected native String[] nativeGetBoolBackupNames();