Remove use of WebContentsImpl from ResourceDispatcherHostImpl

content/browser/loader_delegate_impl.cc

 // Copyright 2016 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 "content/browser/loader_delegate_impl.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_thread.h"
 
 namespace content {
 
 LoaderDelegateImpl::~LoaderDelegateImpl() {}
 
 void LoaderDelegateImpl::LoadStateChanged(
     int child_id,
     int route_id,
     const GURL& url,
     const net::LoadStateWithParam& load_state,
     uint64_t upload_position,
     uint64_t upload_size) {

[mmenke, 2016/06/29 15:17:13]

These should all DCHECK what thread we're on, for documentation purposes, and as
a protection against regressions.

[jam, 2016/06/29 17:19:45]

IMO that's needless, i.e. we don't dcheck every single method in
content/browser/loader that runs on the IO thread. Public methods that are
called by embedders etc are useful to dcheck to make sure they're not calling it
incorrectly. but for a delegate like this that's called by RDH methods, we know
which threads will call it.

[mmenke, 2016/06/29 17:52:52]

I agree that classes that run on a single thread don't need DCHECKs.

This class, however, runs on multiple threads, and is even created on, destroyed
on, and owned by an object on a thread that's different from the one it's
consumed on, so I think that it's a good idea to have them.  They cost us very
little, and they protect it from misuse.

[scottmg, 2016/06/29 17:55:40]

Done. (I agree with jam that it's pretty clear right now, but it's low cost, and
will likely be helpful in the future as this delegate grows more methods)

[jam, 2016/06/29 18:13:23]

Regarding running on multiple threads, that's not the case once we remove the
static methods to anonymous namespace.

Regarding construction/destruction, that's also similar to RDH and many other
classes in content that live on the IO thread but are just created on the UI
thread (i.e. for example all message filters). I do not think it improves the
code quality by putting DCHECKs on all their methods.

[mmenke, 2016/06/29 18:20:51]

RDH was being used in a non-threadsafe manner on the UI thread until quite
recently.  DCHECKs would have prevented that.

[jam, 2016/06/29 20:36:48]

We may be saying the same thing.. i.e. in the bug you mention, who was calling?
if it's public RDH methods that are called by other code in content or chrome,
then adding dchecks sgtm as I mentioned earlier. but let's say there are
internal methods in RDH (or resource loader or RDHDelegate) that are only called
by RDH, then that's specifically what I'm referring to.

[mmenke, 2016/06/29 20:39:24]

They were being called by an external consumer.

   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&LoaderDelegateImpl::NotifyLoadStateChangedOnUI,
                  base::Unretained(this), child_id, route_id, url, load_state,
                  upload_position, upload_size));
 }
 
+void LoaderDelegateImpl::DidGetResourceResponseStart(
+    int render_process_id,
+    int render_frame_host,
+    std::unique_ptr<ResourceRequestDetails> details) {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&LoaderDelegateImpl::DidGetResourceResponseStartOnUI,
+                 base::Unretained(this), render_process_id, render_frame_host,
+                 base::Passed(std::move(details))));
+}
+
+void LoaderDelegateImpl::DidGetRedirectForResourceRequest(
+    int render_process_id,
+    int render_frame_host,
+    std::unique_ptr<ResourceRedirectDetails> details) {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&LoaderDelegateImpl::DidGetRedirectForResourceRequestOnUI,
+                 base::Unretained(this), render_process_id, render_frame_host,

[mmenke, 2016/06/29 15:17:13]

What guarantees that the loader hasn't been deleted by the time this task is
executed?

[mmenke, 2016/06/29 15:53:48]

Actually...Everything we do on the UI thread can just be static, can't it?   Or
better, moved into an anonymous namespace, and out of the header.  So we don't
even need to use |this|, or make assumptions about its lifetimes, as long as it
outlives the RDH...  Which I don't believe it actually does.  What are the
lifetime assumptions RDH makes about the LoaderDelegate?

[jam, 2016/06/29 17:19:46]

+1 to moving these methods to anonymous namespace

[scottmg, 2016/06/29 17:55:40]

LoaderDelegate does need to outlive the RDH, I added a comment to
RDH::SetLoaderDelegate() to that effect. But obviously no point here as you say,
so moved to statics.

+                 base::Passed(std::move(details))));
+}
+
 void LoaderDelegateImpl::NotifyLoadStateChangedOnUI(
     int child_id,
     int route_id,
     const GURL& url,
     const net::LoadStateWithParam& load_state,
     uint64_t upload_position,
     uint64_t upload_size) {
   RenderViewHostImpl* view = RenderViewHostImpl::FromID(child_id, route_id);
   if (view)
     view->LoadStateChanged(url, load_state, upload_position, upload_size);
 }
 
+void LoaderDelegateImpl::DidGetResourceResponseStartOnUI(
+    int render_process_id,
+    int render_frame_host,
+    std::unique_ptr<ResourceRequestDetails> details) {
+  RenderFrameHostImpl* host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_host);
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(host));
+  if (!web_contents)
+    return;
+  web_contents->DidGetResourceResponseStart(*details.get());
+}
+
+void LoaderDelegateImpl::DidGetRedirectForResourceRequestOnUI(
+    int render_process_id,
+    int render_frame_host,
+    std::unique_ptr<ResourceRedirectDetails> details) {
+  RenderFrameHostImpl* host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_host);
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(host));
+  if (!web_contents)
+    return;
+  web_contents->DidGetRedirectForResourceRequest(host, *details.get());
+}
+
 }  // namespace content