[on hold] Remove dependency on RenderViewHost from LoadState notifications

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index da256df980413b76fe334efae34d9216b3394399..d2dc9160001499d2cedef7408a2a0e98c02bc0ad 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -56,7 +56,6 @@
 #include "content/browser/loader/power_save_block_resource_throttle.h"
 #include "content/browser/loader/redirect_to_file_resource_handler.h"
 #include "content/browser/loader/resource_message_filter.h"
-#include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/loader/stream_resource_handler.h"
 #include "content/browser/loader/sync_resource_handler.h"
 #include "content/browser/loader/throttling_resource_handler.h"
@@ -2466,6 +2465,21 @@ bool ResourceDispatcherHostImpl::LoadInfoIsMoreInteresting(const LoadInfo& a,
   return a.load_state.state > b.load_state.state;
 }
 
+ResourceDispatcherHostImpl::LoadInfo::LoadInfo() {}
+
+ResourceDispatcherHostImpl::LoadInfo::~LoadInfo() {}
+
+ResourceDispatcherHostImpl::LoadInfo::LoadInfo(const LoadInfo& info) {
+  url = info.url;
+  load_state = info.load_state;
+  upload_position = info.upload_position;
+  upload_size = info.upload_size;
+  web_contents_getter = info.web_contents_getter;
+}
+
+// Note that GetLoadInfoForAllRoutes de-dupes based on frame, but not based on
+// tab.  Thus, logic here also constructs a map and finds the most interesting
+// load state per WebContents.

[mmenke, 2016/06/01 15:19:32]

Suggest moving this inside the method, or to the header - in general,
function-level comments should go with the function declaration.

[Charlie Harrison, 2016/06/02 14:22:28]

Moved to the header.

 // static
 void ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread(
     std::unique_ptr<LoadInfoMap> info_map) {
@@ -2475,22 +2489,29 @@ void ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "466285 ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread"));
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  std::map<WebContentsImpl*, const LoadInfo*> contents_map;
   for (const auto& load_info : *info_map) {
-    RenderViewHostImpl* view = RenderViewHostImpl::FromID(
-        load_info.first.child_id, load_info.first.route_id);
-    // The view could be gone at this point.
-    if (view) {
-      view->LoadStateChanged(load_info.second.url, load_info.second.load_state,
-                             load_info.second.upload_position,
-                             load_info.second.upload_size);
+    WebContentsImpl* web_contents = static_cast<WebContentsImpl*>(
+        load_info.second.web_contents_getter.Run());
+    if (!web_contents)
+      continue;
+    auto existing = contents_map.find(web_contents);
+    if (existing == contents_map.end() ||
+        LoadInfoIsMoreInteresting(load_info.second, *existing->second)) {
+      contents_map[web_contents] = &load_info.second;
     }
   }
+  for (const auto& it : contents_map) {
+    it.first->LoadStateChanged(it.second->url, it.second->load_state,
+                               it.second->upload_position,
+                               it.second->upload_size);

[mmenke, 2016/06/01 15:19:32]

Is it possible that calling LoadStateChanged on one WebContentsImpl would result
in another being destroyed?  Old code didn't have to worry about that, but this
code does.

Invoke the WC getter to avoid that.

I also assume it's impossible to re-parent a RenderFrame across WebContentses
(WebContenti?).

[nasko, 2016/06/01 16:54:00]

If it is possible with the new code, it was possible in the old one too.
Frames cannot be reparented or moved around at all. The moment they are detached
from the parent, they are destroyed.

[mmenke, 2016/06/01 16:58:49]

The way the old code was written here, though, it didn't matter with respect to
*this* code, as long as the WCGetters could handle it.  Now, however, we cache
the WC pointers, so if one is destroyed, we have a problem.

[Charlie Harrison, 2016/06/02 14:22:28]

You're right. I would say it's reasonable to assume LoadStateChanged will not
delete some other WC, but I'll do the safe thing here.

+  }
 }
 
 std::unique_ptr<ResourceDispatcherHostImpl::LoadInfoMap>
 ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
   // Populate this map with load state changes, and then send them on to the UI
-  // thread where they can be passed along to the respective RVHs.
+  // thread where they can be passed along to the respective WebContents.
   std::unique_ptr<LoadInfoMap> info_map(new LoadInfoMap());
 
   for (const auto& loader : pending_loaders_) {
@@ -2502,8 +2523,11 @@ ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
     load_info.load_state = request->GetLoadState();
     load_info.upload_size = upload_progress.size();
     load_info.upload_position = upload_progress.position();
+    load_info.web_contents_getter =
+        loader.second->GetRequestInfo()->GetWebContentsGetterForRequest();
 
-    GlobalRoutingID id(loader.second->GetRequestInfo()->GetGlobalRoutingID());
+    GlobalFrameRoutingId id(
+        loader.second->GetRequestInfo()->GetGlobalFrameRoutingId());
     LoadInfoMap::iterator existing = info_map->find(id);
 
     if (existing == info_map->end() ||
@@ -2515,6 +2539,8 @@ ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
 }
 
 void ResourceDispatcherHostImpl::UpdateLoadInfo() {
+  // Group load info by frame, then pass to the UI thread where each WebContents

[nasko, 2016/06/01 16:54:00]

nit: s/load info/LoadInfo/

[Charlie Harrison, 2016/06/02 14:22:28]

Done.

+  // picks the most interesting one.
   std::unique_ptr<LoadInfoMap> info_map(GetLoadInfoForAllRoutes());
 
   // Stop the timer if there are no more pending requests. Future new requests