Create a DataUseRecorder instance for each page load in Chrome.

chrome/browser/data_use_measurement/chrome_data_use_ascriber.h

Index: chrome/browser/data_use_measurement/chrome_data_use_ascriber.h
diff --git a/chrome/browser/data_use_measurement/chrome_data_use_ascriber.h b/chrome/browser/data_use_measurement/chrome_data_use_ascriber.h
index e926ebf3cc9494cb8aaac758b7ac0dcae08559eb..08003b6a9cff5f8f4e8a1b4771b7c1513df89e78 100644
--- a/chrome/browser/data_use_measurement/chrome_data_use_ascriber.h
+++ b/chrome/browser/data_use_measurement/chrome_data_use_ascriber.h
@@ -7,6 +7,8 @@
 
 #include <list>
 #include <memory>
+#include <string>
+#include <tuple>
 #include <unordered_map>
 #include <utility>
 
@@ -14,6 +16,7 @@
 #include "base/hash.h"
 #include "base/macros.h"
 #include "base/supports_user_data.h"
+#include "chrome/browser/data_use_measurement/chrome_data_use_recorder.h"
 #include "components/data_use_measurement/core/data_use_ascriber.h"
 #include "content/public/browser/global_request_id.h"
 #include "url/gurl.h"
@@ -52,12 +55,11 @@ class ChromeDataUseAscriber : public DataUseAscriber {
   ~ChromeDataUseAscriber() override;
 
   // DataUseAscriber implementation:
-  DataUseRecorder* GetDataUseRecorder(net::URLRequest* request) override;
+  ChromeDataUseRecorder* GetDataUseRecorder(net::URLRequest* request,
+                                            bool can_create_new) override;
 
-  // Called before a request is sent.
   void OnBeforeUrlRequest(net::URLRequest* request) override;

[RyanSturm, 2016/11/29 20:32:59]

Remove vertical whitespace between DataUseAscriber implementation methods

[Not at Google. Contact bengr, 2016/11/30 00:21:15]

Done.

 
-  // Called when a URLRequest is being destroyed.
   void OnUrlRequestDestroyed(net::URLRequest* request) override;
 
   std::unique_ptr<URLRequestClassifier> CreateURLRequestClassifier()
@@ -91,21 +93,15 @@ class ChromeDataUseAscriber : public DataUseAscriber {
       bool is_same_page_navigation,
       void* navigation_handle);
 
-  // Called when a main frame navigation is redirected.
-  void DidRedirectMainFrameNavigation(GURL gurl,
-                                      int render_process_id,
-                                      int render_frame_id,
-                                      void* navigation_handle);
 
  private:
-  // Use as a key in the render frame map. Corresponds to a unique
-  // RenderFrameHost.
-  typedef std::pair<int, int> RenderFrameHostID;
+  friend class ChromeDataUseAscriberTest;
 
   // Entry in the |data_use_recorders_| list which owns all instances of
   // DataUseRecorder.
-  typedef std::list<std::unique_ptr<data_use_measurement::DataUseRecorder>>::
-      iterator DataUseRecorderEntry;
+  typedef std::list<
+      std::unique_ptr<data_use_measurement::ChromeDataUseRecorder>>::iterator
+      DataUseRecorderEntry;

[RyanSturm, 2016/11/29 20:32:59]

nit: IMO, this would be easier to read if you added 
typedef std::list<std::unique_ptr<ChromeDataUseRecorder>> DataUseRecorderList;
and changed it to 
typedef DataUseRecorderList::iterator DataUseRecorderEntry;

and changed data_use_recorders_ to be DataUserRecordereList

[Not at Google. Contact bengr, 2016/11/30 00:21:15]

Done.

 
   struct GlobalRequestIDHash {
    public:
@@ -128,13 +124,22 @@ class ChromeDataUseAscriber : public DataUseAscriber {
     DataUseRecorderEntry entry_;
   };
 
-  void DeletePendingNavigationEntry(content::GlobalRequestID global_request_id);
+  DataUseRecorderEntry GetDataUseRecorderEntry(net::URLRequest* request,
+                                               bool can_create_new);
+
+  void OnDataUseCompleted(DataUseRecorderEntry entry);
+
+  DataUseRecorderEntry CreateNewDataUseRecorder(net::URLRequest* request);
+
+  bool IsRecorderInPendingNavigationMap(net::URLRequest* request);
+
+  bool IsRecorderInRenderFrameMap(net::URLRequest* request);
 
   // Owner for all instances of DataUseRecorder. An instance is kept in this
   // list if any entity (render frame hosts, URLRequests, pending navigations)
   // that ascribe data use to the instance exists, and deleted when all
   // ascribing entities go away.
-  std::list<std::unique_ptr<DataUseRecorder>> data_use_recorders_;
+  std::list<std::unique_ptr<ChromeDataUseRecorder>> data_use_recorders_;

[RyanSturm, 2016/11/29 20:32:59]

Is there a reason why this is std::list<std::unique_ptr<ChromeDataUseRecorder>>
instead of just std::list<ChromeDataUseRecorder> I opened a few other comments
around dereferencing semantics being hard to read, but I don't see anywhere that
std::moves entries into or out of the list, so it seems like you might not need
the unique_ptr. 

[Not at Google. Contact bengr, 2016/11/30 00:21:15]

Switched to std::list<ChromeDataUseRecorder>.

Using unique_ptr avoids having to define copy constructors for recorders as well
as its fields. It doesn't make sense for a recorder to be copied once it is
created and assigned to a page load so it seemed nice to have this be impossible
statically. But not sure the odd syntax is worth this. Added copy constructor
for recorder and DataUse.

[RyanSturm, 2016/12/01 00:40:46]

I think it improves syntax, and if you use emplace instead of insert, the copy
constructor would never be called AFAIK (but probably still needs a definition).
So at that point it's a style choice between having the copy constructor or
having some of the double pointer dereferencing etc. I think it's a grey area
for style choice, so do whatever looks neater. I just didn't know if there were
plans to std::move the unique_ptrs around at some point.

[Not at Google. Contact bengr, 2016/12/01 01:23:39]

Yes, don't see a reason to std::move the unique ptrs. Could change back if the
need arises.

Using Emplace is a good idea -- it will allow adding back
DISALLOW_COPY_AND_ASSIGN.

 
   // Map from RenderFrameHost to the DataUseRecorderEntry in
   // |data_use_recorders_| that the frame ascribe data use to.