[on hold] Remove dependency on RenderViewHost from LoadState notifications

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 38 matching lines @@
 #include "content/browser/loader/async_revalidation_manager.h"
 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/mime_type_resource_handler.h"
 #include "content/browser/loader/navigation_resource_handler.h"
 #include "content/browser/loader/navigation_resource_throttle.h"
 #include "content/browser/loader/navigation_url_loader_impl_core.h"
 #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"
 #include "content/browser/loader/upload_data_stream_builder.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/resource_context_impl.h"
 #include "content/browser/service_worker/foreign_fetch_request_handler.h"
 #include "content/browser/service_worker/link_header_support.h"
 #include "content/browser/service_worker/service_worker_request_handler.h"
@@ skipping 2389 matching lines @@
   uint64_t b_uploading_size = 0;
   if (b.load_state.state == net::LOAD_STATE_SENDING_REQUEST)
     b_uploading_size = b.upload_size;
 
   if (a_uploading_size != b_uploading_size)
     return a_uploading_size > b_uploading_size;
 
   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) {
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466285
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       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_) {
     net::URLRequest* request = loader.second->request();
     net::UploadProgress upload_progress = request->GetUploadProgress();
 
     LoadInfo load_info;
     load_info.url = request->url();
     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() ||
         LoadInfoIsMoreInteresting(load_info, existing->second)) {
       (*info_map)[id] = load_info;
     }
   }
   return info_map;
 }
 
 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
   // will restart it as necessary.
   // Also stop the timer if there are no loading clients, to avoid waking up
   // unnecessarily when there is a long running (hanging get) request.
   if (info_map->empty() || !scheduler_->HasLoadingClients()) {
     update_load_states_timer_->Stop();
     return;
   }
@@ skipping 162 matching lines @@
   ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(), child_id);
   response->head.security_info = SerializeSecurityInfo(ssl);
 }
 
 CertStore* ResourceDispatcherHostImpl::GetCertStore() {
   return cert_store_for_testing_ ? cert_store_for_testing_
                                  : CertStore::GetInstance();
 }
 
 }  // namespace content