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

chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java
new file mode 100644
index 0000000000000000000000000000000000000000..e09e9a1eb517d80b06658aeada49b72980e8df1d
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ManageSpaceActivity.java
@@ -0,0 +1,254 @@
+// Copyright 2016 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.website;
+
+import android.app.ActivityManager;
+import android.content.Context;
+import android.content.DialogInterface;
+import android.content.Intent;
+import android.content.pm.ActivityInfo;
+import android.content.pm.PackageManager.NameNotFoundException;
+import android.os.Bundle;
+import android.support.v7.app.AlertDialog;
+import android.support.v7.app.AppCompatActivity;
+import android.text.format.Formatter;
+import android.view.View;
+import android.widget.Button;
+import android.widget.TextView;
+
+import org.chromium.base.Log;
+import org.chromium.base.library_loader.ProcessInitException;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
+import org.chromium.chrome.browser.preferences.Preferences;
+import org.chromium.chrome.browser.preferences.PreferencesLauncher;
+import org.chromium.chrome.browser.preferences.website.Website.StoredDataClearedCallback;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This is the target activity for the "Manage Storage" button in the Android Settings UI. This is
+ * configured in the AndroidManifest.xml file by setting the android:manageSpaceActivity.

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

nit: s/the AndroidManifest.xml file/AndroidManifest.xml
     s/the android:manageSpaceActivity/ android:manageSpaceActivity for the
application.

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

Done.

+ */
+public class ManageSpaceActivity extends AppCompatActivity implements View.OnClickListener {
+    private static final String TAG = "ManageSpaceActivity";
+
+    private View mClearAllDataSection;
+    private TextView mUnimportantSiteDataSizeText;
+    private TextView mSiteDataSizeText;
+    private Button mClearUnimportantButton;
+    private Button mManageSiteDataButton;
+    private Button mClearAllDataButton;
+
+    private static boolean sActivityNotExportedChecked;
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        ensureActivityNotExported();
+
+        // The browser process must be started here because this Activity may be started explicitly
+        // from Android settings, or when Android is restoring ManageSpaceActivity after Chrome was
+        // killed, or for tests. This should happen before super.onCreate() because it might
+        // recreate a fragment, and a fragment might depend on the native library.
+        try {
+            ChromeBrowserInitializer.getInstance(this).handleSynchronousStartup();

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

This should be done asynchronously, so that the UI doesn't block while Chrome is
starting up. Cold start of Chrome can take a while. Doing this synchronously may
cause ANRs.

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

I was doing this with BrowserStartupController#startBrowserProcessesAsync, but
that was kind of flaky (it worked initially, but now it doesn't work). Newt said
to use this, as it is what Preferences does I think? Let me know if you know a
better way of doing this, I can't find a common pattern.

[Ted C, 2016/04/30 00:04:18]

startBrowserProcessesAsync is what you want.

The sync startup is fine for preferences since it is really hard to get to
preferences w/o first going through Chrome, which will trigger the browser
process initialization already.

Here, we can trigger this from a general Android entry point that has no
previous Chrome paths, so I think the async thing is much more important.

In general, I want to rid the sync path entirely for UI components, but
first we need to stop adding more.

What about it was flaky?

Also, I think there are a couple ways you can handle the async path:

1.) <the easy less visually pleasant approach> Move everything after
your super.onCreate call into the StartupCallback#onSuccess.

2.) Inflate your UI, disable all your buttons initially.  Wait for
onSuccess and enable them and add your click listeners.

Then in onFailure, you do what you suggest in your next comment and
only enable the third/non-browser process requiring button.

+        } catch (ProcessInitException e) {
+            Log.e(TAG, "Failed to start browser process.", e);
+            // This can only ever happen, if at all, when the activity is started from an Android

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

While this is true for Preferences.java (where this was copied), it's probably
not true for this class. If this fails, the browser process is in a bad way, but
the user should be able to clear Chrome data without the browser process
starting up. Is the browser process only needed to get local storage data size
or is being used in other ways? We should have a fallback here rather than
exiting.

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

Yeah we need it to have functionality for the first two buttons (clear
unimportant, manage storage) to work. We could just disable those and still have
the third one - Clear all data - enabled. How does that sound? I did that.

[Theresa, 2016/05/03 00:04:27]

That sounds great.

+            // notification (or in tests). As such we don't want to show an error messsage to the
+            // user. The application is completely broken at this point, so close it down
+            // completely (not just the activity).
+            System.exit(-1);
+            return;
+        }
+        super.onCreate(savedInstanceState);
+
+        setContentView(R.layout.manage_space_activity);
+
+        mSiteDataSizeText = (TextView) findViewById(R.id.site_data_storage_size_text);

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

This is for all sites?

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

Yes.

+        mSiteDataSizeText.setText(R.string.storage_management_computing_size);
+        mUnimportantSiteDataSizeText =
+                (TextView) findViewById(R.id.unimportant_site_data_storage_size_text);
+        mUnimportantSiteDataSizeText.setText(R.string.storage_management_computing_size);
+        mManageSiteDataButton = (Button) findViewById(R.id.manage_site_data_storage);
+        mManageSiteDataButton.setOnClickListener(this);
+        mClearUnimportantButton = (Button) findViewById(R.id.clear_unimportant_site_data_storage);
+        mClearUnimportantButton.setOnClickListener(this);
+
+        mClearAllDataSection = findViewById(R.id.clear_all_data_section);
+        if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.KITKAT) {
+            mClearAllDataButton = (Button) findViewById(R.id.clear_all_data);
+            mClearAllDataButton.setOnClickListener(this);
+        } else {
+            // The ActivityManager.clearApplicationUserData() method doesn't exist, and there isn't
+            // a way to cleanly do this at all. So we don't support that functionality in this view.
+            // We actually shouldn't even be exposed if we're < KITKAT, as we only set this activity

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

If we can't get into this situation, would it be bette to just assert that
android.os.Build.VERSION_SDK_INT >= android.os.Build.VERSION_CODES.KITKAT?

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

Done.

+            // in our manifest if we're above that version (check the constants.xml file).
+            mClearAllDataSection.setVisibility(View.GONE);
+        }
+    }
+
+    @Override
+    protected void onResume() {
+        super.onResume();

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

Do we need to refetch this every time the activity is resumed?

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

If you're using the app switcher, then this works correctly. Do you have any
other opinions? I can just do this on activity create.

[Theresa, 2016/05/03 00:04:27]

onResume() may be triggered a lot in multi-window -- this is probably a side
case that we don't really need to worry about, so I'm okay with leaving the
number refresh here. That way if the user leaves this activity and does
something in Chrome that changes the size calculations, the numbers get updated
correctly.

+        WebsitePermissionsFetcher fetcher = new WebsitePermissionsFetcher(new SizeCalculator());
+        fetcher.fetchPreferencesForCategory(
+                SiteSettingsCategory.fromString(SiteSettingsCategory.CATEGORY_USE_STORAGE));
+    }
+
+    private void clearUnimportantData() {
+        mSiteDataSizeText.setText(R.string.storage_management_computing_size);
+        mUnimportantSiteDataSizeText.setText(R.string.storage_management_computing_size);
+        WebsitePermissionsFetcher fetcher =

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

Does it make sense to have a new method in
UnimportantSiteDataClearer#clearData() that handles this line and the next so
that it's more obvious what's happening? A comment explaining that data will be
cleared once sites that use storage are fetched would be helpful.

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

Huh. Ok, done.

[Theresa, 2016/05/03 00:04:27]

Thanks!

+                new WebsitePermissionsFetcher(new UnimportantSiteDataClearer());
+        fetcher.fetchPreferencesForCategory(
+                SiteSettingsCategory.fromString(SiteSettingsCategory.CATEGORY_USE_STORAGE));
+    }
+
+    @Override
+    public void onClick(View view) {
+        if (view == mClearUnimportantButton) {
+            AlertDialog.Builder builder = new AlertDialog.Builder(this);
+            builder.setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {
+                @Override
+                public void onClick(DialogInterface dialog, int id) {
+                    clearUnimportantData();
+                }
+            });
+            builder.setNegativeButton(R.string.cancel, null);
+            builder.setTitle(R.string.storage_management_clear_unimportant_dialog_title);
+            builder.setMessage(R.string.storage_management_clear_unimportant_dialog_text);
+            builder.create().show();
+        } else if (view == mManageSiteDataButton) {
+            Intent intent = PreferencesLauncher.createIntentForSettingsPage(
+                    this, SingleCategoryPreferences.class.getName());
+            Bundle initialArguments = new Bundle();
+            initialArguments.putString(SingleCategoryPreferences.EXTRA_CATEGORY,
+                    SiteSettingsCategory.CATEGORY_USE_STORAGE);
+            initialArguments.putString(SingleCategoryPreferences.EXTRA_TITLE,
+                    getString(R.string.website_settings_storage));
+            intent.putExtra(Preferences.EXTRA_SHOW_FRAGMENT_ARGUMENTS, initialArguments);
+            startActivity(intent);
+        } else if (view == mClearAllDataButton) {
+            if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.KITKAT) {

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

Why are we checking this here again?

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

Safety. Removed.

+                return;
+            }
+            final ActivityManager activityManager =
+                    (ActivityManager) getSystemService(Context.ACTIVITY_SERVICE);
+            AlertDialog.Builder builder = new AlertDialog.Builder(this);
+            builder.setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {
+                @Override
+                public void onClick(DialogInterface dialog, int id) {
+                    activityManager.clearApplicationUserData();
+                }
+            });
+            builder.setNegativeButton(R.string.cancel, null);
+            builder.setTitle(R.string.storage_management_reset_app_dialog_title);
+            builder.setMessage(R.string.storage_management_reset_app_dialog_text);
+            builder.create().show();
+        }
+    }
+
+    private void onSiteStorageSizeCalculated(long totalSize, long unimportantSize) {
+        mSiteDataSizeText.setText(Formatter.formatFileSize(this, totalSize));
+        mUnimportantSiteDataSizeText.setText(Formatter.formatFileSize(this, unimportantSize));
+    }
+
+    private void onSiteStorageCleared() {
+        WebsitePermissionsFetcher fetcher = new WebsitePermissionsFetcher(new SizeCalculator());
+        fetcher.fetchPreferencesForCategory(
+                SiteSettingsCategory.fromString(SiteSettingsCategory.CATEGORY_USE_STORAGE));
+    }
+
+    private class SizeCalculator implements WebsitePermissionsFetcher.WebsitePermissionsCallback {
+        @Override
+        public void onWebsitePermissionsAvailable(
+                Map<String, Set<Website>> sitesByOrigin, Map<String, Set<Website>> sitesByHost) {
+            long siteStorageSize = 0;
+            // First we scan origins to get settings from there.
+            Set<Website> sites = new HashSet<>();
+            for (Map.Entry<String, Set<Website>> element : sitesByOrigin.entrySet()) {
+                for (Website site : element.getValue()) {
+                    sites.add(site);
+                }
+            }
+            // Next we add sites that are only accessible by host name.
+            for (Map.Entry<String, Set<Website>> element : sitesByHost.entrySet()) {
+                for (Website site : element.getValue()) {
+                    if (!sites.contains(site)) {

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

Is this check needed? HashSet should be smart enough to to add things that
already exist in the set.

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

Done.

+                        sites.add(site);
+                    }
+                }
+            }

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

nit: Add a comment here? And maybe some spaces to break up the sections? This is
personal preference, so just a suggestion. I like doing something like this:

// 1. Add sites from origins.
...

// 2. Add sites only accessible by host name.
...

// 3. Calculate total storage size.

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

Done.

+            long importantSiteStorageTotal = 0;
+            for (Website site : sites) {
+                siteStorageSize += site.getTotalUsage();
+                if (site.getLocalStorageInfo().isDomainImportant()) {
+                    importantSiteStorageTotal += site.getTotalUsage();
+                }
+            }
+
+            onSiteStorageSizeCalculated(
+                    siteStorageSize, siteStorageSize - importantSiteStorageTotal);
+        }
+    }
+
+    private class UnimportantSiteDataClearer
+            implements WebsitePermissionsFetcher.WebsitePermissionsCallback {
+        @Override
+        public void onWebsitePermissionsAvailable(
+                Map<String, Set<Website>> sitesByOrigin, Map<String, Set<Website>> sitesByHost) {
+            // First we scan origins to get settings from there.
+            Set<Website> sites = new HashSet<>();
+            for (Map.Entry<String, Set<Website>> element : sitesByOrigin.entrySet()) {
+                for (Website site : element.getValue()) {
+                    sites.add(site);
+                }
+            }
+            // Next we add sites that are only accessible by host name.
+            for (Map.Entry<String, Set<Website>> element : sitesByHost.entrySet()) {
+                for (Website site : element.getValue()) {
+                    if (!sites.contains(site)) {
+                        sites.add(site);
+                    }
+                }
+            }

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

The code here looks identical to the function above. Maybe factor out to helper?

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

Done.

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

Or even better, can this whole thing be factored out? e.g. Save the list of all
sites and list of unimportant sites and just clear those when the corresponding
button is clicked, rather than refetching site info.

+            final int[] numLeft = new int[1];
+            numLeft[0] = 0;
+            for (Website site : sites) {
+                if (!site.getLocalStorageInfo().isDomainImportant()) {
+                    numLeft[0]++;
+                    site.clearAllStoredData(new StoredDataClearedCallback() {
+                        @Override
+                        public void onStoredDataCleared() {
+                            numLeft[0]--;
+                            if (numLeft[0] == 0) {
+                                onSiteStorageCleared();
+                            }
+                        }
+                    });
+                }
+            }
+        }
+    }
+
+    private void ensureActivityNotExported() {
+        if (sActivityNotExportedChecked) return;
+        sActivityNotExportedChecked = true;
+        try {
+            ActivityInfo activityInfo = getPackageManager().getActivityInfo(getComponentName(), 0);
+            // If Preferences is exported, then it's vulnerable to a fragment injection exploit:

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

Replace Preferences w/ the name of this class. Or elaborate. This isn't actually
checking that Preferences doesn't get exported.

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

Done.

+            // http://securityintelligence.com/new-vulnerability-android-framework-fragment-injection

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

I would move this comment above the function, then you won't have the >100 col
problem.

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

Done.

+            if (activityInfo.exported) {
+                throw new IllegalStateException("ManageSpaceActivity must not be exported.");
+            }
+        } catch (NameNotFoundException ex) {
+            // Something terribly wrong has happened.
+            throw new RuntimeException(ex);
+        }
+    }
+}