Add metrics for password export on Android, part 2

This CL adds PasswordManager.ExportedPasswordsPerUserInCSV to the code
for exporting passwords from Android settings.

More details about the metrics are in the design doc [1].

(Remaining TODO for metrics, not addressed here: user actions.)

[1]
https://docs.google.com/document/d/1miKr2x0PTNIKgt3RQeur51ICQ66uszlYAmFXJONbG_0/edit?ts=5a313898#heading=h.chwovfmlf7pk

Bug: 788701
Change-Id: I844f7506590d505b1d2e4671e21f5d363d73a2e7
Reviewed-on: https://chromium-review.googlesource.com/926527Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539946}

chrome/android/java/src/org/chromium/chrome/browser/preferences/password/ByteArrayIntCallback.java

+// Copyright 2018 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.preferences.password;
+
+import org.chromium.base.annotations.CalledByNative;
+
+/**
+ * A simple 2-argument callback with a byte array and an int as arguments.
+ *
+ * This is used to call a 2-argument Java callback from C++ code. The generic {@link
+ * org.chromium.base.Callback} cannot be used because it is limited to a single argument.
+ * Alternative approaches like encoding the two arguments into one string or one array of objects
+ * with different types were considered, but their downside was both a lot of boilerplate (creating
+ * the composed object in C++ and checking and decoding it in Java) and lack of clarity. This
+ * 2-argument callback also adds a few code lines but it is clear and the compiler does the type
+ * checking.
+ */
+public interface ByteArrayIntCallback {
+    /**
+     * Invoked with the result of a computation.
+     *
+     * @param byteArray Byte array part of the result.
+     * @param number Integer part of the result.
+     */
+    @CalledByNative
+    void onResult(byte[] byteArray, int number);
+}

chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordManagerHandler.java

@@ -4,8 +4,6 @@
 
 package org.chromium.chrome.browser.preferences.password;
 
-import org.chromium.base.Callback;
-
 /**
  * Interface for retrieving passwords and password exceptions (websites for which Chrome should not
  * save password) from native code.
@@ -67,7 +65,8 @@ public interface PasswordManagerHandler {
     /**
      * Trigger serializing the saved passwords in the background.
      *
-     * @param callback is called on completion, with the serialized passwords as argument.
+     * @param callback is called on completion, with the serialized passwords and their count as
+     * argument.
      */
-    void serializePasswords(Callback<byte[]> callback);
+    void serializePasswords(ByteArrayIntCallback callback);
 }

chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordUIView.java

@@ -4,7 +4,6 @@
 
 package org.chromium.chrome.browser.preferences.password;
 
-import org.chromium.base.Callback;
 import org.chromium.base.annotations.CalledByNative;
 
 /**
@@ -72,7 +71,7 @@ public final class PasswordUIView implements PasswordManagerHandler {
     }
 
     @Override
-    public void serializePasswords(Callback<byte[]> callback) {
+    public void serializePasswords(ByteArrayIntCallback callback) {
         nativeHandleSerializePasswords(mNativePasswordUIViewAndroid, callback);
     }
 
@@ -116,5 +115,5 @@ public final class PasswordUIView implements PasswordManagerHandler {
     private native void nativeDestroy(long nativePasswordUIViewAndroid);
 
     private native void nativeHandleSerializePasswords(
-            long nativePasswordUIViewAndroid, Callback<byte[]> callback);
+            long nativePasswordUIViewAndroid, ByteArrayIntCallback callback);
 }

chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java

@@ -33,7 +33,6 @@ import android.view.View;
 import android.view.inputmethod.EditorInfo;
 
 import org.chromium.base.ApiCompatibilityUtils;
-import org.chromium.base.Callback;
 import org.chromium.base.ContentUriUtils;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.VisibleForTesting;
@@ -102,6 +101,9 @@ public class SavePasswordsPreferences
     // The key for saving |mExportState| to instance bundle.
     private static final String SAVED_STATE_EXPORT_STATE = "saved-state-export-state";
 
+    // The key for saving |mEntriesCount| to instance bundle.
+    private static final String SAVED_STATE_ENTRIES_COUNT = "saved-state-entries-count";
+
     // The key for saving |mExportFileUri| to instance bundle.
     private static final String SAVED_STATE_EXPORT_FILE_URI = "saved-state-export-file-uri";
 
@@ -173,6 +175,10 @@ public class SavePasswordsPreferences
     // this timestamp is assigned the result of System.currentTimeMillis().
     private Long mExportPreparationStart;
 
+    // The number of password entries contained in the most recent serialized data for password
+    // export.
+    private int mEntriesCount;
+
     private MenuItem mHelpItem;
 
     private String mSearchQuery;
@@ -230,6 +236,7 @@ public class SavePasswordsPreferences
             mSearchQuery = savedInstanceState.getString(SAVED_STATE_SEARCH_QUERY);
             mSearchRecorded = mSearchQuery != null; // We record a search when a query is set.
         }
+        mEntriesCount = savedInstanceState.getInt(SAVED_STATE_ENTRIES_COUNT);
     }
 
     @Override
@@ -380,10 +387,11 @@ public class SavePasswordsPreferences
         // they arrive.
         mExportPreparationStart = System.currentTimeMillis();
         PasswordManagerHandlerProvider.getInstance().getPasswordManagerHandler().serializePasswords(
-                new Callback<byte[]>() {
+                new ByteArrayIntCallback() {
                     @Override
-                    public void onResult(byte[] serializedPasswords) {
-                        shareSerializedPasswords(serializedPasswords);
+                    public void onResult(byte[] byteArray, int number) {
+                        mEntriesCount = number;
+                        shareSerializedPasswords(byteArray);
                     }
                 });
         if (!ReauthenticationManager.isScreenLockSetUp(getActivity().getApplicationContext())) {
@@ -619,6 +627,8 @@ public class SavePasswordsPreferences
             ContextUtils.getApplicationContext().startActivity(chooser);
             RecordHistogram.recordEnumeratedHistogram("PasswordManager.ExportPasswordsToCSVResult",
                     EXPORT_RESULT_SUCCESS, EXPORT_RESULT_COUNT);
+            RecordHistogram.recordCountHistogram(
+                    "PasswordManager.ExportedPasswordsPerUserInCSV", mEntriesCount);
         } catch (ActivityNotFoundException e) {
             showExportErrorAndAbort(R.string.save_password_preferences_export_no_app, null,
                     R.string.save_password_preferences_export_learn_google_drive,
@@ -817,6 +827,7 @@ public class SavePasswordsPreferences
     public void onSaveInstanceState(Bundle outState) {
         super.onSaveInstanceState(outState);
         outState.putInt(SAVED_STATE_EXPORT_STATE, mExportState);
+        outState.putInt(SAVED_STATE_ENTRIES_COUNT, mEntriesCount);
         if (mExportFileUri != null) {
             outState.putString(SAVED_STATE_EXPORT_FILE_URI, mExportFileUri.toString());
         }

chrome/android/java_sources.gni

@@ -992,6 +992,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/preferences/languages/LanguageItem.java",
   "java/src/org/chromium/chrome/browser/preferences/languages/LanguageListBaseAdapter.java",
   "java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java",
+  "java/src/org/chromium/chrome/browser/preferences/password/ByteArrayIntCallback.java",
   "java/src/org/chromium/chrome/browser/preferences/password/ExportErrorDialogFragment.java",
   "java/src/org/chromium/chrome/browser/preferences/password/ExportWarningDialogFragment.java",
   "java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java",

chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java

@@ -61,7 +61,6 @@ import org.junit.Test;
 import org.junit.rules.TestRule;
 import org.junit.runner.RunWith;
 
-import org.chromium.base.Callback;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.util.Feature;
@@ -109,7 +108,7 @@ public class SavePasswordsPreferencesTest {
 
         // This is set once {@link #serializePasswords()} is called.
         @Nullable
-        private Callback<byte[]> mExportCallback;
+        private ByteArrayIntCallback mExportCallback;
 
         public void setSavedPasswords(ArrayList<SavedPasswordEntry> savedPasswords) {
             mSavedPasswords = savedPasswords;
@@ -119,7 +118,7 @@ public class SavePasswordsPreferencesTest {
             mSavedPasswordExeptions = savedPasswordExceptions;
         }
 
-        public Callback<byte[]> getExportCallback() {
+        public ByteArrayIntCallback getExportCallback() {
             return mExportCallback;
         }
 
@@ -164,7 +163,7 @@ public class SavePasswordsPreferencesTest {
         }
 
         @Override
-        public void serializePasswords(Callback<byte[]> callback) {
+        public void serializePasswords(ByteArrayIntCallback callback) {
             mExportCallback = callback;
         }
     }
@@ -787,7 +786,7 @@ public class SavePasswordsPreferencesTest {
         reauthenticateAndRequestExport(preferences);
 
         // Pretend that passwords have been serialized to go directly to the intent.
-        mHandler.getExportCallback().onResult(new byte[] {1, 2, 3});
+        mHandler.getExportCallback().onResult(new byte[] {5, 6, 7}, 123);
 
         // Before triggering the sharing intent chooser, stub it out to avoid leaving system UI open
         // after the test is finished.
@@ -798,6 +797,9 @@ public class SavePasswordsPreferencesTest {
                 new MetricsUtils.HistogramDelta("PasswordManager.ExportPasswordsToCSVResult",
                         SavePasswordsPreferences.EXPORT_RESULT_SUCCESS);
 
+        MetricsUtils.HistogramDelta countDelta = new MetricsUtils.HistogramDelta(
+                "PasswordManager.ExportedPasswordsPerUserInCSV", 123);
+
         // Confirm the export warning to fire the sharing intent.
         Espresso.onView(withText(R.string.save_password_preferences_export_action_title))
                 .perform(click());
@@ -809,6 +811,7 @@ public class SavePasswordsPreferencesTest {
         Intents.release();
 
         Assert.assertEquals(1, successDelta.getDelta());
+        Assert.assertEquals(1, countDelta.getDelta());
     }
 
     /**
@@ -844,7 +847,7 @@ public class SavePasswordsPreferencesTest {
         });
 
         // Pretend that passwords have been serialized to go directly to the intent.
-        mHandler.getExportCallback().onResult(new byte[] {1, 2, 3});
+        mHandler.getExportCallback().onResult(new byte[] {1, 2, 3}, 56);
 
         // Before triggering the sharing intent chooser, stub it out to avoid leaving system UI open
         // after the test is finished.
@@ -855,6 +858,9 @@ public class SavePasswordsPreferencesTest {
                 new MetricsUtils.HistogramDelta("PasswordManager.ExportPasswordsToCSVResult",
                         SavePasswordsPreferences.EXPORT_RESULT_SUCCESS);
 
+        MetricsUtils.HistogramDelta countDelta = new MetricsUtils.HistogramDelta(
+                "PasswordManager.ExportedPasswordsPerUserInCSV", 56);
+
         // Confirm the export warning to fire the sharing intent.
         Espresso.onView(withText(R.string.save_password_preferences_export_action_title))
                 .perform(click());
@@ -866,6 +872,7 @@ public class SavePasswordsPreferencesTest {
         Intents.release();
 
         Assert.assertEquals(1, successDelta.getDelta());
+        Assert.assertEquals(1, countDelta.getDelta());
     }
 
     /**
@@ -1055,7 +1062,7 @@ public class SavePasswordsPreferencesTest {
                 .check(matches(isDisplayed()));
 
         // Now pretend that passwords have been serialized.
-        mHandler.getExportCallback().onResult(new byte[] {1, 2, 3});
+        mHandler.getExportCallback().onResult(new byte[] {5, 6, 7}, 12);
 
         // After simulating the serialized passwords being received, check that the progress bar is
         // hidden.

chrome/browser/BUILD.gn

@@ -1870,6 +1870,8 @@ jumbo_split_static_library("browser") {
       "android/browsing_data/browsing_data_counter_bridge.h",
       "android/browsing_data/url_filter_bridge.cc",
       "android/browsing_data/url_filter_bridge.h",
+      "android/byte_array_int_callback.cc",
+      "android/byte_array_int_callback.h",
       "android/chrome_backup_agent.cc",
       "android/chrome_backup_agent.h",
       "android/chrome_backup_watcher.cc",
@@ -4386,6 +4388,7 @@ if (is_android) {
       "../android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java",
       "../android/java/src/org/chromium/chrome/browser/preferences/PreferencesLauncher.java",
       "../android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java",
+      "../android/java/src/org/chromium/chrome/browser/preferences/password/ByteArrayIntCallback.java",
       "../android/java/src/org/chromium/chrome/browser/preferences/password/PasswordUIView.java",
       "../android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java",
       "../android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataCounterBridge.java",

chrome/browser/android/byte_array_int_callback.cc

+// Copyright 2018 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.
+
+// This file encapsulates the JNI headers generated for ByteArrayIntCallback, so
+// that the methods defined in the generated headers only end up in one object
+// file. This is similar to //base/android/callback_android.*.
+
+#include "chrome/browser/android/byte_array_int_callback.h"
+
+#include "base/android/jni_array.h"
+#include "jni/ByteArrayIntCallback_jni.h"
+
+void RunByteArrayIntCallback(JNIEnv* env,
+                             const base::android::JavaRef<jobject>& callback,
+                             const uint8_t* byte_array_arg,
+                             size_t byte_array_arg_size,
+                             int int_arg) {
+  Java_ByteArrayIntCallback_onResult(
+      env, callback,
+      base::android::ToJavaByteArray(env, byte_array_arg, byte_array_arg_size),
+      int_arg);
+}

chrome/browser/android/byte_array_int_callback.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_ANDROID_BYTE_ARRAY_INT_CALLBACK_H_
+#define CHROME_BROWSER_ANDROID_BYTE_ARRAY_INT_CALLBACK_H_
+
+#include <jni.h>
+#include <stddef.h>
+#include <stdint.h>
+
+#include "base/android/scoped_java_ref.h"
+
+// Runs the Java |callback| by calling its onResult method and passing the byte
+// array and |int_arg| as its arguments.
+void RunByteArrayIntCallback(JNIEnv* env,
+                             const base::android::JavaRef<jobject>& callback,
+                             const uint8_t* byte_array_arg,
+                             size_t byte_array_arg_size,
+                             int int_arg);
+
+#endif  // CHROME_BROWSER_ANDROID_BYTE_ARRAY_INT_CALLBACK_H_

chrome/browser/android/password_ui_view_android.cc

@@ -5,9 +5,9 @@
 #include "chrome/browser/android/password_ui_view_android.h"
 
 #include <algorithm>
+#include <memory>
+#include <vector>
 
-#include "base/android/callback_android.h"
-#include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
@@ -15,6 +15,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/strings/string_piece.h"
 #include "base/task_scheduler/post_task.h"
+#include "chrome/browser/android/byte_array_int_callback.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/autofill/core/common/password_form.h"
@@ -188,20 +189,23 @@ static jlong JNI_PasswordUIView_Init(JNIEnv* env,
   return reinterpret_cast<intptr_t>(controller);
 }
 
-std::string PasswordUIViewAndroid::ObtainAndSerializePasswords() {
+PasswordUIViewAndroid::SerializationResult
+PasswordUIViewAndroid::ObtainAndSerializePasswords() {
   // This is run on a backend task runner. Do not access any member variables
   // except for |credential_provider_| and |password_manager_presenter_|.
   password_manager::CredentialProviderInterface* const provider =
       credential_provider_for_testing_ ? credential_provider_for_testing_
                                        : &password_manager_presenter_;
 
-  return password_manager::PasswordCSVWriter::SerializePasswords(
-      provider->GetAllPasswords());
+  std::vector<std::unique_ptr<autofill::PasswordForm>> passwords =
+      provider->GetAllPasswords();
+  return {password_manager::PasswordCSVWriter::SerializePasswords(passwords),
+          static_cast<int>(passwords.size())};
 }
 
 void PasswordUIViewAndroid::PostSerializedPasswords(
     const base::android::JavaRef<jobject>& callback,
-    std::string serialized_passwords) {
+    SerializationResult serialized_passwords) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   switch (state_) {
     case State::ALIVE:
@@ -212,13 +216,11 @@ void PasswordUIViewAndroid::PostSerializedPasswords(
       if (export_target_for_testing_) {
         *export_target_for_testing_ = serialized_passwords;
       } else {
-        JNIEnv* env = base::android::AttachCurrentThread();
-        base::android::RunCallbackAndroid(
-            callback,
-            base::android::ToJavaByteArray(
-                env,
-                reinterpret_cast<const uint8_t*>(serialized_passwords.data()),
-                serialized_passwords.size()));
+        RunByteArrayIntCallback(
+            base::android::AttachCurrentThread(), callback,
+            reinterpret_cast<const uint8_t*>(serialized_passwords.data.data()),
+            serialized_passwords.data.size(),
+            serialized_passwords.entries_count);
       }
       break;
     }

chrome/browser/android/password_ui_view_android.h

@@ -29,6 +29,13 @@ class CredentialProviderInterface;
 // PasswordManagerPresenter.
 class PasswordUIViewAndroid : public PasswordUIView {
  public:
+  // Result of transforming a vector of PasswordForms into their CSV
+  // description, storing the size of that vector in |entries_count|.
+  struct SerializationResult {
+    std::string data;
+    int entries_count;
+  };
+
   PasswordUIViewAndroid(JNIEnv* env, jobject);
   ~PasswordUIViewAndroid() override;
 
@@ -69,7 +76,8 @@ class PasswordUIViewAndroid : public PasswordUIView {
   // Destroy the native implementation.
   void Destroy(JNIEnv*, const base::android::JavaParamRef<jobject>&);
 
-  void set_export_target_for_testing(std::string* export_target_for_testing) {
+  void set_export_target_for_testing(
+      SerializationResult* export_target_for_testing) {
     export_target_for_testing_ = export_target_for_testing;
   }
 
@@ -101,11 +109,11 @@ class PasswordUIViewAndroid : public PasswordUIView {
   // and then PasswordCSVWriter to serialize them. It returns the serialized
   // value. Both steps involve a lot of memory allocation and copying, so this
   // method should be executed on a suitable task runner.
-  std::string ObtainAndSerializePasswords();
+  SerializationResult ObtainAndSerializePasswords();
 
   // Sends |serialized_passwords| to Java via |callback|.
   void PostSerializedPasswords(const base::android::JavaRef<jobject>& callback,
-                               std::string serialized_passwords);
+                               SerializationResult serialized_passwords);
 
   // The |state_| must only be accessed on the main task runner.
   State state_ = State::ALIVE;
@@ -113,7 +121,7 @@ class PasswordUIViewAndroid : public PasswordUIView {
   // If not null, PostSerializedPasswords will write the serialized passwords to
   // |*export_target_for_testing_| instead of passing them to Java. This must
   // remain null in production code.
-  std::string* export_target_for_testing_ = nullptr;
+  SerializationResult* export_target_for_testing_ = nullptr;
 
   PasswordManagerPresenter password_manager_presenter_;
 

chrome/browser/android/password_ui_view_android_unittest.cc

@@ -123,14 +123,15 @@ TEST_F(PasswordUIViewAndroidTest, GetSerializedPasswords) {
   std::unique_ptr<PasswordUIViewAndroid, PasswordUIViewAndroidDestroyDeleter>
       password_ui_view(
           new PasswordUIViewAndroid(env_, JavaParamRef<jobject>(nullptr)));
-  std::string serialized_passwords;
+  PasswordUIViewAndroid::SerializationResult serialized_passwords;
   password_ui_view->set_export_target_for_testing(&serialized_passwords);
   password_ui_view->set_credential_provider_for_testing(&provider);
   password_ui_view->HandleSerializePasswords(
       env_, JavaParamRef<jobject>(nullptr), JavaParamRef<jobject>(nullptr));
 
   content::RunAllTasksUntilIdle();
-  EXPECT_EQ(expected_result, serialized_passwords);
+  EXPECT_EQ(expected_result, serialized_passwords.data);
+  EXPECT_EQ(1, serialized_passwords.entries_count);
 }
 
 // Test that destroying the PasswordUIView when tasks are pending does not lead
@@ -142,7 +143,9 @@ TEST_F(PasswordUIViewAndroidTest, GetSerializedPasswords_Cancelled) {
   std::unique_ptr<PasswordUIViewAndroid, PasswordUIViewAndroidDestroyDeleter>
       password_ui_view(
           new PasswordUIViewAndroid(env_, JavaParamRef<jobject>(nullptr)));
-  std::string serialized_passwords = "this should not get overwritten";
+  PasswordUIViewAndroid::SerializationResult serialized_passwords;
+  serialized_passwords.data = "this should not get overwritten";
+  serialized_passwords.entries_count = 123;
   password_ui_view->set_export_target_for_testing(&serialized_passwords);
   password_ui_view->set_credential_provider_for_testing(&provider);
   password_ui_view->HandleSerializePasswords(
@@ -154,7 +157,8 @@ TEST_F(PasswordUIViewAndroidTest, GetSerializedPasswords_Cancelled) {
   password_ui_view.reset();
   // Now run the background tasks (and the subsequent deletion).
   content::RunAllTasksUntilIdle();
-  EXPECT_EQ("this should not get overwritten", serialized_passwords);
+  EXPECT_EQ("this should not get overwritten", serialized_passwords.data);
+  EXPECT_EQ(123, serialized_passwords.entries_count);
 }
 
 }  //  namespace android