Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Committed: https://crrev.com/187e41f689795ef7ea15dc804ef31289211b12e9
Cr-Original-Commit-Position: refs/heads/master@{#431282}
Cr-Commit-Position: refs/heads/master@{#432672}

[Not at Google. Contact bengr, 2016-10-12 18:05:01]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

This allows NavigationHandles to be associated with their corresponding
MAIN_FRAME URLRequests.

BUG=645188
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

This allows NavigationHandles to be associated with their corresponding
MAIN_FRAME URLRequests.

BUG=645188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Not at Google. Contact bengr, 2016-10-12 18:05:30]

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

[commit-bot: I haz the power, 2016-10-12 18:06:03]

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-10-12 18:22:55]

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

[commit-bot: I haz the power, 2016-10-12 18:23:18]

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-10-12 18:34:41]

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

[commit-bot: I haz the power, 2016-10-12 18:34:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-10-12 18:39:40]

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

[commit-bot: I haz the power, 2016-10-12 18:40:13]

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-10-12 19:53:00]

kundaji@chromium.org changed reviewers:
+ clamy@chromium.org

[Not at Google. Contact bengr, 2016-10-12 19:53:01]

Following up from the email thread, here is a cl to expose GlobalRequestID in
NavigationHandle and ResourceRequestInfo. PTAL.

[Charlie Harrison, 2016-10-12 19:58:03]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2016-10-12 19:58:03]

drive-by: The rule of the land in //content is that nothing gets added to the
public API that is unused.

You should only change NavigationHandle and ResourceRequestInfo in a patch which
actually uses GlobalRequestID().

Also, new code should use "Id" not "ID" per the style guide. I'll defer to
//content owners for whether or not that trumps consistency within a class.

[commit-bot: I haz the power, 2016-10-12 20:12:01]

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

[commit-bot: I haz the power, 2016-10-12 20:12:01]

Dry run: This issue passed the CQ dry run.

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

On 2016/10/12 19:58:03, Charlie Harrison wrote:
> drive-by: The rule of the land in //content is that nothing gets added to the
> public API that is unused.
> 
> You should only change NavigationHandle and ResourceRequestInfo in a patch
which
> actually uses GlobalRequestID().
> 
> Also, new code should use "Id" not "ID" per the style guide. I'll defer to
> //content owners for whether or not that trumps consistency within a class.

My plan is to use the GlobalRequestID from both sources to associate
NavigationHandles with their MAIN_FRAME URLRequests. My concern is that a patch
making use of it will either seem incomplete or get really big.

clamy@: Please let me know if you prefer this however, and we can work out how
much of the use should be in the same patch. Also, please let me know what
naming convention you prefer.

[clamy, 2016-10-13 13:18:23]

On 2016/10/12 21:48:23, kundaji wrote:
> On 2016/10/12 19:58:03, Charlie Harrison wrote:
> > drive-by: The rule of the land in //content is that nothing gets added to
the
> > public API that is unused.
> > 
> > You should only change NavigationHandle and ResourceRequestInfo in a patch
> which
> > actually uses GlobalRequestID().
> > 
> > Also, new code should use "Id" not "ID" per the style guide. I'll defer to
> > //content owners for whether or not that trumps consistency within a class.
> 
> My plan is to use the GlobalRequestID from both sources to associate
> NavigationHandles with their MAIN_FRAME URLRequests. My concern is that a
patch
> making use of it will either seem incomplete or get really big.
> 
> clamy@: Please let me know if you prefer this however, and we can work out how
> much of the use should be in the same patch. Also, please let me know what
> naming convention you prefer.

I'm fine with exposing the request id to the public API, but as Charlie mentions
this should be done in a patch that makes use of it. Considering that the change
in this patch is very mechanical, I don't think it would be an issue to bundle
it with what makes use of it.

Regarding the style issue, I don't have a strong opinion, though I find that
naming the accessor after the name of the class it accesses slightly better from
a consistency POV.

[Not at Google. Contact bengr, 2016-10-13 18:48:15]

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

[commit-bot: I haz the power, 2016-10-13 18:48:34]

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-10-13 19:20:59]

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

[commit-bot: I haz the power, 2016-10-13 19:21:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-10-13 23:41:11]

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

[commit-bot: I haz the power, 2016-10-13 23:41:48]

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-10-13 23:51:21]

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

[commit-bot: I haz the power, 2016-10-13 23:51:45]

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-10-14 00:12:04]

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

[commit-bot: I haz the power, 2016-10-14 00:12:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-10-14 17:29:24]

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

[commit-bot: I haz the power, 2016-10-14 17:29:56]

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-10-14 18:05:50]

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

[commit-bot: I haz the power, 2016-10-14 18:05:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Not at Google. Contact bengr, 2016-10-26 21:07:55]

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

[commit-bot: I haz the power, 2016-10-26 21:09: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-10-26 21:10:09]

Patchset #4 (id:60001) has been deleted

[Not at Google. Contact bengr, 2016-10-26 21:10:23]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-10-26 22:33:25]

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

[commit-bot: I haz the power, 2016-10-26 22:33:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-10-26 22:59:06]

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

[commit-bot: I haz the power, 2016-10-26 22:59:52]

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-10-27 00:41:42]

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

[commit-bot: I haz the power, 2016-10-27 00:41:43]

Dry run: This issue passed the CQ dry run.

[Not at Google. Contact bengr, 2016-10-27 16:42:46]

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

[Not at Google. Contact bengr, 2016-10-27 16:43:03]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

This allows NavigationHandles to be associated with their corresponding
MAIN_FRAME URLRequests.

BUG=645188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Maintain a pending navigation map using GlobalRequestID as key. This
allows NavigationHandles to be associated with their corresponding
MAIN_FRAME URLRequests.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-27 16:43:18]

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-10-27 16:45:36]

Patchset #4 (id:100001) has been deleted

[Not at Google. Contact bengr, 2016-10-27 16:45:50]

Patchset #4 (id:120001) has been deleted

[Not at Google. Contact bengr, 2016-10-27 16:46:03]

Patchset #4 (id:140001) has been deleted

[Not at Google. Contact bengr, 2016-10-27 16:46:55]

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

[commit-bot: I haz the power, 2016-10-27 16:47:24]

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-10-27 16:48:05]

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

[Not at Google. Contact bengr, 2016-10-27 16:49:43]

PTAL

clamy@:
content/*

rajendrant@, ryansturm@:
chrome/browser/data_use_measurement/*

[commit-bot: I haz the power, 2016-10-27 17:08:39]

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

[commit-bot: I haz the power, 2016-10-27 17:08:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Not at Google. Contact bengr, 2016-10-27 17:13:23]

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

[commit-bot: I haz the power, 2016-10-27 17:14:02]

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-10-27 17:44:36]

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

[commit-bot: I haz the power, 2016-10-27 17:45:14]

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-10-27 18:47:06]

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

[commit-bot: I haz the power, 2016-10-27 18:47:07]

Dry run: This issue passed the CQ dry run.

[Raj, 2016-10-27 20:33:35]

On 2016/10/27 16:49:43, kundaji wrote:
> PTAL
> 
> clamy@:
> content/*
> 
> rajendrant@, ryansturm@:
> chrome/browser/data_use_measurement/*

chrome/browser/data_use_measurement/*

lgtm % nits

[Raj, 2016-10-27 20:33:56]

chrome/browser/data_use_measurement/*

lgtm % nits

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:110: auto
navigation_iter = pending_navigation_data_use_map_.find(
nit: This seems to be common code. Maybe wrap it in a function in anonymous
namespace. L110-L118 and L166-L173

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:93: typedef
std::list<std::unique_ptr<data_use_measurement::DataUseRecorder>>::
I wonder whether following typedefs could be easier to understand.

typedef std::list<std::unique_ptr<DataUseRecorder>> DataUseRecorderList;
typedef DataUseRecorderList::iterator DataUseRecorderListIter;

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:110: struct
GlobalRequestIDHash {
nit: Could this be in anonymous namespace in this file.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:117:
std::list<std::unique_ptr<DataUseRecorder>> data_use_recorders_;
If using DataUseRecorderList, change std::list<std::unique_ptr<DataUseRecorder>>

[RyanSturm, 2016-10-27 22:41:21]

bunch of nits, I'll look more at implementation tomorrow.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:82: auto
user_data = static_cast<DataUseRecorderEntryAsUserData*>(
nit: Can you either change auto to DataUseRecorderEntryAsUserData* or change the
DataUseUserData* above to auto?

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:93: typedef
std::list<std::unique_ptr<data_use_measurement::DataUseRecorder>>::
On 2016/10/27 20:33:56, Raj wrote:
> I wonder whether following typedefs could be easier to understand.
> 
> typedef std::list<std::unique_ptr<DataUseRecorder>> DataUseRecorderList;
> typedef DataUseRecorderList::iterator DataUseRecorderListIter;

+1

maybe add a typedef for std::unique_ptr<data_use_measurement::DataUseRecorder>
as well

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:96: class
DataUseRecorderEntryAsUserData : public base::SupportsUserData::Data {
I think you can anonymous namespace this class since you don't reference it in
the header.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:122:
base::hash_map<content::GlobalRequestID,
nit: Can this be?
std::unordered_map<RenderFrameHostID,
std::unique_ptr<data_use_measurement::DataUseRecorder>, GlobalRequestIDHash>

not much difference between the two since GlboalRequestId isn't std::pair, so I
would prefer STL

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
content/public/browser/navigation_handle.h:12: #include
"content/public/browser/global_request_id.h"
nit: can you forward declare this instead?

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
content/public/browser/navigation_handle.h:190: virtual const GlobalRequestID&
GetGlobalRequestID() const = 0;
const isn't allowed in content/public for the most part:
https://www.chromium.org/developers/content-module/content-api

You should be able to leave the const ref, but remove the const method.

Same thing for HasNetworkResponseStarted

[Not at Google. Contact bengr, 2016-10-28 18:42:33]

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

[commit-bot: I haz the power, 2016-10-28 18:43:06]

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-10-28 18:43:33]

Thanks for the review. PTAL.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:82: auto
user_data = static_cast<DataUseRecorderEntryAsUserData*>(
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> nit: Can you either change auto to DataUseRecorderEntryAsUserData* or change
the
> DataUseUserData* above to auto?

Done.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:110: auto
navigation_iter = pending_navigation_data_use_map_.find(
On 2016/10/27 20:33:56, Raj wrote:
> nit: This seems to be common code. Maybe wrap it in a function in anonymous
> namespace. L110-L118 and L166-L173

Done.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:93: typedef
std::list<std::unique_ptr<data_use_measurement::DataUseRecorder>>::
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> On 2016/10/27 20:33:56, Raj wrote:
> > I wonder whether following typedefs could be easier to understand.
> > 
> > typedef std::list<std::unique_ptr<DataUseRecorder>> DataUseRecorderList;
> > typedef DataUseRecorderList::iterator DataUseRecorderListIter;
> 
> +1
> 
> maybe add a typedef for std::unique_ptr<data_use_measurement::DataUseRecorder>
> as well

std::list<std::unique_ptr<DataUseRecorder>> is only used once. So apart from the
statement here, the typedef will not used anywhere else. My preference is to use
typedefs where the type is referred to multiple times. When it is used just
once, it just adds more code. WDYT?

DataUseRecorderList::iterator is already typedefed as DataUseRecorderEntry.

std::unique_ptr<data_use_measurement::DataUseRecorder> type is never  referred
to apart from the same declaration.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:96: class
DataUseRecorderEntryAsUserData : public base::SupportsUserData::Data {
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> I think you can anonymous namespace this class since you don't reference it in
> the header.

This class uses DataUseRecorderEntry which is a private typedef in
ChromeDataUseAscriber.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:110: struct
GlobalRequestIDHash {
On 2016/10/27 20:33:56, Raj wrote:
> nit: Could this be in anonymous namespace in this file.

Yes, but doesn't get us anything. Any file including the .h will be able to use
it.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:117:
std::list<std::unique_ptr<DataUseRecorder>> data_use_recorders_;
On 2016/10/27 20:33:56, Raj wrote:
> If using DataUseRecorderList, change
std::list<std::unique_ptr<DataUseRecorder>>

See above.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:122:
base::hash_map<content::GlobalRequestID,
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> nit: Can this be?
> std::unordered_map<RenderFrameHostID,
> std::unique_ptr<data_use_measurement::DataUseRecorder>, GlobalRequestIDHash>
> 
> not much difference between the two since GlboalRequestId isn't std::pair, so
I
> would prefer STL

Done.

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
content/public/browser/navigation_handle.h:12: #include
"content/public/browser/global_request_id.h"
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> nit: can you forward declare this instead?

Done.

https://codereview.chromium.org/2413663003/diff/240001/content/public/browser...
content/public/browser/navigation_handle.h:190: virtual const GlobalRequestID&
GetGlobalRequestID() const = 0;
On 2016/10/27 22:41:21, Ryan Sturm wrote:
> const isn't allowed in content/public for the most part:
> https://www.chromium.org/developers/content-module/content-api
> 
> You should be able to leave the const ref, but remove the const method.
> 
> Same thing for HasNetworkResponseStarted
> 
Done. 

There is already a const method in this file above (WasStartedFromContextMenu).

clamy@: Any preference here?

[commit-bot: I haz the power, 2016-10-28 20:08:28]

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

[commit-bot: I haz the power, 2016-10-28 20:08:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-10-29 00:07:10]

On 2016/10/28 18:43:33, kundaji wrote:
> clamy@: Any preference here?

(Note: Her status says OOO until Nov 2.)

I'll let her finish the review when she gets back, but just a few nits on
content/public.

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
content/public/browser/navigation_handle.h:186: virtual bool
HasNetworkResponseStarted() = 0;
nit: All methods in content/public should be documented.

Also, should these be in the "Parameters available at network request start
time" section rather than the "Navigation control flow" section?

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
content/public/browser/resource_request_info.h:166: virtual GlobalRequestID
GetGlobalRequestID() const = 0;
nit: All methods in content/public should be documented.

Also, this should be up by GetChildID() and GetRouteID().

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

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

[commit-bot: I haz the power, 2016-11-01 21:09:04]

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-01 21:11:07]

Thanks for the review!

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
content/public/browser/navigation_handle.h:186: virtual bool
HasNetworkResponseStarted() = 0;
On 2016/10/29 00:07:10, Charlie Reis wrote:
> nit: All methods in content/public should be documented.
> 
> Also, should these be in the "Parameters available at network request start
> time" section rather than the "Navigation control flow" section?

Done.

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2413663003/diff/260001/content/public/browser...
content/public/browser/resource_request_info.h:166: virtual GlobalRequestID
GetGlobalRequestID() const = 0;
On 2016/10/29 00:07:10, Charlie Reis wrote:
> nit: All methods in content/public should be documented.
> 
> Also, this should be up by GetChildID() and GetRouteID().

Done.

[commit-bot: I haz the power, 2016-11-01 23:48:11]

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

[commit-bot: I haz the power, 2016-11-01 23:48:12]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[RyanSturm, 2016-11-02 21:41:58]

This is definitely only a partial implementation, so there are a few
implementation details that seem off right now, that make sense with the design
doc.

i.e., the list seems unneccesary for this CL.

That being said, leave the functionality as is, but add comments that explain
what things are and why the list is necessary. If you comment on all of the
fields, it will be a lot easier for someone to read this, especially since it
has very little functional impact right now.

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/240001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:110: struct
GlobalRequestIDHash {
On 2016/10/27 20:33:56, Raj wrote:
> nit: Could this be in anonymous namespace in this file.

This has to be used in this file, and anonymous namespaces don't make that much
sense in header files. Leave it private.

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:63: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
nit: if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) return;

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:105: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
nit: if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) return;

[clamy, 2016-11-04 14:44:17]

Thanks! A few comments below.

https://codereview.chromium.org/2413663003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2413663003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:369: DCHECK_GE(state_,
WILL_PROCESS_RESPONSE);
This should be changed to
DCHECK(state_ == WILL_PROCESS_RESPONSE ||
state_ == DEFERRING_RESPONSE ||
state_ == READY_TO_COMMIT);

See my comment in NavigationHandle for the rationale.

https://codereview.chromium.org/2413663003/diff/280001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:185: // MAIN_FRAME URLRequest
associated with this navigation.
Actually, considering the case of transfer navigations, this only valid in
NavigationThrottle::WillProcessResponse and
WebContentsObserver::AboutToCommitNavigation, since we don't update the ID to
account for the new transferred request. This makes HasNetworkResponseStarted
unnecessary, since it can only be used in the 2 aforementioned methods.

[Not at Google. Contact bengr, 2016-11-04 20:56:13]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Maintain a pending navigation map using GlobalRequestID as key. This
allows NavigationHandles to be associated with their corresponding
MAIN_FRAME URLRequests.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads. 
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Not at Google. Contact bengr, 2016-11-04 21:07:22]

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

[commit-bot: I haz the power, 2016-11-04 21:08:23]

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-04 21:17:55]

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

[commit-bot: I haz the power, 2016-11-04 21:18:40]

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-04 22:40:24]

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

[commit-bot: I haz the power, 2016-11-04 22:40:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-11-07 17:33:19]

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

[commit-bot: I haz the power, 2016-11-07 17:33:54]

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

[RyanSturm, 2016-11-07 19:33:19]

lgtm % nits, add me to review the follow-up CL with core implementation.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:36: 
nit: remove line break

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:83: if
(user_data)
nit: Can you add a comment "If this request is already being tracked, do not
create a new entry."

Can interceptors restart requests meaning this method can get called twice for a
request?

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:53: //
DataUseAscriber:
nit: s/DataUseAscriber:/DataUseAscriber implementation:/

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:135: // Map from
RenderFrameHost to the DataUseRecorderEntry that the frame
nit: s/the DataUseRecorderEntry that/the DataUseRecorderEntry in
|data_use_recorders_| that/

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:140: // Map from
pending navigations to the DataUseRecorderEntry that the
nit: s/the DataUseRecorderEntry that/the DataUseRecorderEntry in
|data_use_recorders_| that/

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h
(right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:54: //
Propagates the event to the |ascriber_| on the IO thread. NavigationHandle
nit: usually referencing private members in public comments is confusing. It
looks like it is a few places that reference this. Would it be possible to say
something like "the global data use ascriber".

Leave it if that doesn't make sense.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/data_use_web_contents_observer.h
(right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/data_use_web_contents_observer.h:35: //
WebContentsObserver:
nit: s/WebContentsObserver/WebContentsObserver implmentation:/

The comments on the methods shouldn't be there as the WCO comments will be more
useful and accurate and these don't add any class specific information.

Remove the method comments and the vertical whitespace between methods.
Similar to
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics...

[commit-bot: I haz the power, 2016-11-07 20:40:24]

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

[commit-bot: I haz the power, 2016-11-07 20:40:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-11-07 22:15:11]

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

[commit-bot: I haz the power, 2016-11-07 22:15:41]

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-07 22:17:18]

Added more comments that explain details, such as why the recorder list is
pertinent in this cl as well. Also updated cl description.

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:63: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
On 2016/11/02 21:41:58, Ryan Sturm wrote:
> nit: if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) return;

Done.

https://codereview.chromium.org/2413663003/diff/260001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:105: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
On 2016/11/02 21:41:58, Ryan Sturm wrote:
> nit: if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) return;

In the current cl there is nothing being done for non main frame case, but this
will change in the next cl. So the early return pattern does not work well.

https://codereview.chromium.org/2413663003/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2413663003/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:369: DCHECK_GE(state_,
WILL_PROCESS_RESPONSE);
On 2016/11/04 14:44:17, clamy (slow) wrote:
> This should be changed to
> DCHECK(state_ == WILL_PROCESS_RESPONSE ||
> state_ == DEFERRING_RESPONSE ||
> state_ == READY_TO_COMMIT);
> 
> See my comment in NavigationHandle for the rationale.

Done.

https://codereview.chromium.org/2413663003/diff/280001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:185: // MAIN_FRAME URLRequest
associated with this navigation.
On 2016/11/04 14:44:17, clamy (slow) wrote:
> Actually, considering the case of transfer navigations, this only valid in
> NavigationThrottle::WillProcessResponse and
> WebContentsObserver::AboutToCommitNavigation, since we don't update the ID to
> account for the new transferred request. This makes HasNetworkResponseStarted
> unnecessary, since it can only be used in the 2 aforementioned methods.

Done.

I'm guessing you mean ReadyToCommitNavigation()? Changed to use
ReadyToCommitNavigation() throughout.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:36: 
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: remove line break

Done.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:83: if
(user_data)
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: Can you add a comment "If this request is already being tracked, do not
> create a new entry."
> 
> Can interceptors restart requests meaning this method can get called twice for
a
> request?

Done.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:53: //
DataUseAscriber:
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: s/DataUseAscriber:/DataUseAscriber implementation:/

Done.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:135: // Map from
RenderFrameHost to the DataUseRecorderEntry that the frame
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: s/the DataUseRecorderEntry that/the DataUseRecorderEntry in
> |data_use_recorders_| that/

Done.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:140: // Map from
pending navigations to the DataUseRecorderEntry that the
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: s/the DataUseRecorderEntry that/the DataUseRecorderEntry in
> |data_use_recorders_| that/

Done.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h
(right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:54: //
Propagates the event to the |ascriber_| on the IO thread. NavigationHandle
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: usually referencing private members in public comments is confusing. It
> looks like it is a few places that reference this. Would it be possible to say
> something like "the global data use ascriber".
> 
> Leave it if that doesn't make sense.

The only purpose of this class is to propagate calls to the
ChromeDataUseAscriber. I could say "the ChromeDataUseAscriber" each time instead
of referencing the field. But seemed to me that the comment as stands is more
specific. But no strong opinion.

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
File chrome/browser/data_use_measurement/data_use_web_contents_observer.h
(right):

https://codereview.chromium.org/2413663003/diff/340001/chrome/browser/data_us...
chrome/browser/data_use_measurement/data_use_web_contents_observer.h:35: //
WebContentsObserver:
On 2016/11/07 19:33:19, Ryan Sturm wrote:
> nit: s/WebContentsObserver/WebContentsObserver implmentation:/
> 
> The comments on the methods shouldn't be there as the WCO comments will be
more
> useful and accurate and these don't add any class specific information.
> 
> Remove the method comments and the vertical whitespace between methods.
> Similar to
>
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/metrics...

Done.

[Not at Google. Contact bengr, 2016-11-07 23:26:36]

Patchset #4 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-11-08 00:56:08]

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

[commit-bot: I haz the power, 2016-11-08 00:56:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Not at Google. Contact bengr, 2016-11-08 22:25:25]

clamy@: Can you please take a look.

[clamy, 2016-11-09 16:06:54]

Thanks! A few comments and this should be good.

https://codereview.chromium.org/2413663003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:138: // This is valid after
the network response has started.
Move this function to the block of implentation of NavigationHandle above and
remove the comment.

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/navigation_handle.h:188: // Returns the ID of the
MAIN_FRAME URLRequest associated with this
This is valid for subframes as well. I think we can remove "MAIN_FRAME" from the
comment.

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/navigation_handle.h:190: // and
WebContentsObserver::ReadyToCommitNavigation.
Can you add something like:
"Note: in the case of transfer navigations, this is the ID of the first request
made. The transferred request's ID will not be tracked by the NavigationHandle."

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/resource_request_info.h:96: // The unique identifier
across processes for this request.
s/The unique identifier across processes for this request./The globally unique
identifier for this request.

[Not at Google. Contact bengr, 2016-11-09 22:25:51]

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

[commit-bot: I haz the power, 2016-11-09 22:26:26]

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-09 22:28:41]

Patchset #14 (id:380001) has been deleted

[Not at Google. Contact bengr, 2016-11-09 22:29:33]

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

[Not at Google. Contact bengr, 2016-11-09 22:30:01]

Thanks for the review! PTAL.

https://codereview.chromium.org/2413663003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:138: // This is valid after
the network response has started.
On 2016/11/09 16:06:53, clamy wrote:
> Move this function to the block of implentation of NavigationHandle above and
> remove the comment.

Done.

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/navigation_handle.h:188: // Returns the ID of the
MAIN_FRAME URLRequest associated with this
On 2016/11/09 16:06:53, clamy wrote:
> This is valid for subframes as well. I think we can remove "MAIN_FRAME" from
the
> comment.

Done.

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/navigation_handle.h:190: // and
WebContentsObserver::ReadyToCommitNavigation.
On 2016/11/09 16:06:53, clamy wrote:
> Can you add something like:
> "Note: in the case of transfer navigations, this is the ID of the first
request
> made. The transferred request's ID will not be tracked by the
NavigationHandle."

Done.

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/2413663003/diff/360001/content/public/browser...
content/public/browser/resource_request_info.h:96: // The unique identifier
across processes for this request.
On 2016/11/09 16:06:54, clamy wrote:
> s/The unique identifier across processes for this request./The globally unique
> identifier for this request.

Done.

[commit-bot: I haz the power, 2016-11-09 22:30:31]

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-10 00:36:22]

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

[commit-bot: I haz the power, 2016-11-10 00:36:25]

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

[clamy, 2016-11-10 17:30:37]

Thanks! Lgtm.

[Not at Google. Contact bengr, 2016-11-10 17:40:08]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-10 17:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rajendrant@chromium.org, ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2413663003/#ps400001
(title: "comments")

[commit-bot: I haz the power, 2016-11-10 17:40:34]

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-10 17:45:48]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads. 
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads. 
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-10 17:45:51]

Committed patchset #14 (id:400001)

[commit-bot: I haz the power, 2016-11-10 18:08:20]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads. 
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

[commit-bot: I haz the power, 2016-11-10 18:08:22]

Patchset 14 (id:??) landed as
https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}

[Dirk Pranke, 2016-11-10 21:48:36]

A revert of this CL (patchset #14 id:400001) has been created in
https://codereview.chromium.org/2498433002/ by dpranke@chromium.org.

The reason for reverting is: Looks like this is causing a test to crash on Win7
debug:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...

So I'm reverting it, after double-checking w/ ryansturm@. Sorry!.

[Not at Google. Contact bengr, 2016-11-10 22:25:05]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

[Not at Google. Contact bengr, 2016-11-10 23:13:49]

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

[commit-bot: I haz the power, 2016-11-10 23:14:40]

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-10 23:25:44]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-11 16:25:39]

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

[commit-bot: I haz the power, 2016-11-11 16:26:06]

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-11 16:31:11]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-11 16:52:50]

Patchset #16 (id:440001) has been deleted

[Not at Google. Contact bengr, 2016-11-11 16:53:41]

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

[commit-bot: I haz the power, 2016-11-11 16:54:03]

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-11 17:05:48]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-11 17:38:27]

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

[commit-bot: I haz the power, 2016-11-11 17:38:48]

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-11 17:43:50]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-14 19:52:48]

Patchset #17 (id:480001) has been deleted

[Not at Google. Contact bengr, 2016-11-14 19:53:00]

Patchset #16 (id:460001) has been deleted

[Not at Google. Contact bengr, 2016-11-14 19:53:12]

Patchset #15 (id:420001) has been deleted

[Not at Google. Contact bengr, 2016-11-15 00:04:47]

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

[commit-bot: I haz the power, 2016-11-15 00:05:48]

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-15 03:39:04]

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

[commit-bot: I haz the power, 2016-11-15 03:39:05]

Dry run: This issue passed the CQ dry run.

[Not at Google. Contact bengr, 2016-11-16 20:00:40]

Patchset #15 (id:500001) has been deleted

[Not at Google. Contact bengr, 2016-11-16 20:59:27]

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

[commit-bot: I haz the power, 2016-11-16 21:00:16]

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-16 22:01:28]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-16 22:36:48]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2016-11-16 22:36:48]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
ryansturm@chromium.org, rajendrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2413663003/#ps520001
(title: "Use OnURLRequestDestroyed")

[commit-bot: I haz the power, 2016-11-16 22:37:46]

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-17 00:15:06]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

[commit-bot: I haz the power, 2016-11-17 00:15:08]

Committed patchset #15 (id:520001)

[commit-bot: I haz the power, 2016-11-17 00:32:49]

Description was changed from

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Cr-Commit-Position: refs/heads/master@{#431282}
==========

to

==========
Expose GlobalRequestID in NavigationHandle and ResourceRequestInfo.

Create DataUseRecorder instances associated with page loads.
Instances are associated with either pending navigations or render
frame hosts depending on the state of the page load. Use
GlobalRequestID to associate MAIN_FRAME URLRequests with their
corresponding pending navigation. Complete life-cycle of the
DataUseRecorders will be implemented in a forthcoming cl.

BUG=660065
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/763c3fe382f8ae7c39cfa28191eab55b1036e4e6
Committed: https://crrev.com/187e41f689795ef7ea15dc804ef31289211b12e9
Cr-Original-Commit-Position: refs/heads/master@{#431282}
Cr-Commit-Position: refs/heads/master@{#432672}
==========

[commit-bot: I haz the power, 2016-11-17 00:32:51]

Patchset 15 (id:??) landed as
https://crrev.com/187e41f689795ef7ea15dc804ef31289211b12e9
Cr-Commit-Position: refs/heads/master@{#432672}