Move DownloadControllerAndroid from content/ to chrome/

chrome/browser/android/download/chrome_download_delegate.cc

Index: chrome/browser/android/download/chrome_download_delegate.cc
diff --git a/chrome/browser/android/download/chrome_download_delegate.cc b/chrome/browser/android/download/chrome_download_delegate.cc
index 00943c8802eb02a1e2c41020569d425e5a73698b..763e26107c5a7fec3b072da08be05ac17e6ccd5a 100644
--- a/chrome/browser/android/download/chrome_download_delegate.cc
+++ b/chrome/browser/android/download/chrome_download_delegate.cc
@@ -17,17 +17,18 @@
 #include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/android/download/android_download_manager_overwrite_infobar_delegate.h"
+#include "chrome/browser/android/download/download_controller_base.h"
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/permissions/permission_update_infobar_delegate_android.h"
 #include "chrome/common/safe_browsing/file_type_policies.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
-#include "content/public/browser/android/download_controller_android.h"
 #include "jni/ChromeDownloadDelegate_jni.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using content::DownloadControllerAndroid;
+using base::android::ConvertUTF8ToJavaString;
+using base::android::ScopedJavaLocalRef;
 
 // Gets the download warning text for the given file name.
 static ScopedJavaLocalRef<jstring> GetDownloadWarningText(
@@ -59,7 +60,7 @@ static void DangerousDownloadValidated(
   std::string download_guid =
       base::android::ConvertJavaStringToUTF8(env, jdownload_guid);
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
-  DownloadControllerAndroid::Get()->DangerousDownloadValidated(
+  DownloadControllerBase::Get()->DangerousDownloadValidated(
       tab_android->web_contents(), download_guid, accept);
 }
 
@@ -112,7 +113,7 @@ static void LaunchPermissionUpdateInfoBar(
   // Convert java long long int to c++ pointer, take ownership.
   static_assert(
       std::is_same<
-          DownloadControllerAndroid::AcquireFileAccessPermissionCallback,
+          DownloadControllerBase::AcquireFileAccessPermissionCallback,
           base::Callback<void(bool)>>::value,
       "Callback types don't match!");
   std::unique_ptr<base::Callback<void(bool)>> cb(
@@ -128,6 +129,80 @@ static void LaunchPermissionUpdateInfoBar(
       *cb);
 }
 
+ChromeDownloadDelegate::ChromeDownloadDelegate(
+    content::WebContents* web_contents) {}

[Ted C, 2016/06/15 17:44:11]

why take web_contents if unused?

[Jinsuk Kim, 2016/06/15 21:05:30]

This follows the pattern required for the classes inheriting
WebContentsUserData.
https://cs-staging.chromium.org/chromium/src/content/public/browser/web_conte...
WebContentsUserData::CreateForWebContents() expects the constructor to have the
parameter.

[Ted C, 2016/06/15 23:56:37]

Acknowledged.

+
+ChromeDownloadDelegate::~ChromeDownloadDelegate() {}
+
+void ChromeDownloadDelegate::Init(JNIEnv* env, jobject jobj) {
+  weak_java_ref_ = JavaObjectWeakGlobalRef(env, jobj);

[Ted C, 2016/06/15 17:44:11]

why a weak ref?

And if we have to use it, we should ensure the object doesn't go away before any
of all calls below.  The fact we aren't checking makes me think this should be a
regular global ref.

[no sievers, 2016/06/15 18:17:28]

I was thinking that it's a normal pattern for a Delegate implementation (i.e.
delegatee) to outlive the source of the calls, which seemed conceptually similar
here except that it also involves JNI.
He put a comment in the header next to the weak_java_ref_ to that extent.

But yea this relies on the implementation to do the right thing i.e. Tab to init
and tear down the delegate correctly.

[no sievers, 2016/06/15 18:34:06]

Oh and this was my other thinking:
This is really just code split out of Tab (so it can be unit-tested and
mocked?).
And the pattern for Java-object-owns-native-instance (with explicit native
init/destroy) seems to be using a weak ref back to Java also. 

But I don't know a good way to enforce that this is not used elsewhere outside
of tab_android.cc in the wrong manner (besides comments.. well we could DCHECK()
I guess but unfortunately it would only trigger in the rare case where a
potential race is lost and it would crash anyhow).

[Jinsuk Kim, 2016/06/15 21:05:30]

Referenced other cases (can't remember what it was right now...) where the
native needs to invoke the Java object which is owned by other Java class (Tab
in this case). I *think* it's safe to expect the Java object will be available
as Tab and the object will outlive underlying (native) WebContents and the
download delegate owned by it. But feel free to let me know if there can be a
chance that this can cause a problem. I'd be happy to change it as suggested.

[Ted C, 2016/06/15 23:56:37]

In general, the cases I've seen for weak refs in java are:

1.) Webview to handle the case that destroy is not required by WebView
    embedders on Android, so they have to handle the case where their
    java objects are GCed but never tell native.
2.) Programmer error.

In the chrome layer, we should always have clear destruction paths.
There are some async cases where you can argue it, but if the java
object has a the ability to destroy the native side then it should
always be a strong ref (or the lifecycle is such that it is never
possible to have one w/o the other).

If we have weakrefs, then we should be null checking them each time they
are used.  tab_android looks like a mistake and something we should undo.

Also, this is an odd flow for initializing the java <-> native stuff.
It should not involve Tab to create the connection.

I think it should either be:

1.) Java ChromeDownloadDelegate constructor calls a new nativeInit (static
    Init on the c++ side) that takes the web contents and constructs the
    native side.
2.) C++ constructor does the reverse with a tab ref or something and whatever
    else it needs.

Ideally, these are self contained units and this isn't yet.

[Jinsuk Kim, 2016/06/16 05:34:12]

Done. Did 1)

+}
+
+void ChromeDownloadDelegate::RequestHTTPGetDownload(
+    const std::string& url,
+    const std::string& user_agent,
+    const std::string& content_disposition,
+    const std::string& mime_type,
+    const std::string& cookie,
+    const std::string& referer,
+    const base::string16& file_name,
+    int64_t content_length,
+    bool has_user_gesture,
+    bool must_download) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jstring> jurl =
+      ConvertUTF8ToJavaString(env, url);
+  ScopedJavaLocalRef<jstring> juser_agent =
+      ConvertUTF8ToJavaString(env, user_agent);
+  ScopedJavaLocalRef<jstring> jcontent_disposition =
+      ConvertUTF8ToJavaString(env, content_disposition);
+  ScopedJavaLocalRef<jstring> jmime_type =
+      ConvertUTF8ToJavaString(env, mime_type);
+  ScopedJavaLocalRef<jstring> jcookie =
+      ConvertUTF8ToJavaString(env, cookie);
+  ScopedJavaLocalRef<jstring> jreferer =
+      ConvertUTF8ToJavaString(env, referer);
+
+  // net::GetSuggestedFilename will fallback to "download" as filename.
+  ScopedJavaLocalRef<jstring> jfilename =
+      base::android::ConvertUTF16ToJavaString(env, file_name);
+  Java_ChromeDownloadDelegate_requestHttpGetDownload(
+      env, weak_java_ref_.get(env).obj(),
+      jurl.obj(), juser_agent.obj(), jcontent_disposition.obj(),
+      jmime_type.obj(), jcookie.obj(), jreferer.obj(), has_user_gesture,
+      jfilename.obj(), content_length, must_download);
+}
+
+void ChromeDownloadDelegate::OnDownloadStarted(const std::string& filename,
+                                               const std::string& mime_type) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jstring> jfilename = ConvertUTF8ToJavaString(
+      env, filename);
+  ScopedJavaLocalRef<jstring> jmime_type =
+      ConvertUTF8ToJavaString(env, mime_type);
+  Java_ChromeDownloadDelegate_onDownloadStarted(
+      env, weak_java_ref_.get(env).obj(), jfilename.obj(), jmime_type.obj());
+}
+
+void ChromeDownloadDelegate::OnDangerousDownload(const std::string& filename,
+                                                 const std::string& guid) {
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jstring> jfilename = ConvertUTF8ToJavaString(
+      env, filename);
+  ScopedJavaLocalRef<jstring> jguid = ConvertUTF8ToJavaString(env, guid);
+  Java_ChromeDownloadDelegate_onDangerousDownload(
+      env, weak_java_ref_.get(env).obj(), jfilename.obj(), jguid.obj());
+}
+
+void ChromeDownloadDelegate::RequestFileAccess(intptr_t callback_id) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_ChromeDownloadDelegate_requestFileAccess(
+      env, weak_java_ref_.get(env).obj(), callback_id);
+}
+
 bool RegisterChromeDownloadDelegate(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeDownloadDelegate);