[Page Load Metrics] Add mojom file to page load metrics.

[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259
Review-Url: https://codereview.chromium.org/2874663005
Cr-Commit-Position: refs/heads/master@{#472881}
Committed: https://chromium.googlesource.com/chromium/src/+/0619e89fda91cf9c1a540fb42c0193ff579216aa

[lpy, 2017-05-11 22:40:50]

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

[lpy, 2017-05-11 22:41:34]

Description was changed from

==========
[Page Load Metrics] Add mojom files to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

BUG=709259
==========

to

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

BUG=709259
==========

[commit-bot: I haz the power, 2017-05-11 22:42:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-11 23:51:04]

lpy@chromium.org changed reviewers:
+ bmcquade@chromium.org, rockot@chromium.org, zhenw@chromium.org

[lpy, 2017-05-11 23:51:05]

PTAL.

This patch adds mojom file and replace all *Timing type with mojom types.

[commit-bot: I haz the power, 2017-05-12 00:16:13]

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

[commit-bot: I haz the power, 2017-05-12 00:16:14]

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

[Bryan McQuade, 2017-05-12 01:32:51]

Thanks very much for this change! One request.

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // TODO(lpy): move
comments from page_load_timing.h to this file.
let's move existing comments from page_load_timing.h over here, since this is
now the canonical source.

[Bryan McQuade, 2017-05-12 01:34:04]

Description was changed from

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

BUG=709259
==========

to

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259
==========

[Bryan McQuade, 2017-05-12 01:59:37]

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
chrome/common/page_load_metrics/page_load_timing.h:15: using PageLoadTiming =
mojom::PageLoadTiming;
are these using statements needed?

[lpy, 2017-05-12 19:24:08]

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

[lpy, 2017-05-12 19:24:27]

Thanks, I updated the patch. PTAL.

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
chrome/common/page_load_metrics/page_load_metrics.mojom:9: // TODO(lpy): move
comments from page_load_timing.h to this file.
On 2017/05/12 01:32:51, Bryan McQuade wrote:
> let's move existing comments from page_load_timing.h over here, since this is
> now the canonical source.

Done.

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
chrome/common/page_load_metrics/page_load_timing.h:15: using PageLoadTiming =
mojom::PageLoadTiming;
On 2017/05/12 01:59:36, Bryan McQuade wrote:
> are these using statements needed?

It's used by other files including this file. So it's used, but not necessary.

[commit-bot: I haz the power, 2017-05-12 19:24:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-12 19:49:26]

Thanks!

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/1/chrome/common/page_load_met...
chrome/common/page_load_metrics/page_load_timing.h:15: using PageLoadTiming =
mojom::PageLoadTiming;
On 2017/05/12 at 19:24:27, lpy wrote:
> On 2017/05/12 01:59:36, Bryan McQuade wrote:
> > are these using statements needed?
> 
> It's used by other files including this file. So it's used, but not necessary.

Ok - I think I'd rather omit this. The google c++ style, which chromium's c++
style guide builds on, recommends against this:
"""
Don't put an alias in your public API just to save typing in the implementation;
do so only if you intend it to be used by your clients. When defining a public
alias, document the intent of the new name, including whether it is guaranteed
to always be the same as the type it's currently aliased to, or whether a more
limited compatibility is intended. This lets the user know whether they can
treat the types as substitutable or whether more specific rules must be
followed, and can help the implementation retain some degree of freedom to
change the alias.
"""
https://google.github.io/styleguide/cppguide.html#Aliases

My sense is that we're just doing this to save typing, which might be
worthwhile, but at the same time we updated most other code to specify the
mojo:: prefix already, which makes things inconsistent if some code uses the
non-mojom:: naming and others don't, so I'm inclined to remove the using
statements and be consistent everywhere.

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_metrics.mojom (right):

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_metrics.mojom:1: // Copyright 2017 The
Chromium Authors. All rights reserved.
thanks for updating this file! looks great now!

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_metrics_param_traits.cc (right):

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_metrics_param_traits.cc:10: namespace
IPC {
on looking at this more, i am wondering if we are better off in the end adding
generic ParamTraits for mojo::StructPtr<>. I'll defer to Ken on this but that
seems nicer to me.

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_metrics_param_traits.cc:12: void
ParamTraits<page_load_metrics::mojom::PageLoadTiming>::GetSize(
the current impl, which i put together quickly, assumes / requires that the
child structs are non-null. this is fine for our code - we basically guarantee
this everywhere - but i'm not sure if IPC folks would prefer us to be defensive
here and handle null gracefully. will defer to ken and others on this.

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:18:
mojo::StructPtr<mojom::PageLoadTiming> CreatePageLoadTiming();
we should just add a short comment here, explaining that it returns an empty
PageLoadTiming with initialized sub-members.

[commit-bot: I haz the power, 2017-05-12 20:08:20]

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

[commit-bot: I haz the power, 2017-05-12 20:08:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lpy, 2017-05-12 20:18:50]

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

[lpy, 2017-05-12 20:19:22]

thanks, I removed the alias.

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/20001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:18:
mojo::StructPtr<mojom::PageLoadTiming> CreatePageLoadTiming();
On 2017/05/12 19:49:26, Bryan McQuade wrote:
> we should just add a short comment here, explaining that it returns an empty
> PageLoadTiming with initialized sub-members.

Done.

[commit-bot: I haz the power, 2017-05-12 20:19:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-12 20:20:59]

page load metrics LGTM, thank you!

[commit-bot: I haz the power, 2017-05-12 21:57:15]

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

[commit-bot: I haz the power, 2017-05-12 21:57:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lpy, 2017-05-12 22:53:33]

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

[commit-bot: I haz the power, 2017-05-12 22:54:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:43:29]

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

[commit-bot: I haz the power, 2017-05-13 00:43:31]

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

[lpy, 2017-05-14 23:35:07]

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

[commit-bot: I haz the power, 2017-05-14 23:35:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 01:01:56]

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

[commit-bot: I haz the power, 2017-05-15 01:01:58]

Dry run: This issue passed the CQ dry run.

[lpy, 2017-05-15 16:44:24]

lpy@chromium.org changed reviewers:
+ pasko@chromium.org

[lpy, 2017-05-15 16:44:25]

+pasko@ for prerender/

Ken, gentle ping for review.

[Ken Rockot(use gerrit already), 2017-05-15 16:55:35]

On 2017/05/15 at 16:44:25, lpy wrote:
> +pasko@ for prerender/
> 
> Ken, gentle ping for review.

Sorry I missed this. Which parts would you like me to review?

[lpy, 2017-05-15 17:05:02]

On 2017/05/15 16:55:35, Ken Rockot wrote:
> On 2017/05/15 at 16:44:25, lpy wrote:
> > +pasko@ for prerender/
> > 
> > Ken, gentle ping for review.
> 
> Sorry I missed this. Which parts would you like me to review?

everything about mojo. :)

[Ken Rockot(use gerrit already), 2017-05-15 17:11:52]

On 2017/05/15 at 17:05:02, lpy wrote:
> On 2017/05/15 16:55:35, Ken Rockot wrote:
> > On 2017/05/15 at 16:44:25, lpy wrote:
> > > +pasko@ for prerender/
> > > 
> > > Ken, gentle ping for review.
> > 
> > Sorry I missed this. Which parts would you like me to review?
> 
> everything about mojo. :)

Can you narrow it down to some specific files? This is a very large CL.

[lpy, 2017-05-15 17:16:10]

On 2017/05/15 17:11:52, Ken Rockot wrote:
> On 2017/05/15 at 17:05:02, lpy wrote:
> > On 2017/05/15 16:55:35, Ken Rockot wrote:
> > > On 2017/05/15 at 16:44:25, lpy wrote:
> > > > +pasko@ for prerender/
> > > > 
> > > > Ken, gentle ping for review.
> > > 
> > > Sorry I missed this. Which parts would you like me to review?
> > 
> > everything about mojo. :)
> 
> Can you narrow it down to some specific files? This is a very large CL.

Could you review the usage of mojom types in chrome/renderer/page_load_metrics/
and the mojom file and IPC param traits in chrome/common/page_load_metrics
please? The rest of this patch uses the mojom types in the same way. Thanks.

[Bryan McQuade, 2017-05-15 17:18:52]

On 2017/05/15 at 17:11:52, rockot wrote:
> On 2017/05/15 at 17:05:02, lpy wrote:
> > On 2017/05/15 16:55:35, Ken Rockot wrote:
> > > On 2017/05/15 at 16:44:25, lpy wrote:
> > > > +pasko@ for prerender/
> > > > 
> > > > Ken, gentle ping for review.
> > > 
> > > Sorry I missed this. Which parts would you like me to review?
> > 
> > everything about mojo. :)
> 
> Can you narrow it down to some specific files? This is a very large CL.

Probably the following are most important:
* chrome/common/page_load_metrics/page_load_metrics.mojom
* chrome/common/page_load_metrics/page_load_metrics_messages.h
* chrome/common/page_load_metrics/page_load_metrics_param_traits.h
* chrome/common/page_load_metrics/page_load_metrics_param_traits.cc


Peiyong any others?

Ken, re: IPC param traits, we're interested to get your feedback on whether we
should just add a paramtraits for mojo::StructPointer, which would alleviate the
need for the custom param traits files we added.

[pasko, 2017-05-15 17:23:04]

chrome/browser/prerender lgtm with a question

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.cc:52: void
InitPageLoadTimingForTest(mojom::PageLoadTiming* timing) {
Not opposing this change, rather asking for self-education.

Having to always call this function on a newly constructed mojom::PageLoadTiming
looks more error-prone than it was before. Is it because mojoms cannot
default-construct nested mojoms? Was default-constructing hard to implement for
mojoms or the reasons are more subtle like it required bigger size binaries?

[Ken Rockot(use gerrit already), 2017-05-15 17:26:35]

lgtm

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:18: bool IsEmpty(const
mojom::DocumentTiming& timing);
This pattern makes me sad, but I defer to page_load_metrics owners

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:51: virtual
mojo::StructPtr<mojom::PageLoadTiming> GetTiming() const;
nit: please use mojom::PageLoadTimingPtr instead of
mojo::StructPtr<mojom::PageLoadTiming>, here and anywhere else you explicitly
name mojo::StructPtr<Foo>

[Ken Rockot(use gerrit already), 2017-05-15 17:27:49]

On 2017/05/15 at 17:18:52, bmcquade wrote:
> On 2017/05/15 at 17:11:52, rockot wrote:
> > On 2017/05/15 at 17:05:02, lpy wrote:
> > > On 2017/05/15 16:55:35, Ken Rockot wrote:
> > > > On 2017/05/15 at 16:44:25, lpy wrote:
> > > > > +pasko@ for prerender/
> > > > > 
> > > > > Ken, gentle ping for review.
> > > > 
> > > > Sorry I missed this. Which parts would you like me to review?
> > > 
> > > everything about mojo. :)
> > 
> > Can you narrow it down to some specific files? This is a very large CL.
> 
> Probably the following are most important:
> * chrome/common/page_load_metrics/page_load_metrics.mojom
> * chrome/common/page_load_metrics/page_load_metrics_messages.h
> * chrome/common/page_load_metrics/page_load_metrics_param_traits.h
> * chrome/common/page_load_metrics/page_load_metrics_param_traits.cc
> 
> 
> Peiyong any others?
> 
> Ken, re: IPC param traits, we're interested to get your feedback on whether we
should just add a paramtraits for mojo::StructPointer, which would alleviate the
need for the custom param traits files we added.

Please do not. We don't actually want people doing this in general.

[lpy, 2017-05-15 17:29:13]

On 2017/05/15 17:23:04, pasko wrote:
> chrome/browser/prerender lgtm with a question
> 
>
https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
> File chrome/common/page_load_metrics/page_load_timing.cc (right):
> 
>
https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
> chrome/common/page_load_metrics/page_load_timing.cc:52: void
> InitPageLoadTimingForTest(mojom::PageLoadTiming* timing) {
> Not opposing this change, rather asking for self-education.
> 
> Having to always call this function on a newly constructed
mojom::PageLoadTiming
> looks more error-prone than it was before. Is it because mojoms cannot
> default-construct nested mojoms? Was default-constructing hard to implement
for
> mojoms or the reasons are more subtle like it required bigger size binaries?

Thanks. As far as I understand, sub-member using Struct in mojom::PageLoadTiming
is a struct ptr and will be set to nullptr by default. I will defer to Ken for a
more detailed answer. Ken@, any idea?

[Bryan McQuade, 2017-05-15 17:34:08]

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:18: bool IsEmpty(const
mojom::DocumentTiming& timing);
On 2017/05/15 at 17:26:34, Ken Rockot wrote:
> This pattern makes me sad, but I defer to page_load_metrics owners

what about it makes you sad? happy to try to address concerns if we can. we do
need some way to know if these structs are "empty".

[Ken Rockot(use gerrit already), 2017-05-15 17:38:23]

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.cc:52: void
InitPageLoadTimingForTest(mojom::PageLoadTiming* timing) {
On 2017/05/15 at 17:23:04, pasko wrote:
> Not opposing this change, rather asking for self-education.
> 
> Having to always call this function on a newly constructed
mojom::PageLoadTiming looks more error-prone than it was before. Is it because
mojoms cannot default-construct nested mojoms? Was default-constructing hard to
implement for mojoms or the reasons are more subtle like it required bigger size
binaries?

Fields are default-constructed, but these fields are optional. The default
constructed state of an optional field is null.

Could we instead have a CreatePageLoadTimingForTest which returns a new instance
with these fields initialized?

[Bryan McQuade, 2017-05-15 17:38:24]

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:51: virtual
mojo::StructPtr<mojom::PageLoadTiming> GetTiming() const;
On 2017/05/15 at 17:26:34, Ken Rockot wrote:
> nit: please use mojom::PageLoadTimingPtr instead of
mojo::StructPtr<mojom::PageLoadTiming>, here and anywhere else you explicitly
name mojo::StructPtr<Foo>

FWIW, I worry a little bit about using PageLoadTimingPtr here. My assumption at
first when reading those types was that they were bare pointer types.
mojo::StructPtr<> OTOH makes it clearer that this is a templated pointer
management class of some kind. My pref would be to use mojo::StructPtr<> if it's
ok, as most of the people using this codebase aren't deeply familiar with mojo,
but if there's a reason it's problematic I'm fine with switching to
PageLoadTimingPtr.

[Bryan McQuade, 2017-05-15 17:40:26]

On 2017/05/15 at 17:38:23, rockot wrote:
>
https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
> File chrome/common/page_load_metrics/page_load_timing.cc (right):
> 
>
https://codereview.chromium.org/2874663005/diff/60001/chrome/common/page_load...
> chrome/common/page_load_metrics/page_load_timing.cc:52: void
InitPageLoadTimingForTest(mojom::PageLoadTiming* timing) {
> On 2017/05/15 at 17:23:04, pasko wrote:
> > Not opposing this change, rather asking for self-education.
> > 
> > Having to always call this function on a newly constructed
mojom::PageLoadTiming looks more error-prone than it was before. Is it because
mojoms cannot default-construct nested mojoms? Was default-constructing hard to
implement for mojoms or the reasons are more subtle like it required bigger size
binaries?
> 
> Fields are default-constructed, but these fields are optional. The default
constructed state of an optional field is null.
> 
> Could we instead have a CreatePageLoadTimingForTest which returns a new
instance with these fields initialized?

Are these fields optional? They're declared like 'DocumentTiming
document_timing;'. If there's a way to force construct child mojo members that'd
be ideal for us - we never want these to be null.

RE: CreatePageLoadTimingForTest() we can do it - lots of existing code uses non
pointer types but we could switch the tests over to pointer types. Why don't we
do this for the prerender_browsertest.cc for now, and we can consider making the
switch for all of the other tests in a subsequent change.

[pasko, 2017-05-15 17:50:07]

Description was changed from

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259
==========

to

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259
==========

[pasko, 2017-05-15 17:50:42]

fyi: this patch increases the size of android release official flavor of
libchrome.so by 1812 bytes in .text. Let me know if you are interested where
each byte is coming from :)

[Bryan McQuade, 2017-05-15 17:51:40]

On 2017/05/15 at 17:50:42, pasko wrote:
> fyi: this patch increases the size of android release official flavor of
libchrome.so by 1812 bytes in .text. Let me know if you are interested where
each byte is coming from :)

If it's not too much work, it'd be great to know where the increase comes from.
Thanks for helping to find this! How significant is an 1812 byte increase?

[Ken Rockot(use gerrit already), 2017-05-15 17:57:37]

On Mon, May 15, 2017 at 10:40 AM, <bmcquade@chromium.org> wrote:

> On 2017/05/15 at 17:38:23, rockot wrote:
> >
> https://codereview.chromium.org/2874663005/diff/60001/
> chrome/common/page_load_metrics/page_load_timing.cc
> > File chrome/common/page_load_metrics/page_load_timing.cc (right):
> >
> >
> https://codereview.chromium.org/2874663005/diff/60001/
> chrome/common/page_load_metrics/page_load_timing.cc#newcode52
> > chrome/common/page_load_metrics/page_load_timing.cc:52: void
> InitPageLoadTimingForTest(mojom::PageLoadTiming* timing) {
> > On 2017/05/15 at 17:23:04, pasko wrote:
> > > Not opposing this change, rather asking for self-education.
> > >
> > > Having to always call this function on a newly constructed
> mojom::PageLoadTiming looks more error-prone than it was before. Is it
> because
> mojoms cannot default-construct nested mojoms? Was default-constructing
> hard to
> implement for mojoms or the reasons are more subtle like it required
> bigger size
> binaries?
> >
> > Fields are default-constructed, but these fields are optional. The
> default
> constructed state of an optional field is null.
> >
> > Could we instead have a CreatePageLoadTimingForTest which returns a new
> instance with these fields initialized?
>
> Are these fields optional? They're declared like 'DocumentTiming
> document_timing;'. If there's a way to force construct child mojo members
> that'd
> be ideal for us - we never want these to be null.
>
> RE: CreatePageLoadTimingForTest() we can do it - lots of existing code
> uses non
> pointer types but we could switch the tests over to pointer types. Why
> don't we
> do this for the prerender_browsertest.cc for now, and we can consider
> making the
> switch for all of the other tests in a subsequent change.
>

Ah, I was reading the wrong definition, and I was also looking at the wrong
generated code bits too. All struct fields are default initialized to null
even for non-nullable fields. We should probably fix that, but for now WDYT
about having a CreateFooForTesting method instead oa  separate Initialize
call?

re the IsEmpty pattern, it's just a (bad) slippery slope argument from my
end, so I'm not pushing it hard. There's not a better solution at the
moment if you really want to use the mojom types everywhere. This wouldn't
scale well to even slightly more complex use cases (more types, or more
common methods spanning them.)

Finally re StructPtr, it's just harder to read Verbose<TemplateCode> and I
don't think the assumptions about Ptr == * are really an issue in practice
(it's just as reasonable to assume they're e.g. a unique_ptr, which they
are in fact quite close to.) We've made the same recommendations for FooPtr
vs InterfacePtr<Foo> and FooRequest vs InterfaceRequest<Foo> and it would
be nice if this code followed the same pattern.


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

-- 
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.

[pasko, 2017-05-15 18:20:21]

On 2017/05/15 17:51:40, Bryan McQuade wrote:
> On 2017/05/15 at 17:50:42, pasko wrote:
> > fyi: this patch increases the size of android release official flavor of
> libchrome.so by 1812 bytes in .text. Let me know if you are interested where
> each byte is coming from :)
> 
> If it's not too much work, it'd be great to know where the increase comes
from.

hm, the tooling I am using (go/crbug698733) is not great:
* does not match in amount of size increase (exaggerates by 3x)
* there is a huge number of per-line differences in libc++ inlines (200 meg text
files with a 17k text diff)
* obviously need a more coarse grained view of this

maybe the new diff tool from agrieve@ can help?

let me know if you want these 200meg files, it takes minutes to open them with
vimdiff

> Thanks for helping to find this! How significant is an 1812 byte increase?

we have not formalized the significance of it yet :/

I know that we increase by ~1MiB every release, and it's about 10K commits, so
it would be roughly 100 bytes per commit, which makes this commit 18 times more
alarming than usual? This is a very unfair estimation because we have tons of
reverts in each release.

[Bryan McQuade, 2017-05-15 18:23:44]

On 2017/05/15 at 18:20:21, pasko wrote:
> On 2017/05/15 17:51:40, Bryan McQuade wrote:
> > On 2017/05/15 at 17:50:42, pasko wrote:
> > > fyi: this patch increases the size of android release official flavor of
> > libchrome.so by 1812 bytes in .text. Let me know if you are interested where
> > each byte is coming from :)
> > 
> > If it's not too much work, it'd be great to know where the increase comes
from.
> 
> hm, the tooling I am using (go/crbug698733) is not great:
> * does not match in amount of size increase (exaggerates by 3x)
> * there is a huge number of per-line differences in libc++ inlines (200 meg
text files with a 17k text diff)
> * obviously need a more coarse grained view of this
> 
> maybe the new diff tool from agrieve@ can help?
> 
> let me know if you want these 200meg files, it takes minutes to open them with
vimdiff
> 
> > Thanks for helping to find this! How significant is an 1812 byte increase?
> 
> we have not formalized the significance of it yet :/
> 
> I know that we increase by ~1MiB every release, and it's about 10K commits, so
it would be roughly 100 bytes per commit, which makes this commit 18 times more
alarming than usual? This is a very unfair estimation because we have tons of
reverts in each release.

Thanks for the analysis! This is not an area I understand deeply, but I'll note
that, once we run a field trial and confirm switching to Mojo doesn't result in
any significant behavior changes (no significant diffs in aggregate metrics),
we'll delete the IPC-half of this code, including the custom param traits etc,
which should hopefully cut down on binary size (not sure if it'll impact .text
specifically though).

[lpy, 2017-05-15 19:41:14]

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

[commit-bot: I haz the power, 2017-05-15 19:41:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 19:47:19]

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

[commit-bot: I haz the power, 2017-05-15 19:47:20]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lpy, 2017-05-15 21:58:01]

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

[commit-bot: I haz the power, 2017-05-15 21:58:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-15 22:40:18]

Bryan, I rebased the code, could you please take a look at it again please?

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (right):

https://codereview.chromium.org/2874663005/diff/60001/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:51: virtual
mojo::StructPtr<mojom::PageLoadTiming> GetTiming() const;
On 2017/05/15 17:26:34, Ken Rockot wrote:
> nit: please use mojom::PageLoadTimingPtr instead of
> mojo::StructPtr<mojom::PageLoadTiming>, here and anywhere else you explicitly
> name mojo::StructPtr<Foo>

Done.

[commit-bot: I haz the power, 2017-05-16 01:24:36]

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

[commit-bot: I haz the power, 2017-05-16 01:24:38]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-16 01:58:55]

Thanks for the update! I went through the whole change. Everything look right to
me - just found two small things to update. Thank you!

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc:44:
bool WasFakeTimingConsumed() const { return IsEmpty(*fake_timing_); }
it appears we never actually call this method, so i guess you can just remove
it. thanks!

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

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:33: // Send an
initial IPC relatively early to help track aborts.
looks like this comment was removed in a recent change. you can remove it here.

[lpy, 2017-05-16 17:27:36]

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

[commit-bot: I haz the power, 2017-05-16 17:28:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-16 17:29:29]

Patchset #7 (id:120001) has been deleted

[lpy, 2017-05-16 17:30:22]

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

[commit-bot: I haz the power, 2017-05-16 17:31:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-16 18:08:55]

Thanks, I updated the patch.

I will land it when all bots are green if there's no objection.

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
(right):

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc:44:
bool WasFakeTimingConsumed() const { return IsEmpty(*fake_timing_); }
On 2017/05/16 01:58:55, Bryan McQuade wrote:
> it appears we never actually call this method, so i guess you can just remove
> it. thanks!

Done.

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

https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:33: // Send an
initial IPC relatively early to help track aborts.
On 2017/05/16 01:58:55, Bryan McQuade wrote:
> looks like this comment was removed in a recent change. you can remove it
here.

Done.

[Bryan McQuade, 2017-05-16 20:04:41]

On 2017/05/16 at 18:08:55, lpy wrote:
> Thanks, I updated the patch.
> 
> I will land it when all bots are green if there's no objection.
> 
>
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> File
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
>
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc:44:
bool WasFakeTimingConsumed() const { return IsEmpty(*fake_timing_); }
> On 2017/05/16 01:58:55, Bryan McQuade wrote:
> > it appears we never actually call this method, so i guess you can just
remove
> > it. thanks!
> 
> Done.
> 
>
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right):
> 
>
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:33: // Send an
initial IPC relatively early to help track aborts.
> On 2017/05/16 01:58:55, Bryan McQuade wrote:
> > looks like this comment was removed in a recent change. you can remove it
here.
> 
> Done.

Sure, let's land, thank you!

[Bryan McQuade, 2017-05-16 20:17:38]

On 2017/05/16 at 20:04:41, Bryan McQuade wrote:
> On 2017/05/16 at 18:08:55, lpy wrote:
> > Thanks, I updated the patch.
> > 
> > I will land it when all bots are green if there's no objection.
> > 
> >
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> > File
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc
(right):
> > 
> >
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> >
chrome/renderer/page_load_metrics/metrics_render_frame_observer_unittest.cc:44:
bool WasFakeTimingConsumed() const { return IsEmpty(*fake_timing_); }
> > On 2017/05/16 01:58:55, Bryan McQuade wrote:
> > > it appears we never actually call this method, so i guess you can just
remove
> > > it. thanks!
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> > File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc
(right):
> > 
> >
https://codereview.chromium.org/2874663005/diff/100001/chrome/renderer/page_l...
> > chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:33: // Send
an initial IPC relatively early to help track aborts.
> > On 2017/05/16 01:58:55, Bryan McQuade wrote:
> > > looks like this comment was removed in a recent change. you can remove it
here.
> > 
> > Done.
> 
> Sure, let's land, thank you!

I think you may need to rebase once more, as there were some changes in
https://codereview.chromium.org/2885053002 that are likely to conflict with this
patch.

[commit-bot: I haz the power, 2017-05-16 20:52:26]

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

[commit-bot: I haz the power, 2017-05-16 20:52:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lpy, 2017-05-17 21:44:40]

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

[lpy, 2017-05-17 21:45:07]

Patchset #8 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-05-17 21:46:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-17 21:47:40]

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

[commit-bot: I haz the power, 2017-05-17 21:49:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-17 21:49:55]

Thanks Bryan, I updated the patch, please take another look. The main changes
are in page load tracker and web contents observer.

[Bryan McQuade, 2017-05-18 00:10:07]

lgtm, but one small request. thanks for this change!

https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:730: mojom::PaintTimingPtr
new_paint_timing = new_timing.paint_timing->Clone();
do we need to do this? can't we just continue to pass
new_timing.new_paint_timing below, and avoid this clone?

[lpy, 2017-05-18 01:48:15]

Thanks, I updated it, will land this patch when waterfall is green again.

https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:730: mojom::PaintTimingPtr
new_paint_timing = new_timing.paint_timing->Clone();
On 2017/05/18 00:10:06, Bryan McQuade wrote:
> do we need to do this? can't we just continue to pass
> new_timing.new_paint_timing below, and avoid this clone?

Done.

[Bryan McQuade, 2017-05-18 02:33:22]

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

[Bryan McQuade, 2017-05-18 02:34:17]

On 2017/05/18 at 01:48:15, lpy wrote:
> Thanks, I updated it, will land this patch when waterfall is green again.
> 
>
https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/page_load_tracker.cc (right):
> 
>
https://codereview.chromium.org/2874663005/diff/180001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/page_load_tracker.cc:730:
mojom::PaintTimingPtr new_paint_timing = new_timing.paint_timing->Clone();
> On 2017/05/18 00:10:06, Bryan McQuade wrote:
> > do we need to do this? can't we just continue to pass
> > new_timing.new_paint_timing below, and avoid this clone?
> 
> Done.

Sounds good, thanks! I just sent this to the CQ dryrun so it's ready to submit
when waterfall is green.

[commit-bot: I haz the power, 2017-05-18 02:35:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 05:22:00]

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

[commit-bot: I haz the power, 2017-05-18 05:22:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lpy, 2017-05-18 05:34:46]

The CQ bit was checked by lpy@chromium.org

[lpy, 2017-05-18 05:34:47]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
rockot@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2874663005/#ps200001
(title: "Remove unnecessary variable")

[commit-bot: I haz the power, 2017-05-18 05:35:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2017-05-18 05:44:21]

lpy@chromium.org changed reviewers:
+ dcheng@chromium.org

[lpy, 2017-05-18 05:44:23]

+dcheng@ for security owner review on chrome/common/page_load_metrics/

dcheng@, ptal.

[commit-bot: I haz the power, 2017-05-18 05:48:35]

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

[commit-bot: I haz the power, 2017-05-18 05:48:37]

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

[lpy, 2017-05-18 16:52:39]

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

[Tom Sepez, 2017-05-18 18:20:38]

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

[Tom Sepez, 2017-05-18 18:20:40]

mojom LGTM

[lpy, 2017-05-18 18:25:51]

lpy@chromium.org changed reviewers:
- dcheng@chromium.org, nasko@chromium.org

[lpy, 2017-05-18 18:26:07]

The CQ bit was checked by lpy@chromium.org

[commit-bot: I haz the power, 2017-05-18 18:27:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 18:38:22]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495131967300430,
"parent_rev": "0b08cea76dc559dfd830933baf444db4438b9825", "commit_rev":
"ed1aee16e4ae34e76d25c07bc7c570e61780e680"}

[commit-bot: I haz the power, 2017-05-18 18:38:44]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495131967300430,
"parent_rev": "bd66f55270c94cae7c3579da95294a6f56dce60d", "commit_rev":
"0619e89fda91cf9c1a540fb42c0193ff579216aa"}

[commit-bot: I haz the power, 2017-05-18 18:38:57]

Description was changed from

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259
==========

to

==========
[Page Load Metrics] Add mojom file to page load metrics.

This patch adds page_load_metrics.mojom to PLM to use in services in the future,
and replaces all *Timing struct usage with mojom types.

A subsequent patch based on https://codereview.chromium.org/2823523003 will
add support for sending page load metrics over Mojo. We'll run a field trial
to verify there are no changes in metrics due to Mojo, then cut over to Mojo
and remove the IPC code.

BUG=709259

Review-Url: https://codereview.chromium.org/2874663005
Cr-Commit-Position: refs/heads/master@{#472881}
Committed:
https://chromium.googlesource.com/chromium/src/+/0619e89fda91cf9c1a540fb42c01...
==========

[commit-bot: I haz the power, 2017-05-18 18:39:00]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/0619e89fda91cf9c1a540fb42c01...