[Storage] Android - ManageSpace UI, Important Origins, and CBD Dialog

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

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
index 85b635d8a05751d684666212e9e1864308713c2f..a9cfa6c67138838cbbdfd48ae2b8c112ad295bf1 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
@@ -6,14 +6,18 @@ package org.chromium.chrome.browser.preferences.privacy;
 
 import android.app.Activity;
 import android.app.ProgressDialog;
+import android.content.Intent;
 import android.os.Bundle;
 import android.preference.Preference;
 import android.preference.PreferenceFragment;
+import android.support.annotation.Nullable;
 import android.widget.ListView;
 
+import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.BrowsingDataType;
+import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.TimePeriod;
 import org.chromium.chrome.browser.help.HelpAndFeedback;
 import org.chromium.chrome.browser.preferences.ButtonPreference;
@@ -36,10 +40,10 @@ import java.util.EnumSet;
  * from which to clear data.
  */
 public class ClearBrowsingDataPreferences extends PreferenceFragment
-        implements PrefServiceBridge.OnClearBrowsingDataListener,
+        implements PrefServiceBridge.ImportantSitesCallback,
+                   PrefServiceBridge.OnClearBrowsingDataListener,
                    PrefServiceBridge.OtherFormsOfBrowsingHistoryListener,
-                   Preference.OnPreferenceClickListener,
-                   Preference.OnPreferenceChangeListener {
+                   Preference.OnPreferenceClickListener, Preference.OnPreferenceChangeListener {
     /**
      * Represents a single item in the dialog.
      */
@@ -132,6 +136,9 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
     private static final String WEB_HISTORY_URL =
             "https://history.google.com/history/?utm_source=chrome_cbd";
 
+    /** Used for the onActivityResult pattern */

[Finnur, 2016/04/27 13:49:15]

Nit: Missing punctuation (the period).

But, I'd rather see the comment describe what the const is, than where it is
used (which is prone to become out of date).

[dmurph, 2016/04/27 20:36:47]

Done.

+    private static final int IMPORTANT_SITES_DIALOG_CODE = 10;
+
     /**
      * The various data types that can be cleared via this screen.
      */
@@ -199,6 +206,12 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
     private ProgressDialog mProgressDialog;
     private Item[] mItems;
 
+    // This means the user asked to clear, and now we're waiting until we get our sites back to do
+    // the clearing.
+    private boolean mWaitingForImportantSitesBeforeClear = false;
+    private String[] mSortedImportantDomains = null;
+    private ConfirmImportantSitesDialogFragment mConfirmImportantSitesDialog;

[Finnur, 2016/04/27 13:49:15]

nit: Document these two members.

[dmurph, 2016/04/27 20:36:47]

Done.

+
     private final EnumSet<DialogOption> getSelectedOptions() {
         EnumSet<DialogOption> selected = EnumSet.noneOf(DialogOption.class);
         for (Item item : mItems) {
@@ -211,7 +224,8 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
      * Requests the browsing data corresponding to the given dialog options to be deleted.
      * @param options The dialog options whose corresponding data should be deleted.

[Theresa, 2016/04/27 21:10:58]

Add a @param for blacklistedDomains. 

nit: in this file the param is blacklistedDomains and in PreferServiceBridge
it's blacklistDomains. It'd be nice if they matched.

      */
-    private final void clearBrowsingData(EnumSet<DialogOption> options) {
+    private final void clearBrowsingData(
+            EnumSet<DialogOption> options, @Nullable String[] blacklistedDomains) {
         showProgressDialog();
 
         int[] dataTypes = new int[options.size()];
@@ -224,7 +238,14 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
         Object spinnerSelection =
                 ((SpinnerPreference) findPreference(PREF_TIME_RANGE)).getSelectedOption();
         int timePeriod = ((TimePeriodSpinnerOption) spinnerSelection).getTimePeriod();
-        PrefServiceBridge.getInstance().clearBrowsingData(this, dataTypes, timePeriod);
+        if (blacklistedDomains != null && blacklistedDomains.length != 0) {
+            Log.i(TAG, "We're blacklisting some domains.");

[Finnur, 2016/04/27 13:49:15]

Nit: Remember to remove this debug statement before checking in. Same below.

[dmurph, 2016/04/27 20:36:47]

Yep, as mentioned in the email I still have to remove all of these.

+            PrefServiceBridge.getInstance().clearBrowsingDataExcludingDomains(
+                    this, dataTypes, timePeriod, blacklistedDomains);
+        } else {
+            Log.i(TAG, "No blacklist.");
+            PrefServiceBridge.getInstance().clearBrowsingData(this, dataTypes, timePeriod);
+        }
     }
 
     private void dismissProgressDialog() {
@@ -239,12 +260,9 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
      * order as they appear in the array.
      */
     private DialogOption[] getDialogOptions() {
-        return new DialogOption[] {
-            DialogOption.CLEAR_HISTORY,
-            DialogOption.CLEAR_COOKIES_AND_SITE_DATA,
-            DialogOption.CLEAR_CACHE,
-            DialogOption.CLEAR_PASSWORDS,
-            DialogOption.CLEAR_FORM_DATA};
+        return new DialogOption[] {DialogOption.CLEAR_HISTORY,
+                DialogOption.CLEAR_COOKIES_AND_SITE_DATA, DialogOption.CLEAR_CACHE,
+                DialogOption.CLEAR_PASSWORDS, DialogOption.CLEAR_FORM_DATA};

[Finnur, 2016/04/27 13:49:15]

Much less readable, in my opinion...

[dmurph, 2016/04/27 20:36:47]

Sorry, clang format fodder

     }
 
     /**
@@ -276,7 +294,7 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
      */
     private boolean isOptionSelectedByDefault(DialogOption option) {
         return PrefServiceBridge.getInstance().getBrowsingDataDeletionPreference(
-            option.getDataType());
+                option.getDataType());
     }
 
     /**
@@ -305,7 +323,12 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
     @Override
     public boolean onPreferenceClick(Preference preference) {
         if (preference.getKey().equals(PREF_CLEAR_BUTTON)) {
-            clearBrowsingData(getSelectedOptions());
+            if (mSortedImportantDomains == null) {

[Theresa, 2016/04/27 21:10:58]

Should we call clearBrowsingData(getSelectedOptions(), null); directly if the
experiment is disabled?

[dmurph, 2016/04/29 23:53:48]

Yes. Thanks, cleaned up.

+                mWaitingForImportantSitesBeforeClear = true;
+                showProgressDialog();
+            } else {
+                maybeShowImportantDialogThenClear();
+            }
             return true;
         }
         return false;
@@ -427,6 +450,7 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
             getPreferenceScreen().removePreference(google_summary);
             general_summary.setSummary(R.string.clear_browsing_data_footnote_site_settings);
         }
+        PrefServiceBridge.fetchImportantSites(this);

[Theresa, 2016/04/27 21:10:58]

Should we only fetch if the experiment is enabled?

[dmurph, 2016/04/29 23:53:48]

Yep, thanks, fixed.

     }
 
     @Override
@@ -462,6 +486,24 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
         return mProgressDialog;
     }
 
+    /**
+     * This method shows the important sites dialog if we're enabled and if we have important
+     * origins. If either of those is false, then it'll clear like normal. After the dialog is
+     * shown, we correctly clear.
+     */
+    private void maybeShowImportantDialogThenClear() {
+        if (mSortedImportantDomains.length == 0
+                || !ChromeFeatureList.isEnabled(ChromeFeatureList.IMPORTANT_SITES_IN_CBD)) {
+            clearBrowsingData(getSelectedOptions(), null);
+            return;
+        }
+        mConfirmImportantSitesDialog =
+                ConfirmImportantSitesDialogFragment.newInstance(mSortedImportantDomains);
+        mConfirmImportantSitesDialog.setTargetFragment(this, IMPORTANT_SITES_DIALOG_CODE);
+        mConfirmImportantSitesDialog.show(
+                getFragmentManager(), ConfirmImportantSitesDialogFragment.FRAGMENT_TAG);
+    }
+
     @Override
     public void showNoticeAboutOtherFormsOfBrowsingHistory() {
         if (getActivity() == null) return;
@@ -487,4 +529,33 @@ public class ClearBrowsingDataPreferences extends PreferenceFragment
     OtherFormsOfHistoryDialogFragment getDialogAboutOtherFormsOfBrowsingHistory() {
         return mDialogAboutOtherFormsOfBrowsingHistory;
     }
+    public static String strJoin(String[] aArr, String sSep) {

[Finnur, 2016/04/27 13:49:15]

Is this a debug function?

[dmurph, 2016/04/27 20:36:47]

Yeah, I forgot to remove it. done.

+        StringBuilder sbStr = new StringBuilder();
+        for (int i = 0, il = aArr.length; i < il; i++) {
+            if (i > 0) sbStr.append(sSep);
+            sbStr.append(aArr[i]);
+        }
+        return sbStr.toString();
+    }
+
+    @Override
+    public void onImportantRegisterableDomainsReady(String[] domains) {
+        mSortedImportantDomains = domains;
+        if (!mWaitingForImportantSitesBeforeClear) return;
+        dismissProgressDialog();
+        maybeShowImportantDialogThenClear();
+    }
+
+    /**
+     * This is the callback for the important domain dialog. We should only clear if we get the
+     * positive button response.
+     */
+    @Override
+    public void onActivityResult(int requestCode, int resultCode, Intent data) {
+        if (requestCode == IMPORTANT_SITES_DIALOG_CODE && resultCode == Activity.RESULT_OK) {
+            String[] deselectedDomains = data.getStringArrayExtra(
+                    ConfirmImportantSitesDialogFragment.DESELECTED_DOMAINS_TAG);
+            clearBrowsingData(getSelectedOptions(), deselectedDomains);

[Theresa, 2016/04/27 21:10:58]

Can you add a comment explaining what selected vs de-selected means? It's not
clear (without looking at other files) that the dialog prompts you to clear data
for checked sites.

[dmurph, 2016/04/29 23:53:48]

I added a comment here and on the tag.

+        }
+    }
 }