Convert PageLoadMetrics to Mojo (issue 2056153002 by tibell@chromium.org)

[tib...@chromium.org]

Reviewers: Sam McNally
CL: https://codereview.chromium.org/2056153002/

Message:
Sam, could you please take a first look before I send this out?

Description:
Convert PageLoadMetrics to Mojo

BUG=618522

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+277, -261 lines):
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
M components/components_tests.gyp
M components/page_load_metrics.gypi
M components/page_load_metrics/DEPS
M components/page_load_metrics/browser/BUILD.gn
M components/page_load_metrics/browser/DEPS
M components/page_load_metrics/browser/metrics_web_contents_observer.h
M components/page_load_metrics/browser/metrics_web_contents_observer.cc
M components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
M components/page_load_metrics/common/BUILD.gn
A + components/page_load_metrics/common/OWNERS
A components/page_load_metrics/common/page_load_metrics.mojom
A components/page_load_metrics/common/page_load_metrics.typemap
M components/page_load_metrics/common/page_load_metrics_messages.h
M components/page_load_metrics/renderer/BUILD.gn
M components/page_load_metrics/renderer/DEPS
A + components/page_load_metrics/renderer/fake_page_load_metrics.h
A + components/page_load_metrics/renderer/fake_page_load_metrics.cc
D components/page_load_metrics/renderer/fake_page_timing_metrics_ipc_sender.h
D components/page_load_metrics/renderer/fake_page_timing_metrics_ipc_sender.cc
M components/page_load_metrics/renderer/metrics_render_frame_observer.h
M components/page_load_metrics/renderer/metrics_render_frame_observer.cc
M components/page_load_metrics/renderer/metrics_render_frame_observer_unittest.cc
M components/page_load_metrics/renderer/page_timing_metrics_sender.h
M components/page_load_metrics/renderer/page_timing_metrics_sender.cc
M components/page_load_metrics/renderer/page_timing_metrics_sender_unittest.cc
M components/typemaps.gni

[sa...@chromium.org]

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.cc
File
components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode908
components/page_load_metrics/browser/metrics_web_contents_observer.cc:908:
registered_interfaces_.push_back(
Do you need to create the interface impls eagerly? Can you delay until a
request arrives?

These seem like they will accumulate as frames are created over the life
of each WebContents.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h
File
components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode260
components/page_load_metrics/browser/metrics_web_contents_observer.h:260:
class PageLoadMetricsImpl : public mojom::PageLoadMetrics {
Can you move this definition into the .cc file?

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode275
components/page_load_metrics/browser/metrics_web_contents_observer.h:275:
MetricsWebContentsObserver* observer_;
const

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode276
components/page_load_metrics/browser/metrics_web_contents_observer.h:276:
content::RenderFrameHost* host_;
const

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics.typemap
File components/page_load_metrics/common/page_load_metrics.typemap
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics.typemap#newcode10
components/page_load_metrics/common/page_load_metrics.typemap:10:
"//ipc",
Add "//components/page_load_metrics/common"

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h
File components/page_load_metrics/common/page_load_metrics_messages.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h#newcode1
components/page_load_metrics/common/page_load_metrics_messages.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
This file perhaps should be renamed to page_load_metrics_param_traits.h
now that there are no messages in it.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h#newcode12
components/page_load_metrics/common/page_load_metrics_messages.h:12:
#define IPC_MESSAGE_START PageLoadMetricsMsgStart
Remove this and remove PageLoadMetricsMsgStart from
ipc/ipc_message_start.h.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/renderer/page_timing_metrics_sender.cc
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/renderer/page_timing_metrics_sender.cc#newcode29
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:29:
DCHECK(page_load_metrics != nullptr);
DCHECK(page_load_metrics)

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

PTAL

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.cc
File
components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.cc#newcode908
components/page_load_metrics/browser/metrics_web_contents_observer.cc:908:
registered_interfaces_.push_back(

On 2016/07/07 05:10:48, Sam McNally wrote:
> Do you need to create the interface impls eagerly? Can you delay until
a request
> arrives?
>
> These seem like they will accumulate as frames are created over the
life of each
> WebContents.

Done.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h
File
components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode260
components/page_load_metrics/browser/metrics_web_contents_observer.h:260:
class PageLoadMetricsImpl : public mojom::PageLoadMetrics {

On 2016/07/07 05:10:48, Sam McNally wrote:
> Can you move this definition into the .cc file?

Done.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode275
components/page_load_metrics/browser/metrics_web_contents_observer.h:275:
MetricsWebContentsObserver* observer_;

On 2016/07/07 05:10:48, Sam McNally wrote:
> const
We call OnTimingUpdated, which isn't const.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/browser/metrics_web_contents_observer.h#newcode276
components/page_load_metrics/browser/metrics_web_contents_observer.h:276:
content::RenderFrameHost* host_;

On 2016/07/07 05:10:48, Sam McNally wrote:
> const

OnTimingUpdated takes a non-const RFH.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics.typemap
File components/page_load_metrics/common/page_load_metrics.typemap
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics.typemap#newcode10
components/page_load_metrics/common/page_load_metrics.typemap:10:
"//ipc",

On 2016/07/07 05:10:48, Sam McNally wrote:
> Add "//components/page_load_metrics/common"

Done.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h
File components/page_load_metrics/common/page_load_metrics_messages.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h#newcode1
components/page_load_metrics/common/page_load_metrics_messages.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.

On 2016/07/07 05:10:48, Sam McNally wrote:
> This file perhaps should be renamed to
page_load_metrics_param_traits.h now that
> there are no messages in it.

Done.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/common/page_load_metrics_messages.h#newcode12
components/page_load_metrics/common/page_load_metrics_messages.h:12:
#define IPC_MESSAGE_START PageLoadMetricsMsgStart

On 2016/07/07 05:10:48, Sam McNally wrote:
> Remove this and remove PageLoadMetricsMsgStart from
ipc/ipc_message_start.h.

Done.

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/renderer/page_timing_metrics_sender.cc
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_metrics/renderer/page_timing_metrics_sender.cc#newcode29
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:29:
DCHECK(page_load_metrics != nullptr);

On 2016/07/07 05:10:49, Sam McNally wrote:
> DCHECK(page_load_metrics)

Done.

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

+inferno for tools/ipc_fuzzer

https://codereview.chromium.org/2056153002/

[cshar...@chromium.org]

drive by: The page load metrics system relies heavily on subtle ordering with
the content API. We've hesitated to convert it to mojo in the past due to these
concerns.

One that we definitely need to ensure is ordered correctly is the timing sent
when a PageTimingMetricsSender is reset in
MetricsRenderFrameObserver::DidCommitProvisionalLoad.

That needs to arrive in the browser process *before* the RenderFrame sends the
DidCommitProvisionalLoad IPC.

https://codereview.chromium.org/2056153002/

[Bryan McQuade]

Thanks for looking at this. Did you reach out to any owners about taking this on before writing the code?

I believe we depend on the delivery order of the IPC sent from render to browser during commit and iiuc there isn't a way to maintain that order if moving to mojo. You can find more information on the timing dependency in the doc linked from the readme file.

If you want to continue with this change it may be worthwhile to set up a VC to discuss. It's important that we not change any behavior and I want to make sure we understand the effects of this change well, in particular how it may change delivery order relative to other ipcs, before moving forward with it.

On Thu, Jul 7, 2016, 9:21 PM <tib...@chromium.org> wrote:

+inferno for tools/ipc_fuzzer

https://codereview.chromium.org/2056153002/

--
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/94eb2c0bdaa6a3430c0537159d05%40google.com.

[tib...@chromium.org]

That's very useful. Thanks!

I'm doing this conversion (and a few others) as a member of the Mojo team to
help us understand how difficult it is to move to Mojo, which should help us
make it easier. Being able to move to Mojo without difficult reasoning about
whether you broke some delicate ordering invariant is important. We want to be
able to (relatively) easily say "this code has the same ordering as the old
code". Any input on whether this is easy to tell in this particular conversion
by someone who knows the code well would be most helpful.

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

I didn't reach out to the owners beforehand, although in perhaps I should have
to make sure I wasn't duplicating work. The main reasons for not reaching out
were:

* I want to know how difficult it would be to convert "blindly" to Mojo. If
it's easy volunteers could help out converting pieces of Chrome they're not
familiar with to Mojo. If it requires a deep understanding of the code in
question that's also something we'd like to know in the Mojo team (perhaps we
could make it easier [1]).

* I expected this to take less time than it did (I just transferred to
Chromium, so that added some learning time.).

My main goal here is to understand how difficult Mojo conversions are. If we
don't end up using this CL I still got most of I wanted out of it, although if
we can use it and save someone else the time (we'll eventually want to move
everything to Mojo) that would be a nice.

There is a way to maintain ordering between messages to different interfaces,
named "associated interfaces", but it's not possible to guarantee ordering
between Mojo messages and Chrome IPCs yet (it's being worked on).

Lets set up a VC. I think it would be useful regardless. Who should take part?

1. One thing I've been considering recently is whether we should reuse the same
pipe for all interfaces by default, to make the conversion keep the current
ordering semantics, and then relax this "over ordering" after we've moved to
Mojo. Just a thought for now, but it might help the transition.

On 2016/07/08 01:33:57, Bryan McQuade wrote:
> Thanks for looking at this. Did you reach out to any owners about taking
> this on before writing the code?
>
> I believe we depend on the delivery order of the IPC sent from render to
> browser during commit and iiuc there isn't a way to maintain that order if
> moving to mojo. You can find more information on the timing dependency in
> the doc linked from the readme file.
>
> If you want to continue with this change it may be worthwhile to set up a
> VC to discuss. It's important that we not change any behavior and I want to
> make sure we understand the effects of this change well, in particular how
> it may change delivery order relative to other ipcs, before moving forward
> with it.

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

Bryan, who would be good to VC with?

https://codereview.chromium.org/2056153002/

[cshar...@chromium.org]

On 2016/07/12 05:04:43, tibell wrote:
> Bryan, who would be good to VC with?

Hi tibell, feel free to schedule a VC with me, bmcquade@, and shivanisha@. We
can summarize discussion here or on the attached crbug.

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

I failed to find a good time overlap (our timezones are a bit incompatible). I
think the best way forward is to wait for rockot's CL to allow for ordering
between Chrome IPCs and Mojo IPCs (https://codereview.chromium.org/2164783005).
With this CL we should be able to just keep the guarantee of ordering we had
before.

https://codereview.chromium.org/2056153002/

[bmcq...@chromium.org]

On 2016/07/20 at 02:59:05, tibell wrote:
> I failed to find a good time overlap (our timezones are a bit incompatible). I
think the best way forward is to wait for rockot's CL to allow for ordering
between Chrome IPCs and Mojo IPCs (https://codereview.chromium.org/2164783005).
With this CL we should be able to just keep the guarantee of ordering we had
before.

Thanks! I have to say I'm a little hesitant to be an early adopter of mojo IPC
ordering, given how critical the accuracy of our metrics is to the rest of our
work. Launching most of our team's other performance optimization work is
dependent on confirming a positive impact on page load metrics. If the mojo
change were to introduce a bug that made the metrics report slightly incorrect
information, that'd set our entire team back and prevent us from being able to
launch the rest of our work. So my thinking is that while we want to migrate to
Mojo eventually, I think our low tolerance for risk does not make us a good
early candidate for migrating to mojo and trying out the mojo/IPC ordering
change. I'd like to let the mojo/IPC ordering change bake for a bit (maybe a
quarter) with other parts of Chromium code, then come back to this change once
we have some confidence that it's working well. Can we table this change for a
quarter and revisit it? I know this is too bad given you already wrote this
change - I think this is a case where reaching out to code owners before
starting this work could have saved you some time.

https://codereview.chromium.org/2056153002/

[tib...@chromium.org]

Sure, if you rather wait for a bit I'll just leave this CL sitting around until
you feel comfortable moving to Mojo.

https://codereview.chromium.org/2056153002/

[Bryan McQuade]

Sounds great thank you. Let's circle back in a quarter.