Fixed OMA download flow for download offline content provider

This CL fixes
1 - Overall OMA download flow for DownloadOfflineContentProvider
2 - Fix auto-open for downloads that should be auto-opened immediately.
3 - Fixed a bug, where OMA mistakenly assumes the given download path
and queries mime type from it, rather than content Uri, since the
given file path and actual file path can be different.

Bug: 989110
Change-Id: I7bad385f0a72d40b555e94b47ee760377519e6e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1727382Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682842}

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerBridge.java

@@ -23,6 +23,8 @@ import org.chromium.base.StrictModeContext;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.task.AsyncTask;
+import org.chromium.base.task.PostTask;
+import org.chromium.base.task.TaskTraits;
 
 import java.io.File;
 import java.lang.reflect.InvocationTargetException;
@@ -134,15 +136,17 @@ public class DownloadManagerBridge {
      */
     @CalledByNative
     public static void removeCompletedDownload(String downloadGuid, boolean externallyRemoved) {
-        long downloadId = removeDownloadIdMapping(downloadGuid);
-
-        // Let Android DownloadManager to remove download only if the user removed the file in
-        // Chrome. If the user renamed or moved the file, Chrome should keep it intact.
-        if (downloadId != INVALID_SYSTEM_DOWNLOAD_ID && !externallyRemoved) {
-            DownloadManager manager =
-                    (DownloadManager) getContext().getSystemService(Context.DOWNLOAD_SERVICE);
-            manager.remove(downloadId);
-        }
+        PostTask.postTask(TaskTraits.BEST_EFFORT_MAY_BLOCK, () -> {
+            long downloadId = removeDownloadIdMapping(downloadGuid);
+
+            // Let Android DownloadManager to remove download only if the user removed the file in
+            // Chrome. If the user renamed or moved the file, Chrome should keep it intact.
+            if (downloadId != INVALID_SYSTEM_DOWNLOAD_ID && !externallyRemoved) {
+                DownloadManager manager =
+                        (DownloadManager) getContext().getSystemService(Context.DOWNLOAD_SERVICE);
+                manager.remove(downloadId);
+            }
+        });
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java

@@ -32,8 +32,6 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.library_loader.LibraryProcessType;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.task.AsyncTask;
-import org.chromium.base.task.PostTask;
-import org.chromium.base.task.TaskTraits;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.download.DownloadManagerBridge.DownloadEnqueueRequest;
@@ -561,7 +559,8 @@ public class DownloadManagerService
                     if (success) item.setSystemDownloadId(systemDownloadId);
                 }
                 boolean canResolve = success
-                        && (isOMADownloadDescription(item.getDownloadInfo().getMimeType())
+                        && (DownloadUtils.isOMADownloadDescription(
+                                    item.getDownloadInfo().getMimeType())
                                 || canResolveDownloadItem(item, isSupportedMimeType));
                 return Pair.create(success, canResolve);
             }
@@ -597,6 +596,11 @@ public class DownloadManagerService
         }
     }
 
+    @CalledByNative
+    private void handleOMADownload(DownloadItem download, long systemDownloadId) {
+        mOMADownloadHandler.handleOMADownload(download.getDownloadInfo(), systemDownloadId);
+    }
+
     /**
      * Handle auto opennable files after download completes.
      * TODO(qinmin): move this to DownloadManagerBridge.
@@ -604,7 +608,7 @@ public class DownloadManagerService
      * @param download A download item.
      */
     private void handleAutoOpenAfterDownload(DownloadItem download) {
-        if (isOMADownloadDescription(download.getDownloadInfo().getMimeType())) {
+        if (DownloadUtils.isOMADownloadDescription(download.getDownloadInfo().getMimeType())) {
             mOMADownloadHandler.handleOMADownload(
                     download.getDownloadInfo(), download.getSystemDownloadId());
             return;
@@ -780,34 +784,12 @@ public class DownloadManagerService
             return;
         }
 
+        // TODO(shaktisahu): We should show this on infobar instead.
         DownloadUtils.showDownloadStartToast(ContextUtils.getApplicationContext());
         addUmaStatsEntry(new DownloadUmaStatsEntry(String.valueOf(response.downloadId),
                 downloadItem.getStartTime(), 0, false, true, 0, 0));
     }
 
-    /**
-     * Determines if the download should be immediately opened after
-     * downloading.
-     *
-     * @param mimeType The mime type of the download.
-     * @param hasUserGesture Whether the download is associated with an user gesture.
-     * @return true if the downloaded content should be opened, or false otherwise.
-     */
-    @VisibleForTesting
-    static boolean shouldOpenAfterDownload(String mimeType, boolean hasUserGesture) {
-        return hasUserGesture && MIME_TYPES_TO_OPEN.contains(mimeType);
-    }
-
-    /**
-     * Returns true if the download is for OMA download description file.
-     *
-     * @param mimeType The mime type of the download.
-     * @return true if the downloaded is OMA download description, or false otherwise.
-     */
-    static boolean isOMADownloadDescription(String mimeType) {
-        return OMADownloadHandler.OMA_DOWNLOAD_DESCRIPTOR_MIME.equalsIgnoreCase(mimeType);
-    }
-
     @Nullable
     static Intent getLaunchIntentForDownload(@Nullable String filePath, long downloadId,
             boolean isSupportedMimeType, String originalUrl, String referrer,
@@ -911,7 +893,7 @@ public class DownloadManagerService
     public static boolean canResolveDownload(
             String filePath, String mimeType, long systemDownloadId) {
         assert !ThreadUtils.runningOnUiThread();
-        if (isOMADownloadDescription(mimeType)) return true;
+        if (DownloadUtils.isOMADownloadDescription(mimeType)) return true;
 
         Intent intent = getLaunchIntentForDownload(filePath, systemDownloadId,
                 DownloadManagerService.isSupportedMimeType(mimeType), null, null, mimeType);
@@ -1153,9 +1135,7 @@ public class DownloadManagerService
             return;
         }
 
-        PostTask.postTask(TaskTraits.BEST_EFFORT_MAY_BLOCK, () -> {
-            DownloadManagerBridge.removeCompletedDownload(downloadGuid, externallyRemoved);
-        });
+        DownloadManagerBridge.removeCompletedDownload(downloadGuid, externallyRemoved);
     }
 
     /**
@@ -1232,7 +1212,9 @@ public class DownloadManagerService
     public void onSuccessNotificationShown(
             DownloadInfo info, boolean canResolve, int notificationId, long systemDownloadId) {
         if (!ChromeFeatureList.isEnabled(ChromeFeatureList.DOWNLOAD_OFFLINE_CONTENT_PROVIDER)) {
-            if (canResolve && shouldOpenAfterDownload(info.getMimeType(), info.hasUserGesture())) {
+            if (canResolve
+                    && DownloadUtils.shouldAutoOpenDownload(
+                            info.getMimeType(), info.hasUserGesture())) {
                 DownloadItem item = new DownloadItem(false, info);
                 item.setSystemDownloadId(systemDownloadId);
                 handleAutoOpenAfterDownload(item);
@@ -1372,7 +1354,7 @@ public class DownloadManagerService
 
                         @Override
                         protected void onPostExecute(Boolean canResolve) {
-                            if (shouldOpenAfterDownload(
+                            if (DownloadUtils.shouldAutoOpenDownload(
                                         result.mimeType, item.getDownloadInfo().hasUserGesture())
                                     && canResolve) {
                                 handleAutoOpenAfterDownload(item);
@@ -1821,10 +1803,11 @@ public class DownloadManagerService
             public Boolean doInBackground() {
                 DownloadInfo info = downloadItem.getDownloadInfo();
                 boolean isSupportedMimeType = isSupportedMimeType(info.getMimeType());
-                boolean canResolve = isOMADownloadDescription(info.getMimeType())
+                boolean canResolve = DownloadUtils.isOMADownloadDescription(info.getMimeType())
                         || canResolveDownloadItem(downloadItem, isSupportedMimeType);
                 return canResolve
-                        && shouldOpenAfterDownload(info.getMimeType(), info.hasUserGesture());
+                        && DownloadUtils.shouldAutoOpenDownload(
+                                info.getMimeType(), info.hasUserGesture());
             }
             @Override
             protected void onPostExecute(Boolean result) {

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java

@@ -83,6 +83,7 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.text.NumberFormat;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.HashMap;
@@ -110,6 +111,14 @@ public class DownloadUtils {
     private static final int[] BYTES_STRINGS = {
             R.string.download_ui_kb, R.string.download_ui_mb, R.string.download_ui_gb};
 
+    // Set will be more expensive to initialize, so use an ArrayList here.
+    private static final List<String> MIME_TYPES_TO_OPEN =
+            new ArrayList<String>(Arrays.asList(OMADownloadHandler.OMA_DOWNLOAD_DESCRIPTOR_MIME,
+                    "application/pdf", "application/x-x509-ca-cert", "application/x-x509-user-cert",
+                    "application/x-x509-server-cert", "application/x-pkcs12",
+                    "application/application/x-pem-file", "application/pkix-cert",
+                    "application/x-wifi-config"));
+
     private static final String TAG = "download";
 
     private static final String DEFAULT_MIME_TYPE = "*/*";
@@ -636,6 +645,30 @@ public class DownloadUtils {
         return ChromeDownloadDelegate.remapGenericMimeType(mimeType, url, filename);
     }
 
+    /**
+     * Returns true if the download is for OMA download description file.
+     *
+     * @param mimeType The mime type of the download.
+     * @return true if the downloaded is OMA download description, or false otherwise.
+     */
+    public static boolean isOMADownloadDescription(String mimeType) {
+        return OMADownloadHandler.OMA_DOWNLOAD_DESCRIPTOR_MIME.equalsIgnoreCase(mimeType);
+    }
+
+    /**
+     * Determines if the download should be immediately opened after
+     * downloading.
+     *
+     * @param mimeType The mime type of the download.
+     * @param hasUserGesture Whether the download is associated with an user gesture.
+     * @return true if the downloaded content should be opened, or false otherwise.
+     */
+    @VisibleForTesting
+    @CalledByNative
+    public static boolean shouldAutoOpenDownload(String mimeType, boolean hasUserGesture) {
+        return hasUserGesture && MIME_TYPES_TO_OPEN.contains(mimeType);
+    }
+
     /**
      * Utility method to open an {@link OfflineItem}, which can be a chrome download, offline page.
      * Falls back to open download home.

chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java

@@ -37,9 +37,11 @@ import org.chromium.base.Log;
 import org.chromium.base.ObserverList;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.task.AsyncTask;
+import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.content.ContentUtils;
 import org.chromium.chrome.browser.download.DownloadManagerBridge.DownloadEnqueueRequest;
 import org.chromium.chrome.browser.download.DownloadManagerBridge.DownloadEnqueueResponse;
+import org.chromium.chrome.browser.download.items.OfflineContentAggregatorFactory;
 import org.chromium.chrome.download.R;
 import org.chromium.components.download.DownloadCollectionBridge;
 import org.chromium.ui.UiUtils;
@@ -326,9 +328,14 @@ public class OMADownloadHandler extends BroadcastReceiver {
 
         @Override
         protected void onPostExecute(OMAInfo omaInfo) {
-            DownloadManagerService.getDownloadManagerService().removeDownload(
-                    mDownloadInfo.getDownloadGuid(), mDownloadInfo.isOffTheRecord(),
-                    false /* externallyRemoved */);
+            if (ChromeFeatureList.isEnabled(ChromeFeatureList.DOWNLOAD_OFFLINE_CONTENT_PROVIDER)) {
+                OfflineContentAggregatorFactory.get().removeItem(mDownloadInfo.getContentId());
+            } else {
+                DownloadManagerService.getDownloadManagerService().removeDownload(
+                        mDownloadInfo.getDownloadGuid(), mDownloadInfo.isOffTheRecord(),
+                        false /* externallyRemoved */);
+            }
+
             if (omaInfo == null) return;
             // Send notification if required attributes are missing.
             if (omaInfo.getTypes().isEmpty() || getSize(omaInfo) <= 0
@@ -827,6 +834,10 @@ public class OMADownloadHandler extends BroadcastReceiver {
             builder.setBytesTotalSize(result.bytesTotal);
             if (!TextUtils.isEmpty(result.fileName)) builder.setFileName(result.fileName);
             if (!TextUtils.isEmpty(result.mimeType)) builder.setMimeType(result.mimeType);
+
+            // Since the requested file path may not be same as the actual file path, set it to
+            // null. This will result in using contentUri instead.
+            builder.setFilePath(null);
             item.setDownloadInfo(builder.build());
 
             showDownloadsUi(downloadId, item, result, installNotifyURI);

chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java

@@ -536,28 +536,21 @@ public class DownloadManagerServiceTest {
     }
 
     /**
-     * Test to make sure {@link DownloadManagerService#shouldOpenAfterDownload}
+     * Test to make sure {@link DownloadUtils#shouldAutoOpenDownload}
      * returns the right result for varying MIME types and Content-Dispositions.
      */
     @Test
     @SmallTest
     @Feature({"Download"})
-    public void testShouldOpenAfterDownload() {
+    public void testshouldAutoOpenDownload() {
         // Should not open any download type MIME types.
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/download", true));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/x-download", true));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/octet-stream", true));
-        Assert.assertTrue(DownloadManagerService.shouldOpenAfterDownload("application/pdf", true));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/pdf", false));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/x-download", true));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/x-download", true));
-        Assert.assertFalse(
-                DownloadManagerService.shouldOpenAfterDownload("application/x-download", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/download", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/x-download", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/octet-stream", true));
+        Assert.assertTrue(DownloadUtils.shouldAutoOpenDownload("application/pdf", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/pdf", false));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/x-download", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/x-download", true));
+        Assert.assertFalse(DownloadUtils.shouldAutoOpenDownload("application/x-download", true));
     }
 }

chrome/browser/android/download/download_manager_service.cc

@@ -259,6 +259,19 @@ void DownloadManagerService::OpenDownload(download::DownloadItem* download,
   Java_DownloadManagerService_openDownloadItem(env, java_ref_, j_item, source);
 }
 
+void DownloadManagerService::HandleOMADownload(download::DownloadItem* download,
+                                               int64_t system_download_id) {
+  if (java_ref_.is_null())
+    return;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> j_item =
+      JNI_DownloadManagerService_CreateJavaDownloadItem(env, download);
+
+  Java_DownloadManagerService_handleOMADownload(env, java_ref_, j_item,
+                                                system_download_id);
+}
+
 void DownloadManagerService::OpenDownload(
     JNIEnv* env,
     jobject obj,

chrome/browser/android/download/download_manager_service.h

@@ -61,6 +61,11 @@ class DownloadManagerService
   // content instead of regular downloads.
   void ShowDownloadManager(bool show_prefetched_content);
 
+  // Called to handle subsequent steps, after a download was determined as a OMA
+  // download type.
+  void HandleOMADownload(download::DownloadItem* download,
+                         int64_t system_download_id);
+
   // Called to open a given download item.
   void OpenDownload(download::DownloadItem* download, int source);
 

chrome/browser/android/download/download_utils.cc

@@ -31,6 +31,9 @@ namespace {
 int kDefaultAutoResumptionSizeLimit = 10 * 1024 * 1024;  // 10 MB
 const char kAutoResumptionSizeLimitParamName[] = "AutoResumptionSizeLimit";
 
+// Mime type for OMA download descriptor.
+const char kOmaDownloadDescriptorMimeType[] = "application/vnd.oma.dd+xml";
+
 }  // namespace
 
 static ScopedJavaLocalRef<jstring> JNI_DownloadUtils_GetFailStateMessage(
@@ -105,3 +108,17 @@ std::string DownloadUtils::RemapGenericMimeType(const std::string& mime_type,
       ConvertUTF8ToJavaString(env, file_name));
   return ConvertJavaStringToUTF8(env, j_remapped_mime_type);
 }
+
+// static
+bool DownloadUtils::ShouldAutoOpenDownload(download::DownloadItem* item) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  return Java_DownloadUtils_shouldAutoOpenDownload(
+      env, ConvertUTF8ToJavaString(env, item->GetMimeType()),
+      item->HasUserGesture());
+}
+
+// static
+bool DownloadUtils::IsOmaDownloadDescription(const std::string& mime_type) {
+  return base::EqualsCaseInsensitiveASCII(mime_type,
+                                          kOmaDownloadDescriptorMimeType);
+}

chrome/browser/android/download/download_utils.h

@@ -24,6 +24,8 @@ class DownloadUtils {
   static std::string RemapGenericMimeType(const std::string& mime_type,
                                           const GURL& url,
                                           const std::string& file_name);
+  static bool ShouldAutoOpenDownload(download::DownloadItem* item);
+  static bool IsOmaDownloadDescription(const std::string& mime_type);
 };
 
 #endif  // CHROME_BROWSER_ANDROID_DOWNLOAD_DOWNLOAD_UTILS_H_

chrome/browser/download/download_offline_content_provider.cc

@@ -23,6 +23,7 @@
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/download/download_manager_bridge.h"
+#include "chrome/browser/android/download/download_manager_service.h"
 #include "chrome/browser/android/download/download_utils.h"
 #endif
 
@@ -505,8 +506,19 @@ void DownloadOfflineContentProvider::AddCompletedDownloadDone(
     DownloadItem* item,
     int64_t system_download_id,
     bool can_resolve) {
-  if (can_resolve && item->HasUserGesture())
+#if defined(OS_ANDROID)
+  if (!can_resolve)
+    return;
+
+  if (DownloadUtils::IsOmaDownloadDescription(item->GetMimeType())) {
+    DownloadManagerService::GetInstance()->HandleOMADownload(
+        item, system_download_id);
+    return;
+  }
+
+  if (DownloadUtils::ShouldAutoOpenDownload(item))
     item->OpenDownload();
+#endif
 }
 
 DownloadItem* DownloadOfflineContentProvider::GetDownload(