PageLoadMetrics renderer and browser implementation.

PageLoadMetrics renderer and browser implementation.

This change addresses issues with lost UMA data, by pushing UMA metrics
to the browser process over IPC.

In the render process, we implement a RenderFrameObserver that attaches
to top-level frames, and pushes interesting document timing metrics to
the browser process.

In the browser process, we implement a WebContentsObserver that
receives the document timing metrics and UMA logs them. We implement
WebContentsObserver because, in a future change, we'll start observing
page close events in order to log cases where a page load is aborted
before a layout occurs.

See https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_sII/edit for additional details.

BUG=382542
patch from issue 1282303002 at patchset 740001 (http://crrev.com/1282303002#ps740001)

Committed: https://crrev.com/9fa62d54d03911d7a4120354d3e206ca69af4840
Cr-Commit-Position: refs/heads/master@{#350038}

[Bryan McQuade, 2015-09-02 18:26:45]

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

[Bryan McQuade, 2015-09-02 18:26:46]

This change is coming along nicely. Here are my comments so far. Feel free to
ping me if you have questions about any of them.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:28:
RecordTimingHistograms();
can we add a short comment here noting why it's correct to record timing
histograms as part of being destroyed? is this the codepath that gets invoked
when a tab is closed?

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
bool handled = true;
just to sanity check, can we add a DCHECK that this method is invoked on the
main browser thread? There's a part of me that worries this may get dispatched
on the IO thread, and if that's the case, we need to add some code to re-route
the dispatch of these methods to the main browser thread (since the other WCO
callbacks are invoked there).

Note that, to ease testing slightly, I think it's very reasonable to do
something like:

bool WebContentsObserver::OnMessageReceived(
    const IPC::Message& message,
    content::RenderFrameHost* render_frame_host) {
  // I'm sure there's a method for this already that you can use
  DCHECK(SomeMethodThatVerifiesWeAreOnTheRightThread());
  return DoDispatchMessage(message);
}

bool WebContentsObserver::DoDispatchMessage(const IPC::Message& message) {
  // the current implementation of OnMessageReceived goes here
}

Then in your tests you can dispatch messages directly to DoDispatchMessage. This
allows you to avoid having to fake out that this code is running on the main
thread in tests, which isn't going to validate anything (since it's stubbed out
using e.g. TestBrowserThreadBundle) and makes writing tests harder.

That said, I'm happy to write tests that verify use of right threads if the
tests we right are dispatching IPCs using the real IPC mechanism. I personally
don't think we'd need to do that though.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:52:
if (!url.SchemeIsHTTPOrHTTPS() && !browser_url.SchemeIsHTTPOrHTTPS())
does this want to be || rather than &&?

in particular, for the chrome://newtab case, where navigation handle returns an
https URL, but GetLastCommittedUrl returns chrome://newtab, seems we should bail
early if either one is not http/https.

let's also add a comment to explain what's going on here, and probably file a
bug noting the discrepancy in what the methods return, and that our code
currently depends on it, but longer term they should probably return the same
thing (and we should make sure that any changes in behavior do not break our
code).

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:69:
if (!GetLastCommittedURL().SchemeIsHTTPOrHTTPS())
let's add a short comment here to note the specific reason why this is needed
(ntp).

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:72:
if (timing.IsChild(current_timing_)) {
in general (throughout this change), you can remove braces for single line if
statements, per the style guide.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:74:
} else if (timing.IsChild(pending_timing_)) {
I'm not following what's going on with these if/elseif/else's - maybe add some
comments to explain why we want to know if timing is a child of current_timing_,
and if it's a child of pending_timing_, etc.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:82:
PageLoadTiming timing = current_timing_;
I don't think you need to take a function-scoped copy of current_timing_ here -
all the code in this class should run on the same thread, and we don't change
current_timing_ anywhere in the scope of the function.

Alternatively, if you want to pass a timing instance as a parameter to this
function, I think that's fine, but I'd pass it as a 'const PageLoadTiming&'
since there's no need to copy it.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:85:
else if (timing.IsEmpty())
nit: I'd do the test for timing.IsEmpty() before the test for
timing.IsComplete() - I generally like to see the bail-out conditions for
functions as early as possible.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:103:
const GURL& WebContentsObserver::GetLastCommittedURL() {
unless you need to override the implementation of this function in a test
(which, despite my having done it, i agree with randy is generally undesirable)
i think it's probably reasonable to just inline the implementation directly and
remove the function.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:41:

randy had suggested keeping all of the overridden methods together (removing
newlines between them), so let's remove this one.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:52:
virtual const GURL& GetLastCommittedURL();
a private method doesn't benefit from being virtual, since by being private it
cannot be overridden in base classes. so i'd suggest making this non-virtual.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:54:
PageLoadTiming current_timing_;
is it possible for us to get rid of having two members, and just have a single
'timing_' member?

if there's some need to keep temporary timing state around during certain
methods, perhaps that can be done using a variable local to a function, rather
than a second member.

i did not look closely, so if this is really needed (if the local variable
solution doesn't work) then we can keep it. but i do think it makes the code a
bit easier to reason about if there isn't both a current and pending timing. If
we do need to keep both, then please add a comment here to explain how they
differ (in what cases will pending be different from current etc).

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.cc:11:
PageLoadTiming::PageLoadTiming(blink::WebPerformance& p) {
in this case I think a single-character variable name is probably reasonably
readable, but I rarely see single-char variables other than 'i' for increment
variables. Can we call this something slightly longer, maybe 'perf'? I know that
doesn't make much of a difference - if for no other reason than for convention
that variable names are generally not just one char.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.cc:21: base::TimeDelta
PageLoadTiming::ClampDelta(double event_ms,
clamp delta doesn't need to be a member function - it can be a non-member
function in an anonymous namespace of the cc file. if you can avoid putting a
method in the header, i generally think it's better to make it just an
implementation detail where possible.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.cc:38: bool
PageLoadTiming::IsChild(const PageLoadTiming& parent) const {
I do think these policy methods are slightly implementation details of the web
contents observer, and might belong there instead. especially given that this is
a struct and shouldn't have a lot of functionality associated with it (see
related comment in header).

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:22:
PageLoadTiming(blink::WebPerformance&);
can you make the function parameter const?

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:25: bool operator==(const
PageLoadTiming& other) const;
I think randy had suggested trying to use the default implementation here (c++11
feature) - can you try changing this to:

bool operator==(const PageLoadTiming& other) const = default;

and removing the implementation in the cc file, and seeing if that works as
intended? I've never used 'default' for equality, so I'm not sure if it's
actually supported (or exactly how it works) but I think it's worth trying and
learning more about.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:29: bool IsChild(const
PageLoadTiming& parent) const;
Though these methods logically make sense as members, the recommendation in the
style guide is that structs should only contain data, and not functions. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C....

Normally I might suggest that we should just change this to a class, but given
that it is sent over IPC, and in general IPC wants to deal with just moving data
around, I think there is value in keeping it a struct.

If the logic in these methods is only needed in the web contents observer, it
may be worth adding them as helper methods that take a 'const PageLoadTiming&'
as an argument, in an anonymous namespace in the cc file.

I think it's worth asking Randy or someone else about this before making a
change though, since it's not 100% clear cut and I'd like to get a second
opinion.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:25:
timer_->Stop();
I know I wrote this bit of code, but I think it would benefit from a comment to
explain why we want to force send if the timer is running in the destructor.

[Charlie Harrison, 2015-09-02 18:38:24]

Thanks a ton for the detailed comments.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:103:
const GURL& WebContentsObserver::GetLastCommittedURL() {
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> unless you need to override the implementation of this function in a test
> (which, despite my having done it, i agree with randy is generally
undesirable)
> i think it's probably reasonable to just inline the implementation directly
and
> remove the function.

The reason is so we don't have to mock an entire WebContents to unittest this
class. We can just subclass it and alter this method. If there's a better way
than completely mocking WebContents, which would be a pretty big undertaking,
I'd definitely do it.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:25:
timer_->Stop();
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> I know I wrote this bit of code, but I think it would benefit from a comment
to
> explain why we want to force send if the timer is running in the destructor.

Yes good idea. This took me a little bit to grok.

[Bryan McQuade, 2015-09-02 18:38:31]

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
bool handled = true;
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> just to sanity check, can we add a DCHECK that this method is invoked on the
> main browser thread? There's a part of me that worries this may get dispatched
> on the IO thread, and if that's the case, we need to add some code to re-route
> the dispatch of these methods to the main browser thread (since the other WCO
> callbacks are invoked there).
> 
> Note that, to ease testing slightly, I think it's very reasonable to do
> something like:
> 
> bool WebContentsObserver::OnMessageReceived(
>     const IPC::Message& message,
>     content::RenderFrameHost* render_frame_host) {
>   // I'm sure there's a method for this already that you can use
>   DCHECK(SomeMethodThatVerifiesWeAreOnTheRightThread());
>   return DoDispatchMessage(message);
> }
> 
> bool WebContentsObserver::DoDispatchMessage(const IPC::Message& message) {
>   // the current implementation of OnMessageReceived goes here
> }
> 
> Then in your tests you can dispatch messages directly to DoDispatchMessage.
This
> allows you to avoid having to fake out that this code is running on the main
> thread in tests, which isn't going to validate anything (since it's stubbed
out
> using e.g. TestBrowserThreadBundle) and makes writing tests harder.
> 
> That said, I'm happy to write tests that verify use of right threads if the
> tests we right are dispatching IPCs using the real IPC mechanism. I personally
> don't think we'd need to do that though.

actually, if you confirm that this does get invoked on the right thread, then
I'm ok just removing the dcheck once we know that, and not adding the extra
method. just want some verification that this doesn't get called on the IO
thread, and if it does, that we add code to re-route dispatch of these messages
to the browser main thread.

[Bryan McQuade, 2015-09-02 18:40:27]

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:103:
const GURL& WebContentsObserver::GetLastCommittedURL() {
On 2015/09/02 18:38:24, csharrison wrote:
> On 2015/09/02 18:26:45, Bryan McQuade wrote:
> > unless you need to override the implementation of this function in a test
> > (which, despite my having done it, i agree with randy is generally
> undesirable)
> > i think it's probably reasonable to just inline the implementation directly
> and
> > remove the function.
> 
> The reason is so we don't have to mock an entire WebContents to unittest this
> class. We can just subclass it and alter this method. If there's a better way
> than completely mocking WebContents, which would be a pretty big undertaking,
> I'd definitely do it.

Ok, that seems like a good reason - in that case you'll just want to change the
method to protected in the header file, to indicate that it is intended to be
overridden by derived implementations. You might add a '// Overridden in
unittests' comment to go along with it.

[Bryan McQuade, 2015-09-02 18:47:08]

a few more test comments

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:22:
const GURL& GetLastCommittedURL() override { return url_; }
you might consider using gmock for this.

gmock allows you to wire up return values to methods via testing::ReturnRef() :
https://code.google.com/p/googlemock/wiki/CookBook#Returning_References_from_...

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:35:
const GURL& GetURL() const override { return url_; }
these also seem like good candidates for gmock.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:36:
bool IsInMainFrame() const override { return is_main_frame_; }
can use testing::Return() to set a gmock expectation for this method.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:37:
bool HasCommittedDocument() const override { return has_committed_document_; }
same

[Bryan McQuade, 2015-09-02 18:57:09]

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:65:
void set_timing(const PageLoadTiming& timing) { timing_ = timing; }
i realize i wrote this, but reviewing the code now, i'm wondering why I used a
setter rather than gmock. seems like this is well covered by gmock. do you see a
reason that gmock would be undesirable here?

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:67:
void set_mock_timer(scoped_ptr<base::Timer> timer) {
same - seems this should be very gmockable, but maybe there was a good reason i
didn't do that. not sure.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:134:

i think it's worth also writing a test for the logic in the destructor. you
won't be able to use metrics_sender_ as you need an object that you can force to
go out of scope in order to test destructor behavior. two possibilities:

1. create a scope within the text body to force the destructor to run at a
certain time:

TEST_F() {
  {
    // This test wants to verify behavior in the PageTimingMetricsSender
    // destructor, so we create our own instance so we can make it go out
    // of scope before the end of the test body.
    TestPageTimingMetricsSender sender(&mock_ipc_sender_);
    // 1. configure the scoped_sender to have a pending timer but don't fire it
    // 2. set a mock expectation that the mock_ipc_sender_ receives the expected
IPC
  }
}
  
2. similar to above, but use a scoped_ptr<TestPageTimingMetricsSender> so you
can control its lifetime (and run the destructor) by calling reset() on it.

[Bryan McQuade, 2015-09-03 12:04:29]

One more comment.

Also, for the places where I suggested trying to use gmock in tests, please see
that as a suggestion rather than a requirement. If the code is simpler without
gmock, I'm supportive of keeping it as is.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:103:
const GURL& WebContentsObserver::GetLastCommittedURL() {
On 2015/09/02 18:40:27, Bryan McQuade wrote:
> On 2015/09/02 18:38:24, csharrison wrote:
> > On 2015/09/02 18:26:45, Bryan McQuade wrote:
> > > unless you need to override the implementation of this function in a test
> > > (which, despite my having done it, i agree with randy is generally
> > undesirable)
> > > i think it's probably reasonable to just inline the implementation
directly
> > and
> > > remove the function.
> > 
> > The reason is so we don't have to mock an entire WebContents to unittest
this
> > class. We can just subclass it and alter this method. If there's a better
way
> > than completely mocking WebContents, which would be a pretty big
undertaking,
> > I'd definitely do it.
> 
> Ok, that seems like a good reason - in that case you'll just want to change
the
> method to protected in the header file, to indicate that it is intended to be
> overridden by derived implementations. You might add a '// Overridden in
> unittests' comment to go along with it.

Actually, it's been a while since I used private virtual, but on reviewing that,
I think what you've done here is correct, and it can remain private virtual
(rather than protected). As you probably know, this allows derived classes to
override the behavior of the base class implementation, without being able to
call the base class implementation, which is I think what you want in this case.

[Charlie Harrison, 2015-09-03 14:00:53]

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:28:
RecordTimingHistograms();
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> can we add a short comment here noting why it's correct to record timing
> histograms as part of being destroyed? is this the codepath that gets invoked
> when a tab is closed?

This gets invoked when a tab is closed, and when we close Chrome completely. In
the former case RenderProcessGone works fine. This is a last resort.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
bool handled = true;
On 2015/09/02 18:38:31, Bryan McQuade wrote:
> On 2015/09/02 18:26:45, Bryan McQuade wrote:
> > just to sanity check, can we add a DCHECK that this method is invoked on the
> > main browser thread? There's a part of me that worries this may get
dispatched
> > on the IO thread, and if that's the case, we need to add some code to
re-route
> > the dispatch of these methods to the main browser thread (since the other
WCO
> > callbacks are invoked there).
> > 
> > Note that, to ease testing slightly, I think it's very reasonable to do
> > something like:
> > 
> > bool WebContentsObserver::OnMessageReceived(
> >     const IPC::Message& message,
> >     content::RenderFrameHost* render_frame_host) {
> >   // I'm sure there's a method for this already that you can use
> >   DCHECK(SomeMethodThatVerifiesWeAreOnTheRightThread());
> >   return DoDispatchMessage(message);
> > }
> > 
> > bool WebContentsObserver::DoDispatchMessage(const IPC::Message& message) {
> >   // the current implementation of OnMessageReceived goes here
> > }
> > 
> > Then in your tests you can dispatch messages directly to DoDispatchMessage.
> This
> > allows you to avoid having to fake out that this code is running on the main
> > thread in tests, which isn't going to validate anything (since it's stubbed
> out
> > using e.g. TestBrowserThreadBundle) and makes writing tests harder.
> > 
> > That said, I'm happy to write tests that verify use of right threads if the
> > tests we right are dispatching IPCs using the real IPC mechanism. I
personally
> > don't think we'd need to do that though.
> 
> actually, if you confirm that this does get invoked on the right thread, then
> I'm ok just removing the dcheck once we know that, and not adding the extra
> method. just want some verification that this doesn't get called on the IO
> thread, and if it does, that we add code to re-route dispatch of these
messages
> to the browser main thread.

Let's add a DCHECK. Do you know an easy way to see what thread we're in?

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:52:
if (!url.SchemeIsHTTPOrHTTPS() && !browser_url.SchemeIsHTTPOrHTTPS())
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> does this want to be || rather than &&?
> 
> in particular, for the chrome://newtab case, where navigation handle returns
an
> https URL, but GetLastCommittedUrl returns chrome://newtab, seems we should
bail
> early if either one is not http/https.
> 
> let's also add a comment to explain what's going on here, and probably file a
> bug noting the discrepancy in what the methods return, and that our code
> currently depends on it, but longer term they should probably return the same
> thing (and we should make sure that any changes in behavior do not break our
> code).

Done. Will draft a bug soon.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:69:
if (!GetLastCommittedURL().SchemeIsHTTPOrHTTPS())
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> let's add a short comment here to note the specific reason why this is needed
> (ntp).

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:74:
} else if (timing.IsChild(pending_timing_)) {
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> I'm not following what's going on with these if/elseif/else's - maybe add some
> comments to explain why we want to know if timing is a child of
current_timing_,
> and if it's a child of pending_timing_, etc.

I think I'm going to remove the pending_timing following our discussion

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:82:
PageLoadTiming timing = current_timing_;
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> I don't think you need to take a function-scoped copy of current_timing_ here
-
> all the code in this class should run on the same thread, and we don't change
> current_timing_ anywhere in the scope of the function.
> 
> Alternatively, if you want to pass a timing instance as a parameter to this
> function, I think that's fine, but I'd pass it as a 'const PageLoadTiming&'
> since there's no need to copy it.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:85:
else if (timing.IsEmpty())
On 2015/09/02 18:26:45, Bryan McQuade wrote:
> nit: I'd do the test for timing.IsEmpty() before the test for
> timing.IsComplete() - I generally like to see the bail-out conditions for
> functions as early as possible.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:41:

On 2015/09/02 18:26:45, Bryan McQuade wrote:
> randy had suggested keeping all of the overridden methods together (removing
> newlines between them), so let's remove this one.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:52:
virtual const GURL& GetLastCommittedURL();
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> a private method doesn't benefit from being virtual, since by being private it
> cannot be overridden in base classes. so i'd suggest making this non-virtual.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:54:
PageLoadTiming current_timing_;
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> is it possible for us to get rid of having two members, and just have a single
> 'timing_' member?
> 
> if there's some need to keep temporary timing state around during certain
> methods, perhaps that can be done using a variable local to a function, rather
> than a second member.
> 
> i did not look closely, so if this is really needed (if the local variable
> solution doesn't work) then we can keep it. but i do think it makes the code a
> bit easier to reason about if there isn't both a current and pending timing.
If
> we do need to keep both, then please add a comment here to explain how they
> differ (in what cases will pending be different from current etc).

Pending timing is the timing information sent from a non-committed navigation.
It isn't fully necessary to keep track of this before commit, so I think it can
be safely removed. We will always get another timing notification from the
renderer post commit.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:22:
const GURL& GetLastCommittedURL() override { return url_; }
On 2015/09/02 18:47:08, Bryan McQuade wrote:
> you might consider using gmock for this.
> 
> gmock allows you to wire up return values to methods via testing::ReturnRef()
:
>
https://code.google.com/p/googlemock/wiki/CookBook#Returning_References_from_...

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:35:
const GURL& GetURL() const override { return url_; }
On 2015/09/02 18:47:08, Bryan McQuade wrote:
> these also seem like good candidates for gmock.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:36:
bool IsInMainFrame() const override { return is_main_frame_; }
On 2015/09/02 18:47:08, Bryan McQuade wrote:
> can use testing::Return() to set a gmock expectation for this method.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:37:
bool HasCommittedDocument() const override { return has_committed_document_; }
On 2015/09/02 18:47:08, Bryan McQuade wrote:
> same

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.cc:11:
PageLoadTiming::PageLoadTiming(blink::WebPerformance& p) {
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> in this case I think a single-character variable name is probably reasonably
> readable, but I rarely see single-char variables other than 'i' for increment
> variables. Can we call this something slightly longer, maybe 'perf'? I know
that
> doesn't make much of a difference - if for no other reason than for convention
> that variable names are generally not just one char.

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:22:
PageLoadTiming(blink::WebPerformance&);
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> can you make the function parameter const?

Done.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:25: bool operator==(const
PageLoadTiming& other) const;
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> I think randy had suggested trying to use the default implementation here
(c++11
> feature) - can you try changing this to:
> 
> bool operator==(const PageLoadTiming& other) const = default;
> 
> and removing the implementation in the cc file, and seeing if that works as
> intended? I've never used 'default' for equality, so I'm not sure if it's
> actually supported (or exactly how it works) but I think it's worth trying and
> learning more about.

According to this: http://en.cppreference.com/w/cpp/language/member_functions,
equality is not a special member function and cannot be defaulted.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/common/page_load_timing.h:29: bool IsChild(const
PageLoadTiming& parent) const;
On 2015/09/02 18:26:46, Bryan McQuade wrote:
> Though these methods logically make sense as members, the recommendation in
the
> style guide is that structs should only contain data, and not functions. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C....
> 
> Normally I might suggest that we should just change this to a class, but given
> that it is sent over IPC, and in general IPC wants to deal with just moving
data
> around, I think there is value in keeping it a struct.
> 
> If the logic in these methods is only needed in the web contents observer, it
> may be worth adding them as helper methods that take a 'const PageLoadTiming&'
> as an argument, in an anonymous namespace in the cc file.
> 
> I think it's worth asking Randy or someone else about this before making a
> change though, since it's not 100% clear cut and I'd like to get a second
> opinion.

Randy said it's reasonable to move them, but he isn't sure of the ins and outs
of IPC code. I'll make the change.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:65:
void set_timing(const PageLoadTiming& timing) { timing_ = timing; }
On 2015/09/02 18:57:09, Bryan McQuade wrote:
> i realize i wrote this, but reviewing the code now, i'm wondering why I used a
> setter rather than gmock. seems like this is well covered by gmock. do you see
a
> reason that gmock would be undesirable here?

Will do.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:67:
void set_mock_timer(scoped_ptr<base::Timer> timer) {
On 2015/09/02 18:57:09, Bryan McQuade wrote:
> same - seems this should be very gmockable, but maybe there was a good reason
i
> didn't do that. not sure.

Gmock expects everything to be copyable, and scoped_ptrs aren't. So I'm going to
leave this one as is.

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:134:

On 2015/09/02 18:57:09, Bryan McQuade wrote:
> i think it's worth also writing a test for the logic in the destructor. you
> won't be able to use metrics_sender_ as you need an object that you can force
to
> go out of scope in order to test destructor behavior. two possibilities:
> 
> 1. create a scope within the text body to force the destructor to run at a
> certain time:
> 
> TEST_F() {
>   {
>     // This test wants to verify behavior in the PageTimingMetricsSender
>     // destructor, so we create our own instance so we can make it go out
>     // of scope before the end of the test body.
>     TestPageTimingMetricsSender sender(&mock_ipc_sender_);
>     // 1. configure the scoped_sender to have a pending timer but don't fire
it
>     // 2. set a mock expectation that the mock_ipc_sender_ receives the
expected
> IPC
>   }
> }
>   
> 2. similar to above, but use a scoped_ptr<TestPageTimingMetricsSender> so you
> can control its lifetime (and run the destructor) by calling reset() on it.

Acknowledged.

[Bryan McQuade, 2015-09-03 20:23:37]

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:34:
bool handled = true;
On 2015/09/03 14:00:52, csharrison wrote:
> On 2015/09/02 18:38:31, Bryan McQuade wrote:
> > On 2015/09/02 18:26:45, Bryan McQuade wrote:
> > > just to sanity check, can we add a DCHECK that this method is invoked on
the
> > > main browser thread? There's a part of me that worries this may get
> dispatched
> > > on the IO thread, and if that's the case, we need to add some code to
> re-route
> > > the dispatch of these methods to the main browser thread (since the other
> WCO
> > > callbacks are invoked there).
> > > 
> > > Note that, to ease testing slightly, I think it's very reasonable to do
> > > something like:
> > > 
> > > bool WebContentsObserver::OnMessageReceived(
> > >     const IPC::Message& message,
> > >     content::RenderFrameHost* render_frame_host) {
> > >   // I'm sure there's a method for this already that you can use
> > >   DCHECK(SomeMethodThatVerifiesWeAreOnTheRightThread());
> > >   return DoDispatchMessage(message);
> > > }
> > > 
> > > bool WebContentsObserver::DoDispatchMessage(const IPC::Message& message) {
> > >   // the current implementation of OnMessageReceived goes here
> > > }
> > > 
> > > Then in your tests you can dispatch messages directly to
DoDispatchMessage.
> > This
> > > allows you to avoid having to fake out that this code is running on the
main
> > > thread in tests, which isn't going to validate anything (since it's
stubbed
> > out
> > > using e.g. TestBrowserThreadBundle) and makes writing tests harder.
> > > 
> > > That said, I'm happy to write tests that verify use of right threads if
the
> > > tests we right are dispatching IPCs using the real IPC mechanism. I
> personally
> > > don't think we'd need to do that though.
> > 
> > actually, if you confirm that this does get invoked on the right thread,
then
> > I'm ok just removing the dcheck once we know that, and not adding the extra
> > method. just want some verification that this doesn't get called on the IO
> > thread, and if it does, that we add code to re-route dispatch of these
> messages
> > to the browser main thread.
> 
> Let's add a DCHECK. Do you know an easy way to see what thread we're in?

I think you can do:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Bryan McQuade, 2015-09-04 12:29:26]

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:41:
bool PageLoadTimingIsEmpty(const PageLoadTiming& timing) {
this one method actually seems reasonable to keep on the struct, as an
is_empty() method. sorry - i should have noted that in the last change.

The other methods do feel like they have a bit more policy specific to our use
of them in WCo, and belong here.

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:81:
RecordTimingHistograms();
I think we probably want to commit the histograms for the previous page even if
the page being navigated to is not a relevant navigation. for example imagine
you navigate from http://www.google.com/ to the new tab page (not sure if it's
possible, but let's say we did). in that case, IsRelevantNavigation() would
return false, but we still want to RecordTimingHistograms for the page being
navigated away from.

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:97:
RecordTimingHistograms();
you may also want to do current_timing_ = PageLoadTiming(); after this, or do it
at the end of the RecordTimingHistograms impl so all calls get a 'reset' of the
timing as part of recording them.

[Charlie Harrison, 2015-09-04 15:19:40]

csharrison@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[Charlie Harrison, 2015-09-04 15:19:40]

Camille: Could you double check the NavigationHandle change? I was unsure if
that's the only place required to set is_same_page.

Bryan: I changed current_timing_ to be a pointer, so we get an extra state
(null) for free. This ended up being a bit hacky, but I preferred it to adding a
private boolean member (did_commit_navigation). Thoughts?

Thanks, everyone :)

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:41:
bool PageLoadTimingIsEmpty(const PageLoadTiming& timing) {
On 2015/09/04 12:29:26, Bryan McQuade wrote:
> this one method actually seems reasonable to keep on the struct, as an
> is_empty() method. sorry - i should have noted that in the last change.
> 
> The other methods do feel like they have a bit more policy specific to our use
> of them in WCo, and belong here.

Done.

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:81:
RecordTimingHistograms();
On 2015/09/04 12:29:26, Bryan McQuade wrote:
> I think we probably want to commit the histograms for the previous page even
if
> the page being navigated to is not a relevant navigation. for example imagine
> you navigate from http://www.google.com/ to the new tab page (not sure if it's
> possible, but let's say we did). in that case, IsRelevantNavigation() would
> return false, but we still want to RecordTimingHistograms for the page being
> navigated away from.

yes! This is a mistake.

https://codereview.chromium.org/1312213010/diff/50001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:97:
RecordTimingHistograms();
On 2015/09/04 12:29:26, Bryan McQuade wrote:
> you may also want to do current_timing_ = PageLoadTiming(); after this, or do
it
> at the end of the RecordTimingHistograms impl so all calls get a 'reset' of
the
> timing as part of recording them.

Done.

[Bryan McQuade, 2015-09-04 20:53:18]

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:57:
RecordTimingHistograms();
let's add a short comment here to note the cases where this happens (is it just
browser shutdown?)

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:69:
RecordTimingHistograms();
we only want to RecordTimingHistograms() if this is a main-frame navigation
that's not in-page. you probably need logic at the beginning of this function
like:

if (!navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage())
  return;

RecordTimingHistograms();

if (IsRelevantNavigation(...))
  current_timing_.reset(new PageLoadTiming());

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:72:
current_navigation_ = navigation_handle;
looks like we're no longer using current_navigation_ - maybe we can remove it?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:77:
RecordTimingHistograms();
let's add a short comment here to note where this happens (render process crash,
and tab close?)

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:80:
bool WebContentsObserver::HandleMessageReceived(
minor suggestion: even though it makes the method ordering different from what's
in the header, i think it'd be better to put the impementation of this method
immediately after OnMessageReceived. That allows readers to just continue
reading w/o having to jump around in the file.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:104:
// See above comment about browser/renderer url disagreement (newtab).
maybe change to "See comment in IsRelevantNavigation about..."

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:104:
// See above comment about browser/renderer url disagreement (newtab).
can we add:
// We may receive notifications from frames that have been navigated away
// from. We simply ignore them.
if (!IsCurrentMainFrame(render_frame_host))
  return false;

you'll need to implement IsCurrentMainFrame as something like:
return render_frame_host == web_contents()->GetMainFrame();
and you may wish to allow overriding it for the purposes of mocking it in tests,
so the unit tests can continue to work with a null web_contents.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:109:
if (PageLoadTimingIsChild(timing, *current_timing_)) {
given that this is the only place we do this 'is child' test, I'm inclined to
just inline the implementation here. it'll be clearer overall to readers - they
can see what it means to be a child right here in the implementation, rather
than having to scroll up to check the implementation.

related: i'd make this another early return path:

if (!current_timing_->navigation_start.is_null() &&
    current_timing_->navigation_start != timing.navigation_start) {
  // UMA log a counter every time this happens.
  return;
}

Also, regarding logging errors in UMA, since there are a few other types of
errors that I think we also want to record, I've seen other similar code that
defines an enum of errors, and then uses the enum value as the bucket when uma
logging. there's an uma macro designed to be used with enum values.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:111:
current_host_ = render_frame_host;
seems like the canonical source for the current render frame host should
probably be web_contents()->GetMainFrame(). Any reason we can't use that (or a
virtual bool IsCurrentMainFrame(...) if you want to override in tests)

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:113:
DCHECK(render_frame_host == current_host_);
IIRC creis said that this can actually happen sometimes, when an old renderer
that has been navigated away from browser side sends IPCs. if that's the case,
then we don't want to DCHECK it.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:114:
current_timing_.reset(new PageLoadTiming(timing));
can you simplify to:
*current_timing_ = timing;

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:121:
else if (PageLoadTimingIsComplete(*current_timing_))
testing validiting of the PageLoadHistograms seems really valuable. instead of
only testing ordering if it's complete, I suggest that we always validate as
much of the PageLoadTiming as we can, even if not complete. Things to validate:

bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
  if (timing.IsEmpty())
    return true;

  // If we have a non-empty timing, it should always have a navigation_start.
  if (timing.navigation_start.is_null())
    return false;

  // If we have a dom content loaded event, we should have a response start.
  if (!timing.dom_content_loaded_event_start.is_zero()) {
    if (timing.response_start.is_zero())
      return false;
  }

  // If we have a load event, we should have both a response start and a DCL.
  if (!timing.load_event_start.is_zero()) {
    if (timing.response_start.is_zero() ||
        timing.dom_content_loaded_event_start.is_zero())
      return false;
  }

  return true;
}

and then at the call site, we should do something like:
if (!IsValidTiming(*current_timing_)) {
  // UMA log that we encountered an invalid timing.
  return;
}

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:52:
friend class MockWebContentsObserver;
does this need to be a friend? looking at the impl, it seems like it shouldn't
be needed.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:61:
bool has_committed_navigation_;
looks like this bool is no longer used. remove?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:62:
content::RenderFrameHost* current_host_;
suggest removing in favor of IsCurrentMainFrame method - see comments in cc file

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:63:
content::NavigationHandle* current_navigation_;
looks like this is also no longer used. remove?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:64:
scoped_ptr<PageLoadTiming> current_timing_;
let's add a short comment to explain when this will be null vs non-null.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:21:
class MockWebContentsObserver : public WebContentsObserver {
this is a case where we should be explicit which WebContentsObserver
impementation we're using, just to ease readability of the code:

class MockWebContentsObserver : public page_load_metrics::WebContentsObserver {

if this doesn't work you can do:

class MockWebContentsObserver : public ::page_load_metrics::WebContentsObserver
{

which should work.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:25:
bool OnMessageReceived(const IPC::Message& message,
I wonder, if we just add a TestBrowserThreadBundle instance as a member of the
WebContentsObserverTest blass below, can we remove this override? Perhaps that
class magically sets up the current thread as the main thread? Can you test that
out? Would be nice if we could avoid overriding this.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:32:
class MockNavigationHandle : public content::NavigationHandle {
this is a really nice mock class to have. one unfortunate drawback is that every
time someone adds a new pure virtual method to NavigationHandle, our code will
have to be updated as well, which is undesirable. I think this class should
actually be moved to content, under content/public/test/, similar to
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes....
That way, every time someone adds a new method to NavigationHandle, they can
also update Mocknavigationhandle to include the additional
MOCK_CONST_METHOD0(...) for those methods, and existing tests will continue to
compile.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:35:
MockNavigationHandle(GURL& url) {
when moved to content, this constructor should be marked as explicit, and 'GURL&
url' should probably be changed to 'GURL url' - since in the general case we
probably cannot guarantee that the passed in url will continue to exist for the
lifetime of the mock, so we should default to making a copy, which is fine since
tests aren't perf critical.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:36:
EXPECT_CALL(*this, GetURL()).WillRepeatedly(
as part of moving this over to content/, I'd make these defaults more neutral:
1. switch to ON_CALL rather than EXPECT_CALL: EXPECT_CALL means you really
expect the method to be called, whereas ON_CALL is used for optional defaults
2. switch from WillRepeatedly to WillByDefault
3. make all boolean methods return false by default (this is the most consistent
and predictable default behavior).
4. add a comment in the MockNavigationHandle header explaining the default
behavior of newly instantiated MockNavigationHandle instances.

then in your code, you can have a
static void SetExpectations(MockNavigationHandle* handle) {
  EXPECT_CALL(*handle, ...)
  to set up any custom expectations that differ from the default expecatations
set up in content.
}

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:67:
TEST_F(WebContentsObserverTest, NoMetrics) {
we may need to use a more specific classname just for the tests, since the
output of the test on the console will show only the class name IIRC and it'd be
good for the class name to uniquely identify the code being tested. take a look
at console output and see.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:18: //
http://www.w3.org/TR/navigation-timing/.
let's add a short comment to note that all TimeDeltas are relative to
navigation_start.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:44: // If you add
additional members, also be sure to update operator== and
should add mention to update IsEmpty as well

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:61:
if (ShouldSendMetrics()) {
can get rid of the braces here

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:108:
if (frame) {
once you fold the line below, can get rid of braces

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
blink::WebPerformance performance = frame->performance();
can get rid of this line and just fold it into the next line

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:117:
new base::OneShotTimer<PageTimingMetricsSender>());
I never fully understood how the template parameter is used here, and whether
PageTimingMetricsSender is the right choice. can you look over the timer code
and see what the template param is used for, and whether PageTimingMetricsSender
is the right thing for us to use?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:37:
virtual bool ShouldSendMetrics() const;
I learned from your use of private virtual. If any of these can be moved to
being private, rather than protected (if they are only overridden in derived
classes, rather than called), I think that's worthwhile.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:42:
explicit MockMainRenderFrameObserver() : MainRenderFrameObserver(nullptr) {
this no longer needs to be explicit (only constructors that take a single arg
need to be explicit)

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:43:
EXPECT_CALL(*this, ShouldSendMetrics()).WillRepeatedly(Return(true));
let's change this to:
ON_CALL(...).WillByDefault(Return(true));

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:100:
// todo
sorry - this todo was here to add expectations on the ipc_interceptor - I think
we should be setting an expectation that the interceptor receives the timing
instance. we also need to call mock_timer->Fire() somewhere to actually trigger
the timer.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:121:
// todo
similar to above

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:132:
// todo
similar to above

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:150:
TEST_F(PageTimingMetricsSenderTest, SendIPCOnExplicitDestructor) {
You don't need both of these (this test, and the one above) - you can pick one
or the other - whichever you think is cleanest.

https://codereview.chromium.org/1312213010/diff/90001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/1312213010/diff/90001/ipc/ipc_message_start.h...
ipc/ipc_message_start.h:127: IPCTestMsgStart,
this diff looks off. IPCTestMsgStart should probably also be showing up in the
left hand side.

[clamy, 2015-09-08 13:25:59]

Thanks! Please find below a few comments on the NavigationHandle part.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:63:
content::NavigationHandle* current_navigation_;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> looks like this is also no longer used. remove?

In particular, the NavigationHandle will likely be deleted before this
WebContentsObserver. If you want to keep track of the NavigationHandle (but I
don't see why you should), you should implement the DidStopNavigation call,
where you will reset current_navigation_;

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:50: bool
NavigationHandleImpl::IsSamePage() const {
Add:
CHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
    << "This accessor should not be called before the navigation has committed."
This will reduce the risk of embedders getting wrong data.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:69: void
set_is_same_page(bool is_same_page) {
Instead of a setter, I would rather have set_is_same_page passed as a paremeter
to DidCommitNavigation and updated there.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:112: // Whether the
navigation resulted in a same-page navigation
I would replace with "Whether the navigation happened in the same page."

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:413:
render_frame_host->navigation_handle()->set_is_same_page(
As mentioned in the NavigationHandleImpl, instead of a setter provide
is_navigation_within_page to the
render_frame_host->navigation_handle()->DidCommitNavigation(); call below (line
490).

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

https://codereview.chromium.org/1312213010/diff/90001/content/public/browser/...
content/public/browser/navigation_handle.h:31: // Whether the navigation is a
same-page navigation
"Whether the navigation happened in the same page. This is only known after the
navigation has committed."

Also note that this may collide with my patch:
https://codereview.chromium.org/1269813002/

[Bryan McQuade, 2015-09-08 14:09:46]

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:121:
else if (PageLoadTimingIsComplete(*current_timing_))
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> testing validiting of the PageLoadHistograms seems really valuable. instead of
> only testing ordering if it's complete, I suggest that we always validate as
> much of the PageLoadTiming as we can, even if not complete. Things to
validate:
> 
> bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
>   if (timing.IsEmpty())
>     return true;
> 
>   // If we have a non-empty timing, it should always have a navigation_start.
>   if (timing.navigation_start.is_null())
>     return false;
> 
>   // If we have a dom content loaded event, we should have a response start.
>   if (!timing.dom_content_loaded_event_start.is_zero()) {
>     if (timing.response_start.is_zero())
>       return false;
>   }
> 
>   // If we have a load event, we should have both a response start and a DCL.
>   if (!timing.load_event_start.is_zero()) {
>     if (timing.response_start.is_zero() ||
>         timing.dom_content_loaded_event_start.is_zero())
>       return false;
>   }
> 
>   return true;
> }
> 
> and then at the call site, we should do something like:
> if (!IsValidTiming(*current_timing_)) {
>   // UMA log that we encountered an invalid timing.
>   return;
> }

oh, and I forgot, we should also test ordering between the different entries
that are expected to be ordered relative to each other, as you had done in
PageLoadTimingIsOrdered.

[Charlie Harrison, 2015-09-08 23:05:16]

Have a bunch of edits ready, just waiting on creis to email me back wrt RFO.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:57:
RecordTimingHistograms();
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> let's add a short comment here to note the cases where this happens (is it
just
> browser shutdown?)

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:69:
RecordTimingHistograms();
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> we only want to RecordTimingHistograms() if this is a main-frame navigation
> that's not in-page. you probably need logic at the beginning of this function
> like:
> 
> if (!navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage())
>   return;
> 
> RecordTimingHistograms();
> 
> if (IsRelevantNavigation(...))
>   current_timing_.reset(new PageLoadTiming());

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:72:
current_navigation_ = navigation_handle;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> looks like we're no longer using current_navigation_ - maybe we can remove it?

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:77:
RecordTimingHistograms();
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> let's add a short comment here to note where this happens (render process
crash,
> and tab close?)

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:80:
bool WebContentsObserver::HandleMessageReceived(
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> minor suggestion: even though it makes the method ordering different from
what's
> in the header, i think it'd be better to put the impementation of this method
> immediately after OnMessageReceived. That allows readers to just continue
> reading w/o having to jump around in the file.

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:111:
current_host_ = render_frame_host;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> seems like the canonical source for the current render frame host should
> probably be web_contents()->GetMainFrame(). Any reason we can't use that (or a
> virtual bool IsCurrentMainFrame(...) if you want to override in tests)

Which side of the equality would you propose changing to GetMainFrame()? I was
keeping track of current_host_ to sanity check that our hosts were not changing 
without our knowing it. It seems reasonable to compare that with the host we are
receiving messages from.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:113:
DCHECK(render_frame_host == current_host_);
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> IIRC creis said that this can actually happen sometimes, when an old renderer
> that has been navigated away from browser side sends IPCs. if that's the case,
> then we don't want to DCHECK it.

Shouldn't that be handled by the !IsCurrentMainFrame check?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:114:
current_timing_.reset(new PageLoadTiming(timing));
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> can you simplify to:
> *current_timing_ = timing;

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:121:
else if (PageLoadTimingIsComplete(*current_timing_))
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> testing validiting of the PageLoadHistograms seems really valuable. instead of
> only testing ordering if it's complete, I suggest that we always validate as
> much of the PageLoadTiming as we can, even if not complete. Things to
validate:
> 
> bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
>   if (timing.IsEmpty())
>     return true;
> 
>   // If we have a non-empty timing, it should always have a navigation_start.
>   if (timing.navigation_start.is_null())
>     return false;
> 
>   // If we have a dom content loaded event, we should have a response start.
>   if (!timing.dom_content_loaded_event_start.is_zero()) {
>     if (timing.response_start.is_zero())
>       return false;
>   }
> 
>   // If we have a load event, we should have both a response start and a DCL.
>   if (!timing.load_event_start.is_zero()) {
>     if (timing.response_start.is_zero() ||
>         timing.dom_content_loaded_event_start.is_zero())
>       return false;
>   }
> 
>   return true;
> }
> 
> and then at the call site, we should do something like:
> if (!IsValidTiming(*current_timing_)) {
>   // UMA log that we encountered an invalid timing.
>   return;
> }

Is a PageLoadTiming with null nav_start really valid?

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:52:
friend class MockWebContentsObserver;
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> does this need to be a friend? looking at the impl, it seems like it shouldn't
> be needed.

Either it's a friend or the explicit constructor must be made protected.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:61:
bool has_committed_navigation_;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> looks like this bool is no longer used. remove?

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:63:
content::NavigationHandle* current_navigation_;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> looks like this is also no longer used. remove?

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:64:
scoped_ptr<PageLoadTiming> current_timing_;
On 2015/09/04 20:53:17, Bryan McQuade wrote:
> let's add a short comment to explain when this will be null vs non-null.

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:25:
bool OnMessageReceived(const IPC::Message& message,
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> I wonder, if we just add a TestBrowserThreadBundle instance as a member of the
> WebContentsObserverTest blass below, can we remove this override? Perhaps that
> class magically sets up the current thread as the main thread? Can you test
that
> out? Would be nice if we could avoid overriding this.

Yeah this works.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:32:
class MockNavigationHandle : public content::NavigationHandle {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> this is a really nice mock class to have. one unfortunate drawback is that
every
> time someone adds a new pure virtual method to NavigationHandle, our code will
> have to be updated as well, which is undesirable. I think this class should
> actually be moved to content, under content/public/test/, similar to
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes....
> That way, every time someone adds a new method to NavigationHandle, they can
> also update Mocknavigationhandle to include the additional
> MOCK_CONST_METHOD0(...) for those methods, and existing tests will continue to
> compile.

Yep, this is the right way to go.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:18: //
http://www.w3.org/TR/navigation-timing/.
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> let's add a short comment to note that all TimeDeltas are relative to
> navigation_start.

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:44: // If you add
additional members, also be sure to update operator== and
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> should add mention to update IsEmpty as well

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:61:
if (ShouldSendMetrics()) {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> can get rid of the braces here

Is this in the style guide? I think braces are good because we have a 2 line
block.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:108:
if (frame) {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> once you fold the line below, can get rid of braces

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:109:
blink::WebPerformance performance = frame->performance();
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> can get rid of this line and just fold it into the next line

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:117:
new base::OneShotTimer<PageTimingMetricsSender>());
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> I never fully understood how the template parameter is used here, and whether
> PageTimingMetricsSender is the right choice. can you look over the timer code
> and see what the template param is used for, and whether
PageTimingMetricsSender
> is the right thing for us to use?

This is the right way to do it, we need the class to properly call the method
Send() on the object. It's just an implementation detail.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h:37:
virtual bool ShouldSendMetrics() const;
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> I learned from your use of private virtual. If any of these can be moved to
> being private, rather than protected (if they are only overridden in derived
> classes, rather than called), I think that's worthwhile.

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:42:
explicit MockMainRenderFrameObserver() : MainRenderFrameObserver(nullptr) {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> this no longer needs to be explicit (only constructors that take a single arg
> need to be explicit)

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:43:
EXPECT_CALL(*this, ShouldSendMetrics()).WillRepeatedly(Return(true));
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> let's change this to:
> ON_CALL(...).WillByDefault(Return(true));

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:100:
// todo
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> sorry - this todo was here to add expectations on the ipc_interceptor - I
think
> we should be setting an expectation that the interceptor receives the timing
> instance. we also need to call mock_timer->Fire() somewhere to actually
trigger
> the timer.

Oops also an oversight on my part. Fixing all these tests

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:150:
TEST_F(PageTimingMetricsSenderTest, SendIPCOnExplicitDestructor) {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> You don't need both of these (this test, and the one above) - you can pick one
> or the other - whichever you think is cleanest.

Done.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:50: bool
NavigationHandleImpl::IsSamePage() const {
On 2015/09/08 13:25:58, clamy wrote:
> Add:
> CHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
>     << "This accessor should not be called before the navigation has
committed."
> This will reduce the risk of embedders getting wrong data.

Done.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:69: void
set_is_same_page(bool is_same_page) {
On 2015/09/08 13:25:58, clamy wrote:
> Instead of a setter, I would rather have set_is_same_page passed as a
paremeter
> to DidCommitNavigation and updated there.

Done.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:112: // Whether the
navigation resulted in a same-page navigation
On 2015/09/08 13:25:58, clamy wrote:
> I would replace with "Whether the navigation happened in the same page."

Done.

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1312213010/diff/90001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:413:
render_frame_host->navigation_handle()->set_is_same_page(
On 2015/09/08 13:25:59, clamy wrote:
> As mentioned in the NavigationHandleImpl, instead of a setter provide
> is_navigation_within_page to the
> render_frame_host->navigation_handle()->DidCommitNavigation(); call below
(line
> 490).

Done.

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

https://codereview.chromium.org/1312213010/diff/90001/content/public/browser/...
content/public/browser/navigation_handle.h:31: // Whether the navigation is a
same-page navigation
On 2015/09/08 13:25:59, clamy wrote:
> "Whether the navigation happened in the same page. This is only known after
the
> navigation has committed."
> 
> Also note that this may collide with my patch:
> https://codereview.chromium.org/1269813002/

Done.

https://codereview.chromium.org/1312213010/diff/90001/ipc/ipc_message_start.h
File ipc/ipc_message_start.h (right):

https://codereview.chromium.org/1312213010/diff/90001/ipc/ipc_message_start.h...
ipc/ipc_message_start.h:127: IPCTestMsgStart,
looks like that never ended up getting committed. Removing it.

[Bryan McQuade, 2015-09-09 16:26:26]

looks really good. haven't done a thorough look at tests yet but i think we're
basically good after this round.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:61:
if (ShouldSendMetrics()) {
On 2015/09/08 23:05:15, csharrison wrote:
> On 2015/09/04 20:53:18, Bryan McQuade wrote:
> > can get rid of the braces here
> 
> Is this in the style guide? I think braces are good because we have a 2 line
> block.

Ah, you're right - I had assumed any single-statement if block could omit, but
it sounds like style guide allows omitting when the statements are short. So
let's keep it as you have it. Style guide for this is here:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:39:
!(timing.response_start < timing.dom_content_loaded_event_start)) {
technically we could have response_start and dom_content_loaded_event_start
happen in order but within the same millisecond, so we should probably change
this from < to <=. or you could flip it around to '... ||
timing.dom_content_loaded_event_start < timing.response_start)'

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:48:
!(timing.dom_content_loaded_event_start < timing.load_event_start)) {
same

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:132:
else
reading the style guide, i see that when there's an else clause, we do use
braces:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals -
sorry - in my own code I had just always used braces for complete clarity so I
didn't know the ins and outs well, but reviewers in chromium have requested
removing them where it's allowed in the style guide, so I'm trying to be
consistent with that.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:44:
bool HandleMessageReceived(const IPC::Message& message,
given the discovery around the test thread bundle, seems this can be made
private (or even folded back into OnMessageReceived - up to you)

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:57:
// Used for testing, so we don't have to mock a whole WebContents
suggest adding a newline before this comment

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:42:
url_ = GURL("https://google.com");
generally preferred to initialize members in the initializer list if possible:

const char kDefaultTestUrl[] = "https://google.com";

...

MockMetricsWebContentsObserver() 
    : MetricsWebContentsObserver(nullptr),
      url_(kDefaultTestUrl) {
...
}

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:43:
EXPECT_CALL(*this, GetLastCommittedURL()).WillRepeatedly(ReturnRef(url_));
should these be ON_CALL since they are default behaviors?

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.h:22: PageLoadTiming(const
blink::WebPerformance&);
should this be marked explicit?
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Explicit_Con...

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:41:
void MainRenderFrameObserver::DidFailProvisionalLoad(
just for my own knowledge, what are the cases where this gets called? do we
continue to need it once we have the DidChangePerformanceTiming callback? If
not, can you add:
// TODO(bmcquade): remove this callback once we switch to
DidChangePerformanceTiming.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:57:
// We only create a PageTimingMetricsSender if the page meets the criteria for
now that we don't hook into DidStartProvisionalLoad to reset the sender, we
should probably add:

// Make sure to release the sender for the previous navigation, if we have one.
page_timing_metrics_sender_.reset();

or you could do this below as part of an if/else block.

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
File content/public/test/mock_navigation_handle.cc (right):

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.cc:10: url_ = url;
nit: can be set up as part of an initializer list.

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.cc:19: testing::Return(true));
I think this and all other methods should return false by default - users of the
class should override as it makes sense for them to do so.

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
File content/public/test/mock_navigation_handle.h (right):

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.h:17: MockNavigationHandle(GURL url);
should be explicit

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.h:18: ~MockNavigationHandle();
should be marked 'override' (i think?)

[Bryan McQuade, 2015-09-09 16:52:25]

one other general change: we should rename cc files to match the class name, so
metrics_web_contents_observer.h/.cc/_unittest.cc

same for the render frame observer. for the RFO, do we want to call it
MetricsMainRenderFrameObserver, to give it a bit more specificity?
MainRenderFrameObserver could also be used to describe many other classes in
chromium. I'd also be fine with MetricsRenderFrameObserver if you want to drop
the 'Main' part.

[Charlie Harrison, 2015-09-09 17:32:03]

I chose MetricsRenderFrameObserver for symmetry.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:35:
MockNavigationHandle(GURL& url) {
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> when moved to content, this constructor should be marked as explicit, and
'GURL&
> url' should probably be changed to 'GURL url' - since in the general case we
> probably cannot guarantee that the passed in url will continue to exist for
the
> lifetime of the mock, so we should default to making a copy, which is fine
since
> tests aren't perf critical.

Done.

https://codereview.chromium.org/1312213010/diff/90001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:36:
EXPECT_CALL(*this, GetURL()).WillRepeatedly(
On 2015/09/04 20:53:18, Bryan McQuade wrote:
> as part of moving this over to content/, I'd make these defaults more neutral:
> 1. switch to ON_CALL rather than EXPECT_CALL: EXPECT_CALL means you really
> expect the method to be called, whereas ON_CALL is used for optional defaults
> 2. switch from WillRepeatedly to WillByDefault
> 3. make all boolean methods return false by default (this is the most
consistent
> and predictable default behavior).
> 4. add a comment in the MockNavigationHandle header explaining the default
> behavior of newly instantiated MockNavigationHandle instances.
> 
> then in your code, you can have a
> static void SetExpectations(MockNavigationHandle* handle) {
>   EXPECT_CALL(*handle, ...)
>   to set up any custom expectations that differ from the default expecatations
> set up in content.
> }

Done.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:39:
!(timing.response_start < timing.dom_content_loaded_event_start)) {
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> technically we could have response_start and dom_content_loaded_event_start
> happen in order but within the same millisecond, so we should probably change
> this from < to <=. or you could flip it around to '... ||
> timing.dom_content_loaded_event_start < timing.response_start)'

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:48:
!(timing.dom_content_loaded_event_start < timing.load_event_start)) {
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> same

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.cc:132:
else
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> reading the style guide, i see that when there's an else clause, we do use
> braces:
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals
-
> sorry - in my own code I had just always used braces for complete clarity so I
> didn't know the ins and outs well, but reviewers in chromium have requested
> removing them where it's allowed in the style guide, so I'm trying to be
> consistent with that.

Actually it only says to do this if one of the clauses already requires braces.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:44:
bool HandleMessageReceived(const IPC::Message& message,
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> given the discovery around the test thread bundle, seems this can be made
> private (or even folded back into OnMessageReceived - up to you)

Done, folded it into OnMessageReceived.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer.h:57:
// Used for testing, so we don't have to mock a whole WebContents
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> suggest adding a newline before this comment

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:42:
url_ = GURL("https://google.com");
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> generally preferred to initialize members in the initializer list if possible:
> 
> const char kDefaultTestUrl[] = "https://google.com";
> 
> ...
> 
> MockMetricsWebContentsObserver() 
>     : MetricsWebContentsObserver(nullptr),
>       url_(kDefaultTestUrl) {
> ...
> }

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:43:
EXPECT_CALL(*this, GetLastCommittedURL()).WillRepeatedly(ReturnRef(url_));
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> should these be ON_CALL since they are default behaviors?

yupp, my mistake

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.h:22: PageLoadTiming(const
blink::WebPerformance&);
On 2015/09/09 16:26:26, Bryan McQuade wrote:
> should this be marked explicit?
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Explicit_Con...

Done.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:41:
void MainRenderFrameObserver::DidFailProvisionalLoad(
This gets called when a load is cancelled or errors and is still provisional.
For non provisional loads, RFO needs to add a DidFailLoad method, which right
now only RVO has.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:57:
// We only create a PageTimingMetricsSender if the page meets the criteria for
On 2015/09/09 16:26:26, Bryan McQuade wrote:
> now that we don't hook into DidStartProvisionalLoad to reset the sender, we
> should probably add:
> 
> // Make sure to release the sender for the previous navigation, if we have
one.
> page_timing_metrics_sender_.reset();
> 
> or you could do this below as part of an if/else block.

Done.

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
File content/public/test/mock_navigation_handle.cc (right):

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.cc:10: url_ = url;
On 2015/09/09 16:26:26, Bryan McQuade wrote:
> nit: can be set up as part of an initializer list.

Done.

https://codereview.chromium.org/1312213010/diff/130001/content/public/test/mo...
content/public/test/mock_navigation_handle.cc:19: testing::Return(true));
On 2015/09/09 16:26:26, Bryan McQuade wrote:
> I think this and all other methods should return false by default - users of
the
> class should override as it makes sense for them to do so.

oversight on my part, done.

[Bryan McQuade, 2015-09-09 17:33:28]

comments for tests

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:43:
EXPECT_CALL(*this, GetLastCommittedURL()).WillRepeatedly(ReturnRef(url_));
On 2015/09/09 16:26:25, Bryan McQuade wrote:
> should these be ON_CALL since they are default behaviors?

(and also WillByDefault rather than WillRepeatedly)

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:66:
MockMetricsWebContentsObserver observer_;
the use of ON_CALL instead of EXPECT_CALL above may cause some noisy logging to
the console when you run the tests. If that happens, you can make this a
NiceMock:
NickMock<MockMetricsWebContentsObserver> observer_;
and the noise should go away (you will probably also need to add using
testing::NiceMock; above)

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:70:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
since we end up doing these 3 lines in a few places, can we pull this out into a
function? void AssertNoHistogramsLogged();

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:83:
SetNavigationHandleExpectations(&test_handle);
rather than calling this in each test, I'd add a
void SetUp() override {
  SetNavigationHandleExpectations(&mock_nav_handle_);
}

and a mock_nav_handle_ member in your MetricsWebContentsObserverTest class.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:93:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
AssertNoHistogramsLogged

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:116:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
AssertNoHistogramsLogged

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:134:

let's call AssertNoHistogramsLogged here just to confirm that it really is the
navigation below that triggers the logging.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:25:
void MainRenderFrameObserver::OnStop() {
likewise, what are the cases where this gets called and is it still needed if we
get the DidChangePerformanceTiming callback?

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:58:
return MainRenderFrameObserver::Send(message);
I know I wrote this bit of code, but it feels a little hairy to me. I'd rather
that we just do:
delete message;
and remote the base class chaining. If we want a certain behavior from the base
class, better to just trigger that behavior directly. That way, if e.g. the base
class changes its Send() implementation to do other things in the future, this
test class won't inadvertently start triggering those behaviors.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:151:
}
let's add a test that includes multiple navigations (calls to
DidCommitProvisionalLoad) to verify that we log things correctly when multiple
navs happen.

[Charlie Harrison, 2015-09-09 19:42:35]

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:66:
MockMetricsWebContentsObserver observer_;
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> the use of ON_CALL instead of EXPECT_CALL above may cause some noisy logging
to
> the console when you run the tests. If that happens, you can make this a
> NiceMock:
> NickMock<MockMetricsWebContentsObserver> observer_;
> and the noise should go away (you will probably also need to add using
> testing::NiceMock; above)

Done, I was getting those logs.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:70:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> since we end up doing these 3 lines in a few places, can we pull this out into
a
> function? void AssertNoHistogramsLogged();

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:83:
SetNavigationHandleExpectations(&test_handle);
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> rather than calling this in each test, I'd add a
> void SetUp() override {
>   SetNavigationHandleExpectations(&mock_nav_handle_);
> }
> 
> and a mock_nav_handle_ member in your MetricsWebContentsObserverTest class.

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:93:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> AssertNoHistogramsLogged

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:116:
histogram_tester_.ExpectTotalCount(kHistogramNameDomContent, 0);
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> AssertNoHistogramsLogged

Done.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_web_contents_observer_unittest.cc:134:

On 2015/09/09 17:33:28, Bryan McQuade wrote:
> let's call AssertNoHistogramsLogged here just to confirm that it really is the
> navigation below that triggers the logging.

Done.

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

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.cc:25:
void MainRenderFrameObserver::OnStop() {
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> likewise, what are the cases where this gets called and is it still needed if
we
> get the DidChangePerformanceTiming callback?

From what I can tell this is synonymous with DidFailProvisionalLoad in some
cases, but other cases it gets called when a load is stopped after the
provisional load succeeds.

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
File
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/130001/components/page_load_m...
components/page_load_metrics/renderer/page_load_metrics_render_frame_observer_unittest.cc:58:
return MainRenderFrameObserver::Send(message);
On 2015/09/09 17:33:28, Bryan McQuade wrote:
> I know I wrote this bit of code, but it feels a little hairy to me. I'd rather
> that we just do:
> delete message;
> and remote the base class chaining. If we want a certain behavior from the
base
> class, better to just trigger that behavior directly. That way, if e.g. the
base
> class changes its Send() implementation to do other things in the future, this
> test class won't inadvertently start triggering those behaviors.

Done.

[Bryan McQuade, 2015-09-10 00:45:58]

Ok, this change looks good to me now. A few minor comments remaining. Thanks for
this change!

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:48:
timing.dom_content_loaded_event_start < timing.response_start) {
this probably wants to be:
timing.load_event_start < timing.dom_content_loaded_event_start

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:48:
timing.dom_content_loaded_event_start < timing.response_start) {
for completeness, we could also add:
timing.load_event_start < timing.response_start

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:136:
PAGE_LOAD_HISTOGRAM("PageLoad.Timing.DOMContentLoadedEventFired",
Thinking about UMA names more, I anticipate that we may not always start timings
at navigation start, so it's probably good to be explicit about the start time
in the metric name. let's update these to:
PageLoadTiming.Navigation.To.DOMContentLoadedEventFired
or something similar
(also need to update histograms.xml and the tests)

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:141:
PAGE_LOAD_HISTOGRAM("PageLoad.Timing.LoadEventFired",
same

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:146:
PAGE_LOAD_HISTOGRAM("PageLoad.Timing.FirstLayout",
same

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
File
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:53:
GURL url_;
since you set this in the initializer list, you should be able to mark it as
const:
const GURL url_;

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.cc:20:
PageLoadTiming::PageLoadTiming(const blink::WebPerformance& perf) {
let's insert a newline between the 2 constructor definitions

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

https://codereview.chromium.org/1312213010/diff/190001/content/public/browser...
content/public/browser/navigation_handle.h:32: // after the navigation has
committed.
let's extend this comment with "It is an error to call this method before the
navigation has committed."

[kinuko (google), 2015-09-10 01:30:02]

kinuko@google.com changed reviewers:
+ kinuko@google.com

[kinuko (google), 2015-09-10 01:30:03]

Sending some drive-by comments, mostly nits only

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:127:
DCHECK(render_frame_host == current_host_);
nit: DCHECK_EQ would have better log

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:155: // we
see from the browser side (e.g. chrome://newtab). We wan't to be
nit: wan't -> want ?

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:156: //
sure here that we aren't logging UMA for internal pages
nit: end comments with period

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:36: //
MetricsWebContentsObserver implementation
nit: 'WebContentsObserver implementation' ?

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:54: // Used
for testing, so we don't have to mock a whole WebContents
nit: end comments with period

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:56: virtual
bool IsCurrentMainFrame(content::RenderFrameHost*);
nit: we don't usually omit parameter names in chromium, I think

[clamy, 2015-09-10 12:16:24]

Thanks! I'm happy with the changes in NavigationHandle, however I'm not ok with
the introduction of the MockNavigationHandle (see my comment below).

https://codereview.chromium.org/1312213010/diff/190001/content/public/test/mo...
File content/public/test/mock_navigation_handle.h (right):

https://codereview.chromium.org/1312213010/diff/190001/content/public/test/mo...
content/public/test/mock_navigation_handle.h:15: class MockNavigationHandle :
public NavigationHandle {
Sorry I missed this on my last pass on the code. I'm not ok with adding this
testing class to the content/public API. It was a deliberate choice on our part
not to include such a class initially. We found that the navigation logic is
very often modified, and that unit tests that fake navigation observers call
would go out of sync with what really happens when the code is run.

In order to avoid this, we want people to use the various navigation methods of
WebContentsTester and RenderFrameHost tester, as this ensure your unit tests
keep matching the logic inside content. For an example of what it looks like
with a NavigationHandle, please have a look at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap....

[Charlie Harrison, 2015-09-10 19:22:41]

clamy: Thanks for the pointer about MockNavigationHandle, those tests should be
fixed with the WebContentsTester.

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:127:
DCHECK(render_frame_host == current_host_);
On 2015/09/10 01:30:03, kinuko (google) wrote:
> nit: DCHECK_EQ would have better log

Done.

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:155: // we
see from the browser side (e.g. chrome://newtab). We wan't to be
On 2015/09/10 01:30:03, kinuko (google) wrote:
> nit: wan't -> want ?

Done.

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:156: //
sure here that we aren't logging UMA for internal pages
On 2015/09/10 01:30:03, kinuko (google) wrote:
> nit: end comments with period

Done.

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/170001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:36: //
MetricsWebContentsObserver implementation
On 2015/09/10 01:30:03, kinuko (google) wrote:
> nit: 'WebContentsObserver implementation' ?

Done. Too eager substitute.

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:48:
timing.dom_content_loaded_event_start < timing.response_start) {
On 2015/09/10 00:45:58, Bryan McQuade wrote:
> for completeness, we could also add:
> timing.load_event_start < timing.response_start

Done.

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:48:
timing.dom_content_loaded_event_start < timing.response_start) {
On 2015/09/10 00:45:58, Bryan McQuade wrote:
> this probably wants to be:
> timing.load_event_start < timing.dom_content_loaded_event_start

Done.

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:136:
PAGE_LOAD_HISTOGRAM("PageLoad.Timing.DOMContentLoadedEventFired",
On 2015/09/10 00:45:58, Bryan McQuade wrote:
> Thinking about UMA names more, I anticipate that we may not always start
timings
> at navigation start, so it's probably good to be explicit about the start time
> in the metric name. let's update these to:
> PageLoadTiming.Navigation.To.DOMContentLoadedEventFired
> or something similar
> (also need to update histograms.xml and the tests)

Done.

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/190001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.cc:20:
PageLoadTiming::PageLoadTiming(const blink::WebPerformance& perf) {
On 2015/09/10 00:45:58, Bryan McQuade wrote:
> let's insert a newline between the 2 constructor definitions

Done.

[Bryan McQuade, 2015-09-10 22:32:54]

Looks great with the new DidChangePerformanceTiming API in place. My last round
of comments, then I think we're ready for review from Nasko.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:60: :
content::WebContentsObserver(web_contents) {}
should add:
current_host_(nullptr)
in the initializer list

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:126:
current_host_ = render_frame_host;
We should be resetting current_host_ = nullptr before the RFH gets deleted. As
your code is written today it's fine since you don't use current_host_ for
anything other than equality tests, but it's best to be defensive and make sure
we don't have pointers pointing at invalid memory, in case someone decides to
use current_host_ for something else in the future.

Maybe add an override of the FrameDeleted method:

void MetricsWebContentsObserver::FrameDeleted(RenderFrameHost*
render_frame_host) {
  if (render_frame_host == current_host_)
    current_host_ = nullptr;
}

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:26: struct
PageLoadTiming;
can remove this since we include page_load_timing.h above

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:46: friend
class MetricsWebContentsObserverTest;
now that we don't subclass MetricsWebContentsObserver, I think you can remove
this and call:
page_load_metrics::MetricsWebContentsObserver::CreateForWebContents(...);
in the tests instead.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:47: friend
class MockMetricsWebContentsObserver;
can remove this line now that the class is gone.

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

https://codereview.chromium.org/1312213010/diff/250001/content/public/browser...
content/public/browser/navigation_handle.h:32: // after the navigation has
committed.
let's extend this comment with "It is an error to call this method before the
navigation has committed."

https://codereview.chromium.org/1312213010/diff/250001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1312213010/diff/250001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29398: +<histogram
name="PageLoad.Timing.DOMContentLoadedEventFired"
these need to be updated

[clamy, 2015-09-11 11:52:32]

Thanks! Lgtm with nits for the NavigationHandle part.

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:36: //
WebContentsObserver implementation
nit: . at the end of the comment.

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
File
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:18:
class RenderFrameHost;
You should include the header and not forward declare in a cc file.

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

https://codereview.chromium.org/1312213010/diff/270001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:108: // Whether the
navigation happened in the same page.
nit: you can just group this parameter with the ones above (NavigationHandle
header provides description for those ones).

[Charlie Harrison, 2015-09-11 14:40:51]

csharrison@chromium.org changed reviewers:
+ asvitkine@chromium.org, jochen@chromium.org

[Charlie Harrison, 2015-09-11 14:40:53]

Thanks all! Adding a few more reviewers for completeness.

jochen:
  chrome/
  components/BUILD.gn
  components/OWNERS
  components/components.gyp

asvitkine: tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:60: :
content::WebContentsObserver(web_contents) {}
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> should add:
> current_host_(nullptr)
> in the initializer list

Done.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:126:
current_host_ = render_frame_host;
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> We should be resetting current_host_ = nullptr before the RFH gets deleted. As
> your code is written today it's fine since you don't use current_host_ for
> anything other than equality tests, but it's best to be defensive and make
sure
> we don't have pointers pointing at invalid memory, in case someone decides to
> use current_host_ for something else in the future.
> 
> Maybe add an override of the FrameDeleted method:
> 
> void MetricsWebContentsObserver::FrameDeleted(RenderFrameHost*
> render_frame_host) {
>   if (render_frame_host == current_host_)
>     current_host_ = nullptr;
> }

Done.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:26: struct
PageLoadTiming;
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> can remove this since we include page_load_timing.h above

Done.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:46: friend
class MetricsWebContentsObserverTest;
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> now that we don't subclass MetricsWebContentsObserver, I think you can remove
> this and call:
> page_load_metrics::MetricsWebContentsObserver::CreateForWebContents(...);
> in the tests instead.

Not sure we can do that because we still need a handle on the observer, and that
method returns void. We need the private explicit constructor still.

https://codereview.chromium.org/1312213010/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:47: friend
class MockMetricsWebContentsObserver;
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> can remove this line now that the class is gone.

Done.

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

https://codereview.chromium.org/1312213010/diff/250001/content/public/browser...
content/public/browser/navigation_handle.h:32: // after the navigation has
committed.
On 2015/09/10 22:32:53, Bryan McQuade wrote:
> let's extend this comment with "It is an error to call this method before the
> navigation has committed."

Done.

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:36: //
WebContentsObserver implementation
On 2015/09/11 11:52:32, clamy wrote:
> nit: . at the end of the comment.

Done.

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
File
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/270001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:18:
class RenderFrameHost;
On 2015/09/11 11:52:32, clamy wrote:
> You should include the header and not forward declare in a cc file.

Done.

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

https://codereview.chromium.org/1312213010/diff/270001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:108: // Whether the
navigation happened in the same page.
On 2015/09/11 11:52:32, clamy wrote:
> nit: you can just group this parameter with the ones above (NavigationHandle
> header provides description for those ones).

Done.

[clamy, 2015-09-11 14:44:14]

Thanks! Just one comment since it came up in one of my reviews.

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

https://codereview.chromium.org/1312213010/diff/290001/content/public/browser...
content/public/browser/navigation_handle.h:34: virtual bool IsSamePage() const =
0;
This came up in another review, this method should be non-const. The other
methods were made const erroneously, so let's not have this one const (see
http://www.chromium.org/developers/content-module/content-api:
"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed.")

[Charlie Harrison, 2015-09-11 15:09:08]

On 2015/09/11 14:44:14, clamy wrote:
> Thanks! Just one comment since it came up in one of my reviews.
> 
>
https://codereview.chromium.org/1312213010/diff/290001/content/public/browser...
> File content/public/browser/navigation_handle.h (right):
> 
>
https://codereview.chromium.org/1312213010/diff/290001/content/public/browser...
> content/public/browser/navigation_handle.h:34: virtual bool IsSamePage() const
=
> 0;
> This came up in another review, this method should be non-const. The other
> methods were made const erroneously, so let's not have this one const (see
> http://www.chromium.org/developers/content-module/content-api:
> "don't add the const identifier to interfaces. For interfaces implemented by
the
> embedder, we can't make assumptions about what the embedder needs to implement
> it. For interfaces implemented by content, the implementation details doesn't
> have to be exposed.")

Done! Good catch.

[Bryan McQuade, 2015-09-11 15:22:16]

lgtm

[Alexei Svitkine (slow), 2015-09-11 21:26:48]

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: }  //
namespace
Nit: Add an empty line below.

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:34: //
WebContentsObserver implementation.
Nit: For new code, I think this comment syntax is preferred:

// content::WebContentsObserver:

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.cc:46: }
Nit: Add an empty line after.

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:1:
// Copyright (c) 2015 The Chromium Authors. All rights reserved.
Nit: No (c)

https://codereview.chromium.org/1312213010/diff/330001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1312213010/diff/330001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29398: +<histogram
name="PageLoadTiming.Navigation.To.DOMContentLoadedEventFired"
This seems to be introducing a new top-level prefix. Why not re-use the existing
PLT prefix? e.g. PLT.Timing.*?

[Charlie Harrison, 2015-09-11 22:12:26]

On 2015/09/11 21:26:48, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> File components/page_load_metrics/browser/metrics_web_contents_observer.cc
> (right):
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> components/page_load_metrics/browser/metrics_web_contents_observer.cc:57: } 
//
> namespace
> Nit: Add an empty line below.
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> File components/page_load_metrics/browser/metrics_web_contents_observer.h
> (right):
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> components/page_load_metrics/browser/metrics_web_contents_observer.h:34: //
> WebContentsObserver implementation.
> Nit: For new code, I think this comment syntax is preferred:
> 
> // content::WebContentsObserver:
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> File components/page_load_metrics/common/page_load_timing.cc (right):
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> components/page_load_metrics/common/page_load_timing.cc:46: }
> Nit: Add an empty line after.
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
> File
> components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1312213010/diff/330001/components/page_load_m...
>
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:1:
> // Copyright (c) 2015 The Chromium Authors. All rights reserved.
> Nit: No (c)
> 
>
https://codereview.chromium.org/1312213010/diff/330001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1312213010/diff/330001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:29398: +<histogram
> name="PageLoadTiming.Navigation.To.DOMContentLoadedEventFired"
> This seems to be introducing a new top-level prefix. Why not re-use the
existing
> PLT prefix? e.g. PLT.Timing.*?

Fair suggestion re: naming. Did you have a reason for introducing a new top
level name, Bryan?

[jochen (gone - plz use gerrit), 2015-09-14 09:47:01]

could you please add //components/page_load_metrics/DEPS to disallow including
//components/page_load_metrics with the exception of ../common

and then add DEPS files to browser and renderer that add the required
directories

[jochen (gone - plz use gerrit), 2015-09-14 09:47:43]

i'm deferring the content/ changes to nasko@

[kinuko, 2015-09-14 09:59:12]

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

[nasko, 2015-09-14 23:32:25]

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:32: // If
we have a non-empty timing, it should always have a navigation start.
Maybe DCHECK these conditions if they should never happen and save the
if/return.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:36: // If
we have a dom content loaded event, we should have a response start.
nit: DOM

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:63: // As
a tab helper, this object is tied to a single WebContent
How is this a tab helper? Isn't it WebContentsObserver?
nit: WebContents

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:92: if
(IsRelevantNavigation(navigation_handle)) {
nit: No need for {} for single line if statements.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:121: //
browser/renderer url disagreement (newtab).
This comment doesn't seem related to the following call at all. Is this
intentional?

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:133:
current_host_ = render_frame_host;
Please avoid storing this value. I don't see what value it brings to this code.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:171:
browser_url.SchemeIsHTTPOrHTTPS();
Why do you need to check both the browser url and the handle url? Aren't they
supposed to be the same?

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:27: // IPC
messages received from a MainRenderFrameObserver.
I don't see a MainRenderFrameObserver. Did you mean MetricsRenderFrameObserver?

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:52:
content::RenderFrameHost* current_host_;
Why do you need this? I get very worried whenever someone is keeping track of
RenderFrameHosts.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:63: const
blink::WebLocalFrame& frame = *render_frame()->GetWebFrame();
Why use a reference and not the pointer itself? I haven't really seen this
syntax used in Chromium, so looks inconsistent with the rest of the codebase.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:95: if
(frame)
I don't think this check is needed, as RenderFrame should always have a valid
WebFrame.

[Charlie Harrison, 2015-09-16 22:37:38]

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:32: // If
we have a non-empty timing, it should always have a navigation start.
On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> Maybe DCHECK these conditions if they should never happen and save the
> if/return.

Sure. I'm keeping the outer ifs for clarity though.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:63: // As
a tab helper, this object is tied to a single WebContent
On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> How is this a tab helper? Isn't it WebContentsObserver?
> nit: WebContents

A tab helper is a WebContentsObserver. See
https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:121: //
browser/renderer url disagreement (newtab).
On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> This comment doesn't seem related to the following call at all. Is this
> intentional?

It's related, I updated it for clarity.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:133:
current_host_ = render_frame_host;
On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> Please avoid storing this value. I don't see what value it brings to this
code.

Fair enough. It was just to sanity check that a single navigation will have a
consistent render_frame_host.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:171:
browser_url.SchemeIsHTTPOrHTTPS();
On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> Why do you need to check both the browser url and the handle url? Aren't they
> supposed to be the same?

Yes except for special cases like https://www.google.com/_/chrome/newtab. I
updated the other comment to reflect this.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:63: const
blink::WebLocalFrame& frame = *render_frame()->GetWebFrame();
On 2015/09/14 23:32:25, nasko (slow to review) wrote:
> Why use a reference and not the pointer itself? I haven't really seen this
> syntax used in Chromium, so looks inconsistent with the rest of the codebase.

Done.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:95: if
(frame)
On 2015/09/14 23:32:25, nasko (slow to review) wrote:
> I don't think this check is needed, as RenderFrame should always have a valid
> WebFrame.

Done.

[nasko, 2015-09-17 20:58:43]

LGTM with a few nits.

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:63: // As
a tab helper, this object is tied to a single WebContent
On 2015/09/16 22:37:38, csharrison wrote:
> On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> > How is this a tab helper? Isn't it WebContentsObserver?
> > nit: WebContents
> 
> A tab helper is a WebContentsObserver. See
> https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md

Yes, tab helper is WebContentsObserver. WebContentsObserver needn't be a tab
helper though. I don't see how this object is a tab helper.

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:46:
timing.dom_content_loaded_event_start <= timing.load_event_start);
It seems weird to have an if block with only DCHECK statements in. Why not use
DCHECK_IMPLIES?

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:110: //
bad url like https://www.google.com/_/chrome/newtab.
Which claims what? It doesn't seem obvious to me why
https://www.google.com/_/chrome/newtab is a bad URL. It seems pretty good
actually, as it is our new tab page.

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:95:
return PageLoadTiming(frame->performance());
nit: return PageLoadTiming(render_frame()->GetWebFrame()->performance());

[Charlie Harrison, 2015-09-17 21:57:27]

jochen + asvitkine, PTAL? Thanks!

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:63: // As
a tab helper, this object is tied to a single WebContent
On 2015/09/17 20:58:43, nasko (slow to review) wrote:
> On 2015/09/16 22:37:38, csharrison wrote:
> > On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> > > How is this a tab helper? Isn't it WebContentsObserver?
> > > nit: WebContents
> > 
> > A tab helper is a WebContentsObserver. See
> > https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md
> 
> Yes, tab helper is WebContentsObserver. WebContentsObserver needn't be a tab
> helper though. I don't see how this object is a tab helper.

Maybe my understanding of tab helper is wrong, but I thought any
WebContentsObservers attached to a tab in TabHelpers::AttachTabHelpers was
necessarily a tab helper.

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:46:
timing.dom_content_loaded_event_start <= timing.load_event_start);
On 2015/09/17 20:58:43, nasko (slow to review) wrote:
> It seems weird to have an if block with only DCHECK statements in. Why not use
> DCHECK_IMPLIES?

Done.

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:110: //
bad url like https://www.google.com/_/chrome/newtab.
On 2015/09/17 20:58:43, nasko (slow to review) wrote:
> Which claims what? It doesn't seem obvious to me why
> https://www.google.com/_/chrome/newtab is a bad URL. It seems pretty good
> actually, as it is our new tab page.

We don't want to track new tab page for our metrics. They'll skew the metrics
because they are really an internal page. Most other urls are "caught" so to
speak in the render frame observer (i.e. the url "chrome://" is detected). New
tab page is not caught there but should be.

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/390001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:95:
return PageLoadTiming(frame->performance());
On 2015/09/17 20:58:43, nasko (slow to review) wrote:
> nit: return PageLoadTiming(render_frame()->GetWebFrame()->performance());

Done.

[nasko, 2015-09-17 23:23:29]

LGTM

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1312213010/diff/370001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:63: // As
a tab helper, this object is tied to a single WebContent
On 2015/09/17 21:57:27, csharrison wrote:
> On 2015/09/17 20:58:43, nasko (slow to review) wrote:
> > On 2015/09/16 22:37:38, csharrison wrote:
> > > On 2015/09/14 23:32:24, nasko (slow to review) wrote:
> > > > How is this a tab helper? Isn't it WebContentsObserver?
> > > > nit: WebContents
> > > 
> > > A tab helper is a WebContentsObserver. See
> > >
https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md
> > 
> > Yes, tab helper is WebContentsObserver. WebContentsObserver needn't be a tab
> > helper though. I don't see how this object is a tab helper.
> 
> Maybe my understanding of tab helper is wrong, but I thought any
> WebContentsObservers attached to a tab in TabHelpers::AttachTabHelpers was
> necessarily a tab helper.

I think this is my bad. I missed the part where this object is indeed added as a
tab helper. Apologies.

[jochen (gone - plz use gerrit), 2015-09-18 08:30:48]

https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
File components/page_load_metrics/DEPS (right):

https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
components/page_load_metrics/DEPS:4: "+third_party/WebKit/public",
that should be in renderer

https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
File components/page_load_metrics/common/page_load_timing.cc (right):

https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
components/page_load_metrics/common/page_load_timing.cc:6: #include
"third_party/WebKit/public/web/WebPerformance.h"
it's not safe to include this header in common/

only enum/POD headers should be included outside of renderer/

[Charlie Harrison, 2015-09-18 15:18:44]

On 2015/09/18 08:30:48, jochen wrote:
>
https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
> File components/page_load_metrics/DEPS (right):
> 
>
https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
> components/page_load_metrics/DEPS:4: "+third_party/WebKit/public",
> that should be in renderer
> 
>
https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
> File components/page_load_metrics/common/page_load_timing.cc (right):
> 
>
https://codereview.chromium.org/1312213010/diff/410001/components/page_load_m...
> components/page_load_metrics/common/page_load_timing.cc:6: #include
> "third_party/WebKit/public/web/WebPerformance.h"
> it's not safe to include this header in common/
> 
> only enum/POD headers should be included outside of renderer/

Thanks Jochen!

[Alexei Svitkine (slow), 2015-09-18 16:38:25]

metrics lgtm % comments

https://codereview.chromium.org/1312213010/diff/450001/components/page_load_m...
File
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/450001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:31:
"PageLoad.Timing.Navigation.To.FirstLayout";
Nit: Put these in the anon namespace at the top with the other constants and use
the "const char kFoo[]" syntax to define them.

https://codereview.chromium.org/1312213010/diff/450001/components/page_load_m...
File
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/1312213010/diff/450001/components/page_load_m...
components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc:150:
}  // namespace
Nit: I don't think the TEST_F()'s themselves should be in the anon namespace.

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

https://codereview.chromium.org/1312213010/diff/450001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:51: CHECK(state_ ==
DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
I think this should be a DCHECK(). CHECK() should only be used for security
issues.

[jochen (gone - plz use gerrit), 2015-09-21 14:59:57]

lgtm with comment

https://codereview.chromium.org/1312213010/diff/470001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1312213010/diff/470001/components/components_...
components/components_tests.gyp:1078: 'components.gyp:page_load_metrics_common',
why do you need _common?

[Charlie Harrison, 2015-09-21 15:18:02]

On 2015/09/21 14:59:57, jochen wrote:
> lgtm with comment
> 
>
https://codereview.chromium.org/1312213010/diff/470001/components/components_...
> File components/components_tests.gyp (right):
> 
>
https://codereview.chromium.org/1312213010/diff/470001/components/components_...
> components/components_tests.gyp:1078:
'components.gyp:page_load_metrics_common',
> why do you need _common?

I don't believe I need that, removing.

[Charlie Harrison, 2015-09-21 16:57:29]

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

[commit-bot: I haz the power, 2015-09-21 16:58:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312213010/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312213010/510001

[commit-bot: I haz the power, 2015-09-21 17:14:10]

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

[commit-bot: I haz the power, 2015-09-21 17:14:12]

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

[Charlie Harrison, 2015-09-21 17:23:05]

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

[Charlie Harrison, 2015-09-21 17:23:07]

+agl
I need a review for ipc dependency. Thanks!

[agl, 2015-09-21 17:38:15]

ipc/ LGTM.

[Charlie Harrison, 2015-09-21 17:41:51]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-09-21 17:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
bmcquade@chromium.org, nasko@chromium.org, asvitkine@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1312213010/#ps510001
(title: "fixed conflicts")

[commit-bot: I haz the power, 2015-09-21 17:42:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312213010/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312213010/510001

[commit-bot: I haz the power, 2015-09-21 17:52:26]

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

[commit-bot: I haz the power, 2015-09-21 17:52:28]

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

[Charlie Harrison, 2015-09-21 21:27:21]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-09-21 21:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
asvitkine@chromium.org, nasko@chromium.org, bmcquade@chromium.org,
clamy@chromium.org, agl@chromium.org
Link to the patchset: https://codereview.chromium.org/1312213010/#ps530001
(title: "Relax DCHECK constraints slightly for tests")

[commit-bot: I haz the power, 2015-09-21 21:29:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312213010/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312213010/530001

[commit-bot: I haz the power, 2015-09-21 22:05:38]

Committed patchset #28 (id:530001)

[commit-bot: I haz the power, 2015-09-21 22:06:25]

Patchset 28 (id:??) landed as
https://crrev.com/9fa62d54d03911d7a4120354d3e206ca69af4840
Cr-Commit-Position: refs/heads/master@{#350038}

[mmenke, 2015-09-22 14:50:59]

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

[mmenke, 2015-09-22 14:51:00]

Drive by comment.

https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
File components/page_load_metrics/OWNERS (right):

https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
components/page_load_metrics/OWNERS:2: csharrison@chromium.org
If you're not yet a full committer, you shouldn't be in any OWNER files, yet,
since you can't actually sign off on code.

[mmenke, 2015-09-22 14:52:15]

On 2015/09/22 14:51:00, mmenke wrote:
> Drive by comment.
> 
>
https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
> File components/page_load_metrics/OWNERS (right):
> 
>
https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
> components/page_load_metrics/OWNERS:2: mailto:csharrison@chromium.org
> If you're not yet a full committer, you shouldn't be in any OWNER files, yet,
> since you can't actually sign off on code.

And I think that might go for bmcquade as well?

[Bryan McQuade, 2015-09-22 14:53:30]

Charlie, can you send mmenke a patch to remove us from both OWNERS files?

On Tue, Sep 22, 2015 at 10:52 AM <mmenke@chromium.org> wrote:

> On 2015/09/22 14:51:00, mmenke wrote:
> > Drive by comment.
>
>
>
>
https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
> > File components/page_load_metrics/OWNERS (right):
>
>
>
>
https://codereview.chromium.org/1312213010/diff/530001/components/page_load_m...
> > components/page_load_metrics/OWNERS:2: mailto:csharrison@chromium.org
> > If you're not yet a full committer, you shouldn't be in any OWNER files,
> > yet,
> > since you can't actually sign off on code.
>
> And I think that might go for bmcquade as well?
>
> https://codereview.chromium.org/1312213010/
>

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

[mmenke, 2015-09-22 14:55:37]

On 2015/09/22 14:53:30, Bryan McQuade wrote:
> Charlie, can you send mmenke a patch to remove us from both OWNERS files?

Think you'll actually need rdsmith or ttuttle to sign off on it, since I don't
own either of the relevant OWNERS files (Note that you'll need to remove
yourselves from both of them).

[Charlie Harrison, 2015-09-22 15:07:11]

On 2015/09/22 14:55:37, mmenke wrote:
> On 2015/09/22 14:53:30, Bryan McQuade wrote:
> > Charlie, can you send mmenke a patch to remove us from both OWNERS files?
> 
> Think you'll actually need rdsmith or ttuttle to sign off on it, since I don't
> own either of the relevant OWNERS files (Note that you'll need to remove
> yourselves from both of them).

Patch is here: https://codereview.chromium.org/1357393003/. I listen jochen to
review because he is an OWNER of components/