Change the CBD dialog on Android to a PreferenceFragment

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 db814e177579224e59d5c75287a42953ea916b85..6159e13367cdaca937f84854742fb4687db9bbbe 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
@@ -4,70 +4,60 @@
 
 package org.chromium.chrome.browser.preferences.privacy;
 
-import android.app.Dialog;
-import android.app.DialogFragment;
 import android.app.ProgressDialog;
-import android.content.DialogInterface;
-import android.content.res.Resources;
 import android.os.Bundle;
-import android.support.v7.app.AlertDialog;
-import android.text.SpannableString;
-import android.text.TextPaint;
-import android.text.method.LinkMovementMethod;
-import android.text.style.ClickableSpan;
-import android.view.View;
-import android.view.ViewGroup;
-import android.widget.AdapterView;
-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 android.preference.CheckBoxPreference;
+import android.preference.Preference;
+import android.preference.PreferenceFragment;
 
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.BrowsingDataType;
+import org.chromium.chrome.browser.preferences.ButtonPreference;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
-import org.chromium.chrome.browser.preferences.Preferences;
 import org.chromium.chrome.browser.preferences.privacy.BrowsingDataCounterBridge.BrowsingDataCounterCallback;
-import org.chromium.chrome.browser.signin.AccountManagementFragment;
 import org.chromium.sync.signin.ChromeSigninController;
-import org.chromium.ui.text.SpanApplier;
-import org.chromium.ui.widget.Toast;
 
+import java.util.Arrays;
 import java.util.EnumSet;
 
 /**
  * Modal dialog with options for selection the type of browsing data
  * to clear (history, cookies), triggered from a preference.
  */
-public class ClearBrowsingDataDialogFragment extends DialogFragment
-        implements PrefServiceBridge.OnClearBrowsingDataListener, DialogInterface.OnClickListener {
+public class ClearBrowsingDataDialogFragment extends PreferenceFragment

[newt (away), 2016/01/29 20:16:02]

I'd call this "ClearBrowsingDataPreferences" to parallel the names of all the
other screens in preferences (MainPreferences, AutofillPreferences, etc).

(Please do the renaming as a separate patch after other comments have been
addressed so that reviewing is easier.)

[msramek, 2016/02/02 12:19:51]

Done. Renamed this, the corresponding XML, and also ClearSyncDataDialogFragment.

+        implements PrefServiceBridge.OnClearBrowsingDataListener,
+                   Preference.OnPreferenceClickListener {
     /**
      * Represents a single item in the dialog.
      */
-    private static class Item implements BrowsingDataCounterCallback {
+    private static class Item implements BrowsingDataCounterCallback,
+                                         Preference.OnPreferenceClickListener {
+        private final ClearBrowsingDataDialogFragment mParent;
         private final DialogOption mOption;
-        private String mTitle;
-        private String mCounterText;
-        private boolean mSelected;
-        private boolean mEnabled;
-        private ArrayAdapter<Item> mParentAdapter;
+        private final CheckBoxPreference mCheckbox;
         private BrowsingDataCounterBridge mCounter;
 
-        public Item(DialogOption option,
-                    String title,
+        public Item(ClearBrowsingDataDialogFragment parent,
+                    DialogOption option,
+                    CheckBoxPreference checkbox,
                     boolean selected,
                     boolean enabled) {
             super();
+            mParent = parent;
             mOption = option;
-            mTitle = title;
-            mSelected = selected;
-            mEnabled = enabled;
-            mCounterText = "";
-            mParentAdapter = null;
             mCounter = new BrowsingDataCounterBridge(this, mOption.getDataType());
+            mCheckbox = checkbox;
+
+            // Due to the connection with BrowsingDataCounter, the state of this preference is
+            // kept in the native PrefService, and need not be duplicated in SharedPreferences.
+            // Therefore, from SharedPreferences's perspective, this preference is not persistent.
+            mCheckbox.setPersistent(false);

[newt (away), 2016/01/29 20:16:02]

This might be too late to set persistent to false; at this point Android may
already have tried to read the default value from shared prefs. I'd probably set
android:persistent="false" directly in the XML.

(Side-note: I'd like to have a general solution to this issue. If you have
ideas, I'm all ears.)

[msramek, 2016/02/02 12:19:51]

Done.

I heard some vague plans about syncing preferences between desktop and mobile.
I'm not sure what is the current status. But if that happens, basically every
preference in Chrome's settings will have to defer to the native PrefService. So
the Sync team might have some ideas.

Perhaps inherit all preferences from a custom ChromePreference, where
ChromePreference.getSharedPreferences() will return ChromeSharedPreferences, a
subclass of SharedPreferences, that in fact delegates everything to the native
PrefService? SharedPreferences seems to have all methods abstract, so that
should be possible. Then put in some DCHECKS so that nobody uses
android.preference.Preference.

[newt (away), 2016/02/02 19:35:03]

These are good ideas. We already have Chrome-specific base classes for most
preference types (e.g. ChromeBaseCheckBoxPreference). We could setPersistent()
false in those, or possibly override the PreferenceManager or the
SharedPreferences, either at the Activity level or at the individual Preference
level.

+
+            mCheckbox.setOnPreferenceClickListener(this);
+            mCheckbox.setEnabled(enabled);
+            mCheckbox.setChecked(enabled && selected);

[newt (away), 2016/01/29 20:16:02]

Are there any cases where a checkbox could be force-checked? i.e. enabled=false
and selected=true? This line wouldn't handle that case correctly.

[msramek, 2016/02/02 12:19:51]

The only case for a disabled checkbox should be a supervised user that is not
allowed to delete their history, in which case the checkbox should be also
unchecked.

However, good that you pointed this out! My handling was incorrect. If it was
the case that the deletion preference was true, but history deletion was
disabled, this would simply show the checkbox as unchecked, but would not toggle
the deletion preference to false.

I removed this, and instead moved the handling to where
mCanDeleteBrowsingHistory is evaluated.

+            mCheckbox.setSummaryOff("");  // No summary when unchecked.
         }
 
         public void destroy() {
@@ -78,107 +68,60 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
             return mOption;
         }
 
-        @Override
-        public String toString() {
-            return mTitle;
-        }
-
         public boolean isSelected() {
-            return mSelected;
+            return mCheckbox.isChecked();
         }
 
-        public void toggle() {
-            mSelected = !mSelected;
-            PrefServiceBridge.getInstance().setBrowsingDataDeletionPreference(
-                    mOption.getDataType(), mSelected);
-
-            // Counter text is only shown with selected items.
-            if (!mSelected) mCounterText = "";
-        }
-
-        public boolean isEnabled() {
-            return mEnabled;
-        }
-
-        public String getCounterText() {
-            return mCounterText;
-        }
+        @Override
+        public boolean onPreferenceClick(Preference preference) {
+            assert mCheckbox == preference;
 
-        public void setParentAdapter(ArrayAdapter<Item> adapter) {
-            mParentAdapter = adapter;
+            mParent.updateButtonState();
+            PrefServiceBridge.getInstance().setBrowsingDataDeletionPreference(
+                    mOption.getDataType(), mCheckbox.isChecked());
+            return true;
         }
 
         @Override
         public void onCounterFinished(String result) {
-            mCounterText = result;
-
-            if (mParentAdapter != null) mParentAdapter.notifyDataSetChanged();
+            mCheckbox.setSummaryOn(result);
         }
     }
 
-    private class ClearBrowsingDataAdapter extends ArrayAdapter<Item> {
-        private ClearBrowsingDataAdapter(Item[] items) {
-            super(getActivity(), R.layout.clear_browsing_data_dialog_item, R.id.title, items);
-            for (Item item : items) item.setParentAdapter(this);
-        }
+    private static final String PREF_HISTORY = "clear_browsing_data_history_checkbox";
+    private static final String PREF_CACHE = "clear_browsing_data_cache_checkbox";
+    private static final String PREF_COOKIES = "clear_browsing_data_cookies_checkbox";
+    private static final String PREF_PASSWORDS = "clear_browsing_data_passwords_checkbox";
+    private static final String PREF_FORM_DATA = "clear_browsing_data_form_data_checkbox";
+    private static final String PREF_BOOKMARKS = "clear_browsing_data_bookmarks_checkbox";
 
-        @Override
-        public boolean hasStableIds() {
-            return true;
-        }
+    private static final String PREF_FOOTNOTE = "clear_browsing_data_footnote";
+    private static final String PREF_FOOTNOTE_SYNCED = "clear_browsing_data_footnote_synced";
 
-        @Override
-        public View getView(int position, View convertView, ViewGroup parent) {
-            LinearLayout view = (LinearLayout) super.getView(position, convertView, parent);
-            CheckedTextView radioButton = (CheckedTextView) view.findViewById(R.id.title);
-            radioButton.setChecked(getItem(position).isSelected());
-            radioButton.setEnabled(getItem(position).isEnabled());
-            TextView counter = (TextView) view.findViewById(R.id.summary);
-            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;
-        }
+    /** The "Clear" button preference. Referenced in tests. */
+    public static final String PREF_CLEAR_BUTTON = "clear_browsing_data_clear_button";
 
-        public void onClick(int position) {
-            if (!getItem(position).isEnabled()) {
-                // Currently only the history deletion can be disabled.
-                Toast.makeText(getActivity(), R.string.can_not_clear_browsing_history_toast,
-                              Toast.LENGTH_SHORT).show();
-                return;
-            }
-            getItem(position).toggle();
-            updateButtonState();
-            notifyDataSetChanged();
-        }
-    }
-
-    /** The tag used when showing the clear browsing fragment. */
-    public static final String FRAGMENT_TAG = "ClearBrowsingDataDialogFragment";
+    /** The tag used for logging. */
+    public static final String TAG = "ClearBrowsingDataDialogFragment";
 
     /**
      * Enum for Dialog options to be displayed in the dialog.
      */
     public enum DialogOption {
-        CLEAR_HISTORY(BrowsingDataType.HISTORY, R.string.clear_history_title),
-        CLEAR_CACHE(BrowsingDataType.CACHE, R.string.clear_cache_title),
-        CLEAR_COOKIES_AND_SITE_DATA(BrowsingDataType.COOKIES,
-                                    R.string.clear_cookies_and_site_data_title),
-        CLEAR_PASSWORDS(BrowsingDataType.PASSWORDS, R.string.clear_passwords_title),
-        CLEAR_FORM_DATA(BrowsingDataType.FORM_DATA, R.string.clear_formdata_title),
+        CLEAR_HISTORY(BrowsingDataType.HISTORY, PREF_HISTORY),
+        CLEAR_CACHE(BrowsingDataType.CACHE, PREF_CACHE),
+        CLEAR_COOKIES_AND_SITE_DATA(BrowsingDataType.COOKIES, PREF_COOKIES),
+        CLEAR_PASSWORDS(BrowsingDataType.PASSWORDS, PREF_PASSWORDS),
+        CLEAR_FORM_DATA(BrowsingDataType.FORM_DATA, PREF_FORM_DATA),
         // Clear bookmarks is only used by ClearSyncData dialog.
-        CLEAR_BOOKMARKS_DATA(BrowsingDataType.BOOKMARKS, R.string.clear_bookmarks_title);
+        CLEAR_BOOKMARKS_DATA(BrowsingDataType.BOOKMARKS, PREF_BOOKMARKS);
 
         private final int mDataType;
-        private final int mResourceId;
+        private final String mPreferenceKey;
 
-        private DialogOption(int dataType, int resourceId) {
+        private DialogOption(int dataType, String preferenceKey) {
             mDataType = dataType;
-            mResourceId = resourceId;
+            mPreferenceKey = preferenceKey;
         }
 
         public int getDataType() {
@@ -186,16 +129,14 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
         }
 
         /**
-         * @return resource id of the Dialog option.
+         * @return String The key of the corresponding preference.
          */
-        public int getResourceId() {
-            return mResourceId;
+        public String getPreferenceKey() {
+            return mPreferenceKey;
         }
     }
 
-    private AlertDialog mDialog;
     private ProgressDialog mProgressDialog;
-    private ClearBrowsingDataAdapter mAdapter;
     private boolean mCanDeleteBrowsingHistory;
     private Item[] mItems;
 
@@ -221,9 +162,9 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     }
 
     protected void dismissProgressDialog() {
-        android.util.Log.i(FRAGMENT_TAG, "in dismissProgressDialog");
+        android.util.Log.i(TAG, "in dismissProgressDialog");
         if (mProgressDialog != null && mProgressDialog.isShowing()) {
-            android.util.Log.i(FRAGMENT_TAG, "progress dialog dismissed");
+            android.util.Log.i(TAG, "progress dialog dismissed");
             mProgressDialog.dismiss();
         }
         mProgressDialog = null;
@@ -257,94 +198,71 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     @Override
     public void onBrowsingDataCleared() {
         dismissProgressDialog();
+        getActivity().finish();
     }
 
     @Override
-    public void onClick(DialogInterface dialog, int whichButton) {
-        if (whichButton == AlertDialog.BUTTON_POSITIVE) {
+    public boolean onPreferenceClick(Preference preference) {
+        if (preference.getKey().equals(PREF_CLEAR_BUTTON)) {
             dismissProgressDialog();
             onOptionSelected();
         }
+        return true;

[newt (away), 2016/01/29 20:16:02]

you should only return true if the click was handled

[msramek, 2016/02/02 12:19:51]

Done.

     }
 
     /**
      * Disable the "Clear" button if none of the options are selected. Otherwise, enable it.
      */
     private void updateButtonState() {
-        Button clearButton = mDialog.getButton(AlertDialog.BUTTON_POSITIVE);
+        ButtonPreference clearButton = (ButtonPreference) findPreference(PREF_CLEAR_BUTTON);
         if (clearButton == null) return;
         boolean isEnabled = !getSelectedOptions().isEmpty();
         clearButton.setEnabled(isEnabled);
-
-        // Work around a bug in the app compat library where disabled buttons in alert dialogs
-        // don't look disabled on pre-L devices. See: http://crbug.com/550784
-        // TODO(newt): remove this workaround when the app compat library is fixed (b/26017217)
-        clearButton.setTextColor(isEnabled ? 0xFF4285F4 : 0x335A5A5A);
     }
 
     @Override
-    public Dialog onCreateDialog(Bundle savedInstanceState) {
+    public void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
         mCanDeleteBrowsingHistory = PrefServiceBridge.getInstance().canDeleteBrowsingHistory();
+        addPreferencesFromResource(R.xml.clear_browsing_data_dialog);
 
         DialogOption[] options = getDialogOptions();
         mItems = new Item[options.length];
-        Resources resources = getResources();
         for (int i = 0; i < options.length; i++) {
             // It is possible to disable the deletion of browsing history.
             boolean enabled = options[i] != DialogOption.CLEAR_HISTORY || mCanDeleteBrowsingHistory;
 
             mItems[i] = new Item(
+                this,
                 options[i],
-                resources.getString(options[i].getResourceId()),
+                (CheckBoxPreference) findPreference(options[i].getPreferenceKey()),
                 isOptionSelectedByDefault(options[i]),
                 enabled);
         }
-        mAdapter = new ClearBrowsingDataAdapter(mItems);
-        final AlertDialog.Builder builder =
-                new AlertDialog.Builder(getActivity(), R.style.AlertDialogTheme)
-                        .setTitle(R.string.clear_browsing_data_title)
-                        .setPositiveButton(R.string.clear_data_delete, this)
-                        .setNegativeButton(R.string.cancel, this)
-                        .setAdapter(mAdapter, null);  // OnClickListener is registered manually.
 
+        // Not all checkboxes defined in the layout are necessarily handled by this class
+        // or a particular subclass. Hide those that are not.
+        EnumSet<DialogOption> unbound_options = EnumSet.allOf(DialogOption.class);

[newt (away), 2016/01/29 20:16:02]

s/upbound_options/unboundOptions/

(checkstyle should have complained about this on git cl upload...)

[msramek, 2016/02/02 12:19:51]

Done. If it's just a warning, I might have missed it.

+        unbound_options.removeAll(Arrays.asList(getDialogOptions()));
+        for (DialogOption option : unbound_options) {
+            getPreferenceScreen().removePreference(findPreference(option.getPreferenceKey()));
+        }
+
+        // The "Clear" button.
+        ButtonPreference clearButton = (ButtonPreference) findPreference(PREF_CLEAR_BUTTON);
+        clearButton.setOnPreferenceClickListener(this);
+        clearButton.setShouldDisableView(true);
+
+        // Only one footnote should be shown, the unsynced or synced version, depending on whether

[newt (away), 2016/01/29 20:16:02]

Rather than removing one of these two preferences, I'd use a single
TextMessagePreference and set its text programmatically here. Inflating views is
quite expensive (typically several ms), so it's best to avoid inflating views
that won't be used.

[msramek, 2016/02/02 12:19:51]

Done. And actually, now that there's just one, let me rename it to "summary".

+        // the user is signed in.
         if (ChromeSigninController.get(getActivity()).isSignedIn()) {
-            final String message = getString(R.string.clear_cookies_no_sign_out_summary);
-            final SpannableString messageWithLink = SpanApplier.applySpans(message,
-                    new SpanApplier.SpanInfo("<link>", "</link>", new ClickableSpan() {
-                        @Override
-                        public void onClick(View widget) {
-                            dismiss();
-                            Preferences prefActivity = (Preferences) getActivity();
-                            prefActivity.startFragment(AccountManagementFragment.class.getName(),
-                                    null);
-                        }
-
-                        // Change link formatting to use no underline
-                        @Override
-                        public void updateDrawState(TextPaint textPaint) {
-                            textPaint.setColor(textPaint.linkColor);
-                            textPaint.setUnderlineText(false);
-                        }
-                    }));
-
-            View view = getActivity().getLayoutInflater().inflate(
-                    R.layout.single_line_bottom_text_dialog, null);
-            TextView summaryView = (TextView) view.findViewById(R.id.summary);
-            summaryView.setText(messageWithLink);
-            summaryView.setMovementMethod(LinkMovementMethod.getInstance());
-            builder.setView(view);
+            getPreferenceScreen().removePreference(findPreference(PREF_FOOTNOTE));
+        } else {
+            getPreferenceScreen().removePreference(findPreference(PREF_FOOTNOTE_SYNCED));
         }
 
-        mDialog = builder.create();
-        mDialog.getListView().setOnItemClickListener(new OnItemClickListener() {
-            @Override
-            public void onItemClick(AdapterView<?> parent, View v, int position, long id) {
-                // The present behaviour of AlertDialog is to dismiss after the onClick event.
-                // Hence we are manually overriding this outside the builder.
-                mAdapter.onClick(position);
-            }
-        });
-        return mDialog;
+        // Activity title.
+        getActivity().setTitle(R.string.clear_browsing_data_title);

[newt (away), 2016/01/29 20:16:02]

nit: move this to near the top of onCreate()

[msramek, 2016/02/02 12:19:51]

Done.

     }
 
     @Override
@@ -357,6 +275,7 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     @Override
     public void onDestroy() {
         super.onDestroy();
+        dismissProgressDialog();
         for (Item item : mItems) {
             item.destroy();
         }
@@ -373,7 +292,7 @@ public class ClearBrowsingDataDialogFragment extends DialogFragment
     protected final void showProgressDialog() {
         if (getActivity() == null) return;
 
-        android.util.Log.i(FRAGMENT_TAG, "progress dialog shown");
+        android.util.Log.i(TAG, "progress dialog shown");
         mProgressDialog = ProgressDialog.show(getActivity(),
                 getActivity().getString(R.string.clear_browsing_data_progress_title),
                 getActivity().getString(R.string.clear_browsing_data_progress_message), true,