Move DownloadControllerAndroid from content/ to chrome/

chrome/browser/android/download/chrome_download_delegate.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/download/chrome_download_delegate.h"
 
 #include <jni.h>
 
 #include <string>
 #include <type_traits>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #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(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,
     const JavaParamRef<jstring>& filename) {
   return base::android::ConvertUTF8ToJavaString(
       env, l10n_util::GetStringFUTF8(
                IDS_PROMPT_DANGEROUS_DOWNLOAD,
                base::android::ConvertJavaStringToUTF16(env, filename)));
@@ skipping 11 matching lines @@
 // Called when a dangerous download is validated.
 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(
+  DownloadControllerBase::Get()->DangerousDownloadValidated(
       tab_android->web_contents(), download_guid, accept);
 }
 
 // static
 bool ChromeDownloadDelegate::EnqueueDownloadManagerRequest(
     jobject chrome_download_delegate,
     bool overwrite,
     jobject download_info) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
@@ skipping 32 matching lines @@
     const JavaParamRef<jstring>& jpermission,
     jlong callback_id) {
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
 
   std::string permission =
       base::android::ConvertJavaStringToUTF8(env, jpermission);
 
   // 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(
       reinterpret_cast<base::Callback<void(bool)>*>(callback_id));
 
   std::vector<std::string> permissions;
   permissions.push_back(permission);
 
   PermissionUpdateInfoBarDelegate::Create(
       tab_android->web_contents(),
       permissions,
       IDS_MISSING_STORAGE_PERMISSION_DOWNLOAD_EDUCATION_TEXT,
       *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);