[Page Load Metrics] PageLoadMetrics Mojofication.

chrome/common/page_load_metrics/page_load_metrics.mojom

Index: chrome/common/page_load_metrics/page_load_metrics.mojom
diff --git a/chrome/common/page_load_metrics/page_load_metrics.mojom b/chrome/common/page_load_metrics/page_load_metrics.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..13b8d18db582c5b6e6d474702310ffb4c8b4b8af
--- /dev/null
+++ b/chrome/common/page_load_metrics/page_load_metrics.mojom
@@ -0,0 +1,56 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module page_load_metrics.mojom;
+
+import "mojo/common/time.mojom";
+
+// See comments in page_load_timing.h for details on each field.
+
+struct DocumentTiming {
+  mojo.common.mojom.TimeDelta? dom_content_loaded_event_start;
+  mojo.common.mojom.TimeDelta? load_event_start;
+  mojo.common.mojom.TimeDelta? first_layout;
+};
+
+struct PaintTiming {
+  mojo.common.mojom.TimeDelta? first_paint;
+  mojo.common.mojom.TimeDelta? first_text_paint;
+  mojo.common.mojom.TimeDelta? first_image_paint;
+  mojo.common.mojom.TimeDelta? first_contentful_paint;
+  mojo.common.mojom.TimeDelta? first_meaningful_paint;
+};
+
+struct ParseTiming {
+  mojo.common.mojom.TimeDelta? parse_start;
+  mojo.common.mojom.TimeDelta? parse_stop;
+  mojo.common.mojom.TimeDelta? parse_blocked_on_script_load_duration;
+  mojo.common.mojom.TimeDelta? parse_blocked_on_script_load_from_document_write_duration;
+  mojo.common.mojom.TimeDelta? parse_blocked_on_script_execution_duration;
+  mojo.common.mojom.TimeDelta? parse_blocked_on_script_execution_from_document_write_duration;
+};
+
+struct StyleSheetTiming {
+  mojo.common.mojom.TimeDelta? author_style_sheet_parse_duration_before_fcp;
+  mojo.common.mojom.TimeDelta? update_style_duration_before_fcp;
+};
+
+struct PageLoadTiming {
+  mojo.common.mojom.Time navigation_start;
+  mojo.common.mojom.TimeDelta? response_start;
+  DocumentTiming document_timing;

[Bryan McQuade, 2017/04/26 15:24:22]

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?

[Ken Rockot(use gerrit already), 2017/04/26 15:28:45]

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.

[Bryan McQuade, 2017/04/26 20:41:51]

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?

[lpy, 2017/04/27 10:58:35]

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.

[Bryan McQuade, 2017/04/27 15:42:32]

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?

[lpy, 2017/05/05 17:49:30]

Acknowledged.

+  PaintTiming paint_timing;
+  ParseTiming parse_timing;
+  StyleSheetTiming style_sheet_timing;
+};
+
+struct PageLoadMetadata {
+  int32 behavior_flags;
+};
+
+// Sent from renderer to browser process when the PageLoadTiming for the
+// associated frame changed.
+interface PageLoadMetrics {
+  UpdateTiming(PageLoadTiming page_load_timing, PageLoadMetadata page_load_metadata);

[Bryan McQuade, 2017/04/26 15:24:22]

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!

[lpy, 2017/05/05 17:49:30]

Acknowledged.

+};