Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc |
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc |
index 2d085ad371a7bb731f13f20d34c7413770ec8e3d..087d91dbd94d1c9bffa833f90eeb70bb0f271fef 100644 |
--- a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc |
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc |
@@ -8,11 +8,16 @@ |
#include "base/macros.h" |
#include "base/test/histogram_tester.h" |
+#include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h" |
#include "chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h" |
#include "chrome/browser/page_load_metrics/page_load_metrics_observer.h" |
#include "chrome/browser/page_load_metrics/page_load_tracker.h" |
+#include "content/public/browser/global_request_id.h" |
+#include "content/public/browser/navigation_handle.h" |
+#include "content/public/browser/navigation_throttle.h" |
#include "content/public/browser/render_frame_host.h" |
#include "content/public/browser/web_contents.h" |
+#include "content/public/browser/web_contents_observer.h" |
#include "content/public/common/resource_type.h" |
#include "content/public/test/navigation_simulator.h" |
#include "content/public/test/test_renderer_host.h" |
@@ -34,6 +39,78 @@ const char kNonAdUrl2[] = "https://bar.com/"; |
const char kAdName[] = "google_ads_iframe_1"; |
const char kNonAdName[] = "foo"; |
+class DelayWillProcessResponseObserver; |
+ |
+// Delays WillProcessResponse until the caller tells it to cancel. |
Charlie Harrison
2017/05/17 18:54:51
Would you add a TODO(csharrison) to add this to th
jkarlin
2017/05/18 13:50:13
Done.
|
+class DelayWillProcessResponseThrottle : public content::NavigationThrottle { |
+ public: |
+ explicit DelayWillProcessResponseThrottle( |
+ content::NavigationHandle* navigation_handle) |
+ : NavigationThrottle(navigation_handle) {} |
+ |
+ // NavigationThrottle overrides |
Charlie Harrison
2017/05/17 18:54:50
// content::NavigationThrottle:
jkarlin
2017/05/18 13:50:13
Done.
|
+ ThrottleCheckResult WillProcessResponse() override { |
+ return NavigationThrottle::DEFER; |
+ } |
+ const char* GetNameForLogging() override { |
+ return "DelayWillProcessResponseThrottle"; |
+ } |
+ |
+ void CancelDeferredNavigation() { |
+ navigation_handle()->CancelDeferredNavigation(NavigationThrottle::CANCEL); |
+ } |
+ |
+ content::GlobalRequestID GetGlobalRequestID() const { |
+ return navigation_handle()->GetGlobalRequestID(); |
+ } |
+ |
+ private: |
+ DISALLOW_COPY_AND_ASSIGN(DelayWillProcessResponseThrottle); |
+}; |
+ |
+// Adds a navigation throttle to the navigation which delays |
+// WillProcessResponse. |
+class DelayWillProcessResponseObserver : public content::WebContentsObserver { |
+ public: |
+ explicit DelayWillProcessResponseObserver(content::WebContents* contents) |
+ : WebContentsObserver(contents), throttle_(nullptr) {} |
Charlie Harrison
2017/05/17 18:54:50
Hm why don't you need content:: prefix to WebConte
Charlie Harrison
2017/05/17 18:54:50
Don't need to set throttle_ to nullptr here since
jkarlin
2017/05/18 13:50:13
Done.
jkarlin
2017/05/18 13:50:13
I have no idea. Added for clarity.
|
+ |
+ // WebContentsObserver override |
Charlie Harrison
2017/05/17 18:54:50
// content::WebContentsObserver:
jkarlin
2017/05/18 13:50:13
Done.
|
+ void DidStartNavigation( |
+ content::NavigationHandle* navigation_handle) override { |
+ std::unique_ptr<content::NavigationThrottle> delay_throttle = |
+ base::MakeUnique<DelayWillProcessResponseThrottle>(navigation_handle); |
+ throttle_ = |
+ static_cast<DelayWillProcessResponseThrottle*>(delay_throttle.get()); |
+ navigation_handle->RegisterThrottleForTesting(std::move(delay_throttle)); |
+ } |
+ |
+ DelayWillProcessResponseThrottle* throttle() const { return throttle_; } |
+ |
+ private: |
+ DelayWillProcessResponseThrottle* throttle_ = nullptr; |
+ content::GlobalRequestID global_request_id_; |
+ |
+ DISALLOW_COPY_AND_ASSIGN(DelayWillProcessResponseObserver); |
+}; |
+ |
+void LoadResourceForMainFrameAndResume( |
Charlie Harrison
2017/05/17 18:54:50
Why not include this as a method in the delay obse
jkarlin
2017/05/18 13:50:13
Done.
|
+ page_load_metrics::MetricsWebContentsObserver* observer, |
+ DelayWillProcessResponseThrottle* throttle) { |
+ // Load a resource for the main frame before it commits. |
+ content::NavigationHandle* navigation_handle = throttle->navigation_handle(); |
+ |
+ observer->OnRequestComplete( |
+ GURL(kNonAdUrl), |
+ navigation_handle->GetRenderFrameHost()->GetFrameTreeNodeId(), |
+ navigation_handle->GetGlobalRequestID(), |
+ content::RESOURCE_TYPE_MAIN_FRAME, false /* was_cached */, |
+ nullptr /* data_reduction_proxy */, 10 * 1024 /* raw_body_bytes */, |
+ 0 /* original_network_content_length */, base::TimeTicks::Now()); |
+ |
+ throttle->CancelDeferredNavigation(); |
+} |
+ |
} // namespace |
class AdsPageLoadMetricsObserverTest |
@@ -605,3 +682,30 @@ TEST_F(AdsPageLoadMetricsObserverTest, FrameWithNoParent) { |
"PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound", |
content::RESOURCE_TYPE_SUB_FRAME, 1); |
} |
+ |
+// Make sure that ads histograms aren't recorded if the tracker never commits |
+// (see https://crbug.com/723219). |
+TEST_F(AdsPageLoadMetricsObserverTest, NoHistogramWithoutCommit) { |
+ // Once the metrics observer has the GlobalRequestID, throttle. |
+ DelayWillProcessResponseObserver delay_observer(web_contents()); |
+ |
+ // Start main-frame navigation |
+ auto navigation_simulator = NavigationSimulator::CreateRendererInitiated( |
+ GURL(kNonAdUrl), web_contents()->GetMainFrame()); |
+ navigation_simulator->Start(); |
+ |
+ // This will be run once WillProcessResponse defers and the navigation |
+ // simulator runs the message loop waiting for the throttles to finish. |
+ base::ThreadTaskRunnerHandle::Get()->PostTask( |
+ FROM_HERE, base::Bind(&LoadResourceForMainFrameAndResume, observer(), |
+ delay_observer.throttle())); |
+ |
+ // The commit will defer after calling WillProcessNavigationResponse, it |
+ // will load a resource, and then the throttle will cancel the commit. |
+ navigation_simulator->Commit(); |
+ |
+ // There shouldn't be any histograms for an aborted main frame. |
+ EXPECT_EQ(0u, histogram_tester() |
+ .GetTotalCountsForPrefix("PageLoad.Clients.Ads.") |
+ .size()); |
+} |