sql::MetaTable.next_download_id, DownloadManager::GetNextId()

content/browser/renderer_host/resource_dispatcher_host.cc

Index: content/browser/renderer_host/resource_dispatcher_host.cc
diff --git a/content/browser/renderer_host/resource_dispatcher_host.cc b/content/browser/renderer_host/resource_dispatcher_host.cc
index 7030b105a227676cd035ece266b5e28cde9fe4c2..4dfae15b16b8298263bd69de8f0c33e9df75afca 100644
--- a/content/browser/renderer_host/resource_dispatcher_host.cc
+++ b/content/browser/renderer_host/resource_dispatcher_host.cc
@@ -22,6 +22,7 @@
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_util.h"
+#include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/renderer_host/download_resource_handler.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/cert_store.h"
@@ -703,12 +704,18 @@ void ResourceDispatcherHost::BeginDownload(
 
   request_id_--;
 
+  ProfileIOData* profile_io_data = reinterpret_cast<ProfileIOData*>(

[willchan no longer on Chromium, 2011/07/23 09:41:07]

content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
directly in content/. The content interface to stuff in ProfileIOData is
ResourceContext. But it's only allowed to expose an interface for content/. My
hunch is something like a GetNextIdThunkType is an implementation detail of
chrome/ and not something that jam@ would want exposed at the content/ level.
You should probably ask him. I don't really know how much of the download
subsystem is needed in the content/ level and how much should live in the
chrome/ level. Definitely all the UI components belong in the chrome/ level.
Anyhow, my suspicion is that the "correct" layering solution is to expose an
interface in ResourceContext that is a DownloadHandlerFactory or something that
produces ResourceHandlers. This DownloadHandlerFactory's implementation in
Chrome would be constructed with this GetNextIdThunkType (and probably also the
DownloadFileManager since I don't think that belongs in content/).

That's just my quick inkling of how this "should" work. You should check with
jam@.

[Randy Smith (Not in Mondays), 2011/07/25 20:20:12]

In the absence of commentary from jam, I'll go with my understanding of Will's
description and jam's plans, and say this should be in ResourceContext.

[benjhayden, 2011/07/27 19:40:54]

DownloadManager is moving to content/. Could a GetNextIdThunkType live in
ProfileIOData once that is done?

[benjhayden, 2011/07/27 19:40:54]

Even though DownloadManager is moving to content/?

[willchan no longer on Chromium, 2011/07/27 19:47:14]

I don't see how DownloadManager moving to content/ is relevant here. content/ is
not allowed to depend on anything in chrome/. In particular, it's not allowed to
depend on Profile&ProfileIOData, as those will always be in chrome/. It doesn't
matter if DownloadManager is moving to content/, Profile&ProfileIOData aren't
moving to content/, so you can't depend on ProfileIOData from content/.

Can't depend on ProfileIOData in content/ :)

[benjhayden, 2011/07/28 15:37:52]

Done.

+      context.GetUserData(NULL));
+  const DownloadManager::GetNextIdThunkType& get_next_id =
+      profile_io_data->next_download_id_thunk();
+
   scoped_refptr<ResourceHandler> handler(
       new DownloadResourceHandler(this,
                                   child_id,
                                   route_id,
                                   request_id_,
                                   url,
+                                  get_next_id,
                                   download_file_manager_.get(),
                                   request,
                                   prompt_for_save_location,
@@ -729,7 +736,7 @@ void ResourceDispatcherHost::BeginDownload(
 
   request->set_method("GET");
   request->set_referrer(MaybeStripReferrer(referrer).spec());
-  request->set_context(context.request_context());
+  request->set_context(request_context);
   request->set_load_flags(request->load_flags() |
       net::LOAD_IS_DOWNLOAD);