[Page Load Metrics] PageLoadMetrics Mojofication.

chrome/renderer/page_load_metrics/page_timing_sender.h

Index: chrome/renderer/page_load_metrics/page_timing_sender.h
diff --git a/chrome/renderer/page_load_metrics/page_timing_sender.h b/chrome/renderer/page_load_metrics/page_timing_sender.h
new file mode 100644
index 0000000000000000000000000000000000000000..4415d140d4ab3faaf4f90f41b71ebfae86898c2f
--- /dev/null
+++ b/chrome/renderer/page_load_metrics/page_timing_sender.h
@@ -0,0 +1,23 @@
+// 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.
+
+#ifndef CHROME_RENDERER_PAGE_LOAD_METRICS_PAGE_TIMING_SENDER_H_
+#define CHROME_RENDERER_PAGE_LOAD_METRICS_PAGE_TIMING_SENDER_H_
+
+#include "chrome/common/page_load_metrics/page_load_timing.h"
+
+namespace page_load_metrics {
+
+// PageTimingSender is an interface that is responsible for sending page load
+// timing. SendTiming() should be implemented to send page load timing over
+// mojo IPC.
+class PageTimingSender {

[Bryan McQuade, 2017/04/26 16:04:34]

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?

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

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.
IMHO, I think the name of PageTimingMetricsSender is misleading, it only does
batching.

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

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

+ public:
+  virtual void SendTiming(const PageLoadTiming& timing,
+                          const PageLoadMetadata& metadata) = 0;
+};
+
+}  // namespace page_load_metrics
+
+#endif  // CHROME_RENDERER_PAGE_LOAD_METRICS_PAGE_TIMING_SENDER_H_