[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 c3392079e77a924e4fca903943b8d2c352987236..35be2887e6bb8ef763e5aee5ae17ce99a1880219 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"
@@ -2464,6 +2463,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.
 // static
 void ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread(
     std::unique_ptr<LoadInfoMap> info_map) {
@@ -2473,22 +2487,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;

[mmenke, 2016/05/17 21:25:36]

Why construct a map, instead of just calling LoadStateChanged here?

[Charlie Harrison, 2016/05/17 21:35:28]

We want to call LoadStateChanged with the load associated with the web contents
that has progressed the most. Calling it multiple times for loads that are in
different places could confuse web contents / web contents delegates that listen
for these notifications. I'm not confident, but I think it could potentially
cause jitter in the throbber.

[mmenke, 2016/05/17 21:44:04]

Ah, I missed the LoadInfoIsMoreInteresting call.  Ok, that makes sense...  Well,
it's a little weird that we do this exact same thing on a per=RenderFrameHost
basis on the IO thread, and then do it again over on the UI thread on a WC
basis, but doesn't seem like we have much choice in the matter, unless we want
to pass the load state of all requests over to the UI thread, and then do one
per-webcontents pass.  Doesn't seem like that gets us anything, though.

[Charlie Harrison, 2016/05/17 21:53:27]

Yeah it's a little awkward. I think this is better because if we send all the
load states, we'd necessarily have to allocate memory to store data (including
urls which can be huge) for all pending_loaders_. This way we can at least cut
that down to O(url * num_frames_with_pending_loads).

     }
   }
+  for (const auto& it : contents_map) {
+    it.first->LoadStateChanged(it.second->url, it.second->load_state,
+                               it.second->upload_position,
+                               it.second->upload_size);
+  }
 }
 
 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_) {
@@ -2500,8 +2521,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() ||
@@ -2513,6 +2537,8 @@ ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
 }
 
 void ResourceDispatcherHostImpl::UpdateLoadInfo() {
+  // Group load info by frame, then pass to the UI thread where each WebContents
+  // 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