Switch to use download GUID to indentify download items

Download GUID is introduced by
http://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1
Unlike download Id, the GUID can live across browser sessions and will
not be reused.
This change switches the java class to also use the
GUID.
Chrome will temporarily use the old download Id as the notification Id.
This is because the android notification requires an int to identify it,
rather than a string.
In the future, we will generate the notification Id from java side.

The download Ids generated by the Android DownloadManager are not
affected by this CL.
However, they are only used in OMA downloads and when we call
addCompletedDownload().

BUG=593020

Review URL: https://codereview.chromium.org/1809203006

Cr-Commit-Position: refs/heads/master@{#383443}

6 files changed, 81 insertions, 51 deletions

diff --git a/chrome/browser/android/download/chrome_download_delegate.cc b/chrome/browser/android/download/chrome_download_delegate.cc
index 5fa5db1..8ce3fac 100644
--- a/chrome/browser/android/download/chrome_download_delegate.cc
+++ b/chrome/browser/android/download/chrome_download_delegate.cc
@@ -48,14 +48,17 @@ static jboolean IsDownloadDangerous(JNIEnv* env,
 }
 
 // Called when a dangerous download is validated.
-static void DangerousDownloadValidated(JNIEnv* env,
-                                       const JavaParamRef<jclass>& clazz,
-                                       const JavaParamRef<jobject>& tab,
-                                       jint download_id,
-                                       jboolean accept) {
+static void DangerousDownloadValidated(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jobject>& tab,
+    const JavaParamRef<jstring>& jdownload_guid,
+    jboolean accept) {
+  std::string download_guid =
+      base::android::ConvertJavaStringToUTF8(env, jdownload_guid);
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DownloadControllerAndroid::Get()->DangerousDownloadValidated(
-      tab_android->web_contents(), download_id, accept);
+      tab_android->web_contents(), download_guid, accept);
 }
 
 // static
diff --git a/chrome/browser/android/download/download_manager_service.cc b/chrome/browser/android/download/download_manager_service.cc
index 0e0ed61..935f174 100644
--- a/chrome/browser/android/download/download_manager_service.cc
+++ b/chrome/browser/android/download/download_manager_service.cc
@@ -57,26 +57,34 @@ DownloadManagerService::~DownloadManagerService() {
     manager_->RemoveObserver(this);
 }
 
-void DownloadManagerService::ResumeDownload(JNIEnv* env,
-                                            jobject obj,
-                                            uint32_t download_id,
-                                            jstring fileName) {
-  ResumeDownloadInternal(download_id, ConvertJavaStringToUTF8(env, fileName),
-                         true);
+void DownloadManagerService::ResumeDownload(
+    JNIEnv* env,
+    jobject obj,
+    uint32_t download_id,
+    const JavaParamRef<jstring>& jdownload_guid,
+    jstring fileName) {
+  std::string download_guid = ConvertJavaStringToUTF8(env, jdownload_guid);
+  ResumeDownloadInternal(download_id, download_guid,
+                         ConvertJavaStringToUTF8(env, fileName), true);
 }
 
-void DownloadManagerService::CancelDownload(JNIEnv* env,
-                                            jobject obj,
-                                            uint32_t download_id) {
-  CancelDownloadInternal(download_id, true);
+void DownloadManagerService::CancelDownload(
+    JNIEnv* env,
+    jobject obj,
+    const JavaParamRef<jstring>& jdownload_guid) {
+  std::string download_guid = ConvertJavaStringToUTF8(env, jdownload_guid);
+  CancelDownloadInternal(download_guid, true);
 }
 
-void DownloadManagerService::PauseDownload(JNIEnv* env,
-                                            jobject obj,
-                                            uint32_t download_id) {
-  content::DownloadItem* item = manager_->GetDownload(download_id);
+void DownloadManagerService::PauseDownload(
+    JNIEnv* env,
+    jobject obj,
+    const JavaParamRef<jstring>& jdownload_guid) {
+  std::string download_guid = ConvertJavaStringToUTF8(env, jdownload_guid);
+  content::DownloadItem* item = manager_->GetDownloadByGuid(download_guid);
   if (item)
     item->Pause();
+  item->RemoveObserver(content::DownloadControllerAndroid::Get());
 }
 
 void DownloadManagerService::ManagerGoingDown(
@@ -96,14 +104,16 @@ void DownloadManagerService::ResumeDownloadItem(content::DownloadItem* item,
     resume_callback_for_testing_.Run(true);
 }
 
-void DownloadManagerService::ResumeDownloadInternal(uint32_t download_id,
-                                                    const std::string& fileName,
-                                                    bool retry) {
+void DownloadManagerService::ResumeDownloadInternal(
+    uint32_t download_id,
+    const std::string& download_guid,
+    const std::string& fileName,
+    bool retry) {
   if (!manager_) {
     OnResumptionFailed(download_id, fileName);
     return;
   }
-  content::DownloadItem* item = manager_->GetDownload(download_id);
+  content::DownloadItem* item = manager_->GetDownloadByGuid(download_guid);
   if (item) {
     ResumeDownloadItem(item, fileName);
     return;
@@ -120,25 +130,27 @@ void DownloadManagerService::ResumeDownloadInternal(uint32_t download_id,
   // created item might not be loaded from download history. So user might wait
   // indefinitely to see the failed notification. See http://crbug.com/577893.
   base::MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&DownloadManagerService::ResumeDownloadInternal,
-                 base::Unretained(this), download_id, fileName, false),
+      FROM_HERE, base::Bind(&DownloadManagerService::ResumeDownloadInternal,
+                            base::Unretained(this), download_id, download_guid,
+                            fileName, false),
       base::TimeDelta::FromMilliseconds(kRetryIntervalInMilliseconds));
 }
 
-void DownloadManagerService::CancelDownloadInternal(uint32_t download_id,
-                                                    bool retry) {
+void DownloadManagerService::CancelDownloadInternal(
+    const std::string& download_guid,
+    bool retry) {
   if (!manager_)
     return;
-  content::DownloadItem* item = manager_->GetDownload(download_id);
+  content::DownloadItem* item = manager_->GetDownloadByGuid(download_guid);
   if (item) {
     item->Cancel(true);
+    item->RemoveObserver(content::DownloadControllerAndroid::Get());
     return;
   }
   if (retry) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE, base::Bind(&DownloadManagerService::CancelDownloadInternal,
-                              base::Unretained(this), download_id, false),
+                              base::Unretained(this), download_guid, false),
         base::TimeDelta::FromMilliseconds(kRetryIntervalInMilliseconds));
   }
 }
diff --git a/chrome/browser/android/download/download_manager_service.h b/chrome/browser/android/download/download_manager_service.h
index f1a5f9e..aa65a32 100644
--- a/chrome/browser/android/download/download_manager_service.h
+++ b/chrome/browser/android/download/download_manager_service.h
@@ -13,6 +13,8 @@
 #include "base/macros.h"
 #include "content/public/browser/download_manager.h"
 
+using base::android::JavaParamRef;
+
 namespace content {
 class DownloadItem;
 }
@@ -29,20 +31,26 @@ class DownloadManagerService : public content::DownloadManager::Observer {
                          content::DownloadManager* manager);
   ~DownloadManagerService() override;
 
-  // Called to resume downloading the item that has ID equal to |download_id|.
-  // If the DownloadItem is not yet created, retry after a while.
+  // Called to resume downloading the item that has GUID equal to
+  // |jdownload_guid|. If the DownloadItem is not yet created, retry after
+  // a while.
   void ResumeDownload(JNIEnv* env,
                       jobject obj,
                       uint32_t download_id,
+                      const JavaParamRef<jstring>& jdownload_guid,
                       jstring fileName);
 
-  // Called to cancel a download item that has ID equal to |download_id|.
+  // Called to cancel a download item that has GUID equal to |jdownload_guid|.
   // If the DownloadItem is not yet created, retry after a while.
-  void CancelDownload(JNIEnv* env, jobject obj, uint32_t download_id);
+  void CancelDownload(JNIEnv* env,
+                      jobject obj,
+                      const JavaParamRef<jstring>& jdownload_guid);
 
-  // Called to pause a download item that has ID equal to |download_id|.
+  // Called to pause a download item that has GUID equal to |jdownload_guid|.
   // If the DownloadItem is not yet created, do nothing as it is already paused.
-  void PauseDownload(JNIEnv* env, jobject obj, uint32_t download_id);
+  void PauseDownload(JNIEnv* env,
+                     jobject obj,
+                     const JavaParamRef<jstring>& jdownload_guid);
 
   // content::DownloadManager::Observer methods.
   void ManagerGoingDown(content::DownloadManager* manager) override;
@@ -58,12 +66,13 @@ class DownloadManagerService : public content::DownloadManager::Observer {
   // Helper function to start the download resumption. If |retry| is true,
   // chrome will retry the resumption if the download item is not loaded.
   void ResumeDownloadInternal(uint32_t download_id,
+                              const std::string& download_guid,
                               const std::string& fileName,
                               bool retry);
 
   // Helper function to cancel a download. If |retry| is true,
   // chrome will retry the cancellation if the download item is not loaded.
-  void CancelDownloadInternal(uint32_t download_id, bool retry);
+  void CancelDownloadInternal(const std::string& download_guid, bool retry);
 
   // Called to notify the java side that download resumption failed.
   void OnResumptionFailed(uint32_t download_id, const std::string& fileName);
diff --git a/chrome/browser/android/download/download_manager_service_unittest.cc b/chrome/browser/android/download/download_manager_service_unittest.cc
index b987c8b..c5d60d7a 100644
--- a/chrome/browser/android/download/download_manager_service_unittest.cc
+++ b/chrome/browser/android/download/download_manager_service_unittest.cc
@@ -36,9 +36,10 @@ class DownloadManagerServiceTest : public testing::Test {
                                        &manager_)),
         finished_(false),
         success_(false) {
-    ON_CALL(manager_, GetDownload(_))
+    ON_CALL(manager_, GetDownloadByGuid(_))
         .WillByDefault(
-            ::testing::Invoke(this, &DownloadManagerServiceTest::GetDownload));
+            ::testing::Invoke(this,
+                              &DownloadManagerServiceTest::GetDownloadByGuid));
   }
 
   void OnResumptionDone(bool success) {
@@ -46,12 +47,14 @@ class DownloadManagerServiceTest : public testing::Test {
     success_ = success;
   }
 
-  void StartDownload(int download_id) {
+  void StartDownload(const std::string& download_guid) {
     JNIEnv* env = base::android::AttachCurrentThread();
     service_->set_resume_callback_for_testing(base::Bind(
         &DownloadManagerServiceTest::OnResumptionDone, base::Unretained(this)));
     service_->ResumeDownload(
-        env, nullptr, download_id,
+        env, nullptr, 0, JavaParamRef<jstring>(
+            env, base::android::ConvertUTF8ToJavaString(
+                env, download_guid).obj()),
         base::android::ConvertUTF8ToJavaString(env, "test").obj());
     while (!finished_)
       message_loop_.RunUntilIdle();
@@ -64,7 +67,9 @@ class DownloadManagerServiceTest : public testing::Test {
   }
 
  protected:
-  content::DownloadItem* GetDownload(uint32_t) { return download_.get(); }
+  content::DownloadItem* GetDownloadByGuid(const std::string&) {
+    return download_.get();
+  }
 
   base::MessageLoop message_loop_;
   scoped_ptr<content::MockDownloadItem> download_;
@@ -78,7 +83,7 @@ class DownloadManagerServiceTest : public testing::Test {
 
 // Test that resumption will fail if no download item is found before times out.
 TEST_F(DownloadManagerServiceTest, ResumptionTimeOut) {
-  StartDownload(1);
+  StartDownload("0000");
   EXPECT_FALSE(success_);
 }
 
@@ -86,13 +91,13 @@ TEST_F(DownloadManagerServiceTest, ResumptionTimeOut) {
 // resumed.
 TEST_F(DownloadManagerServiceTest, ResumptionWithResumableItem) {
   CreateDownloadItem(true);
-  StartDownload(1);
+  StartDownload("0000");
   EXPECT_TRUE(success_);
 }
 
 // Test that resumption fails if the target download item is not resumable.
 TEST_F(DownloadManagerServiceTest, ResumptionWithNonResumableItem) {
   CreateDownloadItem(false);
-  StartDownload(1);
+  StartDownload("0000");
   EXPECT_FALSE(success_);
 }
diff --git a/chrome/browser/android/download/mock_download_controller_android.cc b/chrome/browser/android/download/mock_download_controller_android.cc
index f072e9e..7551330 100644
--- a/chrome/browser/android/download/mock_download_controller_android.cc
+++ b/chrome/browser/android/download/mock_download_controller_android.cc
@@ -34,8 +34,9 @@ void MockDownloadControllerAndroid::StartContextMenuDownload(
 }
 
 void MockDownloadControllerAndroid::DangerousDownloadValidated(
-    content::WebContents* web_contents, int download_id, bool accept) {
-}
+    content::WebContents* web_contents,
+    const std::string& download_guid,
+    bool accept) {}
 
 void MockDownloadControllerAndroid::AcquireFileAccessPermission(
     content::WebContents* web_contents,
diff --git a/chrome/browser/android/download/mock_download_controller_android.h b/chrome/browser/android/download/mock_download_controller_android.h
index 50e621e..d74e7dfe 100644
--- a/chrome/browser/android/download/mock_download_controller_android.h
+++ b/chrome/browser/android/download/mock_download_controller_android.h
@@ -33,9 +33,9 @@ class MockDownloadControllerAndroid
       const content::ContextMenuParams& params,
       content::WebContents* web_contents,
       bool is_link, const std::string& extra_headers) override;
-  void DangerousDownloadValidated(
-      content::WebContents* web_contents, int download_id,
-      bool accept) override;
+  void DangerousDownloadValidated(content::WebContents* web_contents,
+                                  const std::string& download_guid,
+                                  bool accept) override;
   void AcquireFileAccessPermission(
       content::WebContents* web_contents,
       const AcquireFileAccessPermissionCallback& callback) override;