PageLoadMetrics renderer and browser implementation (deprecated)

csharrison has taken over this change. It has moved to https://codereview.chromium.org/1312213010/

[Bryan McQuade, 2015-08-17 13:05:03]

bmcquade@chromium.org changed reviewers:
+ davidben@chromium.org, rdsmith@chromium.org

[Bryan McQuade, 2015-08-17 13:06:24]

rdsmith: looking for your high level review on the overall change

davidben: looking for your review of the WebContentsObserver part of the change
(I'm looking for a reviewer for the RenderFrameObserver part)

Thanks!

[Randy Smith (Not in Mondays), 2015-08-17 19:31:08]

First pass review--I haven't reviewed tests or infrastructure files
(DEPS/gn/gyp).  I'll do those after a pass or two on the code implementation.

https://codereview.chromium.org/1282303002/diff/440001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1282303002/diff/440001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:186: // TODO(bmcquade): run on all platforms
once crbug.com/521010 is resolved.
I'm not sure which way I feel is better architecture, therefore suggestion: Also
avoid observing the RenderFrame on non-Android until this bug is resolved.

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:566: new
page_load_metrics::PageLoadMetricsRenderFrameObserver(render_frame);
This name is very long, and not particularly in keeping with the names of the
other observers attached to the render_frame.  Based on common usage, I'd
suggest shrinking it to just "PageLoadMetrics" or PageLoadMetricsObserver".  

I have no opinion as to whether standard practice on the creation of a component
suggests the extra namespace, but I wouldn't add the namespace unless you see
some evidence that it is a normal part of creating a component (and/or rely
heavily on guidance from a components/OWNER.)

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:566: new
page_load_metrics::PageLoadMetricsRenderFrameObserver(render_frame);
Same comment about size and nature of name.  Maybe
PageLoadMetricsSender/Receiver?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:51:
"PageLoad.TimeTo.DOMContentLoadedEvent", dom_content_loaded_event);
If these are new UMAs, shouldn't they have entries in
tools/metrics/histograms/histograms.xml?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:87:
// TODO(bmcquade): use DidChangePerformanceTiming instead, once available.
Is there a pointer to issue # or CL to include here so that someone coming on
the code later could figure out if this TODO should be executed?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:106:
return;
Does this mean that a same page navigation will nuke sending of metrics (because
DidStartProvisionalLoad() reset page_metrics_sender_) if the/some statistics
haven't yet been sent?  Is that ok?  Would it instead be useful/right to just
keep around the old page_metrics_sender_?  I think it's probably worth
commenting this issue, whatever the answer.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
page_metrics_sender_.reset(new PageMetricsSender(this));
I'd like a comment somewhere (not sure where, but here might be fine) that this
function is when the go/no-go about sending metrics is done, but the actual
sending of the metrics is delayed until we actually have the data.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:111:
page_metrics_sender_.reset();
Under what circumstances will this have an effect?  I suspect there's something
about the life cycle I'm not understanding, since I would think we'd always have
a null page_metrics_sender_ at this point, meaning that this should/could be a
DCHECK?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:172:
MaybeSendLoadEvent(timing);
I'll happily accept this architecture if you tell me it's necessary, but I'll
just call out that if there's a) some linear dependency between these events,
and b) the last event doesn't generally happen "too late" for us to get
statistics, I'd rather hold off until the last event and send all three.  

Even if (a) isn't true (as I suspect it isn't), I'll call out that doing is this
way means that in interpreting the stats, we may need to deal with web pages
that get killed having sent only a completely arbitrary subset of the stats,
which is going to make statistics analysis ... "interesting".  We might want
less data that we can make stronger assumptions about, e.g. waiting until all
three of these things happen and then sending.  Or maybe we want exactly this,
but to track how often we don't get all the information for a web page.  

Basically, I'd like us to think through the analysis portion of these stats
before we finish landing this CL--if you were analyzing the UMA from this CL,
how would you do it and what assumptions would you be able to make and not make?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:41:

I'm used to blank lines marking the end of // <baseclass> implementation blocks,
so I'd be inclined to remove this blank line.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:47:
virtual bool ShouldSendMetrics();
Mark as const to indicate that it really doesn't change state?  (Which seems to
be how it's used; just bringing decoration in line with usage.)

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:52:
scoped_ptr<PageMetricsSender> page_metrics_sender_;
Suggestion: Comment here indicating when this pointer is null and non-null.

[kinuko, 2015-08-18 10:37:38]

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

[kinuko, 2015-08-18 10:37:39]

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:73:
bool did_send_load_event_;
So this object's created for each page/document mainly for book-keeping which
fields of the Performance object are sent?

Do you have a plan about how we send custom metrics that aren't covered by
Performance API?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:131:
if (!url.SchemeIs("http") && !url.SchemeIs("https"))
nit: !url.SchemeIsHTTPOrHTTPS()

[kinuko, 2015-08-18 10:39:18]

kinuko@chromium.org changed reviewers:
+ rdsmith@google.com
- kinuko@chromium.org, rdsmith@chromium.org

[davidben, 2015-08-18 21:39:44]

davidben@chromium.org changed reviewers:
+ nasko@chromium.org

[davidben, 2015-08-18 21:39:45]

+nasko. I've only really glanced at the one file, but this seems to have a lot
of overlap with the Content Navigation API. (Also, this is all about metrics for
navigation, so he's probably a better reviewer than me. Also IPC.)

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:33:
OnDOMContentLoadedEvent)
What's the difference between this and WCO::DocumentLoadedInFrame? I guess it
includes metrics?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_LoadEvent, OnLoadEvent)
What's the difference between this and WCO::DidFinishLoad or
WCO::DidFinishNavigation?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Since this is adding IPCs and such, it'll need IPC review and I think
.../common/OWNERS needs to have the standard "per-file *_messages*.h blah"
lines.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:5: // IPC
messages for extensions.
extensions -> something else?

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:13:
IPC_MESSAGE_ROUTED1(PageLoadMetricsMsg_DOMContentLoadedEvent, base::TimeDelta)
Documentation?

[Bryan McQuade, 2015-08-19 11:45:14]

Thanks all for your comments. Will reply to remaining comments soon.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:33:
OnDOMContentLoadedEvent)
On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> What's the difference between this and WCO::DocumentLoadedInFrame? I guess it
> includes metrics?

Thanks for asking about this. I added some additional information about this in
the design proposal doc:
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s...

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_LoadEvent, OnLoadEvent)
On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> What's the difference between this and WCO::DidFinishLoad or
> WCO::DidFinishNavigation?

Same as above.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:73:
bool did_send_load_event_;
On 2015/08/18 10:37:39, kinuko wrote:
> So this object's created for each page/document mainly for book-keeping which
> fields of the Performance object are sent?
> 
> Do you have a plan about how we send custom metrics that aren't covered by
> Performance API?

The Blink WebPerformance object that exposes the timing information uses the nav
timing API (http://www.w3.org/TR/navigation-timing-2/) for many of its metrics,
but we also expose additional interesting metrics on WebPerformance that aren't
part of nav timing, such as time to first layout. My current plan is for us to
extend WebPerformance to expose other interesting metrics (e.g. time to first
text visible) as long as they are generally useful.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:87:
// TODO(bmcquade): use DidChangePerformanceTiming instead, once available.
On 2015/08/17 19:31:07, rdsmith wrote:
> Is there a pointer to issue # or CL to include here so that someone coming on
> the code later could figure out if this TODO should be executed?

Sure - this should actually be addressed before I submit this change - his code
is in review now: https://codereview.chromium.org/1289053003/.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:106:
return;
On 2015/08/17 19:31:07, rdsmith wrote:
> Does this mean that a same page navigation will nuke sending of metrics
(because
> DidStartProvisionalLoad() reset page_metrics_sender_) if the/some statistics
> haven't yet been sent?  Is that ok?  Would it instead be useful/right to just
> keep around the old page_metrics_sender_?  I think it's probably worth
> commenting this issue, whatever the answer.

DidStartProvisionalLoad doesn't get invoked for same-page navigations. I'll add
a comment for this.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:111:
page_metrics_sender_.reset();
On 2015/08/17 19:31:07, rdsmith wrote:
> Under what circumstances will this have an effect?  I suspect there's
something
> about the life cycle I'm not understanding, since I would think we'd always
have
> a null page_metrics_sender_ at this point, meaning that this should/could be a
> DCHECK?

Good point. A DCHECK seems much better here. Will make that change.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:172:
MaybeSendLoadEvent(timing);
On 2015/08/17 19:31:08, rdsmith wrote:
> I'll happily accept this architecture if you tell me it's necessary, but I'll
> just call out that if there's a) some linear dependency between these events,
> and b) the last event doesn't generally happen "too late" for us to get
> statistics, I'd rather hold off until the last event and send all three.  
> 
> Even if (a) isn't true (as I suspect it isn't), I'll call out that doing is
this
> way means that in interpreting the stats, we may need to deal with web pages
> that get killed having sent only a completely arbitrary subset of the stats,
> which is going to make statistics analysis ... "interesting".  We might want
> less data that we can make stronger assumptions about, e.g. waiting until all
> three of these things happen and then sending.  Or maybe we want exactly this,
> but to track how often we don't get all the information for a web page.  
> 
> Basically, I'd like us to think through the analysis portion of these stats
> before we finish landing this CL--if you were analyzing the UMA from this CL,
> how would you do it and what assumptions would you be able to make and not
make?

It's generally hard to predict when in the page load life cycle these events
will happen. First layout can happen before or after dom content, and before or
after onload. As we add other metrics (e.g. time-to-first-visible-text) they'll
further complicate any possible orderings. An issue with holding off is that we
do want to log events eagerly as they happen - if a tab has a first layout but
gets closed before the onload event fires, we do want to record the first layout
time in UMA, since a user did experience a layout for that page. Future work
will want to record the number of page loads that get aborted before first
layout/first paint, and it wouldn't be possible to record that information if we
deferred sending until a later metric.

Note too that we considered having csharrison's new notification API indicate
which metric or metrics actually changed, which would alleviate the need for
testing to see if each metric is ready to send, but that ends up adding
additional complexity to the notification API (enum to list all possible event
types, plumbing that through various parts of blink) so we decided that it was
overall simpler to have a less complex notification API from blink that only
says 'the metrics changed' and then we'd add logic here to figure out which
metrics are actually available/changed.

[nasko, 2015-08-20 00:22:18]

Some initial comments.

https://codereview.chromium.org/1282303002/diff/480001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1282303002/diff/480001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:588: if
(render_frame->GetRenderView()->GetMainRenderFrame() == render_frame) {
The code as you have it will break very soon. You can use
render_frame->GetWebFrame()->parent() and check for nullptr, since a frame
without a parent is the main one.

I'll spin a separate discussion whether RenderFrame::IsMainFrame should be added
to help such checks.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:13:
IPC_MESSAGE_ROUTED1(PageLoadMetricsMsg_DOMContentLoadedEvent, base::TimeDelta)
Please add a high level comment for the entire file or per-message comments,
whichever you think is better. Overall readers of this file on its own should be
able to deduce what the code here is used for.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:70:
PageLoadMetricsRenderFrameObserver* const parent_;
Parent has a very well defined meaning with frames - it is the parent frame. It
is confusing to see parent here refer to an outer class that initializes this
one.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:104:
// main HTML resource. DidStartProvisionalLoad doesn't get invoked for same
Do we only care about network loads? If so, we need to avoid data: and blob:
URLs as well.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:21:
class Timing {
I'm finding it is hard to read the code in the implementation of
PageLoadMetricsRenderFrameObserver because of the number of additional classes.
Is it possible to simplify this a bit? Especially since Timing is just a very
trivial wrapper around blink::WebPerformance.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:55:
void MaybeSendMetrics();
nit: Why the Maybe prefix? I think callers should always call SendMetrics and
whether it does a send for real or not is implementation detail, not something
that should be surfaced in the name of the method.

[kinuko, 2015-08-20 12:43:24]

On 2015/08/19 11:45:14, Bryan McQuade wrote:
> Thanks all for your comments. Will reply to remaining comments soon.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
> File
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
> (right):
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:33:
> OnDOMContentLoadedEvent)
> On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> > What's the difference between this and WCO::DocumentLoadedInFrame? I guess
it
> > includes metrics?
> 
> Thanks for asking about this. I added some additional information about this
in
> the design proposal doc:
>
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s...
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
> IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_LoadEvent, OnLoadEvent)
> On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> > What's the difference between this and WCO::DidFinishLoad or
> > WCO::DidFinishNavigation?
> 
> Same as above.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
> File
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
> (right):
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:73:
> bool did_send_load_event_;
> On 2015/08/18 10:37:39, kinuko wrote:
> > So this object's created for each page/document mainly for book-keeping
which
> > fields of the Performance object are sent?
> > 
> > Do you have a plan about how we send custom metrics that aren't covered by
> > Performance API?
> 
> The Blink WebPerformance object that exposes the timing information uses the
nav
> timing API (http://www.w3.org/TR/navigation-timing-2/) for many of its
metrics,
> but we also expose additional interesting metrics on WebPerformance that
aren't
> part of nav timing, such as time to first layout. My current plan is for us to
> extend WebPerformance to expose other interesting metrics (e.g. time to first
> text visible) as long as they are generally useful.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:87:
> // TODO(bmcquade): use DidChangePerformanceTiming instead, once available.
> On 2015/08/17 19:31:07, rdsmith wrote:
> > Is there a pointer to issue # or CL to include here so that someone coming
on
> > the code later could figure out if this TODO should be executed?
> 
> Sure - this should actually be addressed before I submit this change - his
code
> is in review now: https://codereview.chromium.org/1289053003/.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:106:
> return;
> On 2015/08/17 19:31:07, rdsmith wrote:
> > Does this mean that a same page navigation will nuke sending of metrics
> (because
> > DidStartProvisionalLoad() reset page_metrics_sender_) if the/some statistics
> > haven't yet been sent?  Is that ok?  Would it instead be useful/right to
just
> > keep around the old page_metrics_sender_?  I think it's probably worth
> > commenting this issue, whatever the answer.
> 
> DidStartProvisionalLoad doesn't get invoked for same-page navigations. I'll
add
> a comment for this.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:111:
> page_metrics_sender_.reset();
> On 2015/08/17 19:31:07, rdsmith wrote:
> > Under what circumstances will this have an effect?  I suspect there's
> something
> > about the life cycle I'm not understanding, since I would think we'd always
> have
> > a null page_metrics_sender_ at this point, meaning that this should/could be
a
> > DCHECK?
> 
> Good point. A DCHECK seems much better here. Will make that change.
> 
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:172:
> MaybeSendLoadEvent(timing);
> On 2015/08/17 19:31:08, rdsmith wrote:
> > I'll happily accept this architecture if you tell me it's necessary, but
I'll
> > just call out that if there's a) some linear dependency between these
events,
> > and b) the last event doesn't generally happen "too late" for us to get
> > statistics, I'd rather hold off until the last event and send all three.  
> > 
> > Even if (a) isn't true (as I suspect it isn't), I'll call out that doing is
> this
> > way means that in interpreting the stats, we may need to deal with web pages
> > that get killed having sent only a completely arbitrary subset of the stats,
> > which is going to make statistics analysis ... "interesting".  We might want
> > less data that we can make stronger assumptions about, e.g. waiting until
all
> > three of these things happen and then sending.  Or maybe we want exactly
this,
> > but to track how often we don't get all the information for a web page.  
> > 
> > Basically, I'd like us to think through the analysis portion of these stats
> > before we finish landing this CL--if you were analyzing the UMA from this
CL,
> > how would you do it and what assumptions would you be able to make and not
> make?
> 
> It's generally hard to predict when in the page load life cycle these events
> will happen. First layout can happen before or after dom content, and before
or
> after onload. As we add other metrics (e.g. time-to-first-visible-text)
they'll
> further complicate any possible orderings. An issue with holding off is that
we
> do want to log events eagerly as they happen - if a tab has a first layout but
> gets closed before the onload event fires, we do want to record the first
layout
> time in UMA, since a user did experience a layout for that page. Future work
> will want to record the number of page loads that get aborted before first
> layout/first paint, and it wouldn't be possible to record that information if
we
> deferred sending until a later metric.
> 
> Note too that we considered having csharrison's new notification API indicate
> which metric or metrics actually changed, which would alleviate the need for
> testing to see if each metric is ready to send, but that ends up adding
> additional complexity to the notification API (enum to list all possible event
> types, plumbing that through various parts of blink) so we decided that it was
> overall simpler to have a less complex notification API from blink that only
> says 'the metrics changed' and then we'd add logic here to figure out which
> metrics are actually available/changed.

I still wonder if doing some bookkeeping here is really better than using an
enum to notify what value is updated and/or solely managing stats data in the
browser process.  I usually expect a module named 'sender' to be a simple one.
In a near future if we may want to associate renderer-side stats with
browser-side stats, moving all bookkeeping logic to the browser side might be
actually simpler?

For example I could imagine an alternative idea like: 1) define a simple
PerformanceTiming struct (which could be a mirror of WebPerformance) in common/,
2) send the entire PerformanceTiming struct with some document ID to the browser
when(ever) we get notification (assuming that cost to send small data is almost
same up to a certain size), and 3) defer recording the data in the browser side
until the corresponding document's gone (or corresponding render frame or
process is gone, which should be able to be detected in the browser side).  If
we do defer recording in the browser side we don't seem to need to worry about
data loss.  (Of course this has its own pros/cons and you may have already
considered it)

[Bryan McQuade, 2015-08-24 15:17:26]

Patchset #1 (id:1) has been deleted

[Bryan McQuade, 2015-08-24 15:27:01]

Patchset #1 (id:20001) has been deleted

[Bryan McQuade, 2015-08-24 15:28:31]

Patchset #1 (id:40001) has been deleted

[Bryan McQuade, 2015-08-24 15:28:45]

Patchset #1 (id:60001) has been deleted

[Bryan McQuade, 2015-08-24 15:28:57]

Patchset #1 (id:80001) has been deleted

[Bryan McQuade, 2015-08-24 15:29:08]

Patchset #1 (id:100001) has been deleted

[Bryan McQuade, 2015-08-24 15:29:20]

Patchset #1 (id:120001) has been deleted

[Bryan McQuade, 2015-08-24 15:29:32]

Patchset #1 (id:140001) has been deleted

[Bryan McQuade, 2015-08-24 15:29:51]

Patchset #1 (id:160001) has been deleted

[Bryan McQuade, 2015-08-24 15:30:02]

Patchset #1 (id:180001) has been deleted

[Bryan McQuade, 2015-08-24 15:30:13]

Patchset #1 (id:200001) has been deleted

[Bryan McQuade, 2015-08-24 15:30:23]

Patchset #1 (id:220001) has been deleted

[Bryan McQuade, 2015-08-24 15:30:34]

Patchset #1 (id:240001) has been deleted

[Bryan McQuade, 2015-08-24 15:30:45]

Patchset #1 (id:260001) has been deleted

[Bryan McQuade, 2015-08-24 15:46:10]

Patchset #1 (id:280001) has been deleted

[Bryan McQuade, 2015-08-24 15:46:38]

Patchset #1 (id:300001) has been deleted

[Bryan McQuade, 2015-08-24 15:46:46]

Patchset #1 (id:320001) has been deleted

[Bryan McQuade, 2015-08-24 15:46:56]

Patchset #1 (id:340001) has been deleted

[Bryan McQuade, 2015-08-24 15:47:05]

Patchset #1 (id:360001) has been deleted

[Bryan McQuade, 2015-08-24 15:47:14]

Patchset #1 (id:380001) has been deleted

[Bryan McQuade, 2015-08-24 15:47:22]

Patchset #1 (id:400001) has been deleted

[Bryan McQuade, 2015-08-24 15:47:33]

Patchset #1 (id:420001) has been deleted

[Bryan McQuade, 2015-08-24 15:47:56]

Patchset #5 (id:520001) has been deleted

[Bryan McQuade, 2015-08-24 15:48:03]

Patchset #4 (id:500001) has been deleted

[Bryan McQuade, 2015-08-24 15:48:12]

Patchset #4 (id:540001) has been deleted

[Bryan McQuade, 2015-08-24 15:48:21]

Patchset #4 (id:560001) has been deleted

[Bryan McQuade, 2015-08-24 15:48:28]

Patchset #4 (id:580001) has been deleted

[Bryan McQuade, 2015-08-24 16:37:11]

Patchset #4 (id:600001) has been deleted

[Bryan McQuade, 2015-08-24 18:11:44]

Patchset #5 (id:640001) has been deleted

[Bryan McQuade, 2015-08-25 01:27:11]

Patchset #5 (id:660001) has been deleted

[Bryan McQuade, 2015-08-25 01:27:23]

Patchset #5 (id:660001) has been deleted

[Bryan McQuade, 2015-08-25 01:29:14]

Patchset #5 (id:680001) has been deleted

[Bryan McQuade, 2015-08-25 01:57:41]

Patchset #5 (id:700001) has been deleted

[Bryan McQuade, 2015-08-25 12:06:06]

Patchset #5 (id:720001) has been deleted

[Bryan McQuade, 2015-08-25 13:46:19]

On Thu, Aug 20, 2015 at 8:43 AM <kinuko@chromium.org> wrote:

> On 2015/08/19 11:45:14, Bryan McQuade wrote:
> > Thanks all for your comments. Will reply to remaining comments soon.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
> > File
>
>
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
> > (right):
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:33:
> > OnDOMContentLoadedEvent)
> > On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> > > What's the difference between this and WCO::DocumentLoadedInFrame? I
> > guess
> it
> > > includes metrics?
>
> > Thanks for asking about this. I added some additional information about
> > this
> in
> > the design proposal doc:
>
>
>
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s...
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
> > IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_LoadEvent, OnLoadEvent)
> > On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> > > What's the difference between this and WCO::DidFinishLoad or
> > > WCO::DidFinishNavigation?
>
> > Same as above.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
> > File
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
> > (right):
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:73:
> > bool did_send_load_event_;
> > On 2015/08/18 10:37:39, kinuko wrote:
> > > So this object's created for each page/document mainly for book-keeping
> which
> > > fields of the Performance object are sent?
> > >
> > > Do you have a plan about how we send custom metrics that aren't covered
> > by
> > > Performance API?
>
> > The Blink WebPerformance object that exposes the timing information uses
> > the
> nav
> > timing API (http://www.w3.org/TR/navigation-timing-2/) for many of its
> metrics,
> > but we also expose additional interesting metrics on WebPerformance that
> aren't
> > part of nav timing, such as time to first layout. My current plan is for
> > us to
> > extend WebPerformance to expose other interesting metrics (e.g. time to
> > first
> > text visible) as long as they are generally useful.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:87:
> > // TODO(bmcquade): use DidChangePerformanceTiming instead, once
> available.
> > On 2015/08/17 19:31:07, rdsmith wrote:
> > > Is there a pointer to issue # or CL to include here so that someone
> > coming
> on
> > > the code later could figure out if this TODO should be executed?
>
> > Sure - this should actually be addressed before I submit this change -
> his
> code
> > is in review now: https://codereview.chromium.org/1289053003/.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:106:
> > return;
> > On 2015/08/17 19:31:07, rdsmith wrote:
> > > Does this mean that a same page navigation will nuke sending of metrics
> > (because
> > > DidStartProvisionalLoad() reset page_metrics_sender_) if the/some
> > statistics
> > > haven't yet been sent?  Is that ok?  Would it instead be useful/right
> to
> just
> > > keep around the old page_metrics_sender_?  I think it's probably worth
> > > commenting this issue, whatever the answer.
>
> > DidStartProvisionalLoad doesn't get invoked for same-page navigations.
> > I'll
> add
> > a comment for this.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:111:
> > page_metrics_sender_.reset();
> > On 2015/08/17 19:31:07, rdsmith wrote:
> > > Under what circumstances will this have an effect?  I suspect there's
> > something
> > > about the life cycle I'm not understanding, since I would think we'd
> > always
> > have
> > > a null page_metrics_sender_ at this point, meaning that this
> > should/could be
> a
> > > DCHECK?
>
> > Good point. A DCHECK seems much better here. Will make that change.
>
>
>
>
https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
>
>
>
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:172:
> > MaybeSendLoadEvent(timing);
> > On 2015/08/17 19:31:08, rdsmith wrote:
> > > I'll happily accept this architecture if you tell me it's necessary,
> but
> I'll
> > > just call out that if there's a) some linear dependency between these
> events,
> > > and b) the last event doesn't generally happen "too late" for us to get
> > > statistics, I'd rather hold off until the last event and send all
> three.
> > >
> > > Even if (a) isn't true (as I suspect it isn't), I'll call out that
> > doing is
> > this
> > > way means that in interpreting the stats, we may need to deal with web
> > pages
> > > that get killed having sent only a completely arbitrary subset of the
> > stats,
> > > which is going to make statistics analysis ... "interesting".  We might
> > want
> > > less data that we can make stronger assumptions about, e.g. waiting
> > until
> all
> > > three of these things happen and then sending.  Or maybe we want
> exactly
> this,
> > > but to track how often we don't get all the information for a web page.
> > >
> > > Basically, I'd like us to think through the analysis portion of these
> > stats
> > > before we finish landing this CL--if you were analyzing the UMA from
> > this
> CL,
> > > how would you do it and what assumptions would you be able to make and
> > not
> > make?
>
> > It's generally hard to predict when in the page load life cycle these
> > events
> > will happen. First layout can happen before or after dom content, and
> > before
> or
> > after onload. As we add other metrics (e.g. time-to-first-visible-text)
> they'll
> > further complicate any possible orderings. An issue with holding off is
> > that
> we
> > do want to log events eagerly as they happen - if a tab has a first
> > layout but
> > gets closed before the onload event fires, we do want to record the first
> layout
> > time in UMA, since a user did experience a layout for that page. Future
> > work
> > will want to record the number of page loads that get aborted before
> first
> > layout/first paint, and it wouldn't be possible to record that
> > information if
> we
> > deferred sending until a later metric.
>
> > Note too that we considered having csharrison's new notification API
> > indicate
> > which metric or metrics actually changed, which would alleviate the need
> > for
> > testing to see if each metric is ready to send, but that ends up adding
> > additional complexity to the notification API (enum to list all possible
> > event
> > types, plumbing that through various parts of blink) so we decided that
> > it was
> > overall simpler to have a less complex notification API from blink that
> > only
> > says 'the metrics changed' and then we'd add logic here to figure out
> > which
> > metrics are actually available/changed.
>
> I still wonder if doing some bookkeeping here is really better than using
> an
> enum to notify what value is updated and/or solely managing stats data in
> the
> browser process.  I usually expect a module named 'sender' to be a simple
> one.
> In a near future if we may want to associate renderer-side stats with
> browser-side stats, moving all bookkeeping logic to the browser side might
> be
> actually simpler?


> For example I could imagine an alternative idea like: 1) define a simple
> PerformanceTiming struct (which could be a mirror of WebPerformance) in
> common/,
> 2) send the entire PerformanceTiming struct with some document ID to the
> browser
> when(ever) we get notification (assuming that cost to send small data is
> almost
> same up to a certain size), and 3) defer recording the data in the browser
> side
> until the corresponding document's gone (or corresponding render frame or
> process is gone, which should be able to be detected in the browser side).
> If
> we do defer recording in the browser side we don't seem to need to worry
> about
> data loss.  (Of course this has its own pros/cons and you may have already
> considered it)
>

Thanks for this suggestion! I updated the change to reflect this. I added a
PageLoadTiming struct that gets sent by the sender whenever stats change.
This has multiple benefits:
* straightforward to batch multiple updates that happen in a short time
period, to
reduce IPC contention
* alleviates the need to have the 'Timing' wrapper class that had added
undesirable complexity previously
* simplifies the sender side of the implementation - sender is now just
pushing the metrics to browser. browser process can do the necessary work
to log the right stats, as you suggest.

One open challenge is communicating which 'page' the timing stats are
associated with from renderer to browser process. Ideally we'd use some
sort of page identifier rather than a document id since there is a period
of navigation where a document object may not exist yet (time between nav
start and nav commit, IIUC). I see there are page_id and nav_entry_id
values communicated between render and browser process that may be useful
here, but more investigation is needed.


>
> https://codereview.chromium.org/1282303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bryan McQuade, 2015-08-25 13:53:36]

Thanks all for your feedback so far. I made some somewhat substantial changes in
response to Kinuko's suggestion. We now batch timing updates to avoid IPC
contention.

Note that the change is not fully finished yet - in particular some parts of the
WebContentsObserver have not been fully implemented. ttuttle is going to take
that part of the change over.

The main open issue is figuring out how to communicate the id of the page the
timing metrics are being sent for from render to browser process, so the browser
process knows which page to associate the received metrics with. I see page_id
or nav_entry_id , which are currently sent from render to browser in
FrameHostMsg_DidCommitProvisionalLoad may be the right identifiers to use for
this, but please let us know if there are better ways to communicate this
information.

https://codereview.chromium.org/1282303002/diff/440001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1282303002/diff/440001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:186: // TODO(bmcquade): run on all platforms
once crbug.com/521010 is resolved.
On 2015/08/17 19:31:07, rdsmith wrote:
> I'm not sure which way I feel is better architecture, therefore suggestion:
Also
> avoid observing the RenderFrame on non-Android until this bug is resolved.

Turns out we can address this issue in the browser process w/o fixing the bug.
I've changed both components to be instantiated unconditionally.

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:566: new
page_load_metrics::PageLoadMetricsRenderFrameObserver(render_frame);
On 2015/08/17 19:31:07, rdsmith wrote:
> This name is very long, and not particularly in keeping with the names of the
> other observers attached to the render_frame.  Based on common usage, I'd
> suggest shrinking it to just "PageLoadMetrics" or PageLoadMetricsObserver".  
> 
> I have no opinion as to whether standard practice on the creation of a
component
> suggests the extra namespace, but I wouldn't add the namespace unless you see
> some evidence that it is a normal part of creating a component (and/or rely
> heavily on guidance from a components/OWNER.)

Agree about shortening names. I used these very verbose names because I have
seen the Windows linker blow up if two object files have the same name, and I
wanted to choose sufficiently unique names to avoid that. But I don't actually
know if Chromium windows builds suffer from this issue - I'll find out. If not,
my inclination is to name these page_load_metrics::MainRenderFrameObserver
(there's some precedent for including 'RenderFrameObserver' in the names of
these classes i.e. ChromeRenderFrameObserver,
extensions::ExtensionsRenderFrameObserver) and
page_load_metrics::WebContentsObserver (precedent:
offline_pages::OfflinePageWebContentsObserver and
extensions::ChromeExtensionWebContentsObserver). 

RE: the namespace,
https://www.chromium.org/developers/design-documents/browser-components/cookbook
says that components should be enclosed in a namespace with the name of the
component directory.

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:566: new
page_load_metrics::PageLoadMetricsRenderFrameObserver(render_frame);
On 2015/08/17 19:31:07, rdsmith wrote:
> Same comment about size and nature of name.  Maybe
> PageLoadMetricsSender/Receiver?

I'm mixed on using the Sender/Receiver name. I think it's helpful to document
the main interface the class implements in the name (self documenting), and I
now have a helper 'PageTimingMetricsSender' class that's technically responsible
for sending the metrics, so I feel that calling the RFO a 'Sender' is not quite
right. I haven't renamed these classes yet though - will do that shortly.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:51:
"PageLoad.TimeTo.DOMContentLoadedEvent", dom_content_loaded_event);
On 2015/08/17 19:31:07, rdsmith wrote:
> If these are new UMAs, shouldn't they have entries in
> tools/metrics/histograms/histograms.xml?

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:5: // IPC
messages for extensions.
On 2015/08/18 21:39:45, David Benjamin (slow) wrote:
> extensions -> something else?

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:13:
IPC_MESSAGE_ROUTED1(PageLoadMetricsMsg_DOMContentLoadedEvent, base::TimeDelta)
On 2015/08/18 21:39:45, David Benjamin wrote:
> Documentation?

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
page_metrics_sender_.reset(new PageMetricsSender(this));
On 2015/08/17 19:31:07, rdsmith wrote:
> I'd like a comment somewhere (not sure where, but here might be fine) that
this
> function is when the go/no-go about sending metrics is done, but the actual
> sending of the metrics is delayed until we actually have the data.

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:131:
if (!url.SchemeIs("http") && !url.SchemeIs("https"))
On 2015/08/18 10:37:39, kinuko wrote:
> nit: !url.SchemeIsHTTPOrHTTPS()

Thanks, did not know about that. Fixed.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:41:

On 2015/08/17 19:31:08, rdsmith wrote:
> I'm used to blank lines marking the end of // <baseclass> implementation
blocks,
> so I'd be inclined to remove this blank line.

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:47:
virtual bool ShouldSendMetrics();
On 2015/08/17 19:31:08, rdsmith wrote:
> Mark as const to indicate that it really doesn't change state?  (Which seems
to
> be how it's used; just bringing decoration in line with usage.)

Done.

https://codereview.chromium.org/1282303002/diff/440001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:52:
scoped_ptr<PageMetricsSender> page_metrics_sender_;
On 2015/08/17 19:31:08, rdsmith wrote:
> Suggestion: Comment here indicating when this pointer is null and non-null.

Done.

https://codereview.chromium.org/1282303002/diff/480001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1282303002/diff/480001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:588: if
(render_frame->GetRenderView()->GetMainRenderFrame() == render_frame) {
On 2015/08/20 00:22:18, nasko wrote:
> The code as you have it will break very soon. You can use
> render_frame->GetWebFrame()->parent() and check for nullptr, since a frame
> without a parent is the main one.
> 
> I'll spin a separate discussion whether RenderFrame::IsMainFrame should be
added
> to help such checks.

Done, thanks!

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:13:
IPC_MESSAGE_ROUTED1(PageLoadMetricsMsg_DOMContentLoadedEvent, base::TimeDelta)
On 2015/08/20 00:22:18, nasko wrote:
> Please add a high level comment for the entire file or per-message comments,
> whichever you think is better. Overall readers of this file on its own should
be
> able to deduce what the code here is used for.

Done.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:70:
PageLoadMetricsRenderFrameObserver* const parent_;
On 2015/08/20 00:22:18, nasko wrote:
> Parent has a very well defined meaning with frames - it is the parent frame.
It
> is confusing to see parent here refer to an outer class that initializes this
> one.

This got refactored so we no longer have a parent_ pointer.

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:104:
// main HTML resource. DidStartProvisionalLoad doesn't get invoked for same
On 2015/08/20 00:22:18, nasko wrote:
> Do we only care about network loads? If so, we need to avoid data: and blob:
> URLs as well.

Ah, interesting. Are you saying that DidStartProvisionalLoad doesn't get invoked
for data: and blob: URLs?

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:21:
class Timing {
On 2015/08/20 00:22:18, nasko wrote:
> I'm finding it is hard to read the code in the implementation of
> PageLoadMetricsRenderFrameObserver because of the number of additional
classes.
> Is it possible to simplify this a bit? Especially since Timing is just a very
> trivial wrapper around blink::WebPerformance.

Agree - I ended up implementing Kinuko's suggestion to send the whole timing
structure when it changes, which had some nice additional benefits:
* makes it easy to batch multiple updates that happen in a short time period, to
reduce IPC contention
* alleviates the need to have this wrapper class
* simplifies the sender side of the implementation

https://codereview.chromium.org/1282303002/diff/480001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:55:
void MaybeSendMetrics();
On 2015/08/20 00:22:18, nasko wrote:
> nit: Why the Maybe prefix? I think callers should always call SendMetrics and
> whether it does a send for real or not is implementation detail, not something
> that should be surfaced in the name of the method.

Done.

[nasko, 2015-08-26 17:27:25]

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:27:
PageLoadMetricsWebContentsObserver::~PageLoadMetricsWebContentsObserver() {}
nit: Be consistent with the constructor. Either {} or {\n}, but not a mix of the
two.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:30:
const IPC::Message& message, content::RenderFrameHost* render_frame_host) {
Each parameter must be on a new line.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:32:
// WebContentsObserver callbacks get invoked on (browser UI thread?).
WebContentsObserver methods are only called on the UI thread. This is guaranteed
by WebContents itself being an UI thread only object.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:39:
// a page navigation starts/finishes.
Look at
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...,
which is a new set of functions we just introduced that make tracking
navigations much easier. They should be self explanatory and if you have
specific questions, feel free to ping me.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/common/page_load_metrics_messages.h:25:
IPC_MESSAGE_ROUTED1(PageLoadMetricsMsg_OnTimingUpdated,
nit: No need for "On" prefix, which we usually use for the handler method names.
The actual event is "TimingUpdated".

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:59:
// those metrics become available.
You won't get all the events in 1 second after the commit, especially on slower
loads and devices. Since you are using one shot timer, it is very likely a
non-trivial portion of the timing data will not contain full information.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
timing.navigation_start =
Why not create a copy constructor for PageLoadTiming that takes
blink::WebPerformance as input? That way we have only one place that does type
transformation.

Alternatively, why do we even need PageLoadTiming? Can't we just reuse the
blink::WebPerformance?

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:19:
IPC::Sender* ipc_sender, int routing_id, scoped_ptr<base::Timer> timer)
Each parameter on a separate line.

[Bryan McQuade, 2015-08-26 18:28:01]

Thanks for the feedback! We'll address these shortly. A couple quick comments in
the interim.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:39:
// a page navigation starts/finishes.
On 2015/08/26 17:27:24, nasko (out until Sept 1st) wrote:
> Look at
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...,
> which is a new set of functions we just introduced that make tracking
> navigations much easier. They should be self explanatory and if you have
> specific questions, feel free to ping me.

Thanks!

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:59:
// those metrics become available.
On 2015/08/26 17:27:24, nasko (out until Sept 1st) wrote:
> You won't get all the events in 1 second after the commit, especially on
slower
> loads and devices. Since you are using one shot timer, it is very likely a
> non-trivial portion of the timing data will not contain full information.

Though we use a OneShotTimer, the timer gets restarted each time new metrics are
received if the timer has already fired (from page_timing_metrics_sender.cc):

if (!timer_->IsRunning())
  timer_->Start(FROM_HERE,
      base::TimeDelta::FromMilliseconds(kTimerDelayMillis),
      base::Bind(&PageTimingMetricsSender::SendNow, base::Unretained(this)));

As best I can tell, OneShotTimer isn't one-shot in the sense that you start it
once, it fires once, and then you discard it, but rather in the sense that it is
non repeating, but can still be started multiple times. At least, that's my
understanding (I see other places in the codebase where it's used that way). If
you do think that's incorrect use, though, happy to figure out how to change it.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
timing.navigation_start =
On 2015/08/26 17:27:24, nasko (out until Sept 1st) wrote:
> Why not create a copy constructor for PageLoadTiming that takes
> blink::WebPerformance as input? That way we have only one place that does type
> transformation.
> 
> Alternatively, why do we even need PageLoadTiming? Can't we just reuse the
> blink::WebPerformance?

+1 to copy constructor that takes a WebPerformance. Will make that change.

I don't think we can use WebPerformance directly without nontrivial refactoring
in blink. As far as I can tell, there's no way to reconstruct a WebPerformance
object from raw data, which we'd need to do on the receiving end of this IPC in
the browser process. There are no setters on WebPerformance
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
and WebPerformanceImpl is backed by objects private to blink, rather than data
members, so adding setter support would be nontrivial. If there's something I'm
missing that does allow us to use WebPerformance directly though, let me know -
would definitely like to have fewer structs/classes if possible.

[Randy Smith (Not in Mondays), 2015-08-26 19:11:47]

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

[Randy Smith (Not in Mondays), 2015-08-26 19:11:48]

Next round.  I'm not yet looking at the tests, because I think there are basic
things about the architecture that haven't been completely settled yet.

WRT the tests, I'll call out that I have a visceral reaction against "make some
methods protected to override them in tests" as I feel like that muddies class
interface contracts and produces fragile tests, but I won't object to the
pattern unless I have a reasonable alternative suggestion, which I don't yet. 
So take this as a possible warning about a future test comment :-}.

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1282303002/diff/440001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:566: new
page_load_metrics::PageLoadMetricsRenderFrameObserver(render_frame);
On 2015/08/25 13:53:35, Bryan McQuade wrote:
> On 2015/08/17 19:31:07, rdsmith wrote:
> > This name is very long, and not particularly in keeping with the names of
the
> > other observers attached to the render_frame.  Based on common usage, I'd
> > suggest shrinking it to just "PageLoadMetrics" or PageLoadMetricsObserver". 

> > 
> > I have no opinion as to whether standard practice on the creation of a
> component
> > suggests the extra namespace, but I wouldn't add the namespace unless you
see
> > some evidence that it is a normal part of creating a component (and/or rely
> > heavily on guidance from a components/OWNER.)
> 
> Agree about shortening names. I used these very verbose names because I have
> seen the Windows linker blow up if two object files have the same name, and I
> wanted to choose sufficiently unique names to avoid that. But I don't actually
> know if Chromium windows builds suffer from this issue - I'll find out. If
not,
> my inclination is to name these page_load_metrics::MainRenderFrameObserver
> (there's some precedent for including 'RenderFrameObserver' in the names of
> these classes i.e. ChromeRenderFrameObserver,
> extensions::ExtensionsRenderFrameObserver) and
> page_load_metrics::WebContentsObserver (precedent:
> offline_pages::OfflinePageWebContentsObserver and
> extensions::ChromeExtensionWebContentsObserver). 
> 
> RE: the namespace,
>
https://www.chromium.org/developers/design-documents/browser-components/cookbook
> says that components should be enclosed in a namespace with the name of the
> component directory.

This all SG TM, though I don't consider myself the authority on these issues
(i.e. in case of disagreement, upvote hypnoOWNERS, ignore Randy :-}).

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.h:20: bool operator==(const
PageLoadTiming& other) const;
I have no objection, but document why this is needed and what semantics it
exports?  If it's all member equals, maybe = default
(http://chromium-cpp.appspot.com/ -> "Default Function Creation") would be
appropriate?

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.h:35: base::Time
first_layout;
As I understand it (please correct me if I'm wrong) on any particular message
any of these may not have been set.  How is that signaled?  Null Time()? 
However it's signaled, I think that it should be documented, both that they can
be null and what they are if they're null.

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1282303002/diff/740001/components/page_load_m...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:35: if
(!timer_->IsRunning())
It's not a lot of code complexity, so I don't object, but I do question whether
we really need to worry about coalescing IPCs.  This is three IPCs over the
course of a page load, none of which will contribute directly to user jank (i.e.
they aren't on any path the user waits on).  And if we do need coallescing, 1s
seems a random number.  So I'd vote against doing this without evidence that
it's needed.  Having said that, I think I should be outvoted by the people
requesting it, so consider this a quixotic comment :-}.

[Randy Smith (Not in Mondays), 2015-08-26 19:13:17]

rdsmith@chromium.org changed reviewers:
- rdsmith@google.com

[davidben, 2015-09-24 20:57:39]

davidben@chromium.org changed reviewers:
- davidben@chromium.org