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

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

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java
new file mode 100644
index 0000000000000000000000000000000000000000..7ec91ddd5c32c9b868bb7d1344206b29f39edd23
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java
@@ -0,0 +1,256 @@
+// 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.privacy;
+
+import android.app.Activity;
+import android.app.Dialog;
+import android.app.DialogFragment;
+import android.content.DialogInterface;
+import android.content.Intent;
+import android.content.res.Resources;
+import android.graphics.Bitmap;
+import android.graphics.drawable.BitmapDrawable;
+import android.graphics.drawable.Drawable;
+import android.os.Bundle;
+import android.support.v7.app.AlertDialog;
+import android.util.LruCache;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.AdapterView;
+import android.widget.ArrayAdapter;
+import android.widget.CheckedTextView;
+import android.widget.ListView;
+
+import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.base.Log;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.favicon.FaviconHelper;
+import org.chromium.chrome.browser.favicon.FaviconHelper.FaviconImageCallback;
+import org.chromium.chrome.browser.profiles.Profile;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Modal dialog that shows a list of important domains to the user which they can uncheck. Used to
+ * allow the user to exclude domains from being cleared by the clear browsing data function.
+ * We use proper bundle construction (through the {@link #newInstance(String[])} method) and
+ * onActivityResult return conventions.
+ */
+public class ConfirmImportantSitesDialogFragment extends DialogFragment {
+    private class ClearBrowsingDataAdapter extends ArrayAdapter<String> {
+        String[] mDomains;
+        private final Drawable mDefaultFavicon;
+        private final int mFaviconSize;
+
+        private ClearBrowsingDataAdapter(String[] domains) {
+            super(getActivity(), R.layout.select_dialog_multichoice_material, domains);
+            mDomains = domains;
+            Resources resources = getActivity().getResources();
+            mFaviconSize = resources.getDimensionPixelSize(R.dimen.default_favicon_size);
+            mDefaultFavicon =
+                    ApiCompatibilityUtils.getDrawable(resources, R.drawable.default_favicon);
+        }
+
+        @Override
+        public int getCount() {
+            return mDomains.length;
+        }
+
+        @Override
+        public boolean hasStableIds() {
+            return true;
+        }
+
+        @Override
+        public View getView(int position, View convertView, ViewGroup parent) {
+            View childView = convertView;
+            if (childView == null) {
+                LayoutInflater inflater = LayoutInflater.from(getActivity());
+                childView = inflater.inflate(
+                        R.layout.select_dialog_multichoice_material, parent, false);
+
+                ViewHolder viewHolder = new ViewHolder();
+                viewHolder.textView = (CheckedTextView) childView;
+                childView.setTag(viewHolder);
+            }
+            ViewHolder viewHolder = (ViewHolder) childView.getTag();
+            configureChildView(position, viewHolder);
+            return childView;
+        }
+
+        public void configureChildView(int position, ViewHolder viewHolder) {
+            String domain = mDomains[position];
+            viewHolder.textView.setChecked(!mDeselectedDomains.contains(domain));
+            viewHolder.textView.setText(domain);
+            loadLocalFavicon(viewHolder, transformDomainToOrigin(domain));
+        }
+
+        public void onClick(int position, View view) {
+            String domain = mDomains[position];
+            ViewHolder viewHolder = (ViewHolder) view.getTag();
+            if (mDeselectedDomains.contains(domain)) {
+                mDeselectedDomains.remove(domain);
+                viewHolder.textView.setChecked(true);

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

Using a CheckedTextView for this seems weird. I think what you really want is a
layout with a checkbox, text, and image. You can set drawables on TextView's
(see android:drawableLeft). Or you may be able to do this with just the Android
CheckBox class by adding a drawableRight and drawableLeft.

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

So this acts weird because it's part of a dialog in a list that supports
checking. I'm not sure why, but playing with these values did weird things, like
having two checkboxes on the left and other strange things. This ended up
working nicely, is it alright if we keep it this way?

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

Instead of using select_dialog_multichoice_material, we could use custom xml  to
define a horizontal LinearLayout with a CheckBox and an ImageView if
drawableRight and drawableLeft don't work. That feels a lot more natural to me
than using a CheckedTextView.

[dmurph, 2016/05/05 21:53:29]

I explored this for a bit, and I'd personally prefer keeping the
CheckedTextView, as it's less code and does exactly what I want. I get automatic
touch-anything-toggle-checkbox behavior as well. If you feel strongly I'll
change it, but I'd like to keep it this way.

+            } else {
+                mDeselectedDomains.add(domain);
+                viewHolder.textView.setChecked(false);
+            }
+        }
+
+        private void loadLocalFavicon(final ViewHolder viewHolder, final String url) {
+            Drawable image;
+            if (url == null) {
+                // URL is null for print jobs, for example.
+                image = mDefaultFavicon;
+            } else {
+                image = mFaviconCache.getLocalFaviconImage(url);
+                if (image == null) {
+                    FaviconImageCallback imageCallback = new FaviconImageCallback() {
+                        @Override
+                        public void onFaviconAvailable(Bitmap bitmap, String iconUrl) {
+                            if (this != viewHolder.imageCallback) return;
+                            Drawable image = faviconDrawable(bitmap);
+                            image = (image == null) ? mDefaultFavicon : image;
+                            mFaviconCache.putLocalFaviconImage(url, image);
+                            viewHolder.textView.setCheckMarkDrawable(image);
+                        }
+                    };
+                    viewHolder.imageCallback = imageCallback;
+                    mFaviconHelper.getLocalFaviconImageForURL(
+                            mProfile, url, mFaviconSize, imageCallback);
+                    image = mDefaultFavicon;
+                }
+            }
+            viewHolder.textView.setCheckMarkDrawable(image);
+        }
+
+        private Drawable faviconDrawable(Bitmap image) {

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

nit: getFaviconDrawable(Bitmap favicon)?

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

Done.

+            if (image == null) return null;
+            return new BitmapDrawable(getActivity().getResources(),
+                    Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, true));
+        }
+    }
+
+    private static class FaviconCache {

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

What's the expected flow for this dialog?

I would expect that the user opens it, clears data and then is done. If that's
the case, won't the cache just be empty every time? What's the point of caching
the favicons inside this class?

Also, take a look at LargeIconBridge#getLargeIconForUrl() -- that class has
built in cache. It's what we use for NTP thumbnails and the bookmarks manager.

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

Yes, it'll be empty. I'm not using that class (I don't want a large icon), and
if I used it it would be the same case as now -- it's one cache per instance.
This is mostly here because I noticed flickering, especially if there was
scrolling, of the icons. The list view also likes asking for a ton of the same
list row, not sure why. So this helps cut down on repeated loads of the favicon.

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

Caching for scrolling makes sense.

The favicons we get for bookmarks end up being displayed at 16dp (the same as
default_favicon_size used in this class). The size you're requesting, 16dp is
large in terms of pixels on devices with higher density. Any page that hasn't
been visited on the current device will most likely only have a 16px favicon
available, which is the minimum we use for bookmarks. I can send you some
screenshots of what this looks in bookmarks when the large size is available vs
not available.

I think using the regular getLocalFaviconImageForUrl() vs LargeIconService will
end up looking the same assuming all of the sites have been visited on the local
device. The code is a bit different; I'm not sure which one performs better, but
since you're only getting 5 sites it probably doesn't matter. 

The nice thing about the favicon service is that it provides a fallback color to
use if the favicon isn't available at the minimum size requested or above.

As an aside, the comment for FaviconHelper#getLocalFaviconImageForURL says it
only retrieves favicons for pages the user has visited on the current device.
Skimming through the code, I'm not sure that's actually true. Will the favicons
shown here always be for pages the user visited locally? Or if I'm signed in
does my desktop browsing affect the important sites calculation?

+        private final LruCache<String, Drawable> mMemoryCache;
+
+        public FaviconCache(int size) {
+            mMemoryCache = new LruCache<String, Drawable>(size);
+        }
+
+        public Drawable getLocalFaviconImage(String url) {
+            return mMemoryCache.get(url);
+        }
+
+        public void putLocalFaviconImage(String url, Drawable image) {
+            mMemoryCache.put(url, image);
+        }
+    }
+
+    /**
+     * ViewHolder class optimizes looking up table row fields. findViewById is only called once
+     * per row view initialization, and the references are cached here. Also stores a reference to
+     * the favicon image callback; so that we can make sure we load the correct favicon.
+     */
+    private static class ViewHolder {

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

Consider using a different class name. ViewHolder already exists as a public
class in RecyclerView.java.

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

Done.

+        public CheckedTextView textView;
+        public FaviconImageCallback imageCallback;
+    }
+
+    /**
+     * Constructs a new instance of the important sites dialog fragment.
+     * @param importantDomains The list of important domains to display.
+     * @return An instance of ConfirmImportantSitesDialogFragment with the bundle arguments set.
+     */
+    public static ConfirmImportantSitesDialogFragment newInstance(String[] importantDomains) {
+        Log.i(FRAGMENT_TAG, "Creating dialog");

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

nit: Debug statement.

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

removed.

+        ConfirmImportantSitesDialogFragment dialogFragment =
+                new ConfirmImportantSitesDialogFragment();
+        Bundle bundle = new Bundle();
+        bundle.putStringArray(IMPORTANT_DOMAINS_TAG, importantDomains);
+        dialogFragment.setArguments(bundle);
+        return dialogFragment;
+    }
+
+    /** The tag used when showing the clear browsing fragment. */
+    public static final String FRAGMENT_TAG = "ConfirmImportantSitesDialogFragment";
+
+    /** The tag used to return the string array of deselected domains. */
+    public static final String DESELECTED_DOMAINS_TAG = "DeselectedDomains";
+
+    /** The tag used to store the important domains in the bundle. */
+    private static final String IMPORTANT_DOMAINS_TAG = "ImportantDomains";
+
+    /* Transforms a domain into an origin for favicon lookup */

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

Nit: Missing period at end.

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

Done.

+    private static String transformDomainToOrigin(String domain) {
+        return "http://www." + domain;
+    }
+
+    private String[] mImportantDomains;

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

nit: I'd go ahead and document those members and functions below and above.

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

Done.

+    private Set<String> mDeselectedDomains;
+    private AlertDialog mDialog;
+    private ClearBrowsingDataAdapter mAdapter;
+    private FaviconHelper mFaviconHelper;
+    private FaviconCache mFaviconCache;
+    private Profile mProfile;
+
+    protected ConfirmImportantSitesDialogFragment() {
+        mProfile = Profile.getLastUsedProfile();
+        mFaviconHelper = new FaviconHelper();
+        mDeselectedDomains = new HashSet<>();
+        mFaviconCache = new FaviconCache(5);

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

Why 5? Seems kind of low to me, but is there a method behind the selection of
cache size here? How does this work in practice? Which sites generally end up in
the cache after a run?

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

We currently only have 5 items max, so this will be sufficient. I can add a
const in the class, and I'll increase it to 7 just in case. you want to have at
least as many in the cache as are on screen.

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

The maximum important sites to fetch in pref_service_bridge.cc is 5 (see
kMaxImportantSites in that file).

This value needs to be declared as a constant. Ideally somewhere that's shared
by Java and native.

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

Done.

+    }
+
+    @Override
+    public void setArguments(Bundle args) {
+        super.setArguments(args);
+        mImportantDomains = args.getStringArray(IMPORTANT_DOMAINS_TAG);
+    }
+
+    @Override
+    public Dialog onCreateDialog(Bundle savedInstanceState) {
+        mAdapter = new ClearBrowsingDataAdapter(mImportantDomains);
+        DialogInterface.OnClickListener listener = new DialogInterface.OnClickListener() {
+            @Override
+            public void onClick(DialogInterface dialog, int which) {
+                if (which == AlertDialog.BUTTON_POSITIVE) {
+                    // Return our deselected domains.
+                    Intent data = getActivity().getIntent();
+                    String[] deselectedDomains = mDeselectedDomains.toArray(new String[0]);
+                    data.putExtra(DESELECTED_DOMAINS_TAG, deselectedDomains);
+                    getTargetFragment().onActivityResult(
+                            getTargetRequestCode(), Activity.RESULT_OK, data);
+                } else {
+                    getTargetFragment().onActivityResult(getTargetRequestCode(),
+                            Activity.RESULT_CANCELED, getActivity().getIntent());
+                }
+            }
+        };
+        // We create our own listview, as AlertDialog doesn't let us set a message and a list

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

nit: ListView

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

Done.

+        // adapter at the same time.
+        ListView listView = (ListView) getActivity().getLayoutInflater().inflate(
+                R.layout.select_dialog_listview, null);
+        listView.setAdapter(mAdapter);
+        listView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
+            @Override
+            public void onItemClick(AdapterView<?> parent, View v, int position, long id) {
+                mAdapter.onClick(position, v);
+            }
+        });
+        final AlertDialog.Builder builder =
+                new AlertDialog.Builder(getActivity(), R.style.AlertDialogTheme)
+                        .setTitle(R.string.clear_browsing_data_important_dialog_title)
+                        .setMessage(R.string.clear_browsing_data_important_dialog_text)
+                        .setPositiveButton(R.string.clear_data_delete, listener)
+                        .setNegativeButton(R.string.cancel, listener)
+                        .setView(listView);
+        // OnClickListener is registered manually.

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

This comment seems a little out of place here since it goes with
.setPositiveButton and .setNegativeButton above.

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

Removed, it was old.

+        mDialog = builder.create();
+
+        return mDialog;
+    }
+}