Create LoadingDataCollector class and have observers rely on it instead of ResourcePrefetchPredictor

chrome/browser/predictors/glowplug_collector.h

Index: chrome/browser/predictors/glowplug_collector.h
diff --git a/chrome/browser/predictors/glowplug_collector.h b/chrome/browser/predictors/glowplug_collector.h
new file mode 100644
index 0000000000000000000000000000000000000000..08c168a36a4637d860b218b2b98f8dd2c24e8bad
--- /dev/null
+++ b/chrome/browser/predictors/glowplug_collector.h
@@ -0,0 +1,55 @@
+// 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_BROWSER_PREDICTORS_GLOWPLUG_COLLECTOR_H_
+#define CHROME_BROWSER_PREDICTORS_GLOWPLUG_COLLECTOR_H_
+
+#include "chrome/browser/predictors/resource_prefetch_predictor.h"
+#include "content/public/common/resource_type.h"
+
+namespace net {
+class URLRequest;
+}
+
+namespace predictors {
+
+// Records navigation events as reported by various observers to the database

[pasko, 2017/05/23 12:24:59]

Does this say that the observers of the database record navigation events? I
thought it's the other way around - navigation observers are used for recording
events and updating the database.

[trevordixon, 2017/06/13 11:40:02]

I'll refactor the sentence to make the meaning clear in a followup CL.

+// and stats collection classes. All the non-static methods of this class need
+// to be called on the UI thread.
+class GlowplugCollector : public base::SupportsWeakPtr<GlowplugCollector> {

[pasko, 2017/05/23 12:24:59]

please try to avoid base::SupportsWeakPtr. See a discussion about it:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/LNK43wiinNo/nsUX2...

if there is absolutely no way to avoid weak refs, WeakPtrFactory is considered
more explicit - less surprising with inheritance, so it makes sense to just
convert to it for consistency reasons, even if no further inheritance for this
class is planned

[trevordixon, 2017/06/13 11:40:02]

Sorry for missing this comment! Thankfully Alex took care of this and other
similar instances. https://chromium-review.googlesource.com/c/530747/

+ public:
+  explicit GlowplugCollector(predictors::ResourcePrefetchPredictor* predictor);

[alexilin, 2017/05/23 08:24:37]

nit:
Sorry that I didn't notice it the first time, but you don't have to specify
`predictors::` namespace because you're already inside it.
So you just could use `ResourcePrefetchPredictor` here and
`ResourcePrefetchPredictor::URLRequestSummary` below.

[trevordixon, 2017/06/13 11:40:02]

Sorry for missing this comment. I'll fix this in a followup CL.

+  ~GlowplugCollector();
+
+  // Thread safe.
+  static bool ShouldRecordRequest(net::URLRequest* request,
+                                  content::ResourceType resource_type);
+  static bool ShouldRecordResponse(net::URLRequest* response);
+  static bool ShouldRecordRedirect(net::URLRequest* response);
+
+  // 'ResourcePrefetchPredictorObserver' calls the below functions to inform the
+  // predictor of main frame and resource requests. Should only be called if the
+  // corresponding Should* functions return true.
+  void RecordURLRequest(
+      const predictors::ResourcePrefetchPredictor::URLRequestSummary& request);
+  void RecordURLResponse(
+      const predictors::ResourcePrefetchPredictor::URLRequestSummary& response);
+  void RecordURLRedirect(
+      const predictors::ResourcePrefetchPredictor::URLRequestSummary& response);
+
+  // Called when the main frame of a page completes loading.
+  void RecordMainFrameLoadComplete(const NavigationID& navigation_id);
+
+  // Called after the main frame's first contentful paint.
+  void RecordFirstContentfulPaint(
+      const NavigationID& navigation_id,
+      const base::TimeTicks& first_contentful_paint);
+
+ private:
+  predictors::ResourcePrefetchPredictor* const predictor_;
+};
+
+}  // namespace predictors
+
+#endif  // CHROME_BROWSER_PREDICTORS_GLOWPLUG_COLLECTOR_H_