Revert of Add RAPPOR support for PageLoadMetrics (patchset #10 id:180001 of https://codereview.chromium.org/1374363002/ )

Reason for revert:
Broke PrerenderBrowserTest.PrerenderHangingUnload on Win7 and Mac ASan 64.

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/42334
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/42335
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/6839
http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/6840

Original issue's description:
> Add RAPPOR support for PageLoadMetrics
>
> This change plumbs a RapporService through the relevant code, and logs
> a simple failure metric on pages that took too long to first layout. We also log a coarse histogram (4 buckets) with a bitfield.
>
> BUG=538217
>
> Committed: https://crrev.com/b11a55c727d5685816486598311e4b331c19291f
> Cr-Commit-Position: refs/heads/master@{#352713}

TBR=bmcquade@chromium.org,holte@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org,avi@chromium.org,csharrison@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=538217

Review URL: https://codereview.chromium.org/1389223002

Cr-Commit-Position: refs/heads/master@{#352765}

4 files changed, 21 insertions, 138 deletions

diff --git a/components/page_load_metrics/browser/DEPS b/components/page_load_metrics/browser/DEPS
index fc1f590..9579c84 100644
--- a/components/page_load_metrics/browser/DEPS
+++ b/components/page_load_metrics/browser/DEPS
@@ -1,6 +1,5 @@
 include_rules = [
   "+content/public/browser",
   "+components/page_load_metrics/browser",
-  "+components/rappor",
   "+net/base",
 ]
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.cc b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
index aa161f8..df37a05 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.cc
@@ -8,8 +8,6 @@
 #include "base/metrics/histogram.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
-#include "components/rappor/rappor_service.h"
-#include "components/rappor/rappor_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_handle.h"
@@ -53,33 +51,11 @@ bool IsValidPageLoadTiming(const PageLoadTiming& timing) {
   return true;
 }
 
-base::Time WallTimeFromTimeTicks(const base::TimeTicks& time) {
+base::Time WallTimeFromTimeTicks(base::TimeTicks time) {
   return base::Time::FromDoubleT(
       (time - base::TimeTicks::UnixEpoch()).InSecondsF());
 }
 
-// The number of buckets in the bitfield histogram. These buckets are described
-// in rappor.xml in PageLoad.CoarseTiming.NavigationToFirstLayout. The bucket
-// flag is defined by 1 << bucket_index, and is the bitfield representing which
-// timing bucket the page load falls into, i.e. 000010 would be the bucket flag
-// showing that the page took between 2 and 4 seconds to load.
-const size_t kNumRapporHistogramBuckets = 6;
-
-uint64_t RapporHistogramBucketIndex(const base::TimeDelta& time) {
-  int64 seconds = time.InSeconds();
-  if (seconds < 2)
-    return 0;
-  if (seconds < 4)
-    return 1;
-  if (seconds < 8)
-    return 2;
-  if (seconds < 16)
-    return 3;
-  if (seconds < 32)
-    return 4;
-  return 5;
-}
-
 }  // namespace
 
 #define PAGE_LOAD_HISTOGRAM(name, sample)                           \
@@ -87,11 +63,8 @@ uint64_t RapporHistogramBucketIndex(const base::TimeDelta& time) {
                              base::TimeDelta::FromMilliseconds(10), \
                              base::TimeDelta::FromMinutes(10), 100)
 
-PageLoadTracker::PageLoadTracker(bool in_foreground,
-                                 rappor::RapporService* const rappor_service)
-    : has_commit_(false),
-      started_in_foreground_(in_foreground),
-      rappor_service_(rappor_service) {
+PageLoadTracker::PageLoadTracker(bool in_foreground)
+    : has_commit_(false), started_in_foreground_(in_foreground) {
   RecordEvent(PAGE_LOAD_STARTED);
 }
 
@@ -101,10 +74,8 @@ PageLoadTracker::~PageLoadTracker() {
   if (timing_.first_layout.is_zero())
     RecordEvent(PAGE_LOAD_ABORTED_BEFORE_FIRST_LAYOUT);
 
-  if (has_commit_) {
+  if (has_commit_)
     RecordTimingHistograms();
-    RecordRappor();
-  }
 }
 
 void PageLoadTracker::WebContentsHidden() {
@@ -114,9 +85,8 @@ void PageLoadTracker::WebContentsHidden() {
   }
 }
 
-void PageLoadTracker::Commit(const GURL& committed_url) {
+void PageLoadTracker::Commit() {
   has_commit_ = true;
-  url_ = committed_url;
 }
 
 bool PageLoadTracker::UpdateTiming(const PageLoadTiming& timing) {
@@ -133,35 +103,17 @@ bool PageLoadTracker::UpdateTiming(const PageLoadTiming& timing) {
   return false;
 }
 
-const GURL& PageLoadTracker::GetCommittedURL() {
+void PageLoadTracker::RecordTimingHistograms() {
   DCHECK(has_commit_);
-  return url_;
-}
-
-// Blink calculates navigation start using TimeTicks, but converts to epoch time
-// in its public API. Thus, to compare time values to navigation start, we
-// calculate the current time since the epoch using TimeTicks, and convert to
-// Time. This method is similar to how blink converts TimeTicks to epoch time.
-// There may be slight inaccuracies due to inter-process timestamps, but
-// this solution is the best we have right now.
-//
-// returns a TimeDelta which is
-//  - Infinity if we were never backgrounded
-//  - null (TimeDelta()) if we started backgrounded
-//  - elapsed time to first background if we started in the foreground and
-//    backgrounded.
-base::TimeDelta PageLoadTracker::GetBackgroundDelta() {
+  // This method is similar to how blink converts TimeTicks to epoch time.
+  // There may be slight inaccuracies due to inter-process timestamps, but
+  // this solution is the best we have right now.
+  base::TimeDelta background_delta;
   if (started_in_foreground_) {
-    if (background_time_.is_null())
-      return base::TimeDelta::Max();
-    return WallTimeFromTimeTicks(background_time_) - timing_.navigation_start;
+    background_delta = background_time_.is_null()
+        ? base::TimeDelta::Max()
+        : WallTimeFromTimeTicks(background_time_) - timing_.navigation_start;
   }
-  return base::TimeDelta();
-}
-
-void PageLoadTracker::RecordTimingHistograms() {
-  DCHECK(has_commit_);
-  base::TimeDelta background_delta = GetBackgroundDelta();
 
   if (!timing_.dom_content_loaded_event_start.is_zero()) {
     if (timing_.dom_content_loaded_event_start < background_delta) {
@@ -202,44 +154,9 @@ void PageLoadTracker::RecordEvent(PageLoadEvent event) {
       "PageLoad.EventCounts", event, PAGE_LOAD_LAST_ENTRY);
 }
 
-void PageLoadTracker::RecordRappor() {
-  DCHECK(!GetCommittedURL().is_empty());
-  if (!rappor_service_)
-    return;
-  // Log the eTLD+1 of sites that show poor loading performance.
-  if (!timing_.first_layout.is_zero() &&
-      timing_.first_layout < GetBackgroundDelta()) {
-    scoped_ptr<rappor::Sample> sample =
-        rappor_service_->CreateSample(rappor::UMA_RAPPOR_TYPE);
-    sample->SetStringField("Domain", rappor::GetDomainAndRegistrySampleFromGURL(
-                                         GetCommittedURL()));
-    sample->SetFlagsField("Bucket",
-                          1 << RapporHistogramBucketIndex(timing_.first_layout),
-                          kNumRapporHistogramBuckets);
-    // The IsSlow flag is just a one bit boolean if the first layout was > 10s.
-    sample->SetFlagsField("IsSlow", timing_.first_layout.InSecondsF() >= 10, 1);
-    rappor_service_->RecordSampleObj(
-        "PageLoad.CoarseTiming.NavigationToFirstLayout", sample.Pass());
-  }
-}
-
-// static
-void MetricsWebContentsObserver::CreateForWebContents(
-    content::WebContents* web_contents,
-    rappor::RapporService* rappor_service) {
-  DCHECK(web_contents);
-  if (!FromWebContents(web_contents)) {
-    web_contents->SetUserData(UserDataKey(), new MetricsWebContentsObserver(
-                                                 web_contents, rappor_service));
-  }
-}
-
 MetricsWebContentsObserver::MetricsWebContentsObserver(
-    content::WebContents* web_contents,
-    rappor::RapporService* rappor_service)
-    : content::WebContentsObserver(web_contents),
-      in_foreground_(false),
-      rappor_service_(rappor_service) {}
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents), in_foreground_(false) {}
 
 MetricsWebContentsObserver::~MetricsWebContentsObserver() {}
 
@@ -265,8 +182,7 @@ void MetricsWebContentsObserver::DidStartNavigation(
   // from the omnibox.
   DCHECK_GT(2ul, provisional_loads_.size());
   provisional_loads_.insert(
-      navigation_handle,
-      make_scoped_ptr(new PageLoadTracker(in_foreground_, rappor_service_)));
+      navigation_handle, make_scoped_ptr(new PageLoadTracker(in_foreground_)));
 }
 
 void MetricsWebContentsObserver::DidFinishNavigation(
@@ -299,7 +215,7 @@ void MetricsWebContentsObserver::DidFinishNavigation(
     return;
 
   committed_load_ = finished_nav.Pass();
-  committed_load_->Commit(navigation_handle->GetURL());
+  committed_load_->Commit();
 }
 
 void MetricsWebContentsObserver::WasShown() {
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.h b/components/page_load_metrics/browser/metrics_web_contents_observer.h
index 8149395..cf39dc9 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -23,10 +23,6 @@ namespace IPC {
 class Message;
 }  // namespace IPC
 
-namespace rappor {
-class RapporService;
-}
-
 namespace page_load_metrics {
 
 // If you add elements from this enum, make sure you update the enum
@@ -60,17 +56,11 @@ enum PageLoadEvent {
   PAGE_LOAD_LAST_ENTRY
 };
 
-// This class tracks a given page load, starting from navigation start /
-// provisional load, until a new navigation commits or the navigation fails. It
-// also records RAPPOR/UMA about the page load.
-// MetricsWebContentsObserver manages a set of provisional PageLoadTrackers, as
-// well as a committed PageLoadTracker.
 class PageLoadTracker {
  public:
-  PageLoadTracker(bool in_foreground,
-                  rappor::RapporService* const rappor_service);
+  explicit PageLoadTracker(bool in_foreground);
   ~PageLoadTracker();
-  void Commit(const GURL& committed_url);
+  void Commit();
   void WebContentsHidden();
 
   // Returns true if the timing was successfully updated.
@@ -78,12 +68,7 @@ class PageLoadTracker {
   void RecordEvent(PageLoadEvent event);
 
  private:
-  // Only valid to call post-commit.
-  const GURL& GetCommittedURL();
-
-  base::TimeDelta GetBackgroundDelta();
   void RecordTimingHistograms();
-  void RecordRappor();
 
   bool has_commit_;
 
@@ -94,12 +79,6 @@ class PageLoadTracker {
   bool started_in_foreground_;
 
   PageLoadTiming timing_;
-  GURL url_;
-
-  // This RapporService is owned by and shares a lifetime with
-  // g_browser_process's MetricsServicesManager. It can be NULL. The underlying
-  // RapporService will be freed when the when the browser process is killed.
-  rappor::RapporService* const rappor_service_;
 
   DISALLOW_COPY_AND_ASSIGN(PageLoadTracker);
 };
@@ -110,10 +89,6 @@ class MetricsWebContentsObserver
     : public content::WebContentsObserver,
       public content::WebContentsUserData<MetricsWebContentsObserver> {
  public:
-  // The caller must guarantee that the RapporService (if non-null) will
-  // outlive the WebContents.
-  static void CreateForWebContents(content::WebContents* web_contents,
-                                   rappor::RapporService* rappor_service);
   ~MetricsWebContentsObserver() override;
 
   // content::WebContentsObserver implementation:
@@ -130,8 +105,7 @@ class MetricsWebContentsObserver
   void RenderProcessGone(base::TerminationStatus status) override;
 
  private:
-  MetricsWebContentsObserver(content::WebContents* web_contents,
-                             rappor::RapporService* rappor_service);
+  explicit MetricsWebContentsObserver(content::WebContents* web_contents);
   friend class content::WebContentsUserData<MetricsWebContentsObserver>;
   friend class MetricsWebContentsObserverTest;
 
@@ -150,8 +124,6 @@ class MetricsWebContentsObserver
       provisional_loads_;
   scoped_ptr<PageLoadTracker> committed_load_;
 
-  rappor::RapporService* const rappor_service_;
-
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };
 
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc b/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
index fb4e5f3..147f094 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
@@ -9,7 +9,6 @@
 #include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
-#include "components/rappor/test_rappor_service.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/test/test_renderer_host.h"
@@ -48,9 +47,7 @@ class MetricsWebContentsObserverTest
 
   void SetUp() override {
     RenderViewHostTestHarness::SetUp();
-    observer_ = make_scoped_ptr(
-        new MetricsWebContentsObserver(web_contents(),
-                                       &rappor_tester_));
+    observer_ = make_scoped_ptr(new MetricsWebContentsObserver(web_contents()));
     observer_->WasShown();
   }
 
@@ -62,7 +59,6 @@ class MetricsWebContentsObserverTest
 
  protected:
   base::HistogramTester histogram_tester_;
-  rappor::TestRapporService rappor_tester_;
   scoped_ptr<MetricsWebContentsObserver> observer_;
 };