Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4041)

Unified Diff: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

Issue 2861433005: [PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts (Closed)
Patch Set: Fix Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
index f6a08a182e620ff14f156528597e3cf507d7550c..01fb0749d0d0c48b212c79301ee6fc9158160bcf 100644
--- a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
@@ -19,25 +19,18 @@
namespace {
-const base::Feature kAdsFeature{"AdsMetrics",
- base::FEATURE_DISABLED_BY_DEFAULT};
-
-bool FrameIsAd(content::NavigationHandle* navigation_handle) {
- content::RenderFrameHost* current_frame_host =
- navigation_handle->GetRenderFrameHost();
- DCHECK(current_frame_host);
- const std::string& name = current_frame_host->GetFrameName();
- const GURL& url = navigation_handle->GetURL();
+const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};
Charlie Harrison 2017/05/04 20:21:14 Please mention in the description that this CL re-
jkarlin 2017/05/05 19:56:28 Done.
+bool FrameIsAd(const GURL& url, const std::string& frame_name) {
// Because sub-resource filtering isn't always enabled, and doesn't work
// well in monitoring mode (no CSS enforcement), it's difficult to identify
// ads. Google ads are prevalent and easy to track, so we'll start by
// tracking those. Note that the frame name can be very large, so be careful
// to avoid full string searches if possible.
// TODO(jkarlin): Track other ad networks that are easy to identify.
- return base::StartsWith(name, "google_ads_iframe",
+ return base::StartsWith(frame_name, "google_ads_iframe",
base::CompareCase::SENSITIVE) ||
- base::StartsWith(name, "google_ads_frame",
+ base::StartsWith(frame_name, "google_ads_frame",
base::CompareCase::SENSITIVE) ||
(url.host_piece() == "tpc.googlesyndication.com" &&
base::StartsWith(url.path_piece(), "/safeframe",
@@ -79,17 +72,29 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
content::NavigationHandle* navigation_handle) {
FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
- if (!navigation_handle->HasCommitted() ||
- navigation_handle->IsSameDocument() || navigation_handle->IsErrorPage()) {
+ if ((!navigation_handle->HasCommitted() ||
+ navigation_handle->IsSameDocument() ||
+ navigation_handle->IsErrorPage()) &&
+ navigation_handle->GetNetErrorCode() != net::ERR_ABORTED) {
Charlie Harrison 2017/05/04 20:21:14 #include net errors
jkarlin 2017/05/05 19:56:28 Done.
// We're not interested in tracking this navigation. In case we've seen a
Charlie Harrison 2017/05/04 20:21:14 IMO pulling this whole comment above the if statem
jkarlin 2017/05/05 19:56:28 Done.
// resource for the navigation before this message, clear it from
// ongoing_navigation_resources_.
+ //
+ // Note that we *do* keep track of net::ERR_ABORTED navigations because
+ // the frame can continue to load resources (either from the previous
+ // navigation or from a document written by doc.write which aborted the
+ // navigation).
Charlie Harrison 2017/05/04 20:21:14 I imagine this is not the only way the navigation
jkarlin 2017/05/05 19:56:28 Done.
ongoing_navigation_resources_.erase(frame_tree_node_id);
return CONTINUE_OBSERVING;
}
- content::RenderFrameHost* parent_frame_host =
- navigation_handle->GetRenderFrameHost()->GetParent();
+ // Get the RFH from the frame tree node id because
+ // NavigationHandle->GetRenderFrameHost doesn't like being called for
Charlie Harrison 2017/05/04 20:21:14 Slightly awkward wording. It's helpful to differen
jkarlin 2017/05/05 19:56:28 Done.
+ // uncomitted navigations (e.g., due to an abort).
+ content::RenderFrameHost* current_frame_host =
+ navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId(
+ frame_tree_node_id);
+ content::RenderFrameHost* parent_frame_host = current_frame_host->GetParent();
DCHECK(parent_frame_host);
bool top_level_subframe = !parent_frame_host->GetParent();
@@ -105,7 +110,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
// This is the top-most frame in the ad.
UMA_HISTOGRAM_BOOLEAN(
"PageLoad.Clients.Ads.Google.Navigations.AdFrameRenavigatedToAd",
- FrameIsAd(navigation_handle));
+ FrameIsAd(navigation_handle->GetURL(),
+ current_frame_host->GetFrameName()));
}
return CONTINUE_OBSERVING;
@@ -114,7 +120,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
// any child frames that were ads, those will still be recorded.
UMA_HISTOGRAM_BOOLEAN(
"PageLoad.Clients.Ads.Google.Navigations.NonAdFrameRenavigatedToAd",
- FrameIsAd(navigation_handle));
+ FrameIsAd(navigation_handle->GetURL(),
+ current_frame_host->GetFrameName()));
} else if (top_level_subframe) {
top_level_subframe_count_ += 1;
}
@@ -125,7 +132,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
DCHECK(parent_id_and_data != ad_frames_data_.end());
AdFrameData* ad_data = parent_id_and_data->second;
- if (!ad_data && FrameIsAd(navigation_handle)) {
+ if (!ad_data && FrameIsAd(navigation_handle->GetURL(),
+ current_frame_host->GetFrameName())) {
// This frame is not nested within an ad frame but is itself an ad.
ad_frames_data_storage_.emplace_back(frame_tree_node_id);
ad_data = &ad_frames_data_storage_.back();

Powered by Google App Engine
This is Rietveld 408576698