Send an event to the page load metrics to track resource starting.

Send an event to the page load metrics to track resource starting.

This will eventually allow page load metrics consumers to calculate
a percentage of known resource request completion dynamically.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0

BUG=699313
Review-Url: https://codereview.chromium.org/2780003003
Cr-Commit-Position: refs/heads/master@{#467011}
Committed: https://chromium.googlesource.com/chromium/src/+/5d47d0d535ba6b9eff503faaf6af0769faa8b8c3

[RyanSturm, 2017-03-29 19:03:46]

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

[RyanSturm, 2017-03-29 19:03:47]

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrom...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrom...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:359: const
GURL& url,
Are you using url here?

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:254:
ExtraRequestInfo extra_request_info(
Debatable whether the two request methods should share a struct or each have
their own. Perhaps ExtraRequestStartInfo and ExtraRequestCompleteInfo. I'm not
sure how much this information will overlap because it seems like only
content::Type would be share right now.

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:373: virtual void
OnStartedResource(const ExtraRequestInfo& extra_request_info) {}
I'd recommend adding an actual observer (and tests) in the same CL.

[Pete Williamson, 2017-04-13 23:10:20]

petewil@chromium.org changed reviewers:
+ bmcquade@google.com

[Pete Williamson, 2017-04-13 23:10:21]

OK, the unit tests are working, I think this is ready for a first look

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrom...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/loader/chrom...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:359: const
GURL& url,
On 2017/03/29 19:03:47, Ryan Sturm wrote:
> Are you using url here?

Done.

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:254:
ExtraRequestInfo extra_request_info(
On 2017/03/29 19:03:47, Ryan Sturm wrote:
> Debatable whether the two request methods should share a struct or each have
> their own. Perhaps ExtraRequestStartInfo and ExtraRequestCompleteInfo. I'm not
> sure how much this information will overlap because it seems like only
> content::Type would be share right now.

OK, I'll separate it into two types (and add the request_type to the both)

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:373: virtual void
OnStartedResource(const ExtraRequestInfo& extra_request_info) {}
On 2017/03/29 19:03:47, Ryan Sturm wrote:
> I'd recommend adding an actual observer (and tests) in the same CL.

Done.

[Pete Williamson, 2017-04-14 00:09:10]

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

[commit-bot: I haz the power, 2017-04-14 00:09:39]

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, 2017-04-14 01:04:47]

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

[commit-bot: I haz the power, 2017-04-14 01:04:48]

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_...)

[Pete Williamson, 2017-04-14 17:57:50]

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

[commit-bot: I haz the power, 2017-04-14 17:58: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, 2017-04-14 18:01:34]

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

[commit-bot: I haz the power, 2017-04-14 18:01:34]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[RyanSturm, 2017-04-14 20:08:43]

It seems like this change does nothing, no UMA or initializing the observer
(chrome/browser/page_load_metrics/page_load_metrics_initialize.cc).

I'm not totally opposed to that in and of itself, but it would be better to have
real product functionality.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:260:
GetTrackerOrNullForRequest(request_id, resource_type, creation_time);
As a note, Ibelieve this won't properly track the main page request as there
won't be a corresponding PageLoadTracker yet. This might be fixed if
GlobalRequestID is set in an earlier callback.

Please add comments about main page requests (can you verify that they are not
tracked properly).

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:135:
extra_request_info) override;
nit: s/extra_request_info/extra_request_complete_info/ (elsewhere as well).

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:31:
int64_t& started_count,
I think there is a style point against non-const reference variables. I'd
personally prefer std::pair as the return type, but passing in pointers instead
of refs is the other option.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:32:
int64_t& started_count,
#include <stdint.h>

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:10:
// const char kFakeUrl[] = "http://www.google.com/nothingotseehere.html";
Switch back to using the commented out kFakeUrl?

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: //
ChromeResourceDispatcherHostDelegate.
I'd adjust this comment to say "from ResourceRequestInfo" if you want to specify
the source of the value. Same below.

[Pete Williamson, 2017-04-17 16:48:52]

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

[commit-bot: I haz the power, 2017-04-17 16:49:16]

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, 2017-04-17 18:30:51]

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

[commit-bot: I haz the power, 2017-04-17 18:30:52]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-04-17 18:53:12]

Addressed all outstanding feedback.

I understand this would be better with a client using it, but it will take
several more changelists before the client is ready.  The plan is to implement
this in three pieces (See design doc in the main comments).  First is this, then
the next change hooks up the background loader offliner, then the last change
aggregates the resource loading notifications to use them as a signal.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:260:
GetTrackerOrNullForRequest(request_id, resource_type, creation_time);
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> As a note, Ibelieve this won't properly track the main page request as there
> won't be a corresponding PageLoadTracker yet. This might be fixed if
> GlobalRequestID is set in an earlier callback.
> 
> Please add comments about main page requests (can you verify that they are not
> tracked properly).

Comment added.  We aren't tracking the main page today, so it should be fine as
is.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:135:
extra_request_info) override;
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> nit: s/extra_request_info/extra_request_complete_info/ (elsewhere as well).

Done everywhere.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:31:
int64_t& started_count,
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> I think there is a style point against non-const reference variables. I'd
> personally prefer std::pair as the return type, but passing in pointers
instead
> of refs is the other option.

Done (Passing pointers instead of references).

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:32:
int64_t& started_count,
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:10:
// const char kFakeUrl[] = "http://www.google.com/nothingotseehere.html";
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> Switch back to using the commented out kFakeUrl?

Done (oops, I was planning to fix that before sending out for code review, sorry
about that.)

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: //
ChromeResourceDispatcherHostDelegate.
On 2017/04/14 20:08:43, Ryan Sturm wrote:
> I'd adjust this comment to say "from ResourceRequestInfo" if you want to
specify
> the source of the value. Same below.

Done in both places.

[RyanSturm, 2017-04-17 19:57:08]

lgtm % nits

Can you wait for bmcquade before submitting?

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:468: //
request if possible so we only have to cross to the main thread once.
Maybe you should create a bug (if you haven't) and reference it in this comment.
Fixing this might get lost otherwise.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:476: // The
lowering of request priority causes issues with scheduling, since
This block seems like a bad re-base.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:32:
// For the specified type, get the count of requests started and completed.
Can you update this comment to reflect that |type| is currently unused until it
is used?

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:40:
// change.};
nit: s/change.};/change./

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:55:
// Start the navigation.  This will create the page load tracker and register
nit: extra whitespace after "navigation."

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:198: // Container
for various information about a request within a page load.
nit: s/about a request/about a completed request/

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:227: // Container
for various information about a request within a page load.
nit: s/about a request/about a started request/

[Pete Williamson, 2017-04-17 20:46:36]

Waiting on BMcquade1's feedback

Fixed all identified issues.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:468: //
request if possible so we only have to cross to the main thread once.
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> Maybe you should create a bug (if you haven't) and reference it in this
comment.
> Fixing this might get lost otherwise.

Done.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:476: // The
lowering of request priority causes issues with scheduling, since
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> This block seems like a bad re-base.

Good catch!  Removed.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:32:
// For the specified type, get the count of requests started and completed.
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> Can you update this comment to reflect that |type| is currently unused until
it
> is used?

Done.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:40:
// change.};
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> nit: s/change.};/change./

Done.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:55:
// Start the navigation.  This will create the page load tracker and register
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> nit: extra whitespace after "navigation."

Done.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:198: // Container
for various information about a request within a page load.
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> nit: s/about a request/about a completed request/

Done.

https://codereview.chromium.org/2780003003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:227: // Container
for various information about a request within a page load.
On 2017/04/17 19:57:08, Ryan Sturm wrote:
> nit: s/about a request/about a started request/

Done.

[Pete Williamson, 2017-04-17 20:46:49]

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

[commit-bot: I haz the power, 2017-04-17 20:47:01]

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, 2017-04-17 22:04:20]

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

[commit-bot: I haz the power, 2017-04-17 22:04:21]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-04-17 23:21:42]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org
- bmcquade@google.com

[Bryan McQuade, 2017-04-17 23:22:42]

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

[Bryan McQuade, 2017-04-17 23:22:56]

Adding csharrison for chrome/browser/loader/ changes.

[Bryan McQuade, 2017-04-17 23:32:14]

Thanks! A couple comments - I think this seems reasonable. We'll need
chrome/browser/loader owners review for the change there. I want to better
understand how the new observer will be used as well, before we land that class.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:14:
class ResourceTrackingPageLoadMetricsObserver
is your plan for this class to extend it to also provide data to your class? if
so i think adding this here is ok. if this is only being added in order to write
unit tests, though, i think i'm inclined to instead move this class to a
unittest-only file to test this behavior.

if your plan is to use this class, do you have a follow up change that shows how
this class will be used? i'd be interested to see what that's going to look like
as well.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:35:
void GetCountsForType(const content::ResourceType type,
we've avoided having getters from any of the observers, for any purpose other
than testing. if this is just for testing purpose then let's call it
GetCountsForTypeForTesting() to make that clear. If the intent is to call this
from other places in chrome, I'd like to see if we can instead have the observer
notify the interested consumer of the updated info. This is what we do in the
android observer for example. The reason to avoid getting the observer is that
its lifetime is bound to a page load & so I generally worry about callers having
a pointer to the object, since it can go away at any time.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:31:
page_load_metrics::PageLoadMetricsObserverTestHarness::SetUp();
we can omit the override here since we're just invoking the base class behavior

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:205:
content::ResourceType found_type);
what does 'found' mean in found_type? Any reason not to call this resource_type?

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type
of the request as gleaned from the ResourceRequestInfo.
if we're already reporting this here, it feels redundant to also report it in
the complete struct, unless the value in the complete struct has been updated in
some way to take additional info into account.

[RyanSturm, 2017-04-17 23:37:32]

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type
of the request as gleaned from the ResourceRequestInfo.
On 2017/04/17 23:32:14, Bryan McQuade wrote:
> if we're already reporting this here, it feels redundant to also report it in
> the complete struct, unless the value in the complete struct has been updated
in
> some way to take additional info into account.

I think it would either need to be GRID or type to determine a completion % by
type, which is the end goal I believe.

[Charlie Harrison, 2017-04-18 16:51:33]

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void
NotifyUIThreadOfRequestStarted(
Can we use WebContentsObserver::DidGetResourceResponseStart instead?

[RyanSturm, 2017-04-18 17:23:58]

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void
NotifyUIThreadOfRequestStarted(
On 2017/04/18 16:51:32, Charlie Harrison wrote:
> Can we use WebContentsObserver::DidGetResourceResponseStart instead?

This seems to be called when headers are received. I think that won't track the
pending requests very well considering we can only have 6 sockets open to the
same host (I'm assuming each request takes 1 socket, but I'm not sure). Suppose
the page wanted to start 100 image requests, when 12 finish it would look like
12/18 are done, which might seem good enough. WDY?T

https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc...

[Charlie Harrison, 2017-04-18 17:38:35]

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:348: void
NotifyUIThreadOfRequestStarted(
On 2017/04/18 17:23:57, Ryan Sturm wrote:
> On 2017/04/18 16:51:32, Charlie Harrison wrote:
> > Can we use WebContentsObserver::DidGetResourceResponseStart instead?
> 
> This seems to be called when headers are received. I think that won't track
the
> pending requests very well considering we can only have 6 sockets open to the
> same host (I'm assuming each request takes 1 socket, but I'm not sure).
Suppose
> the page wanted to start 100 image requests, when 12 finish it would look like
> 12/18 are done, which might seem good enough. WDY?T
> 
>
https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc...

Woops, you're right I completely misread the name :P

[Charlie Harrison, 2017-04-18 17:40:43]

c/b/l LGTM but I only looked at that file. I in servicing your TODO it would be
nice to hook up into WebContentsObserver like the notification when a response
comes in.

[Pete Williamson, 2017-04-18 18:16:43]

On 2017/04/18 17:40:43, Charlie Harrison wrote:
> c/b/l LGTM but I only looked at that file. I in servicing your TODO it would
be
> nice to hook up into WebContentsObserver like the notification when a response
> comes in.

Thanks, updated the bug for refactoring with your comments.

[Pete Williamson, 2017-04-18 18:17:27]

Addressed BMcQuade comments.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:14:
class ResourceTrackingPageLoadMetricsObserver
On 2017/04/17 23:32:13, Bryan McQuade wrote:
> is your plan for this class to extend it to also provide data to your class?
if
> so i think adding this here is ok. if this is only being added in order to
write
> unit tests, though, i think i'm inclined to instead move this class to a
> unittest-only file to test this behavior.
> 
> if your plan is to use this class, do you have a follow up change that shows
how
> this class will be used? i'd be interested to see what that's going to look
like
> as well.

Design doc is here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0
Section titled Hookup the PLMO from offline pages code explains the plan in more
detail.
Basically, the plan is for this observer to forward the data to our Offliner
class.  I haven't coded that
changelist yet, but I will send the change to you for review when it is ready.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:35:
void GetCountsForType(const content::ResourceType type,
On 2017/04/17 23:32:14, Bryan McQuade wrote:
> we've avoided having getters from any of the observers, for any purpose other
> than testing. if this is just for testing purpose then let's call it
> GetCountsForTypeForTesting() to make that clear. If the intent is to call this
> from other places in chrome, I'd like to see if we can instead have the
observer
> notify the interested consumer of the updated info. This is what we do in the
> android observer for example. The reason to avoid getting the observer is that
> its lifetime is bound to a page load & so I generally worry about callers
having
> a pointer to the object, since it can go away at any time.

My original plan was to have the getter available to clients, but everything I
need can be 
accomplished by having the observer pass along notifications to the Offliner, so
I'll change
this into a 'for testing' method.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:31:
page_load_metrics::PageLoadMetricsObserverTestHarness::SetUp();
On 2017/04/17 23:32:14, Bryan McQuade wrote:
> we can omit the override here since we're just invoking the base class
behavior

Removed.

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:205:
content::ResourceType found_type);
On 2017/04/17 23:32:14, Bryan McQuade wrote:
> what does 'found' mean in found_type? Any reason not to call this
resource_type?

the intent is that the resource type can be determined in two ways, either from
the filename (which is what we do for ExtraRequestStartInfo), or from the
headers, which are considered more reliable, but could in theory be different
from the filename.  By 'found', I meant what we discovered in the headers.

Changed to detected_resource_type

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type
of the request as gleaned from the ResourceRequestInfo.
On 2017/04/17 23:32:14, Bryan McQuade wrote:
> if we're already reporting this here, it feels redundant to also report it in
> the complete struct, unless the value in the complete struct has been updated
in
> some way to take additional info into account.

I'm trying to allow for the possibility that we discovered the resource type
changed while parsing the headers.  According to Ryan, that does happen
sometimes.

Also, for each resource type, we need to know how many starts it has so far to
compare against how many completions.  The signal is that, for instance, at a
given time, 5 of 7 images have completed, we need this to count the "7" part. 
We include it again in the completion event to count the '5' part, and to make
corrections if the headers have a different type than the file extension.

[RyanSturm, 2017-04-18 18:37:13]

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:236: // The type
of the request as gleaned from the ResourceRequestInfo.
On 2017/04/18 18:17:27, Pete Williamson wrote:
> On 2017/04/17 23:32:14, Bryan McQuade wrote:
> > if we're already reporting this here, it feels redundant to also report it
in
> > the complete struct, unless the value in the complete struct has been
updated
> in
> > some way to take additional info into account.
> 
> I'm trying to allow for the possibility that we discovered the resource type
> changed while parsing the headers.  According to Ryan, that does happen
> sometimes.
> 
> Also, for each resource type, we need to know how many starts it has so far to
> compare against how many completions.  The signal is that, for instance, at a
> given time, 5 of 7 images have completed, we need this to count the "7" part. 
> We include it again in the completion event to count the '5' part, and to make
> corrections if the headers have a different type than the file extension.
AFAIK, there are basically four ways to determine resource type:
content::ResourceType, which roughly maps to the DOM element or action that
caused the resource to be downloaded (<img> tag for instance); extension type if
present can be useful (img.jpeg); MIME-type is a header that is the server
specifying what the type is and can generally be pretty useful if present and
accurate (image/webp); finally, how the resource is used in blink can give
something close to the ground-truth (this is pretty fuzzy as it can be used more
than once, and it is determined later).

I'm not sure what is best and haven't done an accuracy estimation for it in the
wild. I would say using content::ResourceType is sufficient in most places, and
the complete/started types will match up. XHRs are really the largest area for
the type to be unknown (they are treated as their own type), and a lot of times
MIME-type determination can be a good fallback for XHR types if the headers have
come in.

If you are training a model anyway, I'd just treat XHRs as XHRs and use
content::ResourceType as you are doing here. In terms of tracking pending vs
complete, you could keep a map of GlobalRequestID to resource type for pending
requests, but I think just keep counts of resource types is simpler as it seems
you are doing.

[Bryan McQuade, 2017-04-24 13:56:56]

LGTM, thanks! A couple comments - feel free to commit once you've decided how to
address them.

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:259: // Note
for the main HTML page, the tracker may not exist yet, so
interesting - the fact that the tracker doesn't exist yet is likely an artifact
of the fact that we create trackers in the WillStartRequest callback. Maybe we
should change to creating on DidStartNavigation... can you add a
"TODO(bmcquade): evaluate whether moving tracker creation to DidStartNavigation
would address this" or something similar?

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:35:
page_load_metrics::ResourceTrackingPageLoadMetricsObserver* observer_;
can you add a comment noting that the observer is owned by the last created
PageLoadTracker and its lifetime is dictated by that tracker's lifetime?

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: // The type
of the request as gleaned from the ResourceRequestInfo.
i think someone noted in a previous comment that the resource_type in the
complete info may be more accurate than the value in the start info. If that's
the case, can we note that, and explain in what ways it might be more accurate?
Let's also add a corresponding comment in the resource_type in the start info
that the type in the complete info may be more accurate, so consumers can better
decide which one is more useful for them.

[Pete Williamson, 2017-04-24 19:53:14]

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:259: // Note
for the main HTML page, the tracker may not exist yet, so
On 2017/04/24 13:56:55, Bryan McQuade wrote:
> interesting - the fact that the tracker doesn't exist yet is likely an
artifact
> of the fact that we create trackers in the WillStartRequest callback. Maybe we
> should change to creating on DidStartNavigation... can you add a
> "TODO(bmcquade): evaluate whether moving tracker creation to
DidStartNavigation
> would address this" or something similar?

Done.

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:35:
page_load_metrics::ResourceTrackingPageLoadMetricsObserver* observer_;
On 2017/04/24 13:56:55, Bryan McQuade wrote:
> can you add a comment noting that the observer is owned by the last created
> PageLoadTracker and its lifetime is dictated by that tracker's lifetime?

Done.

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2780003003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: // The type
of the request as gleaned from the ResourceRequestInfo.
On 2017/04/24 13:56:55, Bryan McQuade wrote:
> i think someone noted in a previous comment that the resource_type in the
> complete info may be more accurate than the value in the start info. If that's
> the case, can we note that, and explain in what ways it might be more
accurate?
> Let's also add a corresponding comment in the resource_type in the start info
> that the type in the complete info may be more accurate, so consumers can
better
> decide which one is more useful for them.

Done.

[Pete Williamson, 2017-04-24 19:53:22]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2017-04-24 19:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, csharrison@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2780003003/#ps160001
(title: "Add comments per bmcquade")

[commit-bot: I haz the power, 2017-04-24 19:54:32]

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, 2017-04-24 20:00:13]

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

[commit-bot: I haz the power, 2017-04-24 20:00:14]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pete Williamson, 2017-04-24 23:20:20]

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

[commit-bot: I haz the power, 2017-04-24 23:20:41]

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, 2017-04-25 00:16:11]

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

[commit-bot: I haz the power, 2017-04-25 00:16:13]

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_...)

[Pete Williamson, 2017-04-25 00:41:16]

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

[commit-bot: I haz the power, 2017-04-25 00:41:43]

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, 2017-04-25 02:05:26]

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

[commit-bot: I haz the power, 2017-04-25 02:05:27]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-04-25 16:23:38]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2017-04-25 16:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, ryansturm@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2780003003/#ps180001
(title: "FREEZE.unindexed - took merge and fixed it up")

[commit-bot: I haz the power, 2017-04-25 16:24:11]

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, 2017-04-25 16:28:48]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493137418381250,
"parent_rev": "d66951125e34dafe3843f8091a9314780161d197", "commit_rev":
"5d47d0d535ba6b9eff503faaf6af0769faa8b8c3"}

[commit-bot: I haz the power, 2017-04-25 16:29:00]

Description was changed from

==========
Send an event to the page load metrics to track resource starting.

This will eventually allow page load metrics consumers to calculate
a percentage of known resource request completion dynamically.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0

BUG=699313
==========

to

==========
Send an event to the page load metrics to track resource starting.

This will eventually allow page load metrics consumers to calculate
a percentage of known resource request completion dynamically.

Design doc here:
https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0

BUG=699313

Review-Url: https://codereview.chromium.org/2780003003
Cr-Commit-Position: refs/heads/master@{#467011}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d47d0d535ba6b9eff503faaf6af...
==========

[commit-bot: I haz the power, 2017-04-25 16:29:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/5d47d0d535ba6b9eff503faaf6af...

[Pete Williamson, 2017-06-12 23:20:46]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2940503003/ by petewil@chromium.org.

The reason for reverting is: After talking with the chrome loading team, they
expressed a preference to not use PageLoadMetrics for things happening on the
background thread, so we are changing our approach..