Create a DataUseRecorder instance for each page load in Chrome.

Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
Committed: https://crrev.com/f1a0d5d8b91d0168d3a978f006758bfe2c9888b9
Cr-Commit-Position: refs/heads/master@{#435544}

[Not at Google. Contact bengr, 2016-11-28 23:29:51]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[Not at Google. Contact bengr, 2016-11-28 23:30:37]

Description was changed from

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
==========

to

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
==========

[Not at Google. Contact bengr, 2016-11-28 23:30:37]

kundaji@chromium.org changed reviewers:
+ rajendrant@chromium.org, ryansturm@chromium.org

[commit-bot: I haz the power, 2016-11-28 23:30:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Not at Google. Contact bengr, 2016-11-28 23:31:11]

PTAL. Yet to add more tests.

[Not at Google. Contact bengr, 2016-11-28 23:38:30]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 23:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 00:05:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 00:05:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Not at Google. Contact bengr, 2016-11-29 17:02:25]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 17:02:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 17:57:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:57:30]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-29 20:33:00]

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:50: return entry
== data_use_recorders_.end() ? nullptr : (*entry).get();
nit: s/(*entry)./entry->/ I'm not totally sure that works though.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:79:
(*entry)->data_use().set_description(
nit: This is a little difficult to read. Maybe entry->get()->data_use()... If
that seems worse, leave it as is.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:61: void
OnBeforeUrlRequest(net::URLRequest* request) override;
Remove vertical whitespace between DataUseAscriber implementation methods

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:104:
DataUseRecorderEntry;
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

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:142:
std::list<std::unique_ptr<ChromeDataUseRecorder>> data_use_recorders_;
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.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_recorder.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_recorder.h:24:
RenderFrameHostID main_frame_id() { return main_frame_id_; }
const method

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_recorder.h:34:
content::GlobalRequestID main_frame_request_id() {
const method

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_ascriber.h:46: virtual
std::unique_ptr<URLRequestClassifier> CreateURLRequestClassifier()
add a comment

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.cc (left):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.cc:6: 
Add this whitespace back.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:33:
base::hash_set<net::URLRequest*>& pending_url_requests() {
Can this return a const ref and be a const method?

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:36: const
net::URLRequest* main_url_request() { return main_url_request_; }
const method

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:47: //  void
RemovePendingURLRequest(net::URLRequest* request);
This comment should be replaced by a description, and add a comment to
AddPendingURLRequest

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:83: // For page loads
this is main frame URLRequest.
nit: change this to something like: The main frame URLRequest for page loads.
Null if this is not tracking a page load.

[Not at Google. Contact bengr, 2016-11-29 21:08:02]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 23:17:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:17:31]

Dry run: This issue passed the CQ dry run.

[Not at Google. Contact bengr, 2016-11-29 23:40:02]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 23:41:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Not at Google. Contact bengr, 2016-11-30 00:21:16]

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:50: return entry
== data_use_recorders_.end() ? nullptr : (*entry).get();
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> nit: s/(*entry)./entry->/ I'm not totally sure that works though.

Not applicable since avoiding unique_ptr.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:79:
(*entry)->data_use().set_description(
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> nit: This is a little difficult to read. Maybe entry->get()->data_use()... If
> that seems worse, leave it as is.

Done

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:61: void
OnBeforeUrlRequest(net::URLRequest* request) override;
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> Remove vertical whitespace between DataUseAscriber implementation methods

Done.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:104:
DataUseRecorderEntry;
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> 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

Done.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:142:
std::list<std::unique_ptr<ChromeDataUseRecorder>> data_use_recorders_;
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> 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. 

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.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_recorder.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_recorder.h:24:
RenderFrameHostID main_frame_id() { return main_frame_id_; }
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> const method

Done.

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_recorder.h:34:
content::GlobalRequestID main_frame_request_id() {
On 2016/11/29 20:32:59, Ryan Sturm wrote:
> const method

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_ascriber.h:46: virtual
std::unique_ptr<URLRequestClassifier> CreateURLRequestClassifier()
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> add a comment

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.cc (left):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.cc:6: 
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> Add this whitespace back.

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:33:
base::hash_set<net::URLRequest*>& pending_url_requests() {
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> Can this return a const ref and be a const method?

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:36: const
net::URLRequest* main_url_request() { return main_url_request_; }
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> const method

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:47: //  void
RemovePendingURLRequest(net::URLRequest* request);
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> This comment should be replaced by a description, and add a comment to
> AddPendingURLRequest

Done.

https://codereview.chromium.org/2534023002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.h:83: // For page loads
this is main frame URLRequest.
On 2016/11/29 20:33:00, Ryan Sturm wrote:
> nit: change this to something like: The main frame URLRequest for page loads.
> Null if this is not tracking a page load.

Done.

[commit-bot: I haz the power, 2016-11-30 01:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 01:43:20]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

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

lgtm % comment

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:142:
std::list<std::unique_ptr<ChromeDataUseRecorder>> data_use_recorders_;
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.

https://codereview.chromium.org/2534023002/diff/80001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2534023002/diff/80001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:406:
DataUseRecorderEntry entry = data_use_recorders_.insert(
I think these two inserts (here and above) are slightly more efficient as
emplace(data_use_recorders_.end()) since it won't immediately copy it.

[Not at Google. Contact bengr, 2016-12-01 01:21:13]

The CQ bit was checked by kundaji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 01:21:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Not at Google. Contact bengr, 2016-12-01 01:23:26]

kundaji@chromium.org changed reviewers:
- rajendrant@chromium.org

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

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2534023002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:142:
std::list<std::unique_ptr<ChromeDataUseRecorder>> data_use_recorders_;
On 2016/12/01 00:40:46, Ryan Sturm wrote:
> 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.

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.

https://codereview.chromium.org/2534023002/diff/80001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2534023002/diff/80001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:406:
DataUseRecorderEntry entry = data_use_recorders_.insert(
On 2016/12/01 00:40:46, Ryan Sturm wrote:
> I think these two inserts (here and above) are slightly more efficient as
> emplace(data_use_recorders_.end()) since it won't immediately copy it.

Done.

[Not at Google. Contact bengr, 2016-12-01 01:23:48]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-12-01 01:46:45]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-12-01 01:46:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2534023002/#ps100001
(title: "Use emplace to insert; remove copy constructor")

[commit-bot: I haz the power, 2016-12-01 01:47:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 03:21:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480556805868140,
"parent_rev": "55a303c9fcfaa75bf8c13da30be3088160a53501", "commit_rev":
"f1519a8d71401b7363608729e73621a6af0a25dd"}

[commit-bot: I haz the power, 2016-12-01 03:22:14]

Description was changed from

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
==========

to

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
==========

[commit-bot: I haz the power, 2016-12-01 03:22:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-01 03:24:50]

Description was changed from

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532
==========

to

==========
Create a DataUseRecorder instance for each page load in Chrome.

Map each URLRequest originating from a frame to its DataUseRecorder.
Create a new DataUseRecorder for each URLRequest from Chrome services.
Move DataUseRecorders from pending navigation map to render frame
map when navigation commits.

BUG=663532

Committed: https://crrev.com/f1a0d5d8b91d0168d3a978f006758bfe2c9888b9
Cr-Commit-Position: refs/heads/master@{#435544}
==========

[commit-bot: I haz the power, 2016-12-01 03:24:51]

Patchset 6 (id:??) landed as
https://crrev.com/f1a0d5d8b91d0168d3a978f006758bfe2c9888b9
Cr-Commit-Position: refs/heads/master@{#435544}