Move DownloadControllerAndroid from content/ to chrome/

chrome/browser/android/intercept_download_resource_throttle.cc

Index: chrome/browser/android/intercept_download_resource_throttle.cc
diff --git a/chrome/browser/android/intercept_download_resource_throttle.cc b/chrome/browser/android/intercept_download_resource_throttle.cc
index 30fd0a118f346f7833ec01595fd639fb5622d601..63a5eb18afe2bfc88599c37ca702dbe7492cdbcf 100644
--- a/chrome/browser/android/intercept_download_resource_throttle.cc
+++ b/chrome/browser/android/intercept_download_resource_throttle.cc
@@ -8,11 +8,11 @@
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/android/chrome_feature_list.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
-#include "content/public/browser/android/download_controller_android.h"
 #include "content/public/browser/resource_controller.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
 
 namespace {
 
@@ -52,12 +52,10 @@ InterceptDownloadResourceThrottle::InterceptDownloadResourceThrottle(
     net::URLRequest* request,
     int render_process_id,
     int render_view_id,
-    int request_id,
     bool must_download)
     : request_(request),
       render_process_id_(render_process_id),
       render_view_id_(render_view_id),
-      request_id_(request_id),
       must_download_(must_download) {
 }
 
@@ -120,8 +118,61 @@ void InterceptDownloadResourceThrottle::ProcessDownloadRequest() {
     return;
   }
 
-  content::DownloadControllerAndroid::Get()->CreateGETDownload(
-      render_process_id_, render_view_id_, request_id_, must_download_);
+  // Prepare download.
+  DownloadInfo info(request_);
+
+  net::CookieStore* cookie_store = request_->context()->cookie_store();
+  if (cookie_store) {
+    StartDownloadCB callback = base::Bind(
+        &InterceptDownloadResourceThrottle::StartDownload,
+        base::Unretained(this));
+    cookie_store->GetAllCookiesForURLAsync(

[no sievers, 2016/06/10 19:40:06]

Can't we use GetCookieListWithOptionsAsync() in the first place to pass in
|options| and avoid the need for the second task?
Looks like this method we are using here is marked as deprecated even.

[Jinsuk Kim, 2016/06/13 03:21:12]

Done. Sorry I should have done this in the previous patch.

+        request_->url(),
+        base::Bind(&InterceptDownloadResourceThrottle::CheckCookiePolicy,
+                   base::Unretained(this), info, callback));

[no sievers, 2016/06/10 19:40:06]

|this| might go away...

[Jinsuk Kim, 2016/06/13 03:21:12]

Set |defer| to true to make sure |this| will stay.

+  } else {
+    // Can't get any cookies, start download.
+    StartDownload(info);
+  }
+}
+
+void InterceptDownloadResourceThrottle::CheckCookiePolicy(
+    const DownloadInfo& info, const StartDownloadCB& callback,
+    const net::CookieList& cookie_list) {
+  if (request_->context()->network_delegate()->CanGetCookies(*request_,

[no sievers, 2016/06/10 19:40:06]

And even if |this| is still valid, |request| might have gone away here.

Other implementations of ResourceThrottle seem to assume that |request| is valid
at least until WillProcessResponse(). 

There is a second complication: To fix the race in the current code with
canceling the request we really need to defer the download until our async tasks
for the cookies complete (well, just to be able to keep |this| and the |request|
alive really). Currently we cancel the request too early which initiates
destruction of |this|. But if we don't cancel it from WillProcessResponse() it
will start the download internally. So I think we should set |defer| to |true|
in WillProcessResponse(). 

I think you'd still want to use a WeakPtr for any task we post to |this|. (Even
if we defer the request which probably keeps it alive, it's not totally clear
that the whole stack might not tear down for another reason.)

There is an alternative way also to rewrite this code a bit further so we don't
need to access |request| after the cookie retrieval completes. Let me know in
case you think that's better. (You basically use GetCookieListWithOptionsAsync()
in the first place and also see if reading cookies is allowed right away which
doesn't need the cookie_list. What needs the list is the recording of this event
i.e. TabSpecificContentSettings::CookiesRead() which can happen asynchronously
though.)

@Min: 
On a related note, this makes me wonder how ResourceThrottles like Safe Browsing
currently work since they are added *after* InterceptDownloadResourceThrottle
which cancels the request, see
ChromeResourceDispatcherHostDelegate::DownloadStarting() and
AppendStandardResourceThrottles(). If we push the download to the system manager
we are ignoring these throttles that run afterwards right?

Doesn't the intercepting throttle that delegates to the system download manager
always have to be last?

[Jinsuk Kim, 2016/06/13 03:21:12]

Set |defer| to true to resume the process later when the cookies complete.

[Jinsuk Kim, 2016/06/13 05:34:50]

Didn't split CanGetCookies() because it doesn't look straightforward to get
cookie_setting which is required to check if reading cookies is allowed without
actually calling the method.

I think it acceptable to keep the method as it doesn't seem to make the flow
complicated too much.

+                                                             cookie_list)) {
+    DoLoadCookies(info, callback);
+  } else {
+    callback.Run(info);
+  }
+}
+
+void InterceptDownloadResourceThrottle::DoLoadCookies(
+    const DownloadInfo& info,
+    const StartDownloadCB& callback) {
+  net::CookieOptions options;
+  options.set_include_httponly();
+
+  request_->context()->cookie_store()->GetCookiesWithOptionsAsync(
+      info.url, options,
+      base::Bind(&InterceptDownloadResourceThrottle::OnCookieResponse,
+                 base::Unretained(this), info, callback));
+}
+
+void InterceptDownloadResourceThrottle::OnCookieResponse(
+    DownloadInfo download_info,
+    const StartDownloadCB& callback,
+    const std::string& cookie) {
+  download_info.cookie = cookie;
+
+  // We have everything we need, start download.
+  callback.Run(download_info);
+}
+
+void InterceptDownloadResourceThrottle::StartDownload(
+    const DownloadInfo& info) {
+  DownloadControllerBase::Get()->CreateGETDownload(
+      render_process_id_, render_view_id_, must_download_, info);
   controller()->Cancel();
   RecordInterceptFailureReasons(NO_FAILURE);
 }