PlzNavigate support in data use ascriber

chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc

Index: chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
diff --git a/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc b/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
index d2fcdfdcba47ad3a5287ea2eb757ecea4c244d59..c70be3754985f4936308d63ad5d6e1efdca9ec08 100644
--- a/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
+++ b/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
@@ -57,10 +57,6 @@ ChromeDataUseRecorder* ChromeDataUseAscriber::GetDataUseRecorder(
     const net::URLRequest& request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  // TODO(ryansturm): Handle PlzNavigate (http://crbug/664233).
-  if (content::IsBrowserSideNavigationEnabled())
-    return nullptr;
-
   // If a DataUseRecorder has already been set as user data, then return that.
   auto* user_data = static_cast<DataUseRecorderEntryAsUserData*>(
       request.GetUserData(DataUseRecorderEntryAsUserData::kUserDataKey));
@@ -72,10 +68,6 @@ ChromeDataUseAscriber::GetOrCreateDataUseRecorderEntry(
     net::URLRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  // TODO(ryansturm): Handle PlzNavigate (http://crbug/664233).
-  if (content::IsBrowserSideNavigationEnabled())
-    return data_use_recorders_.end();
-
   // If a DataUseRecorder has already been set as user data, then return that.
   auto* user_data = static_cast<DataUseRecorderEntryAsUserData*>(
       request->GetUserData(DataUseRecorderEntryAsUserData::kUserDataKey));
@@ -95,6 +87,27 @@ ChromeDataUseAscriber::GetOrCreateDataUseRecorderEntry(
     return entry;
   }
 
+  const content::ResourceRequestInfo* request_info =
+      content::ResourceRequestInfo::ForRequest(request);
+  if (!request_info ||
+      request_info->GetGlobalRequestID() == content::GlobalRequestID()) {
+    // Create a new DataUseRecorder for all other requests.
+    DataUseRecorderEntry entry =
+        CreateNewDataUseRecorder(request, DataUse::TrafficType::UNKNOWN);
+    DataUse& data_use = entry->data_use();
+    data_use.set_url(request->url());
+    return entry;
+  }
+
+  if (request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {

[RyanSturm, 2017/05/22 17:22:05]

Should this clause be before the other clause you are adding (more specifically
before the request_info->GetGlobalRequestID() == content::GlobalRequestID()
check)?

AFAIK, request_info->GetGlobalRequestID() == content::GlobalRequestID() could
happen in plzNavigate (not sure), in which case 
pending_navigation_data_use_map_ won't have an entry for it.

[rajendrant, 2017/05/24 07:14:09]

I guess, your question is about the following bug ?
https://bugs.chromium.org/p/chromium/issues/detail?id=680841

I think, only in some tests it may not be populated. Maybe this check can be
removed. I am being conservative here.

[RyanSturm, 2017/05/24 16:08:44]

sgtm.

+    DataUseRecorderEntry new_entry =
+        CreateNewDataUseRecorder(request, DataUse::TrafficType::USER_TRAFFIC);
+    new_entry->set_main_frame_request_id(request_info->GetGlobalRequestID());
+    pending_navigation_data_use_map_.insert(
+        std::make_pair(request_info->GetGlobalRequestID(), new_entry));
+    return new_entry;
+  }
+
   int render_process_id = -1;
   int render_frame_id = -1;
   bool has_valid_frame = content::ResourceRequestInfo::GetRenderFrameForRequest(
@@ -120,26 +133,6 @@ ChromeDataUseAscriber::GetOrCreateDataUseRecorderEntry(
       return data_use_recorders_.end();
     }
 
-    const content::ResourceRequestInfo* request_info =
-        content::ResourceRequestInfo::ForRequest(request);
-    content::ResourceType resource_type =
-        request_info ? request_info->GetResourceType()
-                     : content::RESOURCE_TYPE_LAST_TYPE;
-
-    if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
-      content::GlobalRequestID navigation_key =
-          request_info->GetGlobalRequestID();
-
-      DataUseRecorderEntry new_entry =
-          CreateNewDataUseRecorder(request, DataUse::TrafficType::USER_TRAFFIC);
-      new_entry->set_main_frame_request_id(navigation_key);
-      pending_navigation_data_use_map_.insert(
-          std::make_pair(navigation_key, new_entry));
-
-      return new_entry;
-    }
-
-    DCHECK(frame_iter != main_render_frame_data_use_map_.end());
     auto entry = frame_iter->second;
     request->SetUserData(
         DataUseRecorderEntryAsUserData::kUserDataKey,
@@ -194,25 +187,28 @@ void ChromeDataUseAscriber::OnUrlRequestDestroyed(net::URLRequest* request) {
   if (entry == data_use_recorders_.end())
     return;
 
-  RenderFrameHostID frame_key = entry->main_frame_id();
-  auto frame_iter = main_render_frame_data_use_map_.find(frame_key);
-  bool is_in_render_frame_map =
-      frame_iter != main_render_frame_data_use_map_.end() &&
-      frame_iter->second->HasPendingURLRequest(request);
-
+  bool will_datause_complete = false;
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(request);
-  bool is_in_pending_navigation_map =
-      request_info &&
-      request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME &&
-      pending_navigation_data_use_map_.find(entry->main_frame_request_id()) !=
-          pending_navigation_data_use_map_.end();
+
+  if (request_info &&
+      request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
+    will_datause_complete =
+        pending_navigation_data_use_map_.find(entry->main_frame_request_id()) ==
+        pending_navigation_data_use_map_.end();
+  } else {
+    // Non-mainframe, Services, and other requests.
+    auto frame_iter =
+        main_render_frame_data_use_map_.find(entry->main_frame_id());

[RyanSturm, 2017/05/22 17:22:05]

If the request is destroyed after the next page load commits in the same frame,
will this get deleted correctly? 

[rajendrant, 2017/05/24 07:14:09]

Yes. That happens in mainframe delete or next pageload commit, whichever is
sooner.

https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...
https://cs.chromium.org/chromium/src/chrome/browser/data_use_measurement/chro...

[RyanSturm, 2017/05/24 16:08:44]

I was thinking more along the lines of:

1) request A created for frame (1,1)
2) navigation in frame (1,1) to new page load
3) request A destroyed
4) main_render_frame_data_use_map_.find(entry->main_frame_id()) returns the next
page load because (1,1) is still around
5) frame_iter->second->HasPendingURLRequest(request) returns false since that
page load is active

wouldn't it better to also verify that (entry == frame_iter->second) and that
entry->HasPendingURLRequest, so that you check:
1) the frame is gone (via frame_iter == main_render_frame_data_use_map_.end())
or the page load is gone in the frame (via entry == frame_iter->second)
2) data use is complete for entry (via entry->HasPendingURLRequest as opposed to
frame_iter->second)

+    will_datause_complete =
+        frame_iter == main_render_frame_data_use_map_.end() ||
+        !frame_iter->second->HasPendingURLRequest(request);
+  }
 
   DataUseAscriber::OnUrlRequestDestroyed(request);
   request->RemoveUserData(DataUseRecorderEntryAsUserData::kUserDataKey);
 
-  if (entry->IsDataUseComplete() && !is_in_render_frame_map &&
-      !is_in_pending_navigation_map) {
+  if (entry->IsDataUseComplete() && will_datause_complete) {
     OnDataUseCompleted(entry);
     data_use_recorders_.erase(entry);
   }
@@ -224,9 +220,6 @@ void ChromeDataUseAscriber::RenderFrameCreated(int render_process_id,
                                                int main_render_frame_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (content::IsBrowserSideNavigationEnabled())
-    return;
-
   if (main_render_process_id != -1 && main_render_frame_id != -1) {
     // Create an entry in |subframe_to_mainframe_map_| for this frame mapped to
     // it's parent frame.
@@ -255,9 +248,6 @@ void ChromeDataUseAscriber::RenderFrameDeleted(int render_process_id,
                                                int main_render_frame_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (content::IsBrowserSideNavigationEnabled())
-    return;
-
   RenderFrameHostID key(render_process_id, render_frame_id);
 
   if (main_render_process_id == -1 && main_render_frame_id == -1) {