Move failed URLRequest checking to OnUrlRequestCompleted

chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc

 // Copyright 2016 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.
 
 #include "chrome/browser/data_use_measurement/chrome_data_use_ascriber.h"
 
 #include <string>
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/data_use_measurement/chrome_data_use_recorder.h"
@@ skipping 133 matching lines @@
     return entry;
   }
 
   // Create a new DataUseRecorder for all other requests.
   DataUseRecorderEntry entry = CreateNewDataUseRecorder(request);
   DataUse& data_use = entry->data_use();
   data_use.set_url(request->url());
   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);
 
   DataUseRecorderEntry entry = GetOrCreateDataUseRecorderEntry(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 =
       frame_iter != main_render_frame_data_use_map_.end() &&
       frame_iter->second->HasPendingURLRequest(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);
   }
 }
 
 void ChromeDataUseAscriber::RenderFrameCreated(int render_process_id,
                                                int render_frame_id,
                                                int main_render_process_id,
                                                int main_render_frame_id) {
@@ skipping 209 matching lines @@
                                                    int new_render_process_id,
                                                    int new_render_frame_id) {
   if (visible_main_render_frames_.find(
           RenderFrameHostID(old_render_process_id, old_render_frame_id)) !=
       visible_main_render_frames_.end()) {
     WasShownOrHidden(new_render_process_id, new_render_frame_id, true);
   }
 }
 
 }  // namespace data_use_measurement