Prepare ClearBrowsingDataDialogFragment for browsing data counters.

chrome/browser/android/preferences/pref_service_bridge.cc

Index: chrome/browser/android/preferences/pref_service_bridge.cc
diff --git a/chrome/browser/android/preferences/pref_service_bridge.cc b/chrome/browser/android/preferences/pref_service_bridge.cc
index df1c27c22efd31d592eb1b53987e5abd1182a51f..5b864d446ea699f3b5d25c6f6afa14ed2c3524e4 100644
--- a/chrome/browser/android/preferences/pref_service_bridge.cc
+++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -6,9 +6,11 @@
 
 #include <jni.h>
 #include <stddef.h>
+#include <vector>
 
 #include "base/android/build_info.h"
 #include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/files/file_path.h"
@@ -18,6 +20,7 @@
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browsing_data/browsing_data_counter_utils.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -492,15 +495,49 @@ class ClearBrowsingDataObserver : public BrowsingDataRemover::Observer {
  private:
   JavaObjectWeakGlobalRef weak_chrome_native_preferences_;
 };
+
 }  // namespace
 
+static jboolean GetBrowsingDataDeletionPreference(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    jint data_type) {
+  DCHECK_GE(data_type, 0);
+  DCHECK_LE(data_type, ClankBrowsingDataType::NUM_TYPES);
+
+  // If there is no corresponding preference for this |data_type|, pretend
+  // that it's set to false.
+  // TODO(msramek): Consider defining native-side preferences for all Java UI
+  // data types for consistency.
+  std::string pref;
+  if (GetDeletionPreferenceFromDataType(
+      static_cast<ClankBrowsingDataType>(data_type), &pref)) {
+    return false;
+  }
+
+  return GetOriginalProfile()->GetPrefs()->GetBoolean(pref);
+}
+
+static void SetBrowsingDataDeletionPreference(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    jint data_type,
+    jboolean value) {
+  DCHECK_GE(data_type, 0);
+  DCHECK_LE(data_type, ClankBrowsingDataType::NUM_TYPES);
+
+  std::string pref;
+  if (!GetDeletionPreferenceFromDataType(
+      static_cast<ClankBrowsingDataType>(data_type), &pref)) {
+    return;
+  }
+
+  GetOriginalProfile()->GetPrefs()->SetBoolean(pref, value);
+}
+
 static void ClearBrowsingData(JNIEnv* env,
                               const JavaParamRef<jobject>& obj,
-                              jboolean history,
-                              jboolean cache,
-                              jboolean cookies_and_site_data,
-                              jboolean passwords,
-                              jboolean form_data) {
+                              const JavaParamRef<jintArray>& data_types) {
   // BrowsingDataRemover deletes itself.
   BrowsingDataRemover* browsing_data_remover =
       BrowsingDataRemover::CreateForPeriod(
@@ -509,19 +546,33 @@ static void ClearBrowsingData(JNIEnv* env,
               BrowsingDataRemover::EVERYTHING));
   browsing_data_remover->AddObserver(new ClearBrowsingDataObserver(env, obj));
 
+  std::vector<int> data_types_vector;
+  base::android::JavaIntArrayToIntVector(env, data_types, &data_types_vector);
+
   int remove_mask = 0;
-  if (history)
-    remove_mask |= BrowsingDataRemover::REMOVE_HISTORY;
-  if (cache)
-    remove_mask |= BrowsingDataRemover::REMOVE_CACHE;
-  if (cookies_and_site_data) {
-    remove_mask |= BrowsingDataRemover::REMOVE_COOKIES;
-    remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA;
+  for (const int data_type : data_types_vector) {

[newt (away), 2016/01/12 18:55:26]

Rather than converting from ClankBrowsingDataType values to the
BrowsingDataRemover values, why not just generate a Java enum from
BrowsingDataRemover and get rid of ClankBrowsingDataType altogether? Then this
loop could simply OR together all the values. Or you could pass a single int
containing the bitwise OR of all the desired BrowsingDataRemover values to
clear.

Like you mentioned, it would be nice to reduce the number of different ways of
representing these same values.

[msramek, 2016/01/13 15:27:53]

Hmm, that's a bit tricky.

I would like to have reasonable keys (e.g. enum values) for the data types that
user sees in the UI.

On desktop, the UI has checkboxes that are tied to prefs::kDelete* boolean
preferences, so I mostly used those preferences as keys (you can see that
BrowsingDataCounter takes these prefs in the constructor). I really *should
have* defined such an enum in the beginning, because now it's getting awkward.

So this time, I wanted to get it right at least on Clank by defining such an
enum there (ClankBrowsingDataType). I think we should simplify things by reusing
the same enum on Desktop and Android (and later on iOS). There are some
differences (DOWNLOADS only exists on Desktop; BOOKMARKS only on Clank) that I'm
willing to live with, but most data types are the same.

I added a TODO to reuse BrowsingDataType in the Desktop UI. That shouldn't be a
part of this CL though, as it requires some refactoring.

Regarding the use of BrowsingDataRemover::RemoveDataMask, I'm not so sure right
now. First, it's not a one-to-one correspondence to the user-facing options,
since cookies are represented by REMOVE_COOKIES and REMOVE_SITE_DATA, and the
latter is in fact a convenience bitwise OR of several other options. We would
still have to add stuff like BOOKMARKS, which are currently not defined there
(as they are deleted directly from the Java code on Android). Also, semantics -
we want to enumerate the data types and count them, but not remove them; we'd
have to rename it and remove it from BrowsingDataRemover. I'm not entirely
opposed to that, but it will require more thought and again, some refactoring.

I filed crbug.com/577198.

[newt (away), 2016/01/13 20:01:49]

Thanks for the detailed explanation.

I hadn't looked closely at BrowsingDataRemover; now I see that it doesn't
correspond well to the user-visible data types.

Reusing BrowsingDataType on other platforms sounds great.

As for bookmarks being handled separately on Android: I would love to unify the
deletion of bookmarks on Android with other platforms. In particular, on Android
we currently use ChromeBrowserProviderClient.removeAllUserBookmarks() in
ClearSyncDataDialogFragment to delete bookmarks, but the
ChromeBrowserProviderClient API is deprecated and will eventually be removed. It
looks like bookmark deletion isn't currently handled by BrowsingDataRemover,
though? Is it not possible to delete all bookmarks on other platforms?

[msramek, 2016/01/14 15:45:43]

On Android, we show bookmarks only in the ClearSyncDataDialogFragment, which is
a special case of the CBD dialog that we show to users when they disconnect one
profile and connect another. In that case, we suggest them to delete everything
from the old profile to avoid merging the old and new data.

In the regular CBD dialog, on all platforms, bookmarks are not included. I think
the idea is that bookmarks are somewhat more valuable, so you don't want to
clean them often. Also, unlike most other browsing data types, they are not
automatically recorded, but very explicitly set. 

But I agree that the deletion could be handled by BrowsingDataRemover, so that
everything is in one place. I can take care of that soon-ish.

+    switch (static_cast<ClankBrowsingDataType>(data_type)) {
+      case HISTORY:
+        remove_mask |= BrowsingDataRemover::REMOVE_HISTORY;
+        break;
+      case CACHE:
+        remove_mask |= BrowsingDataRemover::REMOVE_CACHE;
+        break;
+      case COOKIES:
+        remove_mask |= BrowsingDataRemover::REMOVE_COOKIES;
+        remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA;
+        break;
+      case PASSWORDS:
+        remove_mask |= BrowsingDataRemover::REMOVE_PASSWORDS;
+        break;
+      case FORM_DATA:
+        remove_mask |= BrowsingDataRemover::REMOVE_FORM_DATA;
+        break;
+      default:
+        NOTREACHED();
+    }
   }
-  if (passwords)
-    remove_mask |= BrowsingDataRemover::REMOVE_PASSWORDS;
-  if (form_data)
-    remove_mask |= BrowsingDataRemover::REMOVE_FORM_DATA;
+
   browsing_data_remover->Remove(remove_mask,
                                 BrowsingDataHelper::UNPROTECTED_WEB);
 }