Simplify DownloadRequestHandle by using WebContentsGetter

content/browser/download/download_request_handle.cc

Index: content/browser/download/download_request_handle.cc
diff --git a/content/browser/download/download_request_handle.cc b/content/browser/download/download_request_handle.cc
index 67dda4f502923373a848752bf2bdb52be590dac9..3d570bb9e59b5427da638e39f0f00dfe996e0591 100644
--- a/content/browser/download/download_request_handle.cc
+++ b/content/browser/download/download_request_handle.cc
@@ -6,86 +6,43 @@
 
 #include "base/bind.h"
 #include "base/strings/stringprintf.h"
-#include "content/browser/frame_host/navigator.h"
-#include "content/browser/frame_host/render_frame_host_impl.h"
-#include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/common/browser_side_navigation_policy.h"
 
 namespace content {
 
+namespace {
+
+WebContents* NullContents() {

[Charlie Harrison, 2016/02/23 19:22:49]

There may be a better way of doing this (base::Bind([]() { return NULL; }) ? Not
sure if lambdas work with Bind.

[asanka, 2016/02/24 20:02:16]

Or you could make GetWebContents() check for !web_contents_getter.is_null()
before calling Run(). That way you can pass an empty callback for the default
constructor.

Checking for is_null() isn't a bad idea by itself since callbacks don't confer
any guarantee of lifetime.

We could also get rid of the default constructor, which appears to be only used
by a DownloadManagerImpl test.

[Charlie Harrison, 2016/02/25 15:49:55]

I went with using the null callback.

+  return NULL;

[asanka, 2016/02/24 20:02:16]

NULL -> nullptr here and elsewhere.

[Charlie Harrison, 2016/02/25 15:49:55]

Done.

+}
+
+}  // namespace
+
 DownloadRequestHandleInterface::~DownloadRequestHandleInterface() {}
 
 DownloadRequestHandle::~DownloadRequestHandle() {}
 
 DownloadRequestHandle::DownloadRequestHandle()
-    : child_id_(-1),
-      render_view_id_(-1),
-      request_id_(-1),
-      frame_tree_node_id_(-1) {
-}
+    : web_contents_getter_(base::Bind(&NullContents)) {}
 
 DownloadRequestHandle::DownloadRequestHandle(
     const base::WeakPtr<DownloadResourceHandler>& handler,
-    int child_id,
-    int render_view_id,
-    int request_id,
-    int frame_tree_node_id)
-    : handler_(handler),
-      child_id_(child_id),
-      render_view_id_(render_view_id),
-      request_id_(request_id),
-      frame_tree_node_id_(frame_tree_node_id) {
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter)
+    : handler_(handler), web_contents_getter_(web_contents_getter) {
   DCHECK(handler_.get());
 }
 
 WebContents* DownloadRequestHandle::GetWebContents() const {
-  // PlzNavigate: if a FrameTreeNodeId was provided, use it to return the
-  // WebContents.
-  // TODO(davidben): This logic should be abstracted within the ResourceLoader
-  // stack. https://crbug.com/376003
-  if (IsBrowserSideNavigationEnabled()) {
-    FrameTreeNode* frame_tree_node =
-        FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
-    if (frame_tree_node) {
-      return WebContentsImpl::FromFrameTreeNode(frame_tree_node);
-    }
-  }
-
-  RenderViewHostImpl* render_view_host =
-      RenderViewHostImpl::FromID(child_id_, render_view_id_);
-  if (!render_view_host)
-    return nullptr;
-
-  return render_view_host->GetDelegate()->GetAsWebContents();
+  return web_contents_getter_.Run();
 }
 
 DownloadManager* DownloadRequestHandle::GetDownloadManager() const {
-  // PlzNavigate: if a FrameTreeNodeId was provided, use it to return the
-  // DownloadManager.
-  // TODO(davidben): This logic should be abstracted within the ResourceLoader
-  // stack. https://crbug.com/376003
-  if (IsBrowserSideNavigationEnabled()) {

[Charlie Harrison, 2016/02/23 19:22:49]

clamy@, this is all obsolete with the web contents getter right?

-    FrameTreeNode* frame_tree_node =
-        FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
-    if (frame_tree_node) {
-      BrowserContext* context =
-          frame_tree_node->navigator()->GetController()->GetBrowserContext();
-      if (context)
-        return BrowserContext::GetDownloadManager(context);
-    }
-  }
-
-  RenderViewHostImpl* rvh = RenderViewHostImpl::FromID(
-      child_id_, render_view_id_);
-  if (rvh == NULL)
-    return NULL;
-  RenderProcessHost* rph = rvh->GetProcess();
-  if (rph == NULL)
+  WebContents* web_contents = GetWebContents();
+  if (web_contents == NULL)
     return NULL;
-  BrowserContext* context = rph->GetBrowserContext();
+  BrowserContext* context = web_contents->GetBrowserContext();
   if (context == NULL)
     return NULL;
   return BrowserContext::GetDownloadManager(context);
@@ -110,14 +67,7 @@ void DownloadRequestHandle::CancelRequest() const {
 }
 
 std::string DownloadRequestHandle::DebugString() const {
-  return base::StringPrintf("{"
-                            " child_id = %d"
-                            " render_view_id = %d"
-                            " request_id = %d"
-                            "}",
-                            child_id_,
-                            render_view_id_,
-                            request_id_);
+  return "{}";

[Charlie Harrison, 2016/02/23 19:22:49]

The request_id_ was only used for this DebugString, should we keep it?

[asanka, 2016/02/24 20:02:16]

Not necessary. Looks like this is only called from
DownloadCreateInfo::DebugString(), which in turn is called by nobody. We should
be able to nuke this method + the corresponding method in
DownloadRequestHandleInterface + the corresponding method in
UrlDownloader::RequestHandle, and DownloadCreateInfo::DebugString().

[Charlie Harrison, 2016/02/25 15:49:55]

Nice. Done.

 }
 
 }  // namespace content