Move failed URLRequest checking to OnUrlRequestCompleted

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 f7da400c932756f0b3f1683ebdc6e30cfee248a9..d9110c5f5e9896d401842c426135ab12df2627b9 100644
--- a/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
+++ b/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc
@@ -151,6 +151,28 @@ ChromeDataUseAscriber::GetOrCreateDataUseRecorderEntry(
   return entry;
 }
 
+void ChromeDataUseAscriber::OnUrlRequestCompleted(net::URLRequest* request,
+                                                  bool started) {
+  ChromeDataUseRecorder* recorder = GetDataUseRecorder(*request);

[tbansal1, 2017/05/09 01:14:43]

why not GetOrCreateDataUseRecorderEntry (similar to OnUrlRequestDestroyed
below)?

[rajendrant, 2017/05/11 20:31:39]

GetOrCreateDataUseRecorderEntry is not needed here. There is no need to create
new entry and try to delete the same later.

GetOrCreateDataUseRecorderEntry is not needed in OnUrlRequestDestroyed as well.

Actually, GetOrCreateDataUseRecorder gets called in
DataUseAscriber::OnBeforeUrlRequest(), which creates the entry. Rest of the
places calling GetDataUseRecorder() is sufficient.

+
+  if (!recorder)
+    return;
+
+  const content::ResourceRequestInfo* request_info =
+      content::ResourceRequestInfo::ForRequest(request);
+  if (request_info &&

[tbansal1, 2017/05/09 01:14:43]

reverse the if-conditional for early return:
if(!request_info ||  request_info->GetResourceType() !=
content::RESOURCE_TYPE_MAIN_FRAME) {
return;
}

[rajendrant, 2017/05/11 20:31:39]

Done.

+      request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
+    auto navigation_iter = pending_navigation_data_use_map_.find(

[tbansal1, 2017/05/09 01:14:43]

IIUC (please confirm using the spec), you can simply delete the key from the
map:
pending_navigation_data_use_map_.erase(recorder->main_frame_request_id());

There is no need for getting the iterator, and deleting using the iterator. 

If you go this way, it might still be useful to keep the comments.

[rajendrant, 2017/05/11 20:31:39]

Done.

+        recorder->main_frame_request_id());
+    // If request was not successful, then NavigationHandle in
+    // DidFinishMainFrameNavigation will not have GlobalRequestID. So we erase
+    // the DataUseRecorderEntry here.
+    if (navigation_iter != pending_navigation_data_use_map_.end()) {

[tbansal1, 2017/05/09 01:14:43]

no parens.

[rajendrant, 2017/05/11 20:31:39]

Done.

+      pending_navigation_data_use_map_.erase(navigation_iter);
+    }
+  }
+}
+
 void ChromeDataUseAscriber::OnUrlRequestDestroyed(net::URLRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
@@ -159,8 +181,6 @@ void ChromeDataUseAscriber::OnUrlRequestDestroyed(net::URLRequest* request) {
   if (entry == data_use_recorders_.end())
     return;
 
-  DataUseRecorder* recorder = &(*entry);
-
   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 =
@@ -169,30 +189,16 @@ void ChromeDataUseAscriber::OnUrlRequestDestroyed(net::URLRequest* request) {
 
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(request);
-  content::ResourceType resource_type = request_info
-                                            ? request_info->GetResourceType()
-                                            : content::RESOURCE_TYPE_LAST_TYPE;
-
-  bool is_in_pending_navigation_map = false;
-  if (request_info && resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
-    auto navigation_iter = pending_navigation_data_use_map_.find(
-        entry->main_frame_request_id());
-    is_in_pending_navigation_map =
-        navigation_iter != pending_navigation_data_use_map_.end();
-
-    // If request was not successful, then NavigationHandle in
-    // DidFinishMainFrameNavigation will not have GlobalRequestID. So we erase
-    // the DataUseRecorderEntry here.
-    if (is_in_pending_navigation_map && !request->status().is_success()) {
-      pending_navigation_data_use_map_.erase(navigation_iter);
-      is_in_pending_navigation_map = false;
-    }
-  }
+  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();
 
   DataUseAscriber::OnUrlRequestDestroyed(request);
   request->RemoveUserData(DataUseRecorderEntryAsUserData::kUserDataKey);
 
-  if (recorder->IsDataUseComplete() && !is_in_render_frame_map &&
+  if (entry->IsDataUseComplete() && !is_in_render_frame_map &&
       !is_in_pending_navigation_map) {
     OnDataUseCompleted(entry);
     data_use_recorders_.erase(entry);