Pass RapporService to content/browser/

content/public/browser/web_contents_delegate.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_DELEGATE_H_
 #define CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_DELEGATE_H_
 
 #include <stdint.h>
 
 #include <set>
@@ skipping 42 matching lines @@
 struct Referrer;
 struct SecurityStyleExplanations;
 }  // namespace content
 
 namespace gfx {
 class Point;
 class Rect;
 class Size;
 }
 
+namespace rappor {
+class RapporService;
+}
+
 namespace net {
 class X509Certificate;
 }
 
 namespace blink {
 class WebGestureEvent;
 }
 
 namespace content {
 class RenderWidgetHost;
@@ skipping 455 matching lines @@
 
   // Called when the active render widget is forwarding a RemoteChannel
   // compositor proto.  This is used in Blimp mode.
   virtual void ForwardCompositorProto(
       RenderWidgetHost* render_widget_host,
       const std::vector<uint8_t>& proto) {}
 
   // 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();
 
  private:
   friend class WebContentsImpl;
 
   // Called when |this| becomes the WebContentsDelegate for |source|.
   void Attach(WebContents* source);
 
   // Called when |this| is no longer the WebContentsDelegate for |source|.
   void Detach(WebContents* source);
 
   // The WebContents that this is currently a delegate for.
   std::set<WebContents*> attached_contents_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_DELEGATE_H_