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(); |