Convert PageLoadMetrics to Mojo

Convert PageLoadMetrics to Mojo

BUG=618522

[tibell, 2016-06-15 05:13:26]

Description was changed from

==========
Convert PageLoadMetrics to Mojo

BUG=618522
TESTED=//components/page_load_metrics_common:unit_tests
==========

to

==========
Convert PageLoadMetrics to Mojo

BUG=618522
TESTED=components_unittests
==========

[tibell, 2016-07-07 00:00:24]

Description was changed from

==========
Convert PageLoadMetrics to Mojo

BUG=618522
TESTED=components_unittests
==========

to

==========
Convert PageLoadMetrics to Mojo

BUG=618522
==========

[tibell, 2016-07-07 03:00:06]

tibell@chromium.org changed reviewers:
+ sammc@chromium.org

[tibell, 2016-07-07 03:00:07]

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

[Sam McNally, 2016-07-07 05:10:49]

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

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:275:
MetricsWebContentsObserver* observer_;
const

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
File components/page_load_metrics/common/page_load_metrics.typemap (right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
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_m...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

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

[tibell, 2016-07-08 01:12:53]

PTAL

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

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
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_m...
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_m...
File components/page_load_metrics/common/page_load_metrics.typemap (right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
File components/page_load_metrics/common/page_load_metrics_messages.h (right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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_m...
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_m...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/2056153002/diff/260001/components/page_load_m...
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.

[tibell, 2016-07-08 01:19:36]

tibell@chromium.org changed reviewers:
+ dcheng@chromium.org, kinuko@chromium.org

[tibell, 2016-07-08 01:21:01]

tibell@chromium.org changed reviewers:
+ inferno@chromium.org

[tibell, 2016-07-08 01:21:02]

+inferno for tools/ipc_fuzzer

[Charlie Harrison, 2016-07-08 01:27:25]

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

[Charlie Harrison, 2016-07-08 01:27:26]

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.

[tibell, 2016-07-08 01:33:30]

On 2016/07/08 01:27:26, csharrison wrote:
> 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.

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.

[Bryan McQuade, 2016-07-08 01:33:57]

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 <tibell@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-reviews+unsubscribe@chromium.org.
> To post to this group, send email to loading-reviews@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c0bdaa6...
>
<https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c0bdaa6...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tibell, 2016-07-08 01:52:51]

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.

[tibell, 2016-07-12 05:04:43]

Bryan, who would be good to VC with?

[Charlie Harrison, 2016-07-12 13:44:19]

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.

[tibell, 2016-07-20 02:54:43]

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

[commit-bot: I haz the power, 2016-07-20 02:55:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-07-20 02:59:05]

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.

[commit-bot: I haz the power, 2016-07-20 03:03:16]

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

[commit-bot: I haz the power, 2016-07-20 03:03:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Bryan McQuade, 2016-07-20 12:05:55]

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.

[tibell, 2016-07-21 00:14:10]

On 2016/07/20 12:05:55, Bryan McQuade wrote:
> 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.

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

[Bryan McQuade, 2016-07-21 13:00:35]

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

On Wed, Jul 20, 2016, 8:14 PM <tibell@chromium.org> wrote:

> On 2016/07/20 12:05:55, Bryan McQuade wrote:
> > 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.
>
> 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/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bryan McQuade, 2016-08-04 11:36:20]

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

[Bryan McQuade, 2016-08-04 11:36:47]

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

[Bryan McQuade, 2016-08-04 11:36:48]

Dropping myself as reviewer since we're putting this on hold for a bit. Please
re-add me when we're ready to revisit this, thanks!

[Bryan McQuade, 2016-08-04 11:36:59]

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

[Charlie Harrison, 2017-05-06 01:50:50]

Removing myself from reviewer, this should probably be closed as it's superceded
by https://codereview.chromium.org/2823523003/

[Charlie Harrison, 2017-05-06 01:54:33]

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