Pass RapporService to content/browser/

content/browser/renderer_host/render_widget_host_delegate.h

Index: content/browser/renderer_host/render_widget_host_delegate.h
diff --git a/content/browser/renderer_host/render_widget_host_delegate.h b/content/browser/renderer_host/render_widget_host_delegate.h
index 46129fdd012affd19c87fbf2b028bc03bc6fb141..964a1e54ad6fd0a428650adc85355eee3444fcbd 100644
--- a/content/browser/renderer_host/render_widget_host_delegate.h
+++ b/content/browser/renderer_host/render_widget_host_delegate.h
@@ -30,6 +30,10 @@ class Rect;
 class Size;
 }
 
+namespace rappor {
+class RapporService;
+}
+
 namespace content {
 
 class BrowserAccessibilityManager;
@@ -219,6 +223,13 @@ class CONTENT_EXPORT RenderWidgetHostDelegate {
   // Allow the delegate to handle the cursor update. Returns true if handled.
   virtual bool OnUpdateDragCursor();
 
+  // Returns the RapporService from browser process for the metric reporting
+  // purposes per url. See getRapporCommittedUrl for getting the url.

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

nit: I'm not sure what "for the metric reporting purposes per url" means.  Can
you rephrase?

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

Removed this API altogether.

+  virtual ::rappor::RapporService* getRapporService();

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

nit: Capitalize.

There's no uses of this method in the CL, so I'm not sure how it's intended to
be used.  We require that for things in content/public, but I think it might
help in this case as well, at least for understanding what we're adding.

In particular, I don't understand why this is on RenderWidgetHostDelegate. 
Widgets shouldn't be concerned with URLs, because they contain multiple frames
(each with their own URL) and may not correspond to the main frame (e.g., for
out-of-process iframes).  Would RenderFrameHostDelegate have been a better fit?

[Navid Zolghadr, 2016/11/17 20:05:49]

Here is one use of this:
https://codereview.chromium.org/2492793004/

I wanted to report the metric we have in render_widget_host_latency_tracker.h
which is created in RenderWidgetHostImpl. I was under the impression that the
metric we report is only for the main frame. Tim might be able to answer this
one better.

+
+  // Returns the Url of the current page to be used in Rappor metric reporting.
+  virtual std::string getRapporCommittedUrl();

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

nit: Capitalize.

As above, I'm concerned about putting a URL method on something associated with
RenderWidgetHost.  This feels like the wrong place.

I also find it odd that WebContents implements this, because the implementation
doesn't have any WebContents-specific dependencies.  It just calls a static
Rappor method.  Maybe the intended call sites should do that directly instead?

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

Actually RenderFrameHostDelegate already has a method with similar
functionality. But I couldn't find a way to get the reference to that object in
RenderWidgetHostLatencyTracker or RenderWidgetHostImpl. Do you know of another
way I can access this information in RenderWidgetHostLatencyTracker? I assume it
would be okay to report the metric for the main url for now and later we will
take care of how to handle out of process iframes I guess.

+
  protected:
   virtual ~RenderWidgetHostDelegate() {}
 };