PageLoadMetricsObservers observe individual page loads

chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
index da8046aa770ac281e62ba05efefc51798d0168ef..def0c5b3ab34664c6cfb6df2f17ba13ad7a6b95d 100644
--- a/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
@@ -59,7 +59,7 @@ bool ShouldLogEvent(const base::TimeDelta& event,
 
 FromGWSPageLoadMetricsObserver::FromGWSPageLoadMetricsObserver(
     page_load_metrics::PageLoadMetricsObservable* metrics)
-    : navigation_from_gws_(false), metrics_(metrics) {}
+    : PageLoadMetricsObserver(metrics) {}
 
 void FromGWSPageLoadMetricsObserver::OnCommit(
     content::NavigationHandle* navigation_handle) {
@@ -70,8 +70,6 @@ void FromGWSPageLoadMetricsObserver::OnCommit(
 void FromGWSPageLoadMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& extra_info) {
-  if (!navigation_from_gws_)
-    return;
   // Filter out navigations that started in the background.
   if (!extra_info.started_in_foreground)
     return;
@@ -116,13 +114,11 @@ void FromGWSPageLoadMetricsObserver::OnComplete(
   }
 }
 
-void FromGWSPageLoadMetricsObserver::OnPageLoadMetricsGoingAway() {
-  metrics_->RemoveObserver(this);
-  delete this;
-}
-
 void FromGWSPageLoadMetricsObserver::SetCommittedURLAndReferrer(
     const GURL& url,
     const content::Referrer& referrer) {
-  navigation_from_gws_ = IsFromGoogleSearchResult(url, referrer.url);
+  if (!IsFromGoogleSearchResult(url, referrer.url)) {
+    GetObservable()->RemoveObserver(this);
+    delete this;

[Randy Smith (Not in Mondays), 2015/11/28 22:03:13]

To me, this really violates the principle of least astonishment--before looking
at this routine, there's no way I would have exepcted a routine named
SetCommittedURLAndReferrer() to result in a deletion of the object.

[Charlie Harrison, 2015/11/30 16:39:29]

Yeah I think I agree with you. We can ditch most of the changes to this class
actually, I was just trying out a new approach where the observer stop observing
a page load as soon as it stops being relevant. I think I may revert these
changes to simplify the CL.

+  }
 }