Commit fc9831da authored by bmcquade's avatar bmcquade Committed by Commit bot

Update OnCommit to return ObservePolicy.

BUG=647764

Review-Url: https://codereview.chromium.org/2372573005
Cr-Commit-Position: refs/heads/master@{#421299}
parent 598ab147
......@@ -33,6 +33,19 @@
DEFINE_WEB_CONTENTS_USER_DATA_KEY(
page_load_metrics::MetricsWebContentsObserver);
// This macro invokes the specified method on each observer, passing the
// variable length arguments as the method's arguments, and removes the observer
// from the list of observers if the given method returns STOP_OBSERVING.
#define INVOKE_AND_PRUNE_OBSERVERS(observers, Method, ...) \
for (auto it = observers.begin(); it != observers.end();) { \
if ((*it)->Method(__VA_ARGS__) == \
PageLoadMetricsObserver::STOP_OBSERVING) { \
it = observers.erase(it); \
} else { \
++it; \
} \
}
namespace page_load_metrics {
namespace internal {
......@@ -442,9 +455,8 @@ void PageLoadTracker::Commit(content::NavigationHandle* navigation_handle) {
// Some transitions (like CLIENT_REDIRECT) are only known at commit time.
page_transition_ = navigation_handle->GetPageTransition();
user_gesture_ = navigation_handle->HasUserGesture();
for (const auto& observer : observers_) {
observer->OnCommit(navigation_handle);
}
INVOKE_AND_PRUNE_OBSERVERS(observers_, OnCommit, navigation_handle);
LogAbortChainHistograms(navigation_handle);
}
......@@ -475,14 +487,8 @@ void PageLoadTracker::FlushMetricsOnAppEnterBackground() {
}
const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
for (auto it = observers_.begin(); it != observers_.end();) {
if ((*it)->FlushMetricsOnAppEnterBackground(timing_, info) ==
PageLoadMetricsObserver::STOP_OBSERVING) {
it = observers_.erase(it);
} else {
++it;
}
}
INVOKE_AND_PRUNE_OBSERVERS(observers_, FlushMetricsOnAppEnterBackground,
timing_, info);
}
void PageLoadTracker::NotifyClientRedirectTo(
......
......@@ -30,15 +30,15 @@ namespace {
const char kDefaultTestUrl[] = "https://google.com/";
const char kDefaultTestUrlAnchor[] = "https://google.com/#samepage";
const char kDefaultTestUrl2[] = "https://whatever.com/";
const char kFilteredCommitUrl[] = "https://whatever.com/ignore-on-commit";
// Simple PageLoadMetricsObserver that copies observed PageLoadTimings into the
// provided std::vector, so they can be analyzed by unit tests.
class TestPageLoadMetricsObserver : public PageLoadMetricsObserver {
public:
explicit TestPageLoadMetricsObserver(
std::vector<PageLoadTiming>* updated_timings,
std::vector<PageLoadTiming>* complete_timings,
std::vector<GURL>* observed_committed_urls)
TestPageLoadMetricsObserver(std::vector<PageLoadTiming>* updated_timings,
std::vector<PageLoadTiming>* complete_timings,
std::vector<GURL>* observed_committed_urls)
: updated_timings_(updated_timings),
complete_timings_(complete_timings),
observed_committed_urls_(observed_committed_urls) {}
......@@ -71,6 +71,29 @@ class TestPageLoadMetricsObserver : public PageLoadMetricsObserver {
std::vector<GURL>* const observed_committed_urls_;
};
// Test PageLoadMetricsObserver that stops observing page loads with certain
// substrings in the URL.
class FilteringPageLoadMetricsObserver : public PageLoadMetricsObserver {
public:
explicit FilteringPageLoadMetricsObserver(
std::vector<GURL>* completed_filtered_urls)
: completed_filtered_urls_(completed_filtered_urls) {}
ObservePolicy OnCommit(content::NavigationHandle* handle) override {
const bool should_ignore =
handle->GetURL().spec().find("ignore-on-commit") != std::string::npos;
return should_ignore ? STOP_OBSERVING : CONTINUE_OBSERVING;
}
void OnComplete(const PageLoadTiming& timing,
const PageLoadExtraInfo& extra_info) override {
completed_filtered_urls_->push_back(extra_info.committed_url);
}
private:
std::vector<GURL>* const completed_filtered_urls_;
};
class TestPageLoadMetricsEmbedderInterface
: public PageLoadMetricsEmbedderInterface {
public:
......@@ -88,6 +111,8 @@ class TestPageLoadMetricsEmbedderInterface
void RegisterObservers(PageLoadTracker* tracker) override {
tracker->AddObserver(base::MakeUnique<TestPageLoadMetricsObserver>(
&updated_timings_, &complete_timings_, &observed_committed_urls_));
tracker->AddObserver(base::MakeUnique<FilteringPageLoadMetricsObserver>(
&completed_filtered_urls_));
}
const std::vector<PageLoadTiming>& updated_timings() const {
return updated_timings_;
......@@ -101,10 +126,16 @@ class TestPageLoadMetricsEmbedderInterface
return observed_committed_urls_;
}
// committed URLs passed to FilteringPageLoadMetricsObserver::OnComplete().
const std::vector<GURL>& completed_filtered_urls() const {
return completed_filtered_urls_;
}
private:
std::vector<PageLoadTiming> updated_timings_;
std::vector<PageLoadTiming> complete_timings_;
std::vector<GURL> observed_committed_urls_;
std::vector<GURL> completed_filtered_urls_;
bool is_prerendering_;
bool is_ntp_;
};
......@@ -176,6 +207,10 @@ class MetricsWebContentsObserverTest : public ChromeRenderViewHostTestHarness {
return embedder_interface_->observed_committed_urls_from_on_start();
}
const std::vector<GURL>& completed_filtered_urls() const {
return embedder_interface_->completed_filtered_urls();
}
protected:
base::HistogramTester histogram_tester_;
TestPageLoadMetricsEmbedderInterface* embedder_interface_;
......@@ -523,4 +558,27 @@ TEST_F(MetricsWebContentsObserverTest, FlushMetricsOnAppEnterBackground) {
internal::kPageLoadCompletedAfterAppBackground, true, 1);
}
TEST_F(MetricsWebContentsObserverTest, StopObservingOnCommit) {
content::WebContentsTester* web_contents_tester =
content::WebContentsTester::For(web_contents());
ASSERT_TRUE(completed_filtered_urls().empty());
web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
ASSERT_TRUE(completed_filtered_urls().empty());
// kFilteredCommitUrl should stop observing in OnCommit, and thus should not
// reach OnComplete().
web_contents_tester->NavigateAndCommit(GURL(kFilteredCommitUrl));
ASSERT_EQ(std::vector<GURL>({GURL(kDefaultTestUrl)}),
completed_filtered_urls());
web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
ASSERT_EQ(std::vector<GURL>({GURL(kDefaultTestUrl)}),
completed_filtered_urls());
web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
ASSERT_EQ(std::vector<GURL>({GURL(kDefaultTestUrl), GURL(kDefaultTestUrl2)}),
completed_filtered_urls());
}
} // namespace page_load_metrics
......@@ -214,7 +214,8 @@ CorePageLoadMetricsObserver::CorePageLoadMetricsObserver()
CorePageLoadMetricsObserver::~CorePageLoadMetricsObserver() {}
void CorePageLoadMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
CorePageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
transition_ = navigation_handle->GetPageTransition();
initiated_by_user_gesture_ = navigation_handle->HasUserGesture();
......@@ -225,6 +226,7 @@ void CorePageLoadMetricsObserver::OnCommit(
was_no_store_main_resource_ =
headers->HasHeaderValue("cache-control", "no-store");
}
return CONTINUE_OBSERVING;
}
void CorePageLoadMetricsObserver::OnDomContentLoadedEventStart(
......
......@@ -71,7 +71,7 @@ class CorePageLoadMetricsObserver
~CorePageLoadMetricsObserver() override;
// page_load_metrics::PageLoadMetricsObserver:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnDomContentLoadedEventStart(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& extra_info) override;
......
......@@ -91,7 +91,8 @@ DataReductionProxyMetricsObserver::DataReductionProxyMetricsObserver()
DataReductionProxyMetricsObserver::~DataReductionProxyMetricsObserver() {}
// Check if the NavigationData indicates anything about the DataReductionProxy.
void DataReductionProxyMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
DataReductionProxyMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
// This BrowserContext is valid for the lifetime of
// DataReductionProxyMetricsObserver. BrowserContext is always valid and
......@@ -110,15 +111,16 @@ void DataReductionProxyMetricsObserver::OnCommit(
static_cast<ChromeNavigationData*>(
navigation_handle->GetNavigationData());
if (!chrome_navigation_data)
return;
return STOP_OBSERVING;
data_reduction_proxy::DataReductionProxyData* data =
chrome_navigation_data->GetDataReductionProxyData();
if (!data)
return;
return STOP_OBSERVING;
data_ = data->DeepCopy();
// DataReductionProxy page loads should only occur on HTTP navigations.
DCHECK(!data_->used_data_reduction_proxy() ||
!navigation_handle->GetURL().SchemeIsCryptographic());
return CONTINUE_OBSERVING;
}
void DataReductionProxyMetricsObserver::OnComplete(
......
......@@ -51,7 +51,7 @@ class DataReductionProxyMetricsObserver
~DataReductionProxyMetricsObserver() override;
// page_load_metrics::PageLoadMetricsObserver:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnComplete(const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) override;
void OnDomContentLoadedEventStart(
......
......@@ -100,12 +100,13 @@ class TestDataReductionProxyMetricsObserver
~TestDataReductionProxyMetricsObserver() override {}
// page_load_metrics::PageLoadMetricsObserver implementation:
void OnCommit(content::NavigationHandle* navigation_handle) override {
ObservePolicy OnCommit(
content::NavigationHandle* navigation_handle) override {
DataReductionProxyData* data =
DataForNavigationHandle(web_contents_, navigation_handle);
data->set_used_data_reduction_proxy(data_reduction_proxy_used_);
data->set_lofi_requested(lofi_used_);
DataReductionProxyMetricsObserver::OnCommit(navigation_handle);
return DataReductionProxyMetricsObserver::OnCommit(navigation_handle);
}
DataReductionProxyPingbackClient* GetPingbackClient() const override {
......
......@@ -412,7 +412,8 @@ void FromGWSPageLoadMetricsObserver::OnStart(
logger_.SetProvisionalUrl(navigation_handle->GetURL());
}
void FromGWSPageLoadMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
FromGWSPageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
// We'd like to also check navigation_handle->HasUserGesture() here, however
// this signal is not carried forward for navigations that open links in new
......@@ -427,6 +428,7 @@ void FromGWSPageLoadMetricsObserver::OnCommit(
navigation_handle->GetPageTransition()));
logger_.SetNavigationStart(navigation_handle->NavigationStart());
return CONTINUE_OBSERVING;
}
void FromGWSPageLoadMetricsObserver::OnDomContentLoadedEventStart(
......
......@@ -142,7 +142,7 @@ class FromGWSPageLoadMetricsObserver
void OnStart(content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url,
bool started_in_foreground) override;
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnDomContentLoadedEventStart(
const page_load_metrics::PageLoadTiming& timing,
......
......@@ -44,12 +44,13 @@ bool IsGoogleCaptcha(const GURL& url) {
GoogleCaptchaObserver::GoogleCaptchaObserver() : saw_solution_(false) {}
void GoogleCaptchaObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
GoogleCaptchaObserver::OnCommit(content::NavigationHandle* navigation_handle) {
if (!navigation_handle->IsSamePage()
&& IsGoogleCaptcha(navigation_handle->GetURL())) {
RecordGoogleCaptchaEvent(GOOGLE_CAPTCHA_SHOWN);
}
return CONTINUE_OBSERVING;
}
void GoogleCaptchaObserver::OnRedirect(
......
......@@ -19,7 +19,7 @@ class GoogleCaptchaObserver
GoogleCaptchaObserver();
// page_load_metrics::PageLoadMetricsObserver implementation:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnRedirect(content::NavigationHandle* navigation_handle) override;
private:
......
......@@ -32,13 +32,15 @@ NoStatePrefetchPageLoadMetricsObserver::NoStatePrefetchPageLoadMetricsObserver(
NoStatePrefetchPageLoadMetricsObserver::
~NoStatePrefetchPageLoadMetricsObserver() {}
void NoStatePrefetchPageLoadMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
NoStatePrefetchPageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
const net::HttpResponseHeaders* response_headers =
navigation_handle->GetResponseHeaders();
is_no_store_ = response_headers &&
response_headers->HasHeaderValue("cache-control", "no-store");
return CONTINUE_OBSERVING;
}
void NoStatePrefetchPageLoadMetricsObserver::OnFirstContentfulPaint(
......
......@@ -32,7 +32,7 @@ class NoStatePrefetchPageLoadMetricsObserver
private:
// page_load_metrics::PageLoadMetricsObserver:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnFirstContentfulPaint(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& extra_info) override;
......
......@@ -39,21 +39,22 @@ const char kHistogramOfflinePreviewsParseStart[] =
} // namespace internal
PreviewsPageLoadMetricsObserver::PreviewsPageLoadMetricsObserver()
: is_offline_preview_(false) {}
PreviewsPageLoadMetricsObserver::PreviewsPageLoadMetricsObserver() {}
PreviewsPageLoadMetricsObserver::~PreviewsPageLoadMetricsObserver() {}
void PreviewsPageLoadMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
PreviewsPageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
is_offline_preview_ = IsOfflinePreview(navigation_handle->GetWebContents());
return IsOfflinePreview(navigation_handle->GetWebContents())
? CONTINUE_OBSERVING
: STOP_OBSERVING;
}
void PreviewsPageLoadMetricsObserver::OnDomContentLoadedEventStart(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
if (!is_offline_preview_ ||
!WasStartedInForegroundOptionalEventInForeground(
if (!WasStartedInForegroundOptionalEventInForeground(
timing.dom_content_loaded_event_start, info)) {
return;
}
......@@ -65,8 +66,7 @@ void PreviewsPageLoadMetricsObserver::OnDomContentLoadedEventStart(
void PreviewsPageLoadMetricsObserver::OnLoadEventStart(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
if (!is_offline_preview_ ||
!WasStartedInForegroundOptionalEventInForeground(
if (!WasStartedInForegroundOptionalEventInForeground(
timing.dom_content_loaded_event_start, info)) {
return;
}
......@@ -77,8 +77,7 @@ void PreviewsPageLoadMetricsObserver::OnLoadEventStart(
void PreviewsPageLoadMetricsObserver::OnFirstLayout(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
if (!is_offline_preview_ ||
!WasStartedInForegroundOptionalEventInForeground(
if (!WasStartedInForegroundOptionalEventInForeground(
timing.dom_content_loaded_event_start, info)) {
return;
}
......@@ -89,8 +88,7 @@ void PreviewsPageLoadMetricsObserver::OnFirstLayout(
void PreviewsPageLoadMetricsObserver::OnFirstContentfulPaint(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
if (!is_offline_preview_ ||
!WasStartedInForegroundOptionalEventInForeground(
if (!WasStartedInForegroundOptionalEventInForeground(
timing.dom_content_loaded_event_start, info)) {
return;
}
......@@ -101,8 +99,7 @@ void PreviewsPageLoadMetricsObserver::OnFirstContentfulPaint(
void PreviewsPageLoadMetricsObserver::OnParseStart(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
if (!is_offline_preview_ ||
!WasStartedInForegroundOptionalEventInForeground(
if (!WasStartedInForegroundOptionalEventInForeground(
timing.dom_content_loaded_event_start, info)) {
return;
}
......
......@@ -40,7 +40,7 @@ class PreviewsPageLoadMetricsObserver
~PreviewsPageLoadMetricsObserver() override;
// page_load_metrics::PageLoadMetricsObserver:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnDomContentLoadedEventStart(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) override;
......@@ -60,9 +60,6 @@ class PreviewsPageLoadMetricsObserver
// testing.
virtual bool IsOfflinePreview(content::WebContents* web_contents) const;
// Whether this page load was an offline pages preview.
bool is_offline_preview_;
DISALLOW_COPY_AND_ASSIGN(PreviewsPageLoadMetricsObserver);
};
......
......@@ -11,13 +11,15 @@
namespace chrome {
StaleWhileRevalidateMetricsObserver::StaleWhileRevalidateMetricsObserver()
: is_interesting_domain_(false) {}
StaleWhileRevalidateMetricsObserver::StaleWhileRevalidateMetricsObserver() {}
void StaleWhileRevalidateMetricsObserver::OnCommit(
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
StaleWhileRevalidateMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
is_interesting_domain_ = net::IsHostInStaleWhileRevalidateExperimentDomain(
navigation_handle->GetURL().host());
return net::IsHostInStaleWhileRevalidateExperimentDomain(
navigation_handle->GetURL().host())
? CONTINUE_OBSERVING
: STOP_OBSERVING;
}
void StaleWhileRevalidateMetricsObserver::OnComplete(
......@@ -25,9 +27,6 @@ void StaleWhileRevalidateMetricsObserver::OnComplete(
const page_load_metrics::PageLoadExtraInfo& extra_info) {
using page_load_metrics::WasStartedInForegroundOptionalEventInForeground;
if (!is_interesting_domain_)
return;
if (WasStartedInForegroundOptionalEventInForeground(timing.load_event_start,
extra_info)) {
PAGE_LOAD_HISTOGRAM(
......
......@@ -15,16 +15,12 @@ class StaleWhileRevalidateMetricsObserver
public:
StaleWhileRevalidateMetricsObserver();
// page_load_metrics::PageLoadMetricsObserver implementation:
void OnCommit(content::NavigationHandle* navigation_handle) override;
ObservePolicy OnCommit(content::NavigationHandle* navigation_handle) override;
void OnComplete(
const page_load_metrics::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& extra_info) override;
private:
// True if the committed URL is one of the domains of interest to the
// stale-while-revalidate experiment.
bool is_interesting_domain_;
DISALLOW_COPY_AND_ASSIGN(StaleWhileRevalidateMetricsObserver);
};
......
......@@ -44,6 +44,11 @@ FailedProvisionalLoadInfo::FailedProvisionalLoadInfo(base::TimeDelta interval,
FailedProvisionalLoadInfo::~FailedProvisionalLoadInfo() {}
PageLoadMetricsObserver::ObservePolicy PageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
return CONTINUE_OBSERVING;
}
PageLoadMetricsObserver::ObservePolicy
PageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
const PageLoadTiming& timing,
......
......@@ -164,7 +164,9 @@ class PageLoadMetricsObserver {
// the navigation, but will be destroyed soon after this call. Don't hold a
// reference to it.
// Note that this does not get called for same page navigations.
virtual void OnCommit(content::NavigationHandle* navigation_handle) {}
// Observers that return STOP_OBSERVING will not receive any additional
// callbacks, and will be deleted after invocation of this method returns.
virtual ObservePolicy OnCommit(content::NavigationHandle* navigation_handle);
// OnHidden is triggered when a page leaves the foreground. It does not fire
// when a foreground page is permanently closed; for that, listen to
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment