Prepare ClearBrowsingDataDialogFragment for browsing data counters.

chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
index 1f763520c030c36594c2255aa216bee03d364622..9f138495fc18dc2ea38d1c8e4ca7abe7b4b3852c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragment.java
@@ -22,6 +22,7 @@ import android.widget.AdapterView.OnItemClickListener;
 import android.widget.ArrayAdapter;
 import android.widget.Button;
 import android.widget.CheckedTextView;
+import android.widget.LinearLayout;
 import android.widget.TextView;
 
 import org.chromium.base.VisibleForTesting;
@@ -41,16 +42,88 @@ import java.util.EnumSet;
  */
 public class ClearBrowsingDataDialogFragment extends DialogFragment
         implements PrefServiceBridge.OnClearBrowsingDataListener, DialogInterface.OnClickListener {
-    private class ClearBrowsingDataAdapter extends ArrayAdapter<String> {
-        final DialogOption[] mOptions;
-        final EnumSet<DialogOption> mDisabledOptions;
-
-        private ClearBrowsingDataAdapter(DialogOption[] options, String[] optionNames,
-                EnumSet<DialogOption> disabledOptions) {
-            super(getActivity(), R.layout.select_dialog_multichoice_material, optionNames);
-            assert options.length == optionNames.length;
-            mOptions = options;
-            mDisabledOptions = disabledOptions;
+    /**
+     * Represents a single item in the dialog.
+     */
+    public class Item {

[newt (away), 2015/12/17 22:54:59]

Make this static and private. (See comment about adding a
ClearBrowsingDataCounterCallback interface in PrefServiceBridge.)

[msramek, 2016/01/11 11:39:40]

Done.

+        private final int mIndex;
+        private final DialogOption mOption;
+        private String mTitle;
+        private String mCounterText;
+        private long mCounter;
+        private boolean mSelected;
+        private boolean mDisabled;
+        private ArrayAdapter<Item> mParentAdapter;
+
+        public Item(int index,
+                    DialogOption option,
+                    String title,
+                    boolean selected,
+                    boolean disabled) {
+            super();
+            mIndex = index;
+            mOption = option;
+            mTitle = title;
+            mSelected = selected;
+            mDisabled = disabled;
+            mCounter = PrefServiceBridge.getInstance().createBrowsingDataCounter(this, mIndex);
+            mCounterText = "";
+            mParentAdapter = null;
+        }
+
+        public DialogOption getOption() {
+            return mOption;
+        }
+
+        @Override
+        public String toString() {
+            return mTitle;
+        }
+
+        public boolean isSelected() {
+            return mSelected;
+        }
+
+        public void toggle() {
+            mSelected = !mSelected;
+            PrefServiceBridge.getInstance().setBrowsingDataDeletionPreference(mIndex, mSelected);
+
+            // Counter text is only shown with selected items.
+            if (!mSelected) mCounterText = "";
+        }
+
+        public boolean isDisabled() {

[newt (away), 2015/12/17 22:54:58]

I'd flip this around to isEnabled() for consistency with other APIs that you're
interacting with. It also avoids double negatives, i.e. if (!isDisabled())

[msramek, 2016/01/11 11:39:40]

Done.

+            return mDisabled;
+        }
+
+        public String getCounterText() {
+            return mCounterText;
+        }
+
+        public void setParentAdapter(ArrayAdapter<Item> adapter) {
+            mParentAdapter = adapter;
+        }
+
+        public void onCounterFinished(String result) {
+            mCounterText = result;
+
+            if (mParentAdapter != null) mParentAdapter.notifyDataSetChanged();
+        }
+
+        @Override
+        protected void finalize() throws Throwable {

[newt (away), 2015/12/17 22:54:58]

Avoid finalize(). finalize() reduces garbage collection efficiency, runs on a
different thread (which is the most serious problem here), and swallows all
exceptions silently. 

Also see https://google.github.io/styleguide/javaguide.html#s6.4-finalizers

Instead, use the DialogFragment's onDestroy() method to clean up resources. E.g.
add a destroy() method to Item and call it from DialogFragment.onDestroy().

[msramek, 2016/01/11 11:39:41]

Done.

+            try {
+                PrefServiceBridge.getInstance().destroyBrowsingDataCounter(mCounter);
+            } finally {
+                super.finalize();
+            }
+        }
+    }
+
+    private class ClearBrowsingDataAdapter extends ArrayAdapter<Item> {
+        private ClearBrowsingDataAdapter(Item[] items) {
+            super(getActivity(), R.layout.checked_text_view_with_footer, R.id.text, items);
+            for (Item item : items) item.setParentAdapter(this);
         }
 
         @Override
@@ -60,25 +133,29 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
 
         @Override
         public View getView(int position, View convertView, ViewGroup parent) {
-            CheckedTextView view = (CheckedTextView) super.getView(position, convertView, parent);
-            DialogOption option = mOptions[position];
-            view.setChecked(mSelectedOptions.contains(option));
-            view.setEnabled(!mDisabledOptions.contains(option));
+            LinearLayout view = (LinearLayout) super.getView(position, convertView, parent);
+            CheckedTextView radioButton = (CheckedTextView) view.findViewById(R.id.text);
+            radioButton.setChecked(getItem(position).isSelected());
+            radioButton.setEnabled(!getItem(position).isDisabled());
+            TextView counter = (TextView) view.findViewById(R.id.footer);
+            String counterText = getItem(position).getCounterText();
+            counter.setText(counterText);
+
+            // Remove the counter if the text is empty, so when the counters experiment is
+            // disabled, it doesn't break alignment.
+            counter.setVisibility(counterText.isEmpty() ? View.GONE : View.VISIBLE);
+
             return view;
         }
 
         public void onClick(int position) {
-            DialogOption selectedOption = mOptions[position];
-            if (mDisabledOptions.contains(selectedOption)) {
+            if (getItem(position).isDisabled()) {
+                // Currently only the history deletion can be disabled.
                 Toast.makeText(getActivity(), R.string.can_not_clear_browsing_history_toast,
                               Toast.LENGTH_SHORT).show();
                 return;
             }
-            if (mSelectedOptions.contains(selectedOption)) {
-                mSelectedOptions.remove(selectedOption);
-            } else {
-                mSelectedOptions.add(selectedOption);
-            }
+            getItem(position).toggle();
             updateButtonState();
             notifyDataSetChanged();
         }
@@ -117,15 +194,23 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     private ProgressDialog mProgressDialog;
     private ClearBrowsingDataAdapter mAdapter;
     private boolean mCanDeleteBrowsingHistory;
-    private EnumSet<DialogOption> mSelectedOptions;
+    private Item[] mItems;
 
-    protected final void clearBrowsingData(EnumSet<DialogOption> selectedOptions) {
+    protected final EnumSet<DialogOption> getSelectedOptions() {
+        EnumSet<DialogOption> selected = EnumSet.noneOf(DialogOption.class);
+        for (Item item : mItems) {
+            if (item.isSelected()) selected.add(item.getOption());
+        }
+        return selected;
+    }
+
+    protected final void clearBrowsingData() {
         PrefServiceBridge.getInstance().clearBrowsingData(this,
-                selectedOptions.contains(DialogOption.CLEAR_HISTORY),
-                selectedOptions.contains(DialogOption.CLEAR_CACHE),
-                selectedOptions.contains(DialogOption.CLEAR_COOKIES_AND_SITE_DATA),
-                selectedOptions.contains(DialogOption.CLEAR_PASSWORDS),
-                selectedOptions.contains(DialogOption.CLEAR_FORM_DATA));
+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(0),

[newt (away), 2015/12/17 22:54:59]

Please refactor to avoid magic numbers. I think it makes sense to add an "index"
field to DialogOption that corresponds to the indices of the pref_names array
defined in pref_service_bridge.cc. (You could also consider used a shared
C++/Java enum for this purposes. Grep for GENERATED_JAVA_ENUM_PACKAGE in
most_visited_sites.cc for an example.)

Also, since you're already using indices to communicate across the language
boundary, you might as well make PrefServiceBridge.clearBrowsingData() take a
List<Integer> indicating which data types to clear, rather than taking a bunch
of booleans, whose order could easily be mixed up. (in C++ you can use a method
in jni_array.h to convert List<Integer> to a vector<int>)

[msramek, 2016/01/11 11:39:40]

Done. Shared enum between C++ and Java is pretty much what I wanted to do, I
just didn't know it was possible. Added enum ClankBrowsingDataType.

I'm a bit unhappy that we're doing a DialogOption (Java) ->
int/ClankBrowsingDataType (JNI) -> pref (C++) translations everywhere, basically
having three ways to represent the same thing, but I at least tried to polish it
so that variable names make it clear which one of the three is needed where.

+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(1),
+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(2),
+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(3),
+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(4));
     }
 
     protected void dismissProgressDialog() {
@@ -154,24 +239,16 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
      * Get the default selections for the dialog.
      * @return EnumSet containing dialog options to be selected.
      */
-    protected EnumSet<DialogOption> getDefaultDialogOptionsSelections() {
-        EnumSet<DialogOption> defaultOptions = EnumSet.of(DialogOption.CLEAR_CACHE,
-                DialogOption.CLEAR_COOKIES_AND_SITE_DATA, DialogOption.CLEAR_HISTORY);
-        if (!mCanDeleteBrowsingHistory) {
-            defaultOptions.remove(DialogOption.CLEAR_HISTORY);
-        }
-        return defaultOptions;
-    }
-
-    /**
-     * Get the selections that have been disabled for the dialog.
-     * @return EnumSet containing dialog options that have been disabled.
-     */
-    protected EnumSet<DialogOption> getDisabledDialogOptions() {
-        if (!mCanDeleteBrowsingHistory) {
-            return EnumSet.of(DialogOption.CLEAR_HISTORY);
+    protected boolean isOptionSelectedByDefault(DialogOption option) {
+        switch (option) {
+            case CLEAR_HISTORY:
+                return mCanDeleteBrowsingHistory;
+            case CLEAR_COOKIES_AND_SITE_DATA:
+            case CLEAR_CACHE:
+                return true;
+            default:
+                return false;
         }
-        return EnumSet.noneOf(DialogOption.class);
     }
 
     // Called when "clear browsing data" completes.
@@ -185,7 +262,7 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     public void onClick(DialogInterface dialog, int whichButton) {
         if (whichButton == AlertDialog.BUTTON_POSITIVE) {
             dismissProgressDialog();
-            onOptionSelected(mSelectedOptions);
+            onOptionSelected();
         }
     }
 
@@ -195,7 +272,7 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     private void updateButtonState() {
         Button clearButton = mDialog.getButton(AlertDialog.BUTTON_POSITIVE);
         if (clearButton == null) return;
-        boolean isEnabled = !mSelectedOptions.isEmpty();
+        boolean isEnabled = !getSelectedOptions().isEmpty();
         clearButton.setEnabled(isEnabled);
 
         // Work around a bug in the app compat library where disabled buttons in alert dialogs
@@ -209,13 +286,17 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
         mCanDeleteBrowsingHistory = PrefServiceBridge.getInstance().canDeleteBrowsingHistory();
 
         DialogOption[] options = getDialogOptions();
-        String[] optionNames = new String[options.length];
+        mItems = new Item[options.length];
         Resources resources = getResources();
         for (int i = 0; i < options.length; i++) {
-            optionNames[i] = resources.getString(options[i].getResourceId());
+            mItems[i] = new Item(
+                i,
+                options[i],
+                resources.getString(options[i].getResourceId()),
+                PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(i),

[newt (away), 2015/12/17 22:54:59]

This line assumes that items in the "options" have the same order as the
pref_array has items at the same index as the pref_names array in
pref_service_bridge.cc, which is a dangerous assumption. This could also be
fixed by adding an "index" field to DialogOptions, as mentioned above.

[msramek, 2016/01/11 11:39:41]

Done. I called it "datatype" instead, so the semantics are clearer: the index of
a DialogOption in the menu is purely a matter of the Java UI. Therefore, what is
sent over the language boundary is not called "index", but "datatype", as it
describes which browsing data type is bound to that option. In practice, it's
still an int (and it still is the index, as I kept the ordering) but the options
now can be visually rearranged without breaking anything.

+                options[i] == DialogOption.CLEAR_HISTORY && !mCanDeleteBrowsingHistory);
         }
-        mSelectedOptions = getDefaultDialogOptionsSelections();
-        mAdapter = new ClearBrowsingDataAdapter(options, optionNames, getDisabledDialogOptions());
+        mAdapter = new ClearBrowsingDataAdapter(mItems);
         final AlertDialog.Builder builder =
                 new AlertDialog.Builder(getActivity(), R.style.AlertDialogTheme)
                         .setTitle(R.string.clear_browsing_data_title)
@@ -272,11 +353,10 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
 
     /**
      * Called when PositiveButton is clicked for the dialog.
-     * @param selectedOptions options which were selected.
      */
-    protected void onOptionSelected(final EnumSet<DialogOption> selectedOptions) {
+    protected void onOptionSelected() {
         showProgressDialog();
-        clearBrowsingData(selectedOptions);
+        clearBrowsingData();
     }
 
     protected final void showProgressDialog() {