Pass RapporService to content/browser/

content/public/browser/web_contents_delegate.h

Index: content/public/browser/web_contents_delegate.h
diff --git a/content/public/browser/web_contents_delegate.h b/content/public/browser/web_contents_delegate.h
index 2beb83c3b721b8fc03adbb5015f0825567fbc699..c0f26c22689f96148afa63f39fa7fc811c889814 100644
--- a/content/public/browser/web_contents_delegate.h
+++ b/content/public/browser/web_contents_delegate.h
@@ -60,6 +60,10 @@ class Rect;
 class Size;
 }
 
+namespace rappor {
+class RapporService;
+}
+
 namespace net {
 class X509Certificate;
 }
@@ -535,6 +539,8 @@ class CONTENT_EXPORT WebContentsDelegate {
   // Requests the app banner. This method is called from the DevTools.
   virtual void RequestAppBannerFromDevTools(content::WebContents* web_contents);
 
+  virtual ::rappor::RapporService* getRapporService();

[Charlie Reis, 2016/11/16 23:55:22]

nit: All methods in content/public should be documented.  Especially clarify if
it can return null.

nit: Capitalize name.

[ncarter (slow), 2016/11/17 20:39:12]

If we expose the full rappor API to content, GetRapporService() should be a
method in ContentBrowserClient so that it can be called from anywhere.
ContentBrowserClient is the standard way to expose a global variable from chrome
to content. there are dozens of implementations of WebContentsDelegate in
chrome/ alone, with no common base class, so you really only want to use
WebContentsDelegate when you need a behavior that is specific to a particular
flavor of window (e.g. one that needs to differ for extension view vs. devtools
windows vs. normal tabs).

Ruminating over alternatives to exposing all of RapporService: another way we
could do this, would be to extract a subinterface of RapporService, with at
least the following methods:

  virtual std::unique_ptr<Sample> CreateSample(RapporType);
  virtual void RecordSampleObj(const std::string& metric_name,
                               std::unique_ptr<Sample> sample);
  virtual void RecordSample(const std::string& metric_name,
                            RapporType type,
                            const std::string& sample);

And then have RapporService satisfy that interface. This would have the
advantage of hiding Initialize and RegisterPrefs pref stuff and eliminate
exposing stuff to content, than we never want content to interact with.

Then, we need to find a place for |Sample|, |RapporType|, and the
above-mentioned interface to live. The two options would be
//content/public/rappor, or //components/rappor/public. I think the latter is
fine, and content only has to relax DEPS to allow //components/rappor/public?

[Navid Zolghadr, 2016/12/01 18:27:15]

I removed this API from here and wen with the //components/rappor/public
suggestion. Let me know if the new patch is what you had in mind or not.

+
  protected:
   virtual ~WebContentsDelegate();