Add a way to send the resource percentage signal to the RC.

chrome/browser/offline_pages/background_loader_offliner.cc

Index: chrome/browser/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/offline_pages/background_loader_offliner.cc b/chrome/browser/offline_pages/background_loader_offliner.cc
index 6cc5f3d0fd5de7c4d71000927dc4948a3f6a4bcc..2fbded36be60c571becc27c22e336c430ac62319 100644
--- a/chrome/browser/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/offline_pages/background_loader_offliner.cc
@@ -12,6 +12,8 @@
 #include "base/time/time.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
+#include "chrome/browser/offline_pages/offliner_user_data.h"
+#include "chrome/browser/page_load_metrics/page_load_metrics_initialize.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/save_page_request.h"
@@ -33,28 +35,6 @@ const char kContentTransferEncodingBinary[] =
     "Content-Transfer-Encoding: binary";
 const char kXHeaderForSignals[] = "X-Chrome-Loading-Metrics-Data: 1";
 
-class OfflinerData : public content::WebContentsUserData<OfflinerData> {
- public:
-  static void AddToWebContents(content::WebContents* webcontents,
-                               BackgroundLoaderOffliner* offliner) {
-    DCHECK(offliner);
-    webcontents->SetUserData(UserDataKey(), std::unique_ptr<OfflinerData>(
-                                                new OfflinerData(offliner)));
-  }
-
-  explicit OfflinerData(BackgroundLoaderOffliner* offliner) {
-    offliner_ = offliner;
-  }
-  BackgroundLoaderOffliner* offliner() { return offliner_; }
-
- private:
-  // The offliner that the WebContents is attached to. The offliner owns the
-  // Delegate which owns the WebContents that this data is attached to.
-  // Therefore, its lifetime should exceed that of the WebContents, so this
-  // should always be non-null.
-  BackgroundLoaderOffliner* offliner_;
-};
-
 std::string AddHistogramSuffix(const ClientId& client_id,
                                const char* histogram_name) {
   if (client_id.name_space.empty()) {
@@ -105,9 +85,12 @@ BackgroundLoaderOffliner::~BackgroundLoaderOffliner() {}
 // static
 BackgroundLoaderOffliner* BackgroundLoaderOffliner::FromWebContents(
     content::WebContents* contents) {
-  OfflinerData* data = OfflinerData::FromWebContents(contents);
-  if (data)
-    return data->offliner();
+  Offliner* offliner = OfflinerUserData::OfflinerFromWebContents(contents);
+
+  // Today we only have one kind of offliner that uses OfflinerUserData.  If we
+  // add other types, revisit this cast.
+  if (offliner)
+    return static_cast<BackgroundLoaderOffliner*>(offliner);

[chili, 2017/05/17 18:00:07]

just out of curiosity, what would happen if you do static_cast<>(nullptr)?

[Pete Williamson, 2017/05/24 01:01:50]

You get a nullptr with a C++ type "BackgroundLoaderOffliner"

   return nullptr;
 }
 
@@ -238,6 +221,15 @@ bool BackgroundLoaderOffliner::HandleTimeout(int64_t request_id) {
   return false;
 }
 
+void BackgroundLoaderOffliner::ObserveResourceTracking(
+    const ResourceDataType type,
+    int64_t started_count,
+    int64_t completed_count) {
+  // Add the signal to extra data, and use for tracking.
+  if (type == ResourceDataType::IMAGE)
+    AddResourceSignal(type, started_count, completed_count);
+}
+
 void BackgroundLoaderOffliner::MarkLoadStartTime() {
   load_start_time_ = base::TimeTicks::Now();
 }
@@ -465,7 +457,11 @@ void BackgroundLoaderOffliner::ResetLoader() {
 void BackgroundLoaderOffliner::AttachObservers() {
   content::WebContents* contents = loader_->web_contents();
   content::WebContentsObserver::Observe(contents);
-  OfflinerData::AddToWebContents(contents, this);
+  OfflinerUserData::AddToWebContents(contents, this);
+
+  // Attach the metrics observers to the web contents so we can get resoure
+  // loading signals.
+  chrome::InitializePageLoadMetricsForWebContents(contents);

[chili, 2017/05/17 18:00:07]

Do we want ALL the metrics observers? Assuming there's not going to be much
overhead

[Charlie Harrison, 2017/05/17 18:30:31]

+1 can you explain the consequences of this change? Will all of our clients see
volume changes in metrics?

[RyanSturm, 2017/05/17 18:36:58]

I believe the navigation should be entirely in the background, but any
foreground/background agnostic metrics would change.

petewil, can you verify that all events in these cases will be marked as
background  and OnStart started_in_foreground is false.

It's a good point that we may want to add an extra param to
chrome::InitializePageLoadMetricsForWebContents for this if we think it will
drastically (or incorrectly) change the population.

[RyanSturm, 2017/05/17 18:47:54]

1 more thing. Is the background offliner a replacement for the pre-render code,
and if so is the pre-render code path already causing the PLM observers to be
added?

[Pete Williamson, 2017/05/17 19:50:07]

Chili: We do want to use several metrics observers, not just the resource
tracking page load metrics observer.  We eventually want to use the First
Meaningful Paint and Time To Interactive metric.  Foreground web tabs always
call AttachTabHelpers and get all of the observers every time (though our
ResourceTrackingPageLoadMetrics will be turned off for them).

Charlie: This will turn on observers only for background offlining pages. 
Normally, AttachTabHelpers should turn these on for all foreground pages
already, this change just adds the observers for background loaded offline
pages.

Ryan: 
1) all events will be in the background.  We do this by creating a web contents
with no associated tab.  We don't explicitly set started_in_foreground anywhere.
 Does that answer your question?

2) Background offliner is indeed a replacement for the prerenderer.  I'm not
hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we hope
to replace it soon.

[RyanSturm, 2017/05/17 19:54:32]

WRT to point 2.

My question was a little more subtle. I had thought that pre-renderer
navigations were already tracked in PLM. If those offline pre-renderer
navigations are already included by the current offlining logic, then nothing
changes by including the navigations in the new background offlining logic.

[Charlie Harrison, 2017/05/17 19:59:07]

Most observers don't want to be notified of prerenders:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo...

I'm a little bit nervous about initializing all observers on WebContents that
aren't associated with tabs and don't have the standard tab helpers (some
observers have dependencies on other tab helpers).

If possible, it might make sense to add more logic in PageLoadMetrics
initialization to do what we do today for prerender.

+bmcquade for thoughts as well.

[Pete Williamson, 2017/05/17 20:04:57]

OK, let me clarify a bit and see if that helps:
Today we have two different offliners.  We use the prerenderer, and if a certain
flag is set, we use the BackgroundLoaderOffliner instead.  It seems that the
Prerender is already calling AttachTabHelpers, which turns on all the
PageLoadMetrics observers.  This change turns on the observers, but only for the
BackgroundLoaderOffliner.

[Pete Williamson, 2017/05/17 20:09:08]

CHarrison: Ah, I didn't realize that some observers needed the tab helpers.  In
that case, we probably want to limit which observers are used when we use the
BackgroundLoaderOffliner.  Unfortunately, unlike the pre-renderer, the system is
generally not aware of when the BackgroundLoaderOffliner is in use, we'd have to
add some detection in PageLoadMetricsInitialize.  Does adding an additional
argument to InitializePageLoadMetricsForWebContents seem reasonable to you?

[Charlie Harrison, 2017/05/17 20:14:30]

Yeah I think it makes sense to align as closely with prerender as possible in
this case. I think most of our observers just care about normal page loads.

[Pete Williamson, 2017/05/17 22:01:15]

CSHarrison: Done.

[Pete Williamson, 2017/05/24 01:01:50]

We only want the ones we use, and InitializePLMFWC() only starts the ones we
use, not all of them.

 }
 
 void BackgroundLoaderOffliner::OnApplicationStateChange(
@@ -493,6 +489,15 @@ void BackgroundLoaderOffliner::AddLoadingSignal(const char* signal_name) {
   signal_data_.SetDouble(signal_name, delay);
 }
 
-}  // namespace offline_pages
+void BackgroundLoaderOffliner::AddResourceSignal(const ResourceDataType type,
+                                                 int64_t started_count,
+                                                 int64_t completed_count) {
+  double percentage = 100.0 * static_cast<double>(completed_count) /
+                      static_cast<double>(started_count);
+  // TODO(petewil): Use actual signal type instead of hardcoding name to image.
+  signal_data_.SetDouble("ImagePercentage", percentage);
+  signal_data_.SetDouble("StartedImages", started_count);
+  signal_data_.SetDouble("CompletedImages", completed_count);
+}
 
-DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);
+}  // namespace offline_pages