Pass RapporService to content/browser/

content/browser/renderer_host/render_widget_host_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_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_DELEGATE_H_
 #define CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_DELEGATE_H_
 
 #include <stdint.h>
 
 #include <vector>
@@ skipping 12 matching lines @@
 class WebMouseWheelEvent;
 class WebGestureEvent;
 }
 
 namespace gfx {
 class Point;
 class Rect;
 class Size;
 }
 
+namespace rappor {
+class RapporService;
+}
+
 namespace content {
 
 class BrowserAccessibilityManager;
 class RenderWidgetHostImpl;
 class RenderWidgetHostInputEventRouter;
 class RenderViewHostDelegateView;
 class TextInputManager;
 struct ScreenInfo;
 struct NativeWebKeyboardEvent;
 
@@ skipping 169 matching lines @@
   virtual RenderViewHostDelegateView* GetDelegateView();
 
   // Returns the current Flash fullscreen RenderWidgetHostImpl if any. This is
   // not intended for use with other types of fullscreen, such as HTML
   // fullscreen, and will return nullptr for those cases.
   virtual RenderWidgetHostImpl* GetFullscreenRenderWidgetHost() const;
 
   // 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() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_DELEGATE_H_