[Offline Page] Offline page sharing implementation

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
index 54de20eaae5d8078ced3a704d483a71bd0fff364..042b91f8ebca39e38099882a5afd2889ceb9d3ca 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
@@ -4,19 +4,27 @@
 
 package org.chromium.chrome.browser.offlinepages;
 
+import android.app.Activity;
 import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
+import android.graphics.Bitmap;
+import android.net.Uri;
+import android.os.AsyncTask;
 import android.os.BatteryManager;
 import android.os.Environment;
 
+import org.chromium.base.Callback;
+import org.chromium.base.FileUtils;
 import org.chromium.base.Log;
+import org.chromium.base.StreamUtil;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.chrome.browser.share.ShareHelper;
 import org.chromium.chrome.browser.snackbar.Snackbar;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.snackbar.SnackbarManager.SnackbarController;
@@ -29,6 +37,12 @@ import org.chromium.net.ConnectionType;
 import org.chromium.net.NetworkChangeNotifier;
 import org.chromium.ui.base.PageTransition;
 
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.channels.FileChannel;
 import java.util.concurrent.TimeUnit;
 
 /**
@@ -39,6 +53,8 @@ public class OfflinePageUtils {
     /** Background task tag to differentiate from other task types */
     public static final String TASK_TAG = "OfflinePageUtils";
 
+    public static final String EXTERNAL_MHTML_FILE_PATH = "offline-pages";
+
     private static final int DEFAULT_SNACKBAR_DURATION_MS = 6 * 1000; // 6 second
 
     private static final long STORAGE_ALMOST_FULL_THRESHOLD_BYTES = 10L * (1 << 20); // 10M
@@ -253,6 +269,155 @@ public class OfflinePageUtils {
                 TimeUnit.MILLISECONDS);
     }
 
+    /**
+     * Share saved offline page.
+     * @param shareDirectly Whether it should share directly with the activity that was most
+     *                      recently used to share.
+     * @param mainActivity Activity that is used to access package manager

[Theresa, 2016/08/15 20:50:00]

nit: missing period at the end of this line

"... access the package manager."

[Vivian, 2016/08/16 17:40:12]

Done.

+     * @param onlineUrl Online URL associated with the offline page that is used to access the
+     *                  offline page file path.
+     * @param bitmap Screenshot of the page to be shared.
+     * @param currentTab Tab that is used to access offlineUrl and tile.
+     */
+    public static void shareOfflinePage(final boolean shareDirectly, final boolean saveLastUsed,

[Theresa, 2016/08/15 20:50:00]

The JavaDoc is missing some params - saveLastUsed, text, and callback

[Vivian, 2016/08/16 17:40:11]

Done.

+            final Activity mainActivity, final String text, final String onlineUrl,
+            final Bitmap bitmap, final ShareHelper.TargetChosenCallback callback,
+            final Tab currentTab) {
+        final String offlineUrl = currentTab.getUrl();
+        final String title = currentTab.getTitle();
+        OfflinePageBridge offlinePageBridge =
+                OfflinePageBridge.getForProfile(currentTab.getProfile());
+
+        if (offlinePageBridge != null) {
+            Log.e(TAG, "Unable to perform sharing on current tab.");
+            return;
+        }
+
+        offlinePageBridge.getPageByOfflineUrl(offlineUrl, new Callback<OfflinePageItem>() {
+            @Override
+            public void onResult(OfflinePageItem item) {
+                if (item == null) return;
+
+                String offlineFilePath = item.getFilePath();
+                prepareForSharing(shareDirectly, saveLastUsed, mainActivity, title, text, onlineUrl,
+                        bitmap, callback, offlineFilePath);
+            }
+        });
+    }
+
+    private static void prepareForSharing(final boolean shareDirectly, final boolean saveLastUsed,
+            final Activity activity, final String title, final String text, final String onlineUrl,
+            final Bitmap bitmap, final ShareHelper.TargetChosenCallback callback,
+            final String filePath) {
+        new AsyncTask<Void, Void, File>() {
+            @Override
+            protected File doInBackground(Void... params) {
+                File offlinePageOriginal = new File(filePath);
+                File shareableDir = getDirectoryForOfflineSharing(activity);
+                if (shareableDir != null) {

[Theresa, 2016/08/15 20:50:00]

early exit here too:

if (shareableDir == null) {
  Log.e(TAG, ...
  return null;
}

String fileName = ....

[Vivian, 2016/08/16 17:40:11]

Done.

+                    String fileName = rewriteOfflineFileName(offlinePageOriginal.getName());
+                    File offlinePageShareable = new File(shareableDir, fileName);
+
+                    if (offlinePageShareable.exists()) {

[Theresa, 2016/08/15 20:49:59]

Please add a comment explaining why the old offline page shareable file must be
deleted before copying again.

[Vivian, 2016/08/16 17:40:11]

Done.

+                        try {
+                            offlinePageShareable.delete();
+                        } catch (SecurityException e) {
+                            Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e);

[Theresa, 2016/08/15 20:50:00]

If we fail to delete the old shareable file, what happens if we copy (will the
bytes get appended to the existing shareable file or will they overwrite some or
all of the existing bytes)? Should we skip copying?

[Vivian, 2016/08/16 17:40:12]

Good question!
It will overwrite the some of the existing bytes (up to the number of bytes
contained in the file copied from). So there's a chance that some bytes in the
old file is not overwritten.
I decide to keep the delete old page part and added a return. File copying would
be skipped if delete failed.

[Theresa, 2016/08/16 18:47:20]

Sounds good, thanks!

+                        }
+                    }
+                    if (copyToShareableLocation(offlinePageOriginal, offlinePageShareable)) {
+                        return offlinePageShareable;
+                    }
+                } else {
+                    Log.e(TAG, "Unable to create subdirectory in shareable directory");
+                }
+                return null;
+            }
+
+            @Override
+            protected void onPostExecute(File offlinePageShareable) {
+                Uri offlineUri = null;

[Theresa, 2016/08/13 15:58:48]

Eventually we'll want to be able to share the mhtml for an offline page from the
download manger UI as well as the "share" menu item. For the DownloadManagerUi,
it would be helpful if there were a static method that took the offlineUrl (and
possibly the title) and returned a content:// URI.  Can that piece be extracted
into a separate method that can be shared?

[Theresa, 2016/08/15 20:50:00]

I think we can skip this for now.

[Vivian, 2016/08/16 17:40:11]

Acknowledged.

+                if (offlinePageShareable != null) {
+                    offlineUri = Uri.fromFile(offlinePageShareable);
+                }
+                ShareHelper.share(shareDirectly, saveLastUsed, activity, title, text, onlineUrl,
+                        offlineUri, bitmap, callback);
+            }
+        }.execute();

[Theresa, 2016/08/15 20:50:00]

Add AsyncTask.SERIAL_EXECUTOR here. Not specifying leads to inconsistent
behavior since different versions of Android have different default executors.

[Vivian, 2016/08/16 17:40:11]

Done.

+    }
+
+    /**
+     * This method copies the file from internal storage to a sharable directory.
+     * @param src file path of the original file to be copied

[Theresa, 2016/08/15 20:50:00]

nits:
remove "This method"

capitalize the first word of the param description ("Path") and end with a
period.

Also, since the parameter is a File and not a String (path) I think it'd make
more sense to just say "The original file to be copied." and "The destination
file."

[Vivian, 2016/08/16 17:40:11]

Done.

+     * @param dst file path of the destination
+     */
+    @VisibleForTesting
+    static boolean copyToShareableLocation(File src, File dst) {
+        FileInputStream inputStream = null;
+        FileOutputStream outputStream = null;
+
+        try {
+            inputStream = new FileInputStream(src);
+            outputStream = new FileOutputStream(dst);
+
+            FileChannel inChannel = inputStream.getChannel();
+            FileChannel outChannel = outputStream.getChannel();
+            inChannel.transferTo(0, inChannel.size(), outChannel);
+        } catch (FileNotFoundException e) {
+            Log.e(TAG, "Failed to copy the file: " + src.getName(), e);
+            return false;
+        } catch (IOException e) {

[Theresa, 2016/08/15 20:50:00]

This can be combined with the catch above:

...
} catch (FileNotFound | IOException e) {
  ...
} finally {
....

[Vivian, 2016/08/16 17:40:12]

Done.

+            Log.e(TAG, "Failed to copy the file: " + src.getName(), e);
+            return false;
+        } finally {
+            StreamUtil.closeQuietly(inputStream);
+            StreamUtil.closeQuietly(outputStream);
+        }
+        return true;
+    }
+
+    /**
+     * Get a directory for offline page sharing operation.

[Theresa, 2016/08/15 20:50:00]

nit: "Gets the directory to use for sharing offline pages, creating it if
necessary."

[Vivian, 2016/08/16 17:40:12]

Done.

+     * @param context Context that is used to access external cache directory.
+     * @return path to directory for the shared file to be stored

[Theresa, 2016/08/15 20:50:00]

nit: capitalize Path

[Vivian, 2016/08/16 17:40:12]

Done.

+     */
+    @VisibleForTesting
+    static File getDirectoryForOfflineSharing(Context context) {
+        File path = new File(context.getExternalCacheDir(), EXTERNAL_MHTML_FILE_PATH);

[Theresa, 2016/08/15 20:49:59]

I think we should create a static variable that caches this file rather than
creating it each time. This method will become something like:

if (mOfflineSharingDirectory == null {
 mOfflineSharingDirectory = ....
}

return mOfflineSharingDirectory.

[Theresa, 2016/08/16 03:00:10]

*sOfflineSharingDirectory (since it'd be static)

[Vivian, 2016/08/16 17:40:12]

Done.

+        if (!path.exists() && !path.mkdir()) {
+            path = null;
+        }
+        return path;
+    }
+
+    /**
+     * Rewrite file name so that it does not contain periods except the one to separate the file
+     * extension.
+     * This step is used to ensure that file name can be recognized by intent filter (.*\\.mhtml").

[Theresa, 2016/08/15 20:50:00]

nit: "... (.*\\.mhtml) as Android's path..."

[Vivian, 2016/08/16 17:40:12]

Done.

+     * As Android's path pattern only matches the first dot that appears in a file path.
+     * @pram fileName Name of the offline page file.
+     */
+    @VisibleForTesting
+    static String rewriteOfflineFileName(String fileName) {
+        fileName = fileName.replaceAll("\\s+", "");
+        return fileName.replaceAll("\\.(?=.*\\.)", "_");
+    }
+
+    /**
+     * Clears all shared mhtml files.
+     * @param context Context that is used to access external cache directory.
+     */
+    public static void clearSharedOfflineFiles(final Context context) {
+        new AsyncTask<Void, Void, Void>() {
+            @Override
+            protected Void doInBackground(Void... params) {
+                File offlinePath = getDirectoryForOfflineSharing(context);
+                FileUtils.recursivelyDeleteFile(offlinePath);
+                return null;
+            }
+        }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);

[Theresa, 2016/08/15 20:50:00]

This should be done on the SERIAL_EXECUTOR so that clearing shared files can't
get interleaved with creating files for sharing.

[Vivian, 2016/08/16 17:40:11]

Done.

+    }
+
     private static boolean isPowerConnected(Intent batteryStatus) {
         int status = batteryStatus.getIntExtra(BatteryManager.EXTRA_STATUS, -1);
         boolean isConnected = (status == BatteryManager.BATTERY_STATUS_CHARGING