PageLoadMetrics renderer and browser implementation (deprecated)

components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h

Index: components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
diff --git a/components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h b/components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..7004506e4354299db2107b357a75e456f140e807
--- /dev/null
+++ b/components/page_load_metrics/renderer/page_load_metrics_render_frame_observer.h
@@ -0,0 +1,62 @@
+// Copyright 2015 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 COMPONENTS_PAGE_LOAD_METRICS_RENDERER_PAGE_LOAD_METRICS_RENDER_FRAME_OBSERVER_H_
+#define COMPONENTS_PAGE_LOAD_METRICS_RENDERER_PAGE_LOAD_METRICS_RENDER_FRAME_OBSERVER_H_
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
+#include "content/public/renderer/render_frame_observer.h"
+
+namespace page_load_metrics {
+
+// PageLoadMetricsRenderFrameObserver observes RenderFrame notifications, and
+// sends page load timing information to the browser process over IPC.
+class PageLoadMetricsRenderFrameObserver : public content::RenderFrameObserver {
+ public:
+  // The Timing interface provides access to the timing metrics we send to the
+  // browser process.
+  class Timing {

[nasko, 2015/08/20 00:22:18]

I'm finding it is hard to read the code in the implementation of
PageLoadMetricsRenderFrameObserver because of the number of additional classes.
Is it possible to simplify this a bit? Especially since Timing is just a very
trivial wrapper around blink::WebPerformance.

[Bryan McQuade, 2015/08/25 13:53:36]

Agree - I ended up implementing Kinuko's suggestion to send the whole timing
structure when it changes, which had some nice additional benefits:
* makes it easy to batch multiple updates that happen in a short time period, to
reduce IPC contention
* alleviates the need to have this wrapper class
* simplifies the sender side of the implementation

+   public:
+    Timing() {}
+    virtual ~Timing() {}
+    virtual base::Time GetNavigationStart() const = 0;
+    virtual base::Time GetDOMContentLoadedEvent() const = 0;
+    virtual base::Time GetLoadEvent() const = 0;
+    virtual base::Time GetFirstLayout() const = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Timing);
+  };
+
+  explicit PageLoadMetricsRenderFrameObserver(
+      content::RenderFrame* render_frame);
+  ~PageLoadMetricsRenderFrameObserver() override;
+
+  // RenderFrameObserver implementation
+  void DidFinishDocumentLoad() override;
+  void DidFinishLoad() override;
+  void DidStartProvisionalLoad() override;
+  void DidCommitProvisionalLoad(bool is_new_navigation,
+                                bool is_same_page_navigation) override;
+
+ protected:
+  virtual bool ShouldSendMetrics() const;
+  virtual scoped_ptr<Timing> CreateTiming();
+
+ private:
+  class PageMetricsSender;
+
+  // Will be null when we're not actively sending metrics.
+  scoped_ptr<PageMetricsSender> page_metrics_sender_;
+
+  void MaybeSendMetrics();

[nasko, 2015/08/20 00:22:18]

nit: Why the Maybe prefix? I think callers should always call SendMetrics and
whether it does a send for real or not is implementation detail, not something
that should be surfaced in the name of the method.

[Bryan McQuade, 2015/08/25 13:53:36]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(PageLoadMetricsRenderFrameObserver);
+};
+
+}  // namespace page_load_metrics
+
+#endif  // COMPONENTS_PAGE_LOAD_METRICS_RENDERER_PAGE_LOAD_METRICS_RENDER_FRAME_OBSERVER_H_