[Page Load Metrics] PageLoadMetrics Mojofication. (issue 2823523003 by lpy@chromium.org)

[l...@chromium.org]

Reviewers: Bryan McQuade, Zhen Wang
CL: https://codereview.chromium.org/2823523003/

Message:
First round, ptal.

Description:
[Page Load Metrics] PageLoadMetrics Mojofication.

This patch

1. Creates a PageLoadMetrics mojo interface;
2. Adds mojo pipeline behind the legacy IPC.

The PageLoadMetrics mojo interface defines a UpdateTiming interface that is
implemented by MetricsWebContentsObserver. PageLoadMetrics is sensitive to IPC
ordering, thus MetricsWebContentsObserver holds a WebContentsFrameBindingSet
that is used to bind associated interface together with RenderFrameHost, and it
shares the legacy IPC channel so that we make sure we don't break IPC ordering.

BUG=709259

Affected files (+731, -19 lines):
M chrome/browser/chrome_content_browser_manifest_overlay.json
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
M chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
M chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
M chrome/common/BUILD.gn
M chrome/common/page_load_metrics/OWNERS
A chrome/common/page_load_metrics/page_load_metrics.mojom
A chrome/common/page_load_metrics/page_load_metrics.typemap
A chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
A chrome/common/page_load_metrics/page_load_metrics_struct_traits.cc
M chrome/renderer/BUILD.gn
A chrome/renderer/page_load_metrics/fake_page_load_metrics.h
A chrome/renderer/page_load_metrics/fake_page_load_metrics.cc
A chrome/renderer/page_load_metrics/fake_page_timing_sender.h
A chrome/renderer/page_load_metrics/fake_page_timing_sender.cc
M chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
M chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
M chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
M chrome/renderer/page_load_metrics/page_timing_metrics_sender.h
M chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
M chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
A chrome/renderer/page_load_metrics/page_timing_sender.h
M chrome/test/BUILD.gn
M chrome/typemaps.gni

[l...@chromium.org]

I updated the patch with Finch Trial changes that we are going to roll out. I
also added a bug to track Finch Trial progress and updated the design doc.

PTAL

https://codereview.chromium.org/2823523003/

[zh...@chromium.org]

In the description, it mentioned that this CL adds the finch trial. I think it
is adding the flag to control the feature instead of actually adding the finch
trial. I think the complete finch trial will include adding the reporting logic
to UMA, etc. In this CL, you can just clarify that in the description. The
actual reporting part can be in a followup CL.


https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode589
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:589:
OnTimingUpdated(render_frame_host, timing, metadata);
This is using the same timing update function as the old IPC, right? How
do we distinguish which data comes from which?

https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_load_metrics/page_load_metrics.typemap
File chrome/common/page_load_metrics/page_load_metrics.typemap (right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_load_metrics/page_load_metrics.typemap#newcode16
chrome/common/page_load_metrics/page_load_metrics.typemap:16:
"page_load_metrics.mojom.PageLoadMetadata=page_load_metrics::PageLoadMetadata",
Why are DocumentTiming, PaintTiming, ParseTiming not included here?

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode49
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49:
page_load_metrics_->UpdateTiming(timing, metadata);
Why should MetricsRenderFrameObserver care about sending the data? I
think this should still be done by PageTimingMetricsSender.

Ideally, MetricsRenderFrameObserver does not need to worry about whether
it is using old IPC or mojo. PageTimingMetricsSender will take care of
it.

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode85
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:85:
page_timing_sender_->SendTiming(last_timing_, metadata_);
So this actually calls to MetricsRenderFrameObserver::SendTiming(). Can
we just do the following?

page_load_metrics_->UpdateTiming(timing, metadata);

We can also just get the mojo end point here.

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_sender.h
File chrome/renderer/page_load_metrics/page_timing_sender.h (right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_sender.h#newcode15
chrome/renderer/page_load_metrics/page_timing_sender.h:15: class
PageTimingSender {
Why do we need this interface?

SendTiming can be just a function in PageTimingMetricsSender. Or you can
just do it in PageTimingMetricsSender::SendNow().

This is related to the above comment about MetricsRenderFrameObserver
does not need to do the actual sending.

https://codereview.chromium.org/2823523003/

[zh...@chromium.org]

https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode589
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:589:
OnTimingUpdated(render_frame_host, timing, metadata);

On 2017/04/19 18:49:54, Zhen Wang wrote:
> This is using the same timing update function as the old IPC, right?
How do we
> distinguish which data comes from which?

I just realized that you may not need to do any special thing on the
browser side, if you use the flag to toggle between IPC and Mojo. See
the renderer side comment for details.

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode88
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88:
ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated(
We do not need to always send this via IPC. The flag can just toggle
this: either send via IPC or Mojo, but not both. So the IPC one is
control group and the Mojo one is experiment group. This can simplify
the code on the browser side.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

Talked offline.

To clarify how Finch works here. If I understand correctly we don't need to add
more histograms. Histograms sent to UMA will be a different configuration, that
is to say, when we verify histograms, we basically choose between whether mojo
IPC is turnt on or not, and histograms will be grouped by IPC channel.

+rkaplow@ for further clarification if needed.

https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_load_metrics/page_load_metrics.typemap
File chrome/common/page_load_metrics/page_load_metrics.typemap (right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/common/page_load_metrics/page_load_metrics.typemap#newcode16
chrome/common/page_load_metrics/page_load_metrics.typemap:16:
"page_load_metrics.mojom.PageLoadMetadata=page_load_metrics::PageLoadMetadata",

On 2017/04/19 18:49:54, Zhen Wang wrote:

> Why are DocumentTiming, PaintTiming, ParseTiming not included here?

Done.

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode49
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49:

page_load_metrics_->UpdateTiming(timing, metadata);

On 2017/04/19 18:49:54, Zhen Wang wrote:

> Why should MetricsRenderFrameObserver care about sending the data? I
think this
> should still be done by PageTimingMetricsSender.
>
> Ideally, MetricsRenderFrameObserver does not need to worry about
whether it is
> using old IPC or mojo. PageTimingMetricsSender will take care of it.

MetricsRenderFrameObserver is IPC::Sender, PageTimingMetricsSender only
batches page timing, and call MetricsRenderFrameObserver itself to send
the timing over IPC. I maintain this logic here by making
MetricsRenderFrameObserver to implement a PageTimingSender API.

And as it turns out, this is easier to test/verify by separating the
logic of sending and batching. If PageTimingMetricsSender is doing the
actually sending, we need to fake a sender in MetricsRenderFrameObserver
test, and intercept the creation of the sender. See Patch Set #13 in the
prototype CL as an example:
https://codereview.chromium.org/2731583002/#ps380001

In my opinion, the name of PageTimingMetricsSender is misleading, as it
only batches the timing and calls IPC::Sender to send the timing.

https://codereview.chromium.org/2823523003/

[zh...@chromium.org]

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/100001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode49
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:49:
page_load_metrics_->UpdateTiming(timing, metadata);

Oh, I see.

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode584
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:584:
void MetricsWebContentsObserver::UpdateTiming(
nit: Can you put this close to OnMessageReceived?

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode182
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:182:
ptr->UpdateTiming(timing, PageLoadMetadata());
Should this and the above IPC one be controlled by the finch trial flag?
Is it possible to control that in the test?

If that is possible, you may not need to change the expected timing
error number in the tests.

In addition, why do we get duplicated errors, but not duplicated timing
updates (CountUpdatedTimingReported())? So I guess somewhere in the code
determines that the 2nd timing update is not reported. Can you give me a
pointer on that? It feels strange that we have only one report count
when we actually send twice.

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc#newcode703
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:703:
base::TimeDelta::FromMilliseconds(1100);
Is there any reason to change this?

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode157
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:157:
ptr->UpdateTiming(timing, metadata);
Same comment as the one in metrics_web_contents_observer_unittest.cc.

https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode88

chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88:
ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated(

This should not be sent over IPC if the feature is enabled, right?

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

On 2017/04/19 21:52:47, Zhen Wang wrote:
> Should this and the above IPC one be controlled by the finch trial flag? Is it
> possible to control that in the test?

Good suggestion, there's a testing config for Finch on waterfall in
https://cs.chromium.org/chromium/src/testing/variations/fieldtrial_testing_config.json,
for details see the doc:
https://chromium.googlesource.com/chromium/src/+/master/testing/variations/.
Basically it will run perf tests, browser tests with Finch configuration, but
not unit tests when I run the tests locally.

One thing I am not sure is that whether waterfall will run the tests for both
cases, or just for mojo-enabled case, I will upload a patch set to verify it. If
waterfall doesn't run the testing for both cases, then we will lose test
coverage for legacy IPC on browser test.

> If that is possible, you may not need to change the expected timing error
number
> in the tests.
>
> In addition, why do we get duplicated errors, but not duplicated timing
updates
> (CountUpdatedTimingReported())? So I guess somewhere in the code determines
that
> the 2nd timing update is not reported. Can you give me a pointer on that? It
> feels strange that we have only one report count when we actually send twice.

The 2nd timing update is dropped here:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_load_tracker.cc?l=278
This is expected, only when two page timing are the same(which they should be),
will the latter one be dropped. So if we receive two page timing for each
simulation, then I think it is wrong.

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode584
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:584:
void MetricsWebContentsObserver::UpdateTiming(

On 2017/04/19 21:52:47, Zhen Wang wrote:
> nit: Can you put this close to OnMessageReceived?

It's much closer to OnTimingUpdated I think. OnMessageReceived receives
raw IPC signal, but UpdateTiming doesn't, think about it as just a
function call.

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc#newcode703
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:703:
base::TimeDelta::FromMilliseconds(1100);

On 2017/04/19 21:52:47, Zhen Wang wrote:
> Is there any reason to change this?

The overhead of SimulateTimingUpdate increases a little bit.

https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode88
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88:
ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated(

On 2017/04/19 21:52:47, Zhen Wang wrote:
> This should not be sent over IPC if the feature is enabled, right?

Yes.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

>
https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
> File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right):
>
>
https://codereview.chromium.org/2823523003/diff/120001/chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc#newcode88
> chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:88:
> ipc_sender_->Send(new PageLoadMetricsMsg_TimingUpdated(
> On 2017/04/19 21:52:47, Zhen Wang wrote:
> > This should not be sent over IPC if the feature is enabled, right?
>
> Yes.

I mean, no, it should not be sent.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

On 2017/04/20 01:49:24, lpy wrote:
> One thing I am not sure is that whether waterfall will run the tests for both
> cases, or just for mojo-enabled case, I will upload a patch set to verify it.
If
> waterfall doesn't run the testing for both cases, then we will lose test
> coverage for legacy IPC on browser test.

Looks like it doesn't run the tests for both config(enable mojo IPC and disable
mojo IPC)

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

PTAL.

I added extra browser tests for mojofication.

The reason I didn't add it to exist browser test set, is because we need to set
the feature state before the render process is created, in order for it to
inherit the feature state from the browser process. That is to say, we can't
config Finch Trial for individual test cases, we can only turn it on for a
browser test set.

And please ignore Patch Set #5.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks! I will take a look at this soon.

Are we planning to factor the field trial logic into this change as well?

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

great, i see we have the feature support in place to finch this already.

here are some early comments / questions, thanks!


https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(left):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#oldcode67
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:67: //
static
thanks! not sure how this crept in here...

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h#newcode189
chrome/common/chrome_features.h:189: extern const base::Feature
kPageLoadMetricsMojofication;
does this need to be declared in chrome_features.h? Can it instead be
declared in the class that wants to detect whether this feature is
enabled/disabled? that's the more typical way I've seen features like
this declared. see for example kDelayNavigationFeature in
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.h

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See
comments in page_load_timing.h for details on each field.
interesting, do all types sent over mojo have to be declared in a mojom
file? we can't for instance include page_load_timing.h? or convert
page_load_timing.h to a mojom file which just declares structs, and then
include it here, to avoid the duplication?

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode54
chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface
PageLoadMetrics {
though I don't have a better name for this service, I find the name
PageLoadMetrics a little confusing. That name doesn't sound like a
service name that you execute actions on but rather a passive data
structure that holds metrics data. I'm slightly inclined to call this
something like PageLoadMetricsChannel to better describe that it's a
communication channel that the PageLoadMetrics are sent over, but I'm
not sure if that's the best name or not. Can you think of a name to
better describe that this is a service interface that page load metrics
data is sent over?

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode17
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17:
struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView,
interesting, this is a lot of boilerplate that seems like maybe could be
automatically emitted by a mojo compiler. i guess that's not currently
possible but it does seem like this is a fine area for bugs to be
introduced (e.g. we copy/paste a struct trait and reference the wrong
field). do you know if there's any current way or future plan to trim
down the amount of StructTraits code that needs to be written?

i notice in this much older iteration to move this system to mojo
https://codereview.chromium.org/2056153002/ there was no such
StructTraits. is it possible to avoid StructTraits as in that change for
simple types that don't need any special behavior?

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

+rockot@ for mojo review and mojo related questions.

Ken, could you please take a look at the patch and mojo questions raised by
Bryan? Thank you.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h#newcode189
chrome/common/chrome_features.h:189: extern const base::Feature
kPageLoadMetricsMojofication;

On 2017/04/24 21:50:55, Bryan McQuade wrote:
> does this need to be declared in chrome_features.h? Can it instead be
declared
> in the class that wants to detect whether this feature is
enabled/disabled?
> that's the more typical way I've seen features like this declared. see
for
> example kDelayNavigationFeature in
>
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.h

It's declared here because both renderer/ and browser/ need to use it,
renderer checks to decide which IPC it should use to send data, and
browser uses it for browser test only.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See
comments in page_load_timing.h for details on each field.

On 2017/04/24 21:50:55, Bryan McQuade wrote:
> interesting, do all types sent over mojo have to be declared in a
mojom file? we
> can't for instance include page_load_timing.h? or convert
page_load_timing.h to
> a mojom file which just declares structs, and then include it here, to
avoid the
> duplication?

I think that's how mojo works since I am following mojo wiki, that's
also what legacy IPC used to require.

+rockot@ for further answer.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode17
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17:
struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView,

On 2017/04/24 21:50:56, Bryan McQuade wrote:
> interesting, this is a lot of boilerplate that seems like maybe could
be
> automatically emitted by a mojo compiler. i guess that's not currently
possible
> but it does seem like this is a fine area for bugs to be introduced
(e.g. we
> copy/paste a struct trait and reference the wrong field). do you know
if there's
> any current way or future plan to trim down the amount of StructTraits
code that
> needs to be written?
>
> i notice in this much older iteration to move this system to mojo
> https://codereview.chromium.org/2056153002/ there was no such
StructTraits. is
> it possible to avoid StructTraits as in that change for simple types
that don't
> need any special behavior?

I was following the mojo wiki so I am not sure if there's any better
approach or future plan.
+ rockot@ for further answer.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks again for taking this change on! A few more comments / questions.


https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json
File chrome/browser/chrome_content_browser_manifest_overlay.json
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json#newcode34
chrome/browser/chrome_content_browser_manifest_overlay.json:34:
"navigation:frame": {
so i can better understand & review the code, can you explain why we're
under 'navigation:frame'? What does that mean & why does page load
metrics belong underneath it?

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode114
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:114:
friend class FakePageLoadMetrics;
i'm a bit uncomfortable about this - i need to see why the helper
browser test class needs access to private state, but i'd like to try to
avoid this need if at all possible.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode176
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176:
observer_->page_load_metrics_binding_for_testing()
I think I'm inclined to change the SimulateTimingUpdate impl to just
directly invoke OnTimingUpdated, rather than going through the IPC
dispatch patch, and not having to have the test invoke / know about both
IPC and Mojo. This is a unit test so it shouldn't really have to know
about IPC or Mojo dispatch. While it's nice to exercise slightly more
code with IPC dispatch, it adds the complexity of now also having to
invoke the mojo path here, having to expose the
page_load_metrics_binding_for_testing() method on the observer, and we
do also have the browsertest for IPC/Mojo dispatch coverage so I feel it
is a little redundant to also do that here, given the complexity it now
adds.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode148
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:148:
observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated(
same thing here - let's invoke OnTimingUpdated directly and avoid the
need to know about mojo in these unit tests.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode1
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
on first glance i'd really like to see if we can have a single
browsertest file for both mojo and ipc flows, rather than 2 separate.
with them split, we have to fix bugs in both places, add new tests in
both places, etc. ideally the duplication goes away soonish but it's
best not to have the duplication at all if possible. i'll spend some
more time thinking about this & see what we can do here.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode137
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137:
base::RunLoop().RunUntilIdle();
can we add a comment to explain why this is needed? i usually like to
add a comment explaining why calls to RunUntilIdle() are needed, since
it can often be subtle & helps future modifiers of the code to
understand the specific reason.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode173
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173:
metrics_observer_->page_load_metrics_binding_for_testing()
it seems odd that we need to wire this frame mapping up in a browser
test. a browser test should be managing the message routing between
frames correctly on its own. why is this needed?

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode179
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:179:
void AddObserver(PageLoadTimingObserver* observer) {
hmm, this doesn't seem like it should be here. i'm concerned that we're
having to fake out so much behavior here. browser tests are supposed to
exercise the real production codepath. by stubbing this out we're
mitigating much of the value of the browser test. can we spend some time
to see how to get the browser test working w/o the need for a
FakePageLoadMetrics object?

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h#newcode189
chrome/common/chrome_features.h:189: extern const base::Feature
kPageLoadMetricsMojofication;

On 2017/04/25 at 09:03:37, lpy wrote:
> On 2017/04/24 21:50:55, Bryan McQuade wrote:
> > does this need to be declared in chrome_features.h? Can it instead
be declared
> > in the class that wants to detect whether this feature is
enabled/disabled?
> > that's the more typical way I've seen features like this declared.
see for
> > example kDelayNavigationFeature in
> >
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.h
>
> It's declared here because both renderer/ and browser/ need to use it,
renderer checks to decide which IPC it should use to send data, and
browser uses it for browser test only.

Ah, I see, thanks! I'm ok with putting this here, though I'm also open
to moving it somewhere under chrome/common/page_load_metrics/ if there's
a logical place there.

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json
File chrome/browser/chrome_content_browser_manifest_overlay.json
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json#newcode34
chrome/browser/chrome_content_browser_manifest_overlay.json:34:
"navigation:frame": {

On 2017/04/25 at 15:41:13, Bryan McQuade wrote:
> so i can better understand & review the code, can you explain why
we're under 'navigation:frame'? What does that mean & why does page load
metrics belong underneath it?

These are capabilities exposed to render frames specifically. i.e. there
are interfaces which can be requested by individual frames, and
interfaces which can be requested by the render process without any
frame context. navigation:frame capabilities constitute the former. It's
a (IMHO somewhat confusing) detail of the way we scope capabilities in
service manifests today, but this is correct.

We're looking into making this less confusing and subsequently better
documented.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode176
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176:
observer_->page_load_metrics_binding_for_testing()

On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> I think I'm inclined to change the SimulateTimingUpdate impl to just
directly invoke OnTimingUpdated, rather than going through the IPC
dispatch patch, and not having to have the test invoke / know about both
IPC and Mojo. This is a unit test so it shouldn't really have to know
about IPC or Mojo dispatch. While it's nice to exercise slightly more
code with IPC dispatch, it adds the complexity of now also having to
invoke the mojo path here, having to expose the
page_load_metrics_binding_for_testing() method on the observer, and we
do also have the browsertest for IPC/Mojo dispatch coverage so I feel it
is a little redundant to also do that here, given the complexity it now
adds.

+1 - I prefer to avoid going through IPC boundaries when unit-testing if
possible. Seems like unnecessary complexity.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode137
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137:
base::RunLoop().RunUntilIdle();

On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> can we add a comment to explain why this is needed? i usually like to
add a comment explaining why calls to RunUntilIdle() are needed, since
it can often be subtle & helps future modifiers of the code to
understand the specific reason.

I would in fact prefer to never use RunUntilIdle(). It's **almost
always** wrong.

The correct thing to do is to capture a RunLoop's QuitClosure() and
explicitly invoke it when a specific, interesting event occurs, and
simply Run() the loop.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode173
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173:
metrics_observer_->page_load_metrics_binding_for_testing()

On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> it seems odd that we need to wire this frame mapping up in a browser
test. a browser test should be managing the message routing between
frames correctly on its own. why is this needed?

This is true. It should only be necessary to inject a target frame in
unit test code when a message isn't actually arriving from a renderer.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See
comments in page_load_timing.h for details on each field.

On 2017/04/25 at 09:03:37, lpy wrote:
> On 2017/04/24 21:50:55, Bryan McQuade wrote:

> > interesting, do all types sent over mojo have to be declared in a
mojom file? we
> > can't for instance include page_load_timing.h? or convert
page_load_timing.h to
> > a mojom file which just declares structs, and then include it here,
to avoid the
> > duplication?
>
> I think that's how mojo works since I am following mojo wiki, that's
also what legacy IPC used to require.
>
> +rockot@ for further answer.

Absolutely yes, everything sent over Mojo has to be defined in a mojom
file. Otherwise bindings would be limited to C++.

We do support C++ typemapping[1] which allows you to teach the bindings
generator how to serialize some arbitrary C++ type as a mojom struct and
then generate bindings which use that type in any relevant generated C++
interfaces.

We also support some trickery[2] for exploiting existing legacy IPC
traits definitions to avoid duplicate mojom definitions for existing IPC
structs. Use of this feature should generally be discouraged though and
will not be supported indefinitely.

[1]
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings/README.md#Type-Mapping
[2]
https://chromium.googlesource.com/chromium/src/+/master/ipc/README.md#Using-Legacy-IPC-Traits

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode54
chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface
PageLoadMetrics {

On 2017/04/24 at 21:50:55, Bryan McQuade wrote:
> though I don't have a better name for this service, I find the name
PageLoadMetrics a little confusing. That name doesn't sound like a
service name that you execute actions on but rather a passive data
structure that holds metrics data. I'm slightly inclined to call this
something like PageLoadMetricsChannel to better describe that it's a
communication channel that the PageLoadMetrics are sent over, but I'm
not sure if that's the best name or not. Can you think of a name to
better describe that this is a service interface that page load metrics
data is sent over?

terminology nit: it's not a service.

All mojo interfaces are by convention in a mojom subnamespace, so this
will always be read as mojom::PageLoadMetrics in C++ code. That should
be sufficient to convey the detail that it's an IPC interface. I do not
think it is correct for people to start adding nouns like "Channel" or
"Pipe" or whatever to interface names.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode17
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17:
struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView,

On 2017/04/25 at 09:03:37, lpy wrote:

> On 2017/04/24 21:50:56, Bryan McQuade wrote:
> > interesting, this is a lot of boilerplate that seems like maybe
could be
> > automatically emitted by a mojo compiler. i guess that's not
currently possible
> > but it does seem like this is a fine area for bugs to be introduced
(e.g. we
> > copy/paste a struct trait and reference the wrong field). do you
know if there's
> > any current way or future plan to trim down the amount of
StructTraits code that
> > needs to be written?
> >
> > i notice in this much older iteration to move this system to mojo
> > https://codereview.chromium.org/2056153002/ there was no such
StructTraits. is
> > it possible to avoid StructTraits as in that change for simple types
that don't
> > need any special behavior?
>
> I was following the mojo wiki so I am not sure if there's any better
approach or future plan.
> + rockot@ for further answer.

I don't think there is any reasonable way to automate StructTraits
generation. It's almost certainly not worth the complexity to do it well
and cover all the weird possible edge cases.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See
comments in page_load_timing.h for details on each field.

Ah, ok, thanks! Is it possible to declare the structs in a mojom file
and then use them for the IPC code flow as well? That way we have only
one canonical place that we define the structs (mojom file) rather than
having a mojom file and a separate header for the IPC variant.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode54
chrome/common/page_load_metrics/page_load_metrics.mojom:54: interface
PageLoadMetrics {

On 2017/04/25 at 19:31:46, Ken Rockot wrote:
> On 2017/04/24 at 21:50:55, Bryan McQuade wrote:
> > though I don't have a better name for this service, I find the name
PageLoadMetrics a little confusing. That name doesn't sound like a
service name that you execute actions on but rather a passive data
structure that holds metrics data. I'm slightly inclined to call this
something like PageLoadMetricsChannel to better describe that it's a
communication channel that the PageLoadMetrics are sent over, but I'm
not sure if that's the best name or not. Can you think of a name to
better describe that this is a service interface that page load metrics
data is sent over?
>
> terminology nit: it's not a service.
>
> All mojo interfaces are by convention in a mojom subnamespace, so this
will always be read as mojom::PageLoadMetrics in C++ code. That should
be sufficient to convey the detail that it's an IPC interface. I do not
think it is correct for people to start adding nouns like "Channel" or
"Pipe" or whatever to interface names.

the mojom:: namespace does help to clarify the class's purpose here -
sounds fine to me thanks.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode17
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17:
struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView,

ah, ok. what are all the static methods that just return the member
variable needed for? those do seem like they could be autogenerated. the
Read() methods seem like they probably need someone to implement them
though.

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode9
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // See
comments in page_load_timing.h for details on each field.

No, and we won't be supporting that. I mean, you could always call
mojom::Foo::Serialize(foo) to get a std::vector<uint8_t> of serialized
bytes and send that over legacy IPC, but that's pretty awful.

If you have a structure which is already used by legacy IPC code though,
you can define a mojom alias (no duplicate definition in Mojom beyond a
simple name declaration) and the bindings will repurpose the existing
serialization format. Of course this limits the type to use with C++
bindings, but that kind of goes without saying if you plan to use legacy
IPC serialization. This is the feature outlined by [2] in my previous
reply.

Of course if we're talking about a data structure which is not currently
used in any legacy IPC messages, the point is moot because we should not
be introducing any new legacy IPC code without very compelling reasons.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode17
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:17:
struct StructTraits<page_load_metrics::mojom::DocumentTimingDataView,

This is documented at [1], but the gist is, you have a static method per
mojom field which defines a (fast, i.e., copy-free) way of mapping the
object's data into something the serializer knows how to serialize.

They cannot be autogenerated. It might seem that way for the simplest
cases, but only for the simplest cases. For fields that are POD types
for example this is trivial, and you could imagine us adding a helper
macro like:

#define DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(type, name) \
static decltype(std::declval<type>().name) name(const type& value) {
\
return value.name; }

so then you could write

DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(page_load_metrics::DocumentTiming,
first_layout);
DEFINE_TRIVIAL_STRUCT_TRAITS_FIELD(page_load_metrics::DocumentTiming,
load_event_start);

etc, instead of

static base::Optional<base::TimeDelta> first_layout(
const page_load_metrics::DocumentTiming& value) {
return value.first_layout;
}

but it saves very little typing and hides behind a magic macro what
would otherwise be easily understandable code.

Keep in mind that many field accessors are not trivial, and do not
return the exact same type as the native type's field (e.g. static
std::string accessors typically return a base::StringPiece); or they
don't correspond verbatim to an existing field on the object (e.g. maybe
they're accessed by some other public accessor method or for various
reasons don't have the same name as the mojom field). In some cases they
need to be mapped to some other serializer-compatible view type (e.g.
maybe you use mojo::CArray<uint8_t> to serialize some specialized buffer
type as a mojom array<uint8>), and so on.

[1]
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings/README.md#StructTraits-Reference

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

Thanks for the feedback, I made a few updates, ptal.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(left):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#oldcode67
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:67: //
static

On 2017/04/24 21:50:55, Bryan McQuade wrote:

> thanks! not sure how this crept in here...

Acknowledged.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode176
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:176:
observer_->page_load_metrics_binding_for_testing()

On 2017/04/25 19:31:46, Ken Rockot wrote:
> On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> > I think I'm inclined to change the SimulateTimingUpdate impl to just
directly
> invoke OnTimingUpdated, rather than going through the IPC dispatch
patch, and
> not having to have the test invoke / know about both IPC and Mojo.
This is a
> unit test so it shouldn't really have to know about IPC or Mojo
dispatch. While
> it's nice to exercise slightly more code with IPC dispatch, it adds
the
> complexity of now also having to invoke the mojo path here, having to
expose the
> page_load_metrics_binding_for_testing() method on the observer, and we
do also
> have the browsertest for IPC/Mojo dispatch coverage so I feel it is a
little
> redundant to also do that here, given the complexity it now adds.
>
> +1 - I prefer to avoid going through IPC boundaries when unit-testing
if
> possible. Seems like unnecessary complexity.

Done.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode148
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:148:
observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated(

On 2017/04/25 15:41:14, Bryan McQuade wrote:
> same thing here - let's invoke OnTimingUpdated directly and avoid the
need to
> know about mojo in these unit tests.

Done.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode1
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:1:
// Copyright 2015 The Chromium Authors. All rights reserved.

On 2017/04/25 15:41:14, Bryan McQuade wrote:
> on first glance i'd really like to see if we can have a single
browsertest file
> for both mojo and ipc flows, rather than 2 separate. with them split,
we have to
> fix bugs in both places, add new tests in both places, etc. ideally
the
> duplication goes away soonish but it's best not to have the
duplication at all
> if possible. i'll spend some more time thinking about this & see what
we can do
> here.

The reason I didn't add it to exist browser test set, is because we need
to set the feature state before the render process is created, in order
for it to inherit the feature state from the browser process. That is to
say, we can't config Finch Trial for individual test cases, we can only
turn it on for a browser test set.

Please also see SetUpCommandLine in
PageLoadMetricsMojoficationBrowserTest.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode137
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:137:
base::RunLoop().RunUntilIdle();

On 2017/04/25 19:31:46, Ken Rockot wrote:
> On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> > can we add a comment to explain why this is needed? i usually like
to add a
> comment explaining why calls to RunUntilIdle() are needed, since it
can often be
> subtle & helps future modifiers of the code to understand the specific
reason.
>
> I would in fact prefer to never use RunUntilIdle(). It's **almost
always**
> wrong.
>
> The correct thing to do is to capture a RunLoop's QuitClosure() and
explicitly
> invoke it when a specific, interesting event occurs, and simply Run()
the loop.

Done.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode173
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:173:
metrics_observer_->page_load_metrics_binding_for_testing()

On 2017/04/25 19:31:46, Ken Rockot wrote:
> On 2017/04/25 at 15:41:14, Bryan McQuade wrote:
> > it seems odd that we need to wire this frame mapping up in a browser
test. a
> browser test should be managing the message routing between frames
correctly on
> its own. why is this needed?
>
> This is true. It should only be necessary to inject a target frame in
unit test
> code when a message isn't actually arriving from a renderer.

We are re-directing the mojo incoming request to the FakePageLoadMetrics
by using FakePageLoadMetricsBinder, so the MetricsWebContentsObserver
won't get called, but we still want messages to be delivered fully in
order to do histogram check, so we need to manually bind render frame
host context to the web contents frame binding set. And call it
manually.

https://codereview.chromium.org/2823523003/diff/180001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode179
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:179:
void AddObserver(PageLoadTimingObserver* observer) {

On 2017/04/25 15:41:14, Bryan McQuade wrote:
> hmm, this doesn't seem like it should be here. i'm concerned that
we're having
> to fake out so much behavior here. browser tests are supposed to
exercise the
> real production codepath. by stubbing this out we're mitigating much
of the
> value of the browser test. can we spend some time to see how to get
the browser
> test working w/o the need for a FakePageLoadMetrics object?

The basic idea is the same as the old browser test, which is blocking
code execution until IPC is fully delivered.

Unlike legacy IPC which we can use filter to filter out specific IPC
messages, mojo doesn't have similar mechanism as far as I understand.
Thus we fake an implementation so that mojo IPC sent from renderer would
arrive to the fake implementation, and we manually dispatch to the
original one and do whatever we want, which is unblocking execution
here.

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2823523003/diff/180001/chrome/common/chrome_features.h#newcode189
chrome/common/chrome_features.h:189: extern const base::Feature
kPageLoadMetricsMojofication;

On 2017/04/25 15:41:14, Bryan McQuade wrote:
> On 2017/04/25 at 09:03:37, lpy wrote:
> > On 2017/04/24 21:50:55, Bryan McQuade wrote:

> > > does this need to be declared in chrome_features.h? Can it instead
be
> declared
> > > in the class that wants to detect whether this feature is
enabled/disabled?
> > > that's the more typical way I've seen features like this declared.
see for
> > > example kDelayNavigationFeature in
> > >
>
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.h
> >
> > It's declared here because both renderer/ and browser/ need to use
it,
> renderer checks to decide which IPC it should use to send data, and
browser uses
> it for browser test only.
>
> Ah, I see, thanks! I'm ok with putting this here, though I'm also open
to moving
> it somewhere under chrome/common/page_load_metrics/ if there's a
logical place
> there.

Acknowledged.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

a couple more questions/comments, thanks!


https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;
In looking at the generated code for this struct in the .mojom.h file,
it appears that the DocumentTiming, PaintTiming, ParseTiming,
StyleSheetTiming are declared as:

page_load_metrics::DocumentTiming document_timing;
page_load_metrics::PaintTiming paint_timing;
page_load_metrics::ParseTiming parse_timing;
page_load_metrics::StyleSheetTiming style_sheet_timing;

which makes me wonder - are we using the
page_load_metrics::mojom::DocumentTiming and other related structs at
all?

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode55
chrome/common/page_load_metrics/page_load_metrics.mojom:55:
UpdateTiming(PageLoadTiming page_load_timing, PageLoadMetadata
page_load_metadata);
similarly, the generated code appears to be using
page_load_metrics::PageLoadTiming rather than the mojom version you
declare in this file. It may just be that I don't fully understand how
mojo works, but on first glance it doesn't seem like we're using the
types you declared in this mojom file. If I'm misunderstanding please
let me know - I'm still trying to learn how mojo works. Thanks!

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;

On 2017/04/26 at 15:24:22, Bryan McQuade wrote:
> In looking at the generated code for this struct in the .mojom.h file,
it appears that the DocumentTiming, PaintTiming, ParseTiming,
StyleSheetTiming are declared as:
>
> page_load_metrics::DocumentTiming document_timing;
> page_load_metrics::PaintTiming paint_timing;
> page_load_metrics::ParseTiming parse_timing;
> page_load_metrics::StyleSheetTiming style_sheet_timing;
>
> which makes me wonder - are we using the
page_load_metrics::mojom::DocumentTiming and other related structs at
all?

Yes, this is the point of typemapping. You typemap mojom::Foo to Foo,
and we generate interfaces that use the more friendly Foo type where
appropriate (e.g. struct fields and message arguments.)

The mojom still specifies the precise wire format of the data, and
StructTraits defines how to efficiently map between the friendly type
and the wire format in a safe and reliable way.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Got it, makes sense, thanks! In our case, since the page_load_metrics::mojom::
versions of the structs are identical to the typemapped structs, is there any
reason for us to not use the mojom versions directly, and get rid of the structs
we are typemapping to? Or are teams encouraged to always typemap even if the
struct definitions are identical?

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

It's entirely a matter of preference IMHO. I would love it if more code just
used mojom types directly, but it's often not feasible for one of two reasons:

1. You want your common types to have helper methods which cannot be
autogenerated - e.g., gfx::Rect is a very simple data structure but also has
tons of useful helper methods for common rectangle operations.

2. You want any kind of universal custom validation when deserializing the type
- e.g., GURL rejects in its StructTraits' Read() if the incoming string data
exceeds a maximum length.


https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode13
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:13:
#include "base/run_loop.h"
can remove this include

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode149
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:149:
observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated(
let's switch this to invoke OnTimingUpdated directly, rather than
invoking the IPC and Mojo codepaths.

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc#newcode227
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:227:
if (!mock_page_load_metrics_) {
just so i better understand, why is this needed in this browsertest if
we've written a separate browsertest for exercising the mojo codepath?

fwiw i am still hopeful we can merge the 2 browsertests into one, but in
the absence of a merge it seems odd that we have to do so much mojo
setup for a browsertest that isn't actually doing anything with mojo

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178:
void AddObserver(PageLoadTimingObserver* observer) {
ah, I see now why you have this class and this method. I still worry
about having to stub/wrap so much of the mojo code in a browser test.
Ideally we'd be able to accomplish the goal of waiting for a mojo
message to arrive without having to wrap the mojom::PageLoadMetrics like
this.

Ken, the test cases in this file to wait until a message received over
the mojo channel meets a certain constraint before allowing the test to
continue. Is there a canonical/best way for browser tests to do this,
without having to wrap the mojom::PageLoadMetrics object like this?

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h#newcode62
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:62:
mojom::PageLoadMetricsAssociatedPtr page_load_metrics_;
Let's add a short comment explaining why we use a
PageLoadMetricsAssociatedPtr here (rather than a plain PageLoadMetrics).

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h
File chrome/renderer/page_load_metrics/page_timing_sender.h (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h#newcode15
chrome/renderer/page_load_metrics/page_timing_sender.h:15: class
PageTimingSender {
i like this abstraction, but let's do this slightly differently:

let's define an IPCPageTimingSender, which uses an IPC::Sender*
internally, and a MojoPageTimingSender, which uses mojo.

When instantiating a PageTimingSender, we do something like:

std::unique_ptr<page_load_metrics::PageTimingSender>
CreateTimingSender() {
if
(base::FeatureList::IsEnabled(features::kPageLoadMetricsMojofication))
return new MojoPageTimingSender();
else
return new IPCPageTimingSender();
}

Additionally, you can use a MockPageTimingSender in unit tests, and none
of the unit tests need to know about mojo or IPC.

then none of the downstream code needs to know anything about IPC or
Mojo. I prefer this to having downstream code checking whether a feature
is enabled, as you have things currently.

Over time if we kill the IPC impl I'd be fine with removing this class
and inlining the mojo class.

My only concern is that the name of this class PageTimingSender is very
close to the name of PageTimingMetricsSender. Perhaps we should move
your new class's definition into PageTimingMetricsSender and call it
PageTimingMetricsSender::SenderInterface (inner class) or
PageTimingMetricsSenderEmbedderInterface, or something along those
lines?

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178:
void AddObserver(PageLoadTimingObserver* observer) {

On 2017/04/26 at 16:04:34, Bryan McQuade wrote:
> ah, I see now why you have this class and this method. I still worry
about having to stub/wrap so much of the mojo code in a browser test.
Ideally we'd be able to accomplish the goal of waiting for a mojo
message to arrive without having to wrap the mojom::PageLoadMetrics like
this.
>
> Ken, the test cases in this file to wait until a message received over
the mojo channel meets a certain constraint before allowing the test to
continue. Is there a canonical/best way for browser tests to do this,
without having to wrap the mojom::PageLoadMetrics object like this?

The messages are always going to be dispatched to some implementation of
mojom::PageLoadMetrics. You can either do this (dispatch to a test-only
impl) or add testing hooks to the non-test impl. I don't see much room
for other options if a test needs to inspect incoming messages.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Makes sense, thanks! Looking around the mojo code a bit, I see the
MessageReceiver interface. Is it possible to use Binding::AddFilter() or
something similar to observe messages as they are being processed? That would
allow us to avoid wrapping the PageLoadMetrics itself, which seems desirable to
avoid if possible.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

(and just to clarify, we'd only ever do this in test code - never in real
production code)

https://codereview.chromium.org/2823523003/

[Ken Rockot]

I wouldn't recommend it. Filters are applied pre-deserialization, so you'd have to manually inspect the message using DataView objects. I don't think this will end up being better than what you're doing now.

[Bryan McQuade]

Thanks! I appreciate your helping us to work through this. I'll spend a little time thinking through whether wrapping the interface vs adding hooks to the non test class is the better path. I currently have a slight pref for wrapping as done currently but will think through a bit more.

Ken, if you do see any opportunities to simplify the way lpy@ is wrapping things currently, please do let us know. Generally speaking, the simpler the wrapping code the better for things like this.

--
You received this message because you are subscribed to the Google Groups "loading-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to loading-revie...@chromium.org.
To post to this group, send email to loading...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/CA%2BapAgHWfy3tQsHFLb-q9fTgdD0D5M%2BxrbXP4Lf3kW_txOGLnQ%40mail.gmail.com.

[bmcq...@chromium.org]

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178:
void AddObserver(PageLoadTimingObserver* observer) {

On 2017/04/26 at 16:11:41, Ken Rockot wrote:
> On 2017/04/26 at 16:04:34, Bryan McQuade wrote:
> > ah, I see now why you have this class and this method. I still worry
about having to stub/wrap so much of the mojo code in a browser test.
Ideally we'd be able to accomplish the goal of waiting for a mojo
message to arrive without having to wrap the mojom::PageLoadMetrics like
this.
> >
> > Ken, the test cases in this file to wait until a message received
over the mojo channel meets a certain constraint before allowing the
test to continue. Is there a canonical/best way for browser tests to do
this, without having to wrap the mojom::PageLoadMetrics object like
this?
>
> The messages are always going to be dispatched to some implementation
of mojom::PageLoadMetrics. You can either do this (dispatch to a
test-only impl) or add testing hooks to the non-test impl. I don't see
much room for other options if a test needs to inspect incoming
messages.

I decided that adding testing hooks to the non-test impl was better
overall. It has no real cost to non-test code, and allows us to avoid
having to wrap code in tests, which I think is desirable.

Peiyong, I put together a change for this here:
https://codereview.chromium.org/2847513002 which I hope eases the rest
of your work to migrate from IPC to mojo. Let me know what you think.

Thanks,
Bryan

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;

On 2017/04/26 at 15:28:45, Ken Rockot wrote:
> On 2017/04/26 at 15:24:22, Bryan McQuade wrote:
> > In looking at the generated code for this struct in the .mojom.h
file, it appears that the DocumentTiming, PaintTiming, ParseTiming,
StyleSheetTiming are declared as:
> >
> > page_load_metrics::DocumentTiming document_timing;
> > page_load_metrics::PaintTiming paint_timing;
> > page_load_metrics::ParseTiming parse_timing;
> > page_load_metrics::StyleSheetTiming style_sheet_timing;
> >
> > which makes me wonder - are we using the
page_load_metrics::mojom::DocumentTiming and other related structs at
all?
>
> Yes, this is the point of typemapping. You typemap mojom::Foo to Foo,
and we generate interfaces that use the more friendly Foo type where
appropriate (e.g. struct fields and message arguments.)
>
> The mojom still specifies the precise wire format of the data, and
StructTraits defines how to efficiently map between the friendly type
and the wire format in a safe and reliable way.

Based on other discussions on the thread, I'm inclined to remove the
structs in page_load_timing.h and switch all code over to using the
structs that come from the mojom file. Peiyong, what do you think?

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

I updated the patch, ptal.

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
File
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc#newcode13
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:13:
#include "base/run_loop.h"

On 2017/04/26 16:04:34, Bryan McQuade wrote:
> can remove this include

Done.

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode149
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:149:
observer_->OnMessageReceived(PageLoadMetricsMsg_TimingUpdated(

On 2017/04/26 16:04:34, Bryan McQuade wrote:
> let's switch this to invoke OnTimingUpdated directly, rather than
invoking the
> IPC and Mojo codepaths.

Done. Forgot to change this part

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc#newcode227
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:227:
if (!mock_page_load_metrics_) {

On 2017/04/26 16:04:34, Bryan McQuade wrote:
> just so i better understand, why is this needed in this browsertest if
we've
> written a separate browsertest for exercising the mojo codepath?
>
> fwiw i am still hopeful we can merge the 2 browsertests into one, but
in the
> absence of a merge it seems odd that we have to do so much mojo setup
for a
> browsertest that isn't actually doing anything with mojo

Done, I forgot to remove this part.

As I mentioned before, the reason I didn't add it to exist browser test

set, is because we need to set the feature state before the render
process is created, in order for it to inherit the feature state from
the browser process. That is to say, we can't config Finch Trial for

individual test case, we can only turn it on for a browser test set.

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode178
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:178:
void AddObserver(PageLoadTimingObserver* observer) {

It looks good.

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;

I prefer not to do it. I don't see clear benefit except saving some
typing and avoiding typemap, while the compiler discovers missing
typemap for us. And it is not scalable/extendable as far as I
understand.

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.h#newcode62
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:62:
mojom::PageLoadMetricsAssociatedPtr page_load_metrics_;

On 2017/04/26 16:04:34, Bryan McQuade wrote:
> Let's add a short comment explaining why we use a
PageLoadMetricsAssociatedPtr
> here (rather than a plain PageLoadMetrics).

Done.

They are two different concepts. A plain PageLoadMetrics means a
reference to a PageLoadMetrics implementation, a
PageLoadMetricsAssociatedPtr is one end of a mojo channel.

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h
File chrome/renderer/page_load_metrics/page_timing_sender.h (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h#newcode15
chrome/renderer/page_load_metrics/page_timing_sender.h:15: class
PageTimingSender {

On 2017/04/26 16:04:34, Bryan McQuade wrote:
> i like this abstraction, but let's do this slightly differently:
>
> let's define an IPCPageTimingSender, which uses an IPC::Sender*
internally, and
> a MojoPageTimingSender, which uses mojo.
>
> When instantiating a PageTimingSender, we do something like:
>
> std::unique_ptr<page_load_metrics::PageTimingSender>
CreateTimingSender() {
> if
(base::FeatureList::IsEnabled(features::kPageLoadMetricsMojofication))
> return new MojoPageTimingSender();
> else
> return new IPCPageTimingSender();
> }
> Additionally, you can use a MockPageTimingSender in unit tests, and
none of the
> unit tests need to know about mojo or IPC.

I assume the owner of PageTimingSender should be
MetricsRenderFrameObserver, since it has all information needed to send
two types of IPC. We will need to intercept the creation of
PageTimingSender in this case, and I don't have a clean way to do it.

> then none of the downstream code needs to know anything about IPC or
Mojo. I
> prefer this to having downstream code checking whether a feature is
enabled, as
> you have things currently.
>
> Over time if we kill the IPC impl I'd be fine with removing this class
and
> inlining the mojo class.
>
> My only concern is that the name of this class PageTimingSender is
very close to
> the name of PageTimingMetricsSender. Perhaps we should move your new
class's
> definition into PageTimingMetricsSender and call it
> PageTimingMetricsSender::SenderInterface (inner class) or
> PageTimingMetricsSenderEmbedderInterface, or something along those
lines?

IMHO, I think the name of PageTimingMetricsSender is misleading, it only
does batching.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks! I landed the refactor to break test deps on IPC. Can you sync and
incorporate that into your change, and then we can continue reviewing? Thanks!

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;

Ok. Just so I better understand, if we were writing this code from
scratch and had no legacy code, would you also use the typemapping
solution? Or do you only find it to be sensible because we already have
existing structs?

In general I strongly prefer avoiding duplication as much as possible,
so I do worry about this a bit. If someone adds a new field to the mojo
message they would now have to:
1. add it to the mojom file
2. add it to the struct traits
3. add it to the page_load_timing.h
4. add it to the relevant parts of page_load_timing.cc
5. update the IPC serialization code to include the field

That's a lot of code to have to update. If we removed the typemaps, we'd
at least get rid of 3-5.

I'm mildly opposed to the duplication but I think we can do this for
now. I'd like to remove the page_load_timing.h structs in a follow up CL
if possible though (I can do that, or you can). Do you see a reason not
to do that?

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h
File chrome/renderer/page_load_metrics/page_timing_sender.h (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/renderer/page_load_metrics/page_timing_sender.h#newcode15
chrome/renderer/page_load_metrics/page_timing_sender.h:15: class
PageTimingSender {

On 2017/04/27 at 10:58:35, lpy wrote:
> On 2017/04/26 16:04:34, Bryan McQuade wrote:
> > i like this abstraction, but let's do this slightly differently:
> >
> > let's define an IPCPageTimingSender, which uses an IPC::Sender*
internally, and
> > a MojoPageTimingSender, which uses mojo.
> >
> > When instantiating a PageTimingSender, we do something like:
> >
> > std::unique_ptr<page_load_metrics::PageTimingSender>
CreateTimingSender() {
> > if
(base::FeatureList::IsEnabled(features::kPageLoadMetricsMojofication))
> > return new MojoPageTimingSender();
> > else
> > return new IPCPageTimingSender();
> > }
> > Additionally, you can use a MockPageTimingSender in unit tests, and
none of the
> > unit tests need to know about mojo or IPC.
>
> I assume the owner of PageTimingSender should be
MetricsRenderFrameObserver, since it has all information needed to send
two types of IPC. We will need to intercept the creation of
PageTimingSender in this case, and I don't have a clean way to do it.

I'll have to look at the code more to understand why we can't do this
cleanly. Creating the sender once in MetricsRFO and then providing it to
each PageTimingMetricsSender as it is constructed seems very clean to
me. Is that not possible for some reason?

>
> > then none of the downstream code needs to know anything about IPC or
Mojo. I
> > prefer this to having downstream code checking whether a feature is
enabled, as
> > you have things currently.
> >
> > Over time if we kill the IPC impl I'd be fine with removing this
class and
> > inlining the mojo class.
> >
> > My only concern is that the name of this class PageTimingSender is
very close to
> > the name of PageTimingMetricsSender. Perhaps we should move your new
class's
> > definition into PageTimingMetricsSender and call it
> > PageTimingMetricsSender::SenderInterface (inner class) or
> > PageTimingMetricsSenderEmbedderInterface, or something along those
lines?
>
> IMHO, I think the name of PageTimingMetricsSender is misleading, it
only does batching.

I think you are right, with the factoring of the actual sending logic
out into your new interface, PageTimingMetricsSender is no longer
responsible for the sending. Its jobs are:
1. track updates to timing data on a per page load basis (the render
frame observer doesn't understand page lifetimes)
2. batch updates to avoid sending too frequently

Given these, what do you think we should rename PageTimingMetricsSender
to?

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Given that the lifetime of PageTimingMetricsSender is scoped to a committed
document's lifetime, perhaps we call it
CommittedDocumentMetricsCollector/Accumulator (or just
DocumentMetricsCollector/Accumulator/etc)?

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

On 2017/04/27 15:42:32, Bryan McQuade wrote:
> Thanks! I landed the refactor to break test deps on IPC. Can you sync and
> incorporate that into your change, and then we can continue reviewing? Thanks!

Great! Thanks! Will update the patch.

What is the IPC serialization code we need to update?

> I'm mildly opposed to the duplication but I think we can do this for now. I'd
> like to remove the page_load_timing.h structs in a follow up CL if possible
> though (I can do that, or you can). Do you see a reason not to do that?

It's not scalable/extendable. And we also need to have operations like IsEmpty
standalone in a separate file.
But I don't object here, a follow-up patch sounds good to me, and we can always
go back to typemap if we encounter problems.

The creation is very clear, the interception of creation is not clean.

We have `page_timing_sender_ = PageTimgingSender::CreatePageTimingSender(this,
routing_id(), render_frame());` somewhere, and in
MetricsRFO::DidCommitProvisionalLoad, we have:

page_timing_metrics_sender_.reset(
new PageTimingMetricsSender(page_timing_sender_.get(), CreateTimer(),
timing));

So I think we either wrap around `page_timing_sender_ =
PageTimgingSender::CreatePageTimingSender(this, routing_id(), render_frame());`
into a method and override it in test, or expose page_timing_sender_ through a
setter and set it to a mocked version in test. Both ways are not clean to me
though.

WDYT?

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

On 2017/04/27 at 15:42:32, bmcquade wrote:
> Thanks! I landed the refactor to break test deps on IPC. Can you sync and
incorporate that into your change, and then we can continue reviewing? Thanks!
>
>

I think you're overestimating the maintenance cost here.

If you were writing this from scratch and had no legacy code, you would not add
IPC serialization code in the first place, so step 5 is not there.

Meanwhile, without Mojo, if you were writing this from scratch and using legacy
IPC, you would still:

1. add it to page_load_timing.h
2. add it to the relevant parts of page_load_timing.cc
3. update the IPC serialization code to include the field

Adding to the mojom file is a trivial extra step, and updating StructTraits is
roughly equivalent to updating IPC serialization code. As soon as these types
are no longer used in legacy IPC messages, updating IPC serialization code is no
longer required (i.e. that code will be deleted.)

>
> I'm mildly opposed to the duplication but I think we can do this for now. I'd
like to remove the page_load_timing.h structs in a follow up CL if possible
though (I can do that, or you can). Do you see a reason not to do that?
>
>

> > > then none of the downstream code needs to know anything about IPC or Mojo.
I
> > > prefer this to having downstream code checking whether a feature is
enabled, as
> > > you have things currently.
> > >
> > > Over time if we kill the IPC impl I'd be fine with removing this class and
> > > inlining the mojo class.
> > >
> > > My only concern is that the name of this class PageTimingSender is very
close to
> > > the name of PageTimingMetricsSender. Perhaps we should move your new
class's
> > > definition into PageTimingMetricsSender and call it
> > > PageTimingMetricsSender::SenderInterface (inner class) or
> > > PageTimingMetricsSenderEmbedderInterface, or something along those lines?
> >
> > IMHO, I think the name of PageTimingMetricsSender is misleading, it only
does batching.
>
> I think you are right, with the factoring of the actual sending logic out into
your new interface, PageTimingMetricsSender is no longer responsible for the
sending. Its jobs are:
> 1. track updates to timing data on a per page load basis (the render frame
observer doesn't understand page lifetimes)
> 2. batch updates to avoid sending too frequently
>
> Given these, what do you think we should rename PageTimingMetricsSender to?

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Sure, I'm not sure anything you said is in disagreement with what I said. If we
are in a world with just mojo, we have:

1. update mojom file
2. update struct traits

If we're in a world where we have canonical struct definitions in the mojom file
and use those in both mojo and legacy IPC, we have:

1. update mojom file
2. update struct traits
3. update IPC serialization (page_load_metrics_messages.h)

If we're in a world where we have mojom and also typemap to legacy structs with
identical definitions, we then have the 1-5 I mentioned above.

This patch as written puts us in the last world where we have 5 steps. I'd like
us to be in the world where we have just 3 steps, and ideally once we confirm
that IPC and Mojo have the same behavior, we move to the first world where we
have just the 2 steps.

FWIW we have had real bugs emerge in this code due to the need to update
multiple code sites (such as IsEmpty() and Equals() implementations) in the
existing legacy structs when adding new fields, so I'm keen to reduce rather
than increase duplication / number of steps needed to add new fields to these
protos. Mojo does make it possible for us to reduce the number of places to
update code once we kill the legacy IPC side, which I see as a win. But I see it
as a loss if we're also typemapping to identical structs - more duplication =
more opportunity for bugs to creep in.

Peiyong points out that we don't have a definition of IsEmpty() in the mojo
structs. I'd be totally happy writing static helpers for this one case though.
And we get Equal() for free in the mojo structs, rather than having to write it
by hand in our typemapped structs, which is another nice improvement.

[l...@chromium.org]

I rebased the patch, ptal.

https://codereview.chromium.org/2823523003/

[roc...@chromium.org]

I have nothing new to add so FWIW this LGTM

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks! Here are a couple comments.

Looks like some of my comments from earlier rounds of reviews didn't get
addressed. Can you go through & reply to each so I know you didn't miss them,
and if you decided not to address them can you give an explanation why?


https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc#newcode19
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:19:
#include "chrome/browser/ui/browser.h"
looks like you can revert this file

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode175
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:175:
ASSERT_TRUE(
i think you can get rid of these assertions in each test. if you want
you could override SetUp() and verify that this assertion holds in that
method (SetUp is invoked once at the start of each test fixture)

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode49
chrome/common/page_load_metrics/page_load_metrics.mojom:49: int32
behavior_flags;
will this get a default value of zero? or do we need to write custom
init code to set it to zero by default?

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode1
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
i see unittests for other struct traits elsewhere in chromium code.
could we add some unittest coverage for these as well?

ideally, we'd have tests that do something like:
1. create a PageLoadTiming, where every field is set and each has a
distinct value
2. serialize it using mojo
3. unserialize it using mojo
4. verify that the resulting output is equal to the original

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

I addressed the latest comments and updated the patch.

I need to think about the PageTimingSender API again.

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode42
chrome/common/page_load_metrics/page_load_metrics.mojom:42:
DocumentTiming document_timing;

> I'm mildly opposed to the duplication but I think we can do this for
now. I'd
> like to remove the page_load_timing.h structs in a follow up CL if
possible
> though (I can do that, or you can). Do you see a reason not to do
that?

Acknowledged.

https://codereview.chromium.org/2823523003/diff/200001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode55
chrome/common/page_load_metrics/page_load_metrics.mojom:55:
UpdateTiming(PageLoadTiming page_load_timing, PageLoadMetadata
page_load_metadata);

On 2017/04/26 15:24:22, Bryan McQuade wrote:
> similarly, the generated code appears to be using
> page_load_metrics::PageLoadTiming rather than the mojom version you
declare in
> this file. It may just be that I don't fully understand how mojo
works, but on
> first glance it doesn't seem like we're using the types you declared
in this
> mojom file. If I'm misunderstanding please let me know - I'm still
trying to
> learn how mojo works. Thanks!

Acknowledged.

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc#newcode19
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:19:
#include "chrome/browser/ui/browser.h"

On 2017/05/03 19:03:05, Bryan McQuade wrote:
> looks like you can revert this file

We have two FailAllNetworkTransactions in browsertests, so I move them
to anonymous namespace.

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode175
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:175:
ASSERT_TRUE(

On 2017/05/03 19:03:05, Bryan McQuade wrote:
> i think you can get rid of these assertions in each test. if you want
you could
> override SetUp() and verify that this assertion holds in that method
(SetUp is
> invoked once at the start of each test fixture)

Done.

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics.mojom
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics.mojom#newcode49
chrome/common/page_load_metrics/page_load_metrics.mojom:49: int32
behavior_flags;

On 2017/05/03 19:03:05, Bryan McQuade wrote:
> will this get a default value of zero? or do we need to write custom
init code
> to set it to zero by default?

yes

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
File chrome/common/page_load_metrics/page_load_metrics_struct_traits.h
(right):

https://codereview.chromium.org/2823523003/diff/280001/chrome/common/page_load_metrics/page_load_metrics_struct_traits.h#newcode1
chrome/common/page_load_metrics/page_load_metrics_struct_traits.h:1: //
Copyright 2017 The Chromium Authors. All rights reserved.

On 2017/05/03 19:03:05, Bryan McQuade wrote:
> i see unittests for other struct traits elsewhere in chromium code.
could we add
> some unittest coverage for these as well?
>
> ideally, we'd have tests that do something like:
> 1. create a PageLoadTiming, where every field is set and each has a
distinct
> value
> 2. serialize it using mojo
> 3. unserialize it using mojo
> 4. verify that the resulting output is equal to the original

Done.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks! Here are a few more comments. RE: the PageTimingSender refactor, I think
that'll help to simplify renderer tests quite a bit - you should be able to get
rid of both FakePageLoadMetricsImpl and FakePageTimingSender if you provide an
interface for PageTimingSender with implementations for IPC and Mojo (as well as
a mock version for use in the unittest).


https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode150
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:150:
page_load_metrics_binding_for_testing() {
looks like this is no longer used. remove?

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode156
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:156:
friend class FakePageLoadMetrics;
can remove - this class doesn't appear to exist anymore

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode157
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157:
friend class MetricsWebContentsObserverTest;
can remove - don't need friend anymore

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode158
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:158:
friend class PageLoadMetricsObserverTestHarness;
can remove - don't need friend anymore

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode9
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9:

looks like this file can be reverted

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode9
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9:

looks like this file can be reverted

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode177
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:177:
ASSERT_TRUE(
looks like you can remove this one as well

https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc
File
chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc#newcode16
chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc:16:
TEST(DocumentTimingStructTraitsTest, Basic) {
Let's make all of these part of PageLoadMetricsStructTraitsTest with
more descriptive names for each than 'Basic', for example
TEST(PageLoadMetricsStructTraitsTest, DocumentTiming) {
...
}
TEST(PageLoadMetricsStructTraitsTest, PaintTiming) {
...
}

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode617
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:617:
content::RenderFrameHost* render_frame_host =
can we DCHECK that the mojofication feature is enabled here? and
likewise, dcheck that it's not enabled when the IPC codepath is hit?
that probably means adding an extra method besides OnTimingUpdated so
you have an IPC-specific method to dcheck in.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode617
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:617:
content::RenderFrameHost* render_frame_host =

On 2017/05/07 19:48:13, Bryan McQuade wrote:
> can we DCHECK that the mojofication feature is enabled here? and
likewise,
> dcheck that it's not enabled when the IPC codepath is hit? that
probably means
> adding an extra method besides OnTimingUpdated so you have an
IPC-specific
> method to dcheck in.

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode150
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:150:
page_load_metrics_binding_for_testing() {

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> looks like this is no longer used. remove?

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode156
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:156:
friend class FakePageLoadMetrics;

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> can remove - this class doesn't appear to exist anymore

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode157
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157:
friend class MetricsWebContentsObserverTest;

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> can remove - don't need friend anymore

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode158
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:158:
friend class PageLoadMetricsObserverTestHarness;

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> can remove - don't need friend anymore

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc#newcode9
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:9:

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> looks like this file can be reverted

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
File
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc#newcode177
chrome/browser/page_load_metrics/page_load_metrics_mojofication_browsertest.cc:177:
ASSERT_TRUE(

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> looks like you can remove this one as well

Done.

https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc
File
chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/300001/chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc#newcode16
chrome/common/page_load_metrics/page_load_metrics_struct_traits_unittest.cc:16:
TEST(DocumentTimingStructTraitsTest, Basic) {

On 2017/05/07 19:44:41, Bryan McQuade wrote:
> Let's make all of these part of PageLoadMetricsStructTraitsTest with
more
> descriptive names for each than 'Basic', for example
> TEST(PageLoadMetricsStructTraitsTest, DocumentTiming) {
> ...
> }
> TEST(PageLoadMetricsStructTraitsTest, PaintTiming) {
> ...
> }

Done. I don't think these names are more descriptive, so I made them to
TEST(PageLoadMetricsStructTraitsTest, DocumentTimingBasic).

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

Thanks! I think all that's left here is to refactor to have implementations of
PageTimingSender for IPC and for Mojo (and a gmock version in the unit test),
which should also allow you to get rid of FakePageLoadMetrics and
FakePageTimingSender.


https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode157
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157:
void OnUpdateTiming(content::RenderFrameHost* render_frame_host,
nit: just to clarify things for readers of the code, let's call this
OnUpdateTimingOverIPC. otherwise the name OnUpdateTiming is very similar
to OnTimingUpdated & could be confusing

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

I updated the PageTimingSender API, ptal.

https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2823523003/diff/320001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode157
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:157:
void OnUpdateTiming(content::RenderFrameHost* render_frame_host,

On 2017/05/08 21:13:29, Bryan McQuade wrote:
> nit: just to clarify things for readers of the code, let's call this
> OnUpdateTimingOverIPC. otherwise the name OnUpdateTiming is very
similar to
> OnTimingUpdated & could be confusing

Done.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

ping for review.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

[bmcq...@chromium.org]

Thanks! This is much easier to review with the initial mojo change landed.
Basically looks good - just some small things.


https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h
File chrome/renderer/page_load_metrics/fake_page_timing_sender.h
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h#newcode29
chrome/renderer/page_load_metrics/fake_page_timing_sender.h:29: // soon
use base::Optional which uses aligned memory, so we're forced to roll
this comment can be updated - it's out of date - to say "PageLoadTiming
uses base::Optional"

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h#newcode35
chrome/renderer/page_load_metrics/fake_page_timing_sender.h:35: class
PageTimingValidator {
thanks for factoring this out into its own class - cleaner this way.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h#newcode69
chrome/renderer/page_load_metrics/fake_page_timing_sender.h:69:
PageTimingValidator* validator_;
nit: since this is a constructor arg, can use
const PageTimingValidator* validator_;

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode173
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:173:
return PageTimingSender::CreatePageTimingSender(render_frame(),
routing_id());
given that the static PageTimingSender::CreatePageTimingSender method is
only called in this one place, I'd rather that we inline it into this
method, and move the IPC and Mojo sender impls to an anonymous namespace
in this file.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
File
chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc#newcode36
chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc:36:
base::WrapUnique<PageTimingSender>(
nit: we should use base::MakeUnique here instead. there's supposed to be
a linter warning about this.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc
File chrome/renderer/page_load_metrics/page_timing_sender.cc (right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode18
chrome/renderer/page_load_metrics/page_timing_sender.cc:18: class
LegacyIPCPageTimingSender : public IPC::Sender, public PageTimingSender
{
does this need to implement IPC::Sender anymore? seems like the Send()
method can just be inlined into SendTiming now

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode22
chrome/renderer/page_load_metrics/page_timing_sender.cc:22: :
render_frame_(render_frame), routing_id_(routing_id) {}
can the render frame ever be null? iirc we had some checks for this when
we used the concrete sender class in unit tests, but with your refactor,
maybe we can be certain that we never receive a null renderframe here?
you do dcheck this condition in the mojo sender so it seems like it's
probably a valid assumption for this class as well.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode32
chrome/renderer/page_load_metrics/page_timing_sender.cc:32: bool
Send(IPC::Message* message) override {
let's inline this up into SendTiming() above

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode35
chrome/renderer/page_load_metrics/page_timing_sender.cc:35: delete
message;
do we want to delete here? I was under the impression that the
RenderFrame takes ownership and manages the object lifetime

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode51
chrome/renderer/page_load_metrics/page_timing_sender.cc:51: if
(!page_load_metrics_) {
I'd rather initialize this in the constructor (instead of lazily in the
send method). That also allow us to avoid keeping render_frame_ as a
member.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

Thanks, I updated the patch.

+Tom@ for security review on mojo, please take a look.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h
File chrome/renderer/page_load_metrics/fake_page_timing_sender.h
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h#newcode29
chrome/renderer/page_load_metrics/fake_page_timing_sender.h:29: // soon
use base::Optional which uses aligned memory, so we're forced to roll

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> this comment can be updated - it's out of date - to say
"PageLoadTiming uses
> base::Optional"

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/fake_page_timing_sender.h#newcode69
chrome/renderer/page_load_metrics/fake_page_timing_sender.h:69:
PageTimingValidator* validator_;

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> nit: since this is a constructor arg, can use
> const PageTimingValidator* validator_;

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode173
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:173:
return PageTimingSender::CreatePageTimingSender(render_frame(),
routing_id());

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> given that the static PageTimingSender::CreatePageTimingSender method
is only
> called in this one place, I'd rather that we inline it into this
method, and
> move the IPC and Mojo sender impls to an anonymous namespace in this
file.

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
File
chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc
(right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc#newcode36
chrome/renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc:36:
base::WrapUnique<PageTimingSender>(

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> nit: we should use base::MakeUnique here instead. there's supposed to
be a
> linter warning about this.

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc
File chrome/renderer/page_load_metrics/page_timing_sender.cc (right):

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode18
chrome/renderer/page_load_metrics/page_timing_sender.cc:18: class
LegacyIPCPageTimingSender : public IPC::Sender, public PageTimingSender
{

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> does this need to implement IPC::Sender anymore? seems like the Send()
method
> can just be inlined into SendTiming now

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode22
chrome/renderer/page_load_metrics/page_timing_sender.cc:22: :
render_frame_(render_frame), routing_id_(routing_id) {}

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> can the render frame ever be null? iirc we had some checks for this
when we used
> the concrete sender class in unit tests, but with your refactor, maybe
we can be
> certain that we never receive a null renderframe here? you do dcheck
this
> condition in the mojo sender so it seems like it's probably a valid
assumption
> for this class as well.

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode32
chrome/renderer/page_load_metrics/page_timing_sender.cc:32: bool
Send(IPC::Message* message) override {

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> let's inline this up into SendTiming() above

Done.

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode35
chrome/renderer/page_load_metrics/page_timing_sender.cc:35: delete
message;

On 2017/05/19 14:06:19, Bryan McQuade wrote:
> do we want to delete here? I was under the impression that the
RenderFrame takes
> ownership and manages the object lifetime

https://cs.chromium.org/chromium/src/content/public/renderer/render_frame_observer.cc?l=36

https://codereview.chromium.org/2823523003/diff/460001/chrome/renderer/page_load_metrics/page_timing_sender.cc#newcode51
chrome/renderer/page_load_metrics/page_timing_sender.cc:51: if
(!page_load_metrics_) {

On 2017/05/19 14:06:18, Bryan McQuade wrote:
> I'd rather initialize this in the constructor (instead of lazily in
the send
> method). That also allow us to avoid keeping render_frame_ as a
member.

Done.

https://codereview.chromium.org/2823523003/

[tse...@chromium.org]

lgtm

RS - I saw only cosmetic changes in .mojom plus one manifest.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

LGTM, thanks! Just a couple small things.


https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode52
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52:
content::RenderFrame* render_frame_ = nullptr;
since this is set in the constructor, you can make it
const content::RenderFrame* render_frame_;
(and thus no need to set it to nullptr by default)

https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode59
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:59:
DCHECK(render_frame && render_frame->GetWebFrame());
we can omit the render_frame->GetWebFrame() part of the DCHECK since we
don't appear to depend on that anywhere.

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

Thanks, I addressed your comments, will land this after trybot goes green.

https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode52
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52:
content::RenderFrame* render_frame_ = nullptr;

On 2017/05/20 00:09:53, Bryan McQuade wrote:
> since this is set in the constructor, you can make it
> const content::RenderFrame* render_frame_;
> (and thus no need to set it to nullptr by default)

Done.

https://codereview.chromium.org/2823523003/diff/480001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode59
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:59:
DCHECK(render_frame && render_frame->GetWebFrame());

On 2017/05/20 00:09:53, Bryan McQuade wrote:
> we can omit the render_frame->GetWebFrame() part of the DCHECK since
we don't
> appear to depend on that anywhere.

Done.

https://codereview.chromium.org/2823523003/

[bmcq...@chromium.org]

https://codereview.chromium.org/2823523003/diff/500001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):

https://codereview.chromium.org/2823523003/diff/500001/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc#newcode52
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:52:
content::RenderFrame* const render_frame_;
ah, thanks, it's been a while - you got the const placement right here.
:)

https://codereview.chromium.org/2823523003/

[l...@chromium.org]

A revert of this CL (patchset #19 id:520001) has been created in
https://codereview.chromium.org/2897243002/ by l...@chromium.org.

The reason for reverting is: Revert this patch due to windows crash and we are
at the corner of branch day..

https://codereview.chromium.org/2823523003/