[Offline pages] Adding capability to free up space used by Offline pages

chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java b/chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
new file mode 100644
index 0000000000000000000000000000000000000000..ca05c8202d205599cde7a24c4188826570dffa63
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
@@ -0,0 +1,104 @@
+// Copyright 2015 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.offline_pages;
+
+import android.app.Dialog;
+import android.app.DialogFragment;
+import android.content.DialogInterface;
+import android.os.Bundle;
+import android.support.v7.app.AlertDialog;
+import android.text.format.Formatter;
+
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.offline_pages.OfflinePageBridge.DeletePageCallback;
+import org.chromium.components.bookmarks.BookmarkId;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Helper class showing the dialog to allowing to "Free up space" taken by offline pages.

[jianli, 2015/08/21 21:06:27]

I don't think this is a helper class. How about:
  Shows a "Free up space" dialog to clean up storage for offline pages.

[fgorski, 2015/08/21 22:09:39]

Done.

+ */
+public class OfflinePageFreeUpSpaceDialog
+        extends DialogFragment implements DialogInterface.OnClickListener {
+    OfflinePageBridge mOfflinePageBridge;
+    List<OfflinePageItem> mOfflinePagesToDelete;
+    final FreeUpSpaceCallback mCallback;

[Kibeom Kim (inactive), 2015/08/21 20:19:39]

nit: private for these variables?

[fgorski, 2015/08/21 22:09:39]

Done. Thanks for catching that.

+
+    /**
+     * Interface for informing the caller that the clean up operation is completed.
+     */
+    public interface FreeUpSpaceCallback {
+        /** Called when clean up operation is completed, or the dialog is cancelled. */
+        void onCleanUpSpaceDone();

[jianli, 2015/08/21 21:06:27]

add public

[fgorski, 2015/08/21 22:09:38]

Interface methods are public by default.

http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.4

It is permitted, but discouraged as a matter of style, to redundantly
specify the public and/or abstract modifier for a method declared
in an interface.

+    }
+
+    /**
+     * Creates the dialog. If the passed bridge instance needs to be destroyed after the dialog
+     * is finished, it should be taken care of in the callback implementation.

[Kibeom Kim (inactive), 2015/08/21 20:19:39]

Hmm... this is subtle because OfflinePageBridge is destroyed by
OfflinePageBridge#destroy() call and there is no guarantee that this dialog
instance is garbage collected before that. But I guess it should be fine as this
dialog will outlive the underlying enhanced bookmark UI.

[Kibeom Kim (inactive), 2015/08/21 21:15:08]

Correction:
Said the last sentence backward, and what I meant to say was that this class
won't be using mOfflinePageBridge after the underlying enhanced bookmark UI even
if it's garbage collected later.

[fgorski, 2015/08/21 22:09:39]

This dialog will also be shown after a snackbar:
1. Create a bookmark with offline copy
2. Wait, we are short on space, let's tell the user:
3. Show snackbar that says, you saved the page, but run low on space:
https://codereview.chromium.org/1305883002/
4. User clicks the snackbar and gets the "free up space" dialog, and that is
   where the EBModel with OPBridge might be kept for a little longer.

In the header scenario I agree with you.

Does that sound reasonable?

+     *
+     * @param offlinePageBridge An object to access offline page functionality.
+     * @param callback An object that will be called when the dialog finishes. Can be null.
+     * @see FreeUpSpaceCallback
+     */
+    public OfflinePageFreeUpSpaceDialog(
+            OfflinePageBridge offlinePageBridge, FreeUpSpaceCallback callback) {
+        assert offlinePageBridge != null;
+        mOfflinePageBridge = offlinePageBridge;
+        mCallback = callback;
+    }

[Kibeom Kim (inactive), 2015/08/21 20:19:39]

Android tries to call an empty constructor on DialogFragmnet recreation(steps
mentioned below) so I believe this will crash in that case.

So making a static newInstance(...) constructor is convention.
http://developer.android.com/intl/ko/reference/android/app/DialogFragment.html

[fgorski, 2015/08/21 22:09:38]

Done.

+
+    @Override
+    public Dialog onCreateDialog(Bundle savedInstanceState) {

[Kibeom Kim (inactive), 2015/08/21 20:19:39]

I believe that this is recreated when Chrome activity is recreated.

Steps:
1. Android settings -> Developer options -> Turn on "Don't keep activities"
2. Open this dialog
3. Switch to another app
4. Come back to Chrome

I don't think restoring this dialog is necessary so let's just dismiss.

FYI: I had to fix the same issue for another DialogFragment
https://codereview.chromium.org/1248373003/

[fgorski, 2015/08/21 22:09:38]

Done.

+        AlertDialog.Builder builder =
+                new AlertDialog.Builder(getActivity(), R.style.AlertDialogTheme)
+                        .setTitle(R.string.offline_pages_free_up_space_title)
+                        .setPositiveButton(R.string.delete, this)
+                        .setNegativeButton(R.string.cancel, this);
+        if (mOfflinePageBridge != null) {

[Kibeom Kim (inactive), 2015/08/21 20:19:39]

I believe that this is not needed?

[fgorski, 2015/08/21 22:09:39]

Done.

+            mOfflinePagesToDelete = mOfflinePageBridge.getPagesToCleanUp();
+            builder.setMessage(getString(R.string.offline_pages_free_up_space_message,
+                    mOfflinePagesToDelete.size(),
+                    Formatter.formatFileSize(getActivity(), getTotalSize())));
+        } else {
+            builder.setMessage(getString(R.string.offline_pages_free_up_space_message, 0, "0MB"));
+        }
+        return builder.create();
+    }
+
+    @Override
+    public void onClick(DialogInterface dialog, int id) {
+        if (id != AlertDialog.BUTTON_POSITIVE || mOfflinePageBridge == null) {

[jianli, 2015/08/21 21:06:27]

Since we already assert at line 48, the 2nd condition check should not be
needed.

[fgorski, 2015/08/21 22:09:39]

Done.

+            dialog.cancel();
+            if (mCallback != null) mCallback.onCleanUpSpaceDone();
+            return;
+        }
+
+        mOfflinePageBridge.deletePages(getBookmarkIdsToDelete(), new DeletePageCallback() {
+            @Override
+            public void onDeletePageDone(int deletePageResult) {
+                if (mCallback != null) mCallback.onCleanUpSpaceDone();
+            }
+        });
+    }
+
+    /** @return A list of Bookmark IDs for which the offline pages will be deleted. */
+    private List<BookmarkId> getBookmarkIdsToDelete() {
+        List<BookmarkId> bookmarkIds = new ArrayList<BookmarkId>();
+        for (OfflinePageItem offlinePage : mOfflinePagesToDelete) {
+            bookmarkIds.add(offlinePage.getBookmarkId());
+        }
+        return bookmarkIds;
+    }
+
+    /** @return A total size of offline pages that will be deleted. */
+    private long getTotalSize() {
+        long totalSize = 0;
+        for (OfflinePageItem offlinePage : mOfflinePagesToDelete) {
+            totalSize += offlinePage.getFileSize();
+        }
+        return totalSize;
+    }
+}