[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 106027043db0a10c3070b964590dfbeb35bdde21..c5c9eee5995b132a31517a0c1223be235692117b 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,12 +4,17 @@
 
 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.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
@@ -17,6 +22,7 @@ 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 +35,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 +51,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 SNACKBAR_DURATION = 6 * 1000; // 6 second
 
     private static final long STORAGE_ALMOST_FULL_THRESHOLD_BYTES = 10L * (1 << 20); // 10M
@@ -250,6 +264,163 @@ 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
+     * @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 mContext The application context.
+     * @param currentTab Tab that is used to access offlineUrl and tile.
+     */
+    public static void shareOfflinePage(final boolean shareDirectly, final boolean saveLastUsed,
+            final Activity mainActivity, final String text, final String onlineUrl,
+            final Bitmap bitmap, final ShareHelper.TargetChosenCallback callback,
+            final Context mContext, final Tab currentTab) {
+        final String offlineUrl = currentTab.getUrl();
+        final String title = currentTab.getTitle();
+        OfflinePageBridge offlinePageBridge =
+                OfflinePageBridge.getForProfile(currentTab.getProfile());
+        if (offlinePageBridge != null) {

[gone, 2016/08/12 01:26:12]

Avoid indentation by early exiting:

if (offlinePageBridge == null) {
    Log.e(TAG, "Unable to perform sharing on current tab");
    return;
}

[Vivian, 2016/08/12 23:07:12]

Done.

+            offlinePageBridge.getPageByOfflineUrl(offlineUrl, new Callback<OfflinePageItem>() {
+                @Override
+                public void onResult(OfflinePageItem item) {

[gone, 2016/08/12 01:26:13]

nit: When indentation gets deep in simple functions, it's sometimes nicer to do
the early return there, too:

if (item == null) return;
String offlineFilePath = item.getFilePath();
prepareForSharing(shareDirectly, saveLastUsed, mainActivity, title, text,
        onlineUrl, bitmap, callback, offlineFilePath, mContext);

[Vivian, 2016/08/12 23:07:12]

Done.

+                    if (item != null) {
+                        String offlineFilePath = item.getFilePath();
+                        prepareForSharing(shareDirectly, saveLastUsed, mainActivity, title, text,
+                                onlineUrl, bitmap, callback, offlineFilePath, mContext);
+                    }
+                }
+            });
+        } else {
+            Log.e(TAG, "Unable to perform sharing on current tab.");
+        }
+    }
+
+    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, final Context context) {
+        new AsyncTask<Void, Void, File>() {
+            @Override
+            protected File doInBackground(Void... params) {
+                File offlinePageOriginal = new File(filePath);
+                File shareableDir = getDirectoryForOfflineSharing(context);
+                if (shareableDir != null) {
+                    String fileName = rewriteOfflineFileName(offlinePageOriginal.getName());
+                    File offlinePageShareable = new File(shareableDir, fileName);

[Theresa, 2016/08/12 17:36:44]

Do we really need to rewrite the offline page file name, delete the original
then copy it to a "shareable location"?

We should be able to share from the original directory by adding it to
file_paths.xml so that we can provide a URI through our ContentProvider.

https://cs.chromium.org/chromium/src/chrome/android/java/res/xml/file_paths.x...


See my recent CL to do something similar for other files in Downloads/
https://codereview.chromium.org/2230803004/

[Vivian, 2016/08/12 23:07:12]

This was my initial approach. The problem is, we want to be able to use ShareIt
and Xender to share the page. but they won't recognize a content URI generated
this way. Instead, using file URI can make it work. Read permission of file
stored in internal directory cannot be granted for file URI. That's the reason
I'm copying the files to external cache directory.

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

That really surprises me. The Android documentation talks explicitly about being
able to grant read permission on an internal directory to enable sharing a
content:// URI generated from an internal file:
https://developer.android.com/reference/android/support/v4/content/FileProvid...

When you try to share to Xender and ShareIt using the approach where a new path
is added to file_paths.xml, what's the error in adb logcat? 

[Theresa, 2016/08/13 16:05:55]

To expand a little more on why I'm pushing on this - I think doing operations
that require disk IO after the user requests to share a file is the wrong
approach. It may be sufficiently fast for one file (but maybe not, have we
tested this on a svelte device?), but it certainly doesn't scale to sharing
multiple files because there may likely end up being a perceptible delay in
between the user tapping a button to share and the share intent getting created.

+
+                    if (offlinePageShareable.exists()) {
+                        try {
+                            offlinePageShareable.delete();
+                        } catch (SecurityException e) {
+                            Log.e(TAG,
+                                    "failed to delete the file: " + offlinePageOriginal.getName());

[gone, 2016/08/12 01:26:12]

nit: Might be worth printing the exception:

Log.e(TAG, "Failed to delete: " + offlinePageOriginal.getName(), e);

[Vivian, 2016/08/12 23:07:12]

Done.

+                        }
+                    }
+                    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) {
+                if (offlinePageShareable == null) return;
+                Uri offlineUri = Uri.fromFile(offlinePageShareable);
+                ShareHelper.share(shareDirectly, saveLastUsed, activity, title, text, onlineUrl,
+                        offlineUri, bitmap, callback);

[gone, 2016/08/12 01:26:12]

If sharing the offline page fails, you should fall back to just sharing the URL.

[Vivian, 2016/08/12 23:07:12]

Done.

+            }
+        }.execute();
+    }
+
+    /**
+     * This method copies the file from internal storage to a sharable directory.
+     * @param src file path of the original file to be copied
+     * @param dst file path of the destination
+     */
+    @VisibleForTesting
+    static boolean copyToShareableLocation(File src, File dst) {
+        try {
+            FileChannel inChannel = new FileInputStream(src).getChannel();
+            FileChannel outChannel = new FileOutputStream(dst).getChannel();
+
+            inChannel.transferTo(0, inChannel.size(), outChannel);
+
+            if (inChannel != null) inChannel.close();
+            if (outChannel != null) outChannel.close();

[gone, 2016/08/12 01:26:12]

1) Log.e takes an Exception as the third argument, so you don't
   need to + e it at the end.
2) Do what DocumentModeAssassin does for consistency:
==============
FileInputStream inputStream = null;
FileInputStream outputStream = null;

try {
    inputStream = new FileInputStream(src);
    outStream = 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) {
    Log.e(TAG, "Failed to copy the file: " + src.getName(), e);
    return false;
} finally {
    StreamUtil.closeQuietly(inChannel);
    StreamUtil.closeQuietly(outChannel);
}

[Vivian, 2016/08/12 23:07:12]

Done.

+        } catch (FileNotFoundException e) {
+            Log.e(TAG, "failed to copy the file: " + src.getName() + ", " + e);
+            return false;
+        } catch (IOException e) {
+            Log.e(TAG, "failed to copy the file: " + src.getName() + ", " + e);
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Get a directory for offline page sharing operation.
+     * @param context Context that is used to access external cache directory.
+     * @return path to directory for the shared file to be stored
+     */
+    private static File getDirectoryForOfflineSharing(Context context) {
+        File path = new File(context.getExternalCacheDir(), EXTERNAL_MHTML_FILE_PATH);
+        boolean success = true;

[gone, 2016/08/12 01:26:13]

Useless variable?

[Vivian, 2016/08/12 23:07:12]

Yes. Nice catch! Deleted it.

+        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").
+     * 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("\\.(?=.*\\.)", "_");
+    }
+
+    /**
+     * Delete a shared mhtml file. If the file path is a directory, delete all files in directory.
+     * @param file File object of the offline page.
+     */
+    @VisibleForTesting
+    static void deleteSharedOfflineFiles(File file) {

[gone, 2016/08/12 01:26:13]

Just use FileUtils.recursivelyDeleteFile(File file) instead.

[Vivian, 2016/08/12 23:07:12]

Done.

+        if (!file.exists()) return;
+        if (file.isDirectory()) {
+            for (File f : file.listFiles()) deleteSharedOfflineFiles(f);
+        }
+        if (!file.delete()) {
+            Log.w(TAG, "Failed to delete shared offline file: %s", file.getAbsolutePath());
+        }
+    }
+
+    /**
+     * 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);
+                deleteSharedOfflineFiles(offlinePath);
+                return null;
+            }
+        }.execute();

[gone, 2016/08/12 01:26:12]

AsyncTask#execute() does different things, depending on what version of Android
you're on.  Explicitly use executeOnExecutor instead.

AsyncTask#executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR)

[Vivian, 2016/08/12 23:07:12]

Done.

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