[PageLoadMetrics] Keep track of Ad Sizes on Pages

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h

Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..5709588193df2329016db08e8b6db4c0acf908c9
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
@@ -0,0 +1,77 @@
+// 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_PAGE_LOAD_METRICS_OBSERVERS_ADS_PAGE_LOAD_METRICS_OBSERVER_H_
+#define CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ADS_PAGE_LOAD_METRICS_OBSERVER_H_
+
+#include <list>
+#include <memory>
+#include <unordered_map>
+
+#include "base/macros.h"
+#include "chrome/browser/page_load_metrics/page_load_metrics_observer.h"
+#include "net/http/http_response_info.h"
+
+class AdsPageLoadMetricsObserver
+    : public page_load_metrics::PageLoadMetricsObserver {
+ public:
+  // Returns a new AdsPageLoadMetricObserver. If the feature is disabled it
+  // returns nullptr.
+  static std::unique_ptr<AdsPageLoadMetricsObserver> CreateIfNeeded();
+
+  AdsPageLoadMetricsObserver();
+  ~AdsPageLoadMetricsObserver() override;
+
+  // page_load_metrics::PageLoadMetricsObserver:s

[Charlie Harrison, 2017/04/14 20:16:26]

nit: remove the s.

[jkarlin, 2017/04/24 17:27:59]

Done.

+  ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
+  ObservePolicy OnCommitSubFrame(
+      content::NavigationHandle* navigation_handle) override;
+  ObservePolicy FlushMetricsOnAppEnterBackground(
+      const page_load_metrics::PageLoadTiming& timing,
+      const page_load_metrics::PageLoadExtraInfo& extra_info) override;
+  void OnLoadedResource(
+      const page_load_metrics::ExtraRequestInfo& extra_request_info) override;
+  void OnComplete(const page_load_metrics::PageLoadTiming& timing,
+                  const page_load_metrics::PageLoadExtraInfo& info) override;
+
+ private:
+  struct AdFrameData {
+    size_t frame_bytes;
+    size_t frame_bytes_uncached;
+  };
+
+  using FrameTreeNodeId = int;
+
+  void RecordHistograms();
+
+  // Checks to see if a resource is waiting for |frame_tree_node_id| to commit

[Charlie Harrison, 2017/04/14 20:16:26]

for a navigation with the given |frame_tree_node_id| to commit

[jkarlin, 2017/04/24 17:27:59]

Done.

+  // before it can be processed. If so, call OnLoadedResource for the delayed
+  // resource.
+  void ProcessOngoingNavigationResources(FrameTreeNodeId frame_tree_node_id);

[Bryan McQuade, 2017/04/17 22:00:04]

nit: when i read this name, which has Resources plural in the name, I assumed it
was going to process multiple resources. It only processes one. I see now the
name is derived from the member it operates on, but I think it's clearer to name
it based on what it does, which is processes the single ongoing resource for the
given frame tree node id. Perhaps ProcessOngoingNavigationResourceForFrame or
just ProcessOngoingNavigationResource (singular)?

[jkarlin, 2017/04/24 17:27:59]

Done.

+
+  // Stores the size data of each ad frame. Pointed to by ad_frames_ so use a
+  // data structure that won't move the data around.
+  std::list<AdFrameData> ad_frames_data_storage_;

[Bryan McQuade, 2017/04/17 22:00:04]

My sense is that ad_frames_data_storage_ and ad_frames_data_ could be wrapped up
in a helper class that hides some common logic. What do you think of something
like:

class AdFrameDataManager {
 public:
  const std::list<AdFrameData>& ad_frame_data() const;

  bool HasEntry(FrameTreeNodeId id) const;

  // Registers the given frame if not already registered, by associating it with
  // AdFrameData for an ancestor ad frame if any, then returns the associated
  // AdFrameData, or nullptr.
  AdFrameData* RegisterFrameAndGetData(content::RenderFrameHost* host);

  // Creates a new AdFrameData for the given frame. This method should only be
  // called if RegisterFrameAndGetData returns nullptr for the given frame.
  AdFrameData* CreateAdFrameData(FrameTreeNodeId id);

  // (if return false, the frame for the given ExtraRequestInfo is not yet
known)
  bool ProcessLoadedResource(const ExtraRequestInfo& info);

 private:
  // Stores the size data of each ad frame ...
  std::list<AdFrameData> ad_frames_data_storage_;

  // Maps a frame (by id) to the AdFrameData responsible for ...
  std::unordered_map<FrameTreeNodeId, AdFrameData*> ad_frames_data_;
};

This API can probably be cleaned up a bit, but I think this will simplify code
that currently interacts with these fields directly - those call sites can
instead focus on their policy and not worry about managing / updating these
structs as well. WDYT?

OnSubFrameCommit might look something like:

if (manager.HasEntry(id)) {
  // An existing subframe is navigating again.
  // ... rest of repeat frame commit code here ...
}

AdFrameData* data = manager.RegisterFrameAndGetData(frame_host);

if (!data && FrameIsAd(...))
  data = manager.CreateAdFrameData(id);

[jkarlin, 2017/04/24 17:27:59]

I think pretty much all of the logic would wind up in this new class. We'd also
need iterators over all of the ad frames (or to return the
ad_frames_data_storage_) so we could report metrics. Can we postpone a
refactoring to a followup? 

[Bryan McQuade, 2017/04/24 19:28:22]

Sure - I'm fine with that.

+
+  // Maps a frame (by id) to the AdFrameData responsible for the frame. The
+  // responsible frame is the top-most frame labeled as an ad in the frame's

[Bryan McQuade, 2017/04/14 19:19:23]

it sounds like it's possible for multiple FrameTreeNodeIds to point at the same
AdFrameData* - is that right? If so, might be worth stating this explicitly in
the comment.

[jkarlin, 2017/04/24 17:27:59]

Done.

+  // ancestry, which may be itself. If no responsible frame is found, the data
+  // is nullptr.
+  std::unordered_map<FrameTreeNodeId, AdFrameData*> ad_frames_data_;
+
+  // When the observer receives report of a document resource loading for a
+  // sub-frame before the sub-frame commit occurs, hold onto the resource
+  // request info (delay it) until the sub-frame commits.
+  std::unordered_map<FrameTreeNodeId, page_load_metrics::ExtraRequestInfo>
+      ongoing_navigation_resources_;
+
+  size_t page_bytes_ = 0u;
+  size_t uncached_page_bytes_ = 0u;
+  int top_level_subframe_count_ = 0;
+  int top_level_ad_frame_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(AdsPageLoadMetricsObserver);
+};
+
+#endif  // CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ADS_PAGE_LOAD_METRICS_OBSERVER_H_