Pass RapporService to content/browser/

Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025
TBR=droger@chromium.org, gcasto@chromium.org

Committed: https://crrev.com/d87a308d26eac76bcbcf06fc52e1bfa84d153bfb
Cr-Commit-Position: refs/heads/master@{#436962}

[Navid Zolghadr, 2016-11-16 20:07:42]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 20:08:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-11-16 20:09:26]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 20:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-11-16 20:09:57]

Description was changed from

==========
Pass the RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025
==========

to

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025
==========

[Navid Zolghadr, 2016-11-16 20:41:46]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 20:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-11-16 20:45:15]

nzolghadr@chromium.org changed reviewers:
+ creis@chromium.org, sky@chromium.org, tdresser@chromium.org

[Navid Zolghadr, 2016-11-16 20:45:15]

I wanted to pass the RapporService and current page url to the renderer_host for
reporting some metrics using Rappor. There seemed to be quite a lot of different
paths to pass the fields through. I was wondering if this looks okay.

Also I wanted to see whether there is a way to test the RapporService instance
or even if a test is required for that API.

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

creis@chromium.org changed reviewers:
+ nick@chromium.org

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

[+nick for Rappor in content/ question]

I'm a little hesitant to add the dependency, but I could easily be convinced.  I
think we'll probably want to change how it's exposed if we proceed, though.

https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:237: ::rappor::RapporService* getRapporService()
override;
Is there a reason this is listed in the "Constructors, Creation, Showing"
section?  I think it belongs with the other WebCotnentsDelegate overrides around
line 463.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:3: # See comment in content/DEPS for which components are
allowed.
It's not clear to me whether rappor passes the bar mentioned in content/DEPS or
not.  This service only seems to exist in chrome/, so this feels uncomfortably
close to introducing a dependency on chrome/ in content/.

Nick, do you have a sense for whether this is ok?  I know you added some Rappor
metrics and have dealt with the chrome/content divide.  (I'd ask jam@, but he's
OOO.)

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:227: // purposes per
url. See getRapporCommittedUrl for getting the url.
nit: I'm not sure what "for the metric reporting purposes per url" means.  Can
you rephrase?

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:228: virtual
::rappor::RapporService* getRapporService();
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?

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:231: virtual
std::string getRapporCommittedUrl();
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?

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:146: ::rappor::RapporService*
getRapporService() override;
These need to be in the correct override section of the file.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:631:
->getRapporCommittedUrl());
This doesn't seem related to this test.  We should add a separate test for this,
so we don't lose coverage if this unrelated one gets disabled or changed.

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:542: virtual
::rappor::RapporService* getRapporService();
nit: All methods in content/public should be documented.  Especially clarify if
it can return null.

nit: Capitalize name.

[Charlie Reis, 2016-11-16 23:59:54]

Offline, Nick suggested this might fit better on ContentBrowserClient.  In fact,
he found something like it already exists in ContentRendererClient:

https://cs.chromium.org/chromium/src/content/public/renderer/content_renderer...

[ncarter (slow), 2016-11-17 00:08:31]

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:3: # See comment in content/DEPS for which components are
allowed.
On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> It's not clear to me whether rappor passes the bar mentioned in content/DEPS
or
> not.  This service only seems to exist in chrome/, so this feels uncomfortably
> close to introducing a dependency on chrome/ in content/.
> 
> Nick, do you have a sense for whether this is ok?  I know you added some
Rappor
> metrics and have dealt with the chrome/content divide.  (I'd ask jam@, but
he's
> OOO.)

If we've already got rappor metrics plumbed into content/renderer without
needing a DEPS relaxation, then I would expect it should be possible to use a
similar API to get it working from content/browser. If that suffices for this
use case.

In general I think of rappor as being very close to base/metrics, so this
doesn't seem particularly crazy.

[commit-bot: I haz the power, 2016-11-17 00:37:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 00:37:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Navid Zolghadr, 2016-11-17 20:05:50]

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:3: # See comment in content/DEPS for which components are
allowed.
On 2016/11/17 00:08:30, ncarter wrote:
> On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > It's not clear to me whether rappor passes the bar mentioned in content/DEPS
> or
> > not.  This service only seems to exist in chrome/, so this feels
uncomfortably
> > close to introducing a dependency on chrome/ in content/.
> > 
> > Nick, do you have a sense for whether this is ok?  I know you added some
> Rappor
> > metrics and have dealt with the chrome/content divide.  (I'd ask jam@, but
> he's
> > OOO.)
> 
> If we've already got rappor metrics plumbed into content/renderer without
> needing a DEPS relaxation, then I would expect it should be possible to use a
> similar API to get it working from content/browser. If that suffices for this
> use case.
> 
> In general I think of rappor as being very close to base/metrics, so this
> doesn't seem particularly crazy.

The existing methods that we use in content/renderer are recording simple stuff
like a string. Originally I wanted to expose the rappor::sample so one can
record anything. Here is the original review for this having this change and one
example of using it in one of our metrics:
https://codereview.chromium.org/2492793004/

So I should either add the dependency or find a way to expose more APIs that
satisfy our needs for now here
https://cs.chromium.org/chromium/src/components/rappor/rappor_recorder_impl.h...

to the RapporRecorder. Which way do you think is better? Having what I have in
this review or do the other suggestion?

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:228: virtual
::rappor::RapporService* getRapporService();
On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> 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?

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.

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

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:542: virtual
::rappor::RapporService* getRapporService();
On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> nit: All methods in content/public should be documented.  Especially clarify
if
> it can return null.
> 
> nit: Capitalize name.

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:22:36]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:22:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-12-01 18:24:48]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:25:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-12-01 18:27:14]

nzolghadr@chromium.org changed reviewers:
+ holte@chromium.org

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

ptal.
I also cleaned up the record function names.
It seems that it was also possible to do more refactoring in chrome/browser to
use RapporPublicService instead of RapporService in all the places as far as I
quickly checked. I might have missed a few places though. I wasn't sure you
wanted me to fix those instances here as well or just leave them for later. I
left them for now to keep this change small.

https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:237: ::rappor::RapporService* getRapporService()
override;
On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> Is there a reason this is listed in the "Constructors, Creation, Showing"
> section?  I think it belongs with the other WebCotnentsDelegate overrides
around
> line 463.

I removed the changes to this file as per Nick's suggestion for putting things
in ContentBorwserClient.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS#ne...
content/browser/DEPS:3: # See comment in content/DEPS for which components are
allowed.
On 2016/11/17 20:05:49, Navid Zolghadr wrote:
> On 2016/11/17 00:08:30, ncarter wrote:
> > On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > > It's not clear to me whether rappor passes the bar mentioned in
content/DEPS
> > or
> > > not.  This service only seems to exist in chrome/, so this feels
> uncomfortably
> > > close to introducing a dependency on chrome/ in content/.
> > > 
> > > Nick, do you have a sense for whether this is ok?  I know you added some
> > Rappor
> > > metrics and have dealt with the chrome/content divide.  (I'd ask jam@, but
> > he's
> > > OOO.)
> > 
> > If we've already got rappor metrics plumbed into content/renderer without
> > needing a DEPS relaxation, then I would expect it should be possible to use
a
> > similar API to get it working from content/browser. If that suffices for
this
> > use case.
> > 
> > In general I think of rappor as being very close to base/metrics, so this
> > doesn't seem particularly crazy.
> 
> The existing methods that we use in content/renderer are recording simple
stuff
> like a string. Originally I wanted to expose the rappor::sample so one can
> record anything. Here is the original review for this having this change and
one
> example of using it in one of our metrics:
> https://codereview.chromium.org/2492793004/
> 
> So I should either add the dependency or find a way to expose more APIs that
> satisfy our needs for now here
>
https://cs.chromium.org/chromium/src/components/rappor/rappor_recorder_impl.h...
> 
> to the RapporRecorder. Which way do you think is better? Having what I have in
> this review or do the other suggestion?
> 
> 

I added the subclass/interface under component/rappor/public and I managed to
makes the DEPS to use that. Would this be enough? I realized it is also possible
to make use of the abstracted interface in a lot of other places in
chrome/browser as well. I wasn't sure if you wanted to me to refactor those as
well or write a one pager or something for it first or not. I also renamed one
of the functions as holte@ added a TODO in the code.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:227: // purposes per
url. See getRapporCommittedUrl for getting the url.
On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> nit: I'm not sure what "for the metric reporting purposes per url" means.  Can
> you rephrase?

Removed this API altogether.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:231: virtual
std::string getRapporCommittedUrl();
On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> 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?

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.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:146: ::rappor::RapporService*
getRapporService() override;
On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> These need to be in the correct override section of the file.

Done.

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:631:
->getRapporCommittedUrl());
On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> This doesn't seem related to this test.  We should add a separate test for
this,
> so we don't lose coverage if this unrelated one gets disabled or changed.

Done.

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:542: virtual
::rappor::RapporService* getRapporService();
On 2016/11/17 20:39:12, ncarter wrote:
> On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> > nit: All methods in content/public should be documented.  Especially clarify
> if
> > it can return null.
> > 
> > nit: Capitalize name.
> 
> 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?

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.

[commit-bot: I haz the power, 2016-12-01 18:28:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 18:28:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Navid Zolghadr, 2016-12-01 18:45:42]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:46:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 18:50:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 18:50:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Navid Zolghadr, 2016-12-01 18:56:14]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:56:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 19:06:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 19:06:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Navid Zolghadr, 2016-12-01 19:21:48]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:22:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-12-01 19:23:53]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:24:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 19:42:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 19:42:53]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[sky, 2016-12-01 20:14:27]

On 2016/12/01 18:27:15, Navid Zolghadr wrote:
> ptal.
> I also cleaned up the record function names.
> It seems that it was also possible to do more refactoring in chrome/browser to
> use RapporPublicService instead of RapporService in all the places as far as I
> quickly checked. I might have missed a few places though. I wasn't sure you
> wanted me to fix those instances here as well or just leave them for later. I
> left them for now to keep this change small.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
> File chrome/browser/ui/browser.h (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/chrome/browser/ui/brows...
> chrome/browser/ui/browser.h:237: ::rappor::RapporService* getRapporService()
> override;
> On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > Is there a reason this is listed in the "Constructors, Creation, Showing"
> > section?  I think it belongs with the other WebCotnentsDelegate overrides
> around
> > line 463.
> 
> I removed the changes to this file as per Nick's suggestion for putting things
> in ContentBorwserClient.
> 
> https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/DEPS#ne...
> content/browser/DEPS:3: # See comment in content/DEPS for which components are
> allowed.
> On 2016/11/17 20:05:49, Navid Zolghadr wrote:
> > On 2016/11/17 00:08:30, ncarter wrote:
> > > On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > > > It's not clear to me whether rappor passes the bar mentioned in
> content/DEPS
> > > or
> > > > not.  This service only seems to exist in chrome/, so this feels
> > uncomfortably
> > > > close to introducing a dependency on chrome/ in content/.
> > > > 
> > > > Nick, do you have a sense for whether this is ok?  I know you added some
> > > Rappor
> > > > metrics and have dealt with the chrome/content divide.  (I'd ask jam@,
but
> > > he's
> > > > OOO.)
> > > 
> > > If we've already got rappor metrics plumbed into content/renderer without
> > > needing a DEPS relaxation, then I would expect it should be possible to
use
> a
> > > similar API to get it working from content/browser. If that suffices for
> this
> > > use case.
> > > 
> > > In general I think of rappor as being very close to base/metrics, so this
> > > doesn't seem particularly crazy.
> > 
> > The existing methods that we use in content/renderer are recording simple
> stuff
> > like a string. Originally I wanted to expose the rappor::sample so one can
> > record anything. Here is the original review for this having this change and
> one
> > example of using it in one of our metrics:
> > https://codereview.chromium.org/2492793004/
> > 
> > So I should either add the dependency or find a way to expose more APIs that
> > satisfy our needs for now here
> >
>
https://cs.chromium.org/chromium/src/components/rappor/rappor_recorder_impl.h...
> > 
> > to the RapporRecorder. Which way do you think is better? Having what I have
in
> > this review or do the other suggestion?
> > 
> > 
> 
> I added the subclass/interface under component/rappor/public and I managed to
> makes the DEPS to use that. Would this be enough? I realized it is also
possible
> to make use of the abstracted interface in a lot of other places in
> chrome/browser as well. I wasn't sure if you wanted to me to refactor those as
> well or write a one pager or something for it first or not. I also renamed one
> of the functions as holte@ added a TODO in the code.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_delegate.h (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_delegate.h:227: // purposes
per
> url. See getRapporCommittedUrl for getting the url.
> On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > nit: I'm not sure what "for the metric reporting purposes per url" means. 
Can
> > you rephrase?
> 
> Removed this API altogether.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_delegate.h:231: virtual
> std::string getRapporCommittedUrl();
> On 2016/11/16 23:55:21, Charlie Reis (slow) wrote:
> > 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?
> 
> 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.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl.h:146: ::rappor::RapporService*
> getRapporService() override;
> On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> > These need to be in the correct override section of the file.
> 
> Done.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
> File content/browser/web_contents/web_contents_impl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl_browsertest.cc:631:
> ->getRapporCommittedUrl());
> On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> > This doesn't seem related to this test.  We should add a separate test for
> this,
> > so we don't lose coverage if this unrelated one gets disabled or changed.
> 
> Done.
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
> File content/public/browser/web_contents_delegate.h (right):
> 
>
https://codereview.chromium.org/2510803003/diff/40001/content/public/browser/...
> content/public/browser/web_contents_delegate.h:542: virtual
> ::rappor::RapporService* getRapporService();
> On 2016/11/17 20:39:12, ncarter wrote:
> > On 2016/11/16 23:55:22, Charlie Reis (slow) wrote:
> > > nit: All methods in content/public should be documented.  Especially
clarify
> > if
> > > it can return null.
> > > 
> > > nit: Capitalize name.
> > 
> > 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?
> 
> 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.

Navid, you've listed a bunch of reviewers. Please indicate who needs to review
which files.
Thanks,

  -Scott

[Navid Zolghadr, 2016-12-01 20:27:13]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:27:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-12-01 20:37:23]

> Navid, you've listed a bunch of reviewers. Please indicate who needs to review
> which files.
> Thanks,
> 
>   -Scott

Yeah. You are right. I'm sorry. The CL used to be much smaller and there was a
slightly important change in Chrome/browser in the first patch which I added you
as the owner. But after that it is all just renaming/adding a few includes in
chrome/browser. I wasn't sure if I should remove you then or not. Anyway.

sky@chromium.org: Please review changes in 
chrome/browser/*

cries@chromium.org: Please review changes in 
content/*

holte@chromium.org, ncarter@chromium.org: Please review changes in
components/rappor/*

[commit-bot: I haz the power, 2016-12-01 20:45:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 20:45:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ncarter (slow), 2016-12-01 21:14:00]

nick@chromium.org changed reviewers:
- creis@chromium.org

[ncarter (slow), 2016-12-01 21:14:02]

[-charlie] Since I'm reviewing the CL anyway, I'll take care of content/
approval too

[Steven Holte, 2016-12-01 21:25:41]

lgtm, thanks!

[ncarter (slow), 2016-12-01 22:33:33]

Thanks so much for doing this overhaul. I'm happy with the direction here.

Please update the CL description.

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:1: 
Remove.

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:640:
std::move(sample));
Indentation needs fixing (or git cl format).

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/tab_con...
File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/tab_con...
chrome/browser/tab_contents/navigation_metrics_recorder.cc:1: 
Remove.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/BUILD.gn (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/BUILD.gn:5: source_set("public") {
Do you reference this anywhere?

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_parameters.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_parameters.h:6: #define
COMPONENTS_RAPPOR_RAPPOR_PARAMETERS_H_
Update this guard (PSA: when moving files around, use scripts like:
https://cs.chromium.org/chromium/src/tools/git/move_source_file.py , they take
care of this)

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_parameters.h:130: {128 /* Num cohorts */, 4 /*
Bloom filter size bytes */,
totally optional: if you want to preserve the old one-per-line formatting,
clang-format is happy with this:

    // UMA_RAPPOR_TYPE
    {
        128,                   // Num cohorts
        4,                     // Bloom filter size bytes
        2,                     // Bloom filter hash count
        rappor::NORMAL_NOISE,  // Noise level
        UMA_RAPPOR_GROUP,      // Recording group
    },

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_public_service.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:19: class RapporPublicService :
public base::SupportsWeakPtr<RapporService> {
I'm not opposed to this name (and ultimately it's a components/rappor OWNER's
call). But the following name pairs strike me as being more conventional
choices:

public/ RapporService & non-public RapporServiceImpl

or 

public/ RapporServiceInterface & non-public RapporService

or call it something based on what features it actually exposes (something like
rappor::SampleCollector).

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_utils.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_utils.h:6: #define
COMPONENTS_RAPPOR_RAPPOR_UTILS_H_
Update this guard.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/sample.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/sample.h:6: #define COMPONENTS_RAPPOR_SAMPLE_H_
Update this guard

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
File components/rappor/rappor_parameters.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
components/rappor/rappor_parameters.cc:5: #include
"components/rappor/public/rappor_parameters.h"
What prevents us from moving the .cc file as well in this case?

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.cc:96: return nullptr;
?!

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:240: virtual
std::string GetRapporCommittedUrl();
As we were discussing before, this looks out of place here here because the URL
is not widget state -- widgets don't navigate or load things from the network.

However, I had a look at your motivating CL, and understand now the problem that
this solves. What's happening is that we have gathered some stats way down in a
stats-tracking helper, and we need to percolate up to the delegate to add the
eTLD+1 context.

What if we structured the method like this:

// Augment a Rappor sample with eTLD+1 context. The caller is still responsible
// for logging the sample to the RapporService. Returns false if the eTLD+1
// is not known for |render_widget_host|.
virtual bool AddDomainInfoToRapporSample(
    RenderWidgetHostImpl* render_widget_host,
    rappor::Sample* sample);

Does that work?

[sky, 2016-12-01 23:02:19]

LGTM

[Navid Zolghadr, 2016-12-02 20:47:35]

ptal.

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:1: 
On 2016/12/01 22:33:31, ncarter wrote:
> Remove.

Done.

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:640:
std::move(sample));
On 2016/12/01 22:33:32, ncarter wrote:
> Indentation needs fixing (or git cl format).

Done.

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/tab_con...
File chrome/browser/tab_contents/navigation_metrics_recorder.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/chrome/browser/tab_con...
chrome/browser/tab_contents/navigation_metrics_recorder.cc:1: 
On 2016/12/01 22:33:32, ncarter wrote:
> Remove.

Done.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/BUILD.gn (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/BUILD.gn:5: source_set("public") {
On 2016/12/01 22:33:32, ncarter wrote:
> Do you reference this anywhere?

There is one reference from components/rappor/BUILD.gn. But I wasn't very
confident what to put in this file. I mainly looked around and found similar
stuff. Should I change this to something else?

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_parameters.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_parameters.h:6: #define
COMPONENTS_RAPPOR_RAPPOR_PARAMETERS_H_
On 2016/12/01 22:33:32, ncarter wrote:
> Update this guard (PSA: when moving files around, use scripts like:
> https://cs.chromium.org/chromium/src/tools/git/move_source_file.py , they take
> care of this)

Oops. I wish I knew this before. Thanks for letting me know. I'll keep this in
mind. I fixed it manually this time.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_parameters.h:130: {128 /* Num cohorts */, 4 /*
Bloom filter size bytes */,
On 2016/12/01 22:33:32, ncarter wrote:
> totally optional: if you want to preserve the old one-per-line formatting,
> clang-format is happy with this:
> 
>     // UMA_RAPPOR_TYPE
>     {
>         128,                   // Num cohorts
>         4,                     // Bloom filter size bytes
>         2,                     // Bloom filter hash count
>         rappor::NORMAL_NOISE,  // Noise level
>         UMA_RAPPOR_GROUP,      // Recording group
>     },

I guess git cl format made it like this. I changed it back to what it was. It
looked better before.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_public_service.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/12/01 22:33:32, ncarter wrote:
> 2016

Done.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:19: class RapporPublicService :
public base::SupportsWeakPtr<RapporService> {
On 2016/12/01 22:33:32, ncarter wrote:
> I'm not opposed to this name (and ultimately it's a components/rappor OWNER's
> call). But the following name pairs strike me as being more conventional
> choices:
> 
> public/ RapporService & non-public RapporServiceImpl
> 
> or 
> 
> public/ RapporServiceInterface & non-public RapporService
> 
> or call it something based on what features it actually exposes (something
like
> rappor::SampleCollector).

Although holte@ gave lgtm but I believe what you are suggesting is better that
what it is. I used RapporService and RapporServiceImpl. This made this change a
little bigger. But I have the renaming in a separate commit from the rest of
your comments. Let me know if you would like me to break the renaming into
another CL or is it good to keep it as is.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_utils.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_utils.h:6: #define
COMPONENTS_RAPPOR_RAPPOR_UTILS_H_
On 2016/12/01 22:33:32, ncarter wrote:
> Update this guard.

Done.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/sample.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/sample.h:6: #define COMPONENTS_RAPPOR_SAMPLE_H_
On 2016/12/01 22:33:32, ncarter wrote:
> Update this guard

Done.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
File components/rappor/rappor_parameters.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
components/rappor/rappor_parameters.cc:5: #include
"components/rappor/public/rappor_parameters.h"
On 2016/12/01 22:33:32, ncarter wrote:
> What prevents us from moving the .cc file as well in this case?

Nothing I believe. I was under the impression that you wanted only the header
files in that public folder to make it like all the prototypes and no
implementation. Do you want me to move this there too?

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.cc:96: return nullptr;
On 2016/12/01 22:33:32, ncarter wrote:
> ?!

Oh! I see. Fixed.

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:240: virtual
std::string GetRapporCommittedUrl();
On 2016/12/01 22:33:32, ncarter wrote:
> As we were discussing before, this looks out of place here here because the
URL
> is not widget state -- widgets don't navigate or load things from the network.
> 
> However, I had a look at your motivating CL, and understand now the problem
that
> this solves. What's happening is that we have gathered some stats way down in
a
> stats-tracking helper, and we need to percolate up to the delegate to add the
> eTLD+1 context.
> 
> What if we structured the method like this:
> 
> // Augment a Rappor sample with eTLD+1 context. The caller is still
responsible
> // for logging the sample to the RapporService. Returns false if the eTLD+1
> // is not known for |render_widget_host|.
> virtual bool AddDomainInfoToRapporSample(
>     RenderWidgetHostImpl* render_widget_host,
>     rappor::Sample* sample);
> 
> Does that work?

Sure. Sounds good. But why do I need RenderWidgetHostImpl*?

[ncarter (slow), 2016-12-05 19:50:50]

lgtm

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/BUILD.gn (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/BUILD.gn:5: source_set("public") {
On 2016/12/02 20:47:35, Navid Zolghadr wrote:
> On 2016/12/01 22:33:32, ncarter wrote:
> > Do you reference this anywhere?
> 
> There is one reference from components/rappor/BUILD.gn. But I wasn't very
> confident what to put in this file. I mainly looked around and found similar
> stuff. Should I change this to something else?

I'm kind of clueless too about BUILD.gn. I'm basically okay with anything that
compiles.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
File components/rappor/public/rappor_public_service.h (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:19: class RapporPublicService :
public base::SupportsWeakPtr<RapporService> {
On 2016/12/02 20:47:35, Navid Zolghadr wrote:
> On 2016/12/01 22:33:32, ncarter wrote:
> > I'm not opposed to this name (and ultimately it's a components/rappor
OWNER's
> > call). But the following name pairs strike me as being more conventional
> > choices:
> > 
> > public/ RapporService & non-public RapporServiceImpl
> > 
> > or 
> > 
> > public/ RapporServiceInterface & non-public RapporService
> > 
> > or call it something based on what features it actually exposes (something
> like
> > rappor::SampleCollector).
> 
> Although holte@ gave lgtm but I believe what you are suggesting is better that
> what it is. I used RapporService and RapporServiceImpl. This made this change
a
> little bigger. But I have the renaming in a separate commit from the rest of
> your comments. Let me know if you would like me to break the renaming into
> another CL or is it good to keep it as is.

Thanks for doing the rename.

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
File components/rappor/rappor_parameters.cc (right):

https://codereview.chromium.org/2510803003/diff/160001/components/rappor/rapp...
components/rappor/rappor_parameters.cc:5: #include
"components/rappor/public/rappor_parameters.h"
On 2016/12/02 20:47:35, Navid Zolghadr wrote:
> On 2016/12/01 22:33:32, ncarter wrote:
> > What prevents us from moving the .cc file as well in this case?
> 
> Nothing I believe. I was under the impression that you wanted only the header
> files in that public folder to make it like all the prototypes and no
> implementation. Do you want me to move this there too?

Up to you -- this isn't strictly against the style guide, but I think the most
common thing is for .cc files to be located as close to their .h files as
possible.

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2510803003/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:240: virtual
std::string GetRapporCommittedUrl();
On 2016/12/02 20:47:35, Navid Zolghadr wrote:
> On 2016/12/01 22:33:32, ncarter wrote:
> > As we were discussing before, this looks out of place here here because the
> URL
> > is not widget state -- widgets don't navigate or load things from the
network.
> > 
> > However, I had a look at your motivating CL, and understand now the problem
> that
> > this solves. What's happening is that we have gathered some stats way down
in
> a
> > stats-tracking helper, and we need to percolate up to the delegate to add
the
> > eTLD+1 context.
> > 
> > What if we structured the method like this:
> > 
> > // Augment a Rappor sample with eTLD+1 context. The caller is still
> responsible
> > // for logging the sample to the RapporService. Returns false if the eTLD+1
> > // is not known for |render_widget_host|.
> > virtual bool AddDomainInfoToRapporSample(
> >     RenderWidgetHostImpl* render_widget_host,
> >     rappor::Sample* sample);
> > 
> > Does that work?
> 
> Sure. Sounds good. But why do I need RenderWidgetHostImpl*?

There can be arbitrarily many RenderWidgetHosts inside a WebContents, all
sharing a delegate, so I default to expecting that methods on the delegate will
pass the pointer. However, it's okay to plumb this as needed.

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/rappor_public_service.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016 (I think you missed this)

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:5: #ifndef
COMPONENTS_RAPPOR_PUBLIC_RAPPOR_PUBLIC_SERVICE_H_
This file should be renamed to rappor/public/rappor_service.h right?

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/rappor_utils.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_utils.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/sample.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/sample.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/rapp...
File components/rappor/rappor_metric.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/rapp...
components/rappor/rappor_metric.h:23: // using RapporServiceImpl::RecordSample
instead.
RapporServiceImpl -> RapporService here, or list both?

[Navid Zolghadr, 2016-12-06 19:10:04]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[Navid Zolghadr, 2016-12-06 19:10:33]

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/rappor_public_service.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/12/05 19:50:50, ncarter wrote:
> 2016 (I think you missed this)

Done.

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_public_service.h:5: #ifndef
COMPONENTS_RAPPOR_PUBLIC_RAPPOR_PUBLIC_SERVICE_H_
On 2016/12/05 19:50:50, ncarter wrote:
> This file should be renamed to rappor/public/rappor_service.h right?

Done.

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/rappor_utils.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/rappor_utils.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/12/05 19:50:50, ncarter wrote:
> 2016

Done.

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
File components/rappor/public/sample.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/publ...
components/rappor/public/sample.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/12/05 19:50:50, ncarter wrote:
> 2016

Done.

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/rapp...
File components/rappor/rappor_metric.h (right):

https://codereview.chromium.org/2510803003/diff/180001/components/rappor/rapp...
components/rappor/rappor_metric.h:23: // using RapporServiceImpl::RecordSample
instead.
On 2016/12/05 19:50:50, ncarter wrote:
> RapporServiceImpl -> RapporService here, or list both?

I'll list them both for now.

[commit-bot: I haz the power, 2016-12-06 19:10:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-12-06 19:17:20]

girard@chromium.org: Please review changes in 
content/browser/android/vr_shell/*

boliu@chromium.org: Please review changes in 
android_webview/*

estark@chromium.org: Please review changes in 
components/security_interstitials/core/*

gcasto@chromium.org: Please review changes in 
components/autofill/*

droger@chromium.org: Please review changes in 
ios/chrome/*

pkasting@chromium.org: Please review changes in 
components/search_engines/*

[commit-bot: I haz the power, 2016-12-06 19:25:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 19:25:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Navid Zolghadr, 2016-12-06 20:08:08]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 20:08:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 20:33:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 20:33:41]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Navid Zolghadr, 2016-12-06 20:37:07]

The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 20:37:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 21:44:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 21:44:16]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2016-12-06 21:50:39]

nzolghadr@chromium.org changed reviewers:
+ boliu@chromium.org, droger@chromium.org, estark@chromium.org,
gcasto@chromium.org, girard@chromium.org, pkasting@chromium.org

[Navid Zolghadr, 2016-12-06 21:50:41]

girard@chromium.org: Please review changes in 
content/browser/android/vr_shell/*

boliu@chromium.org: Please review changes in 
android_webview/*

estark@chromium.org: Please review changes in 
components/security_interstitials/core/*

gcasto@chromium.org: Please review changes in 
components/autofill/*

droger@chromium.org: Please review changes in 
ios/chrome/*

pkasting@chromium.org: Please review changes in 
components/search_engines/*

[boliu, 2016-12-06 22:22:39]

On 2016/12/06 21:50:41, Navid Zolghadr wrote:
> mailto:girard@chromium.org: Please review changes in 
> content/browser/android/vr_shell/*
> 
> mailto:boliu@chromium.org: Please review changes in 
> android_webview/*

rs lgtm

> 
> mailto:estark@chromium.org: Please review changes in 
> components/security_interstitials/core/*
> 
> mailto:gcasto@chromium.org: Please review changes in 
> components/autofill/*
> 
> mailto:droger@chromium.org: Please review changes in 
> ios/chrome/*
> 
> mailto:pkasting@chromium.org: Please review changes in 
> components/search_engines/*

[estark, 2016-12-07 00:45:03]

components/security_interstitials lgtm

https://codereview.chromium.org/2510803003/diff/240001/components/security_in...
File components/security_interstitials/core/metrics_helper.cc (right):

https://codereview.chromium.org/2510803003/diff/240001/components/security_in...
components/security_interstitials/core/metrics_helper.cc:120: const GURL&
request_url,
did `git cl format` do this?

[Navid Zolghadr, 2016-12-07 00:49:34]

https://codereview.chromium.org/2510803003/diff/240001/components/security_in...
File components/security_interstitials/core/metrics_helper.cc (right):

https://codereview.chromium.org/2510803003/diff/240001/components/security_in...
components/security_interstitials/core/metrics_helper.cc:120: const GURL&
request_url,
On 2016/12/07 00:45:02, estark wrote:
> did `git cl format` do this?

I certainly did not do it manually. I just re-ran "git cl format" locally and it
didn't change it. So I assume this is the format it liked and it changed it the
first time I ran the command on this patch.

[girard, 2016-12-07 02:01:20]

lgtm for VR-Shell

[girard, 2016-12-07 02:01:24]

lgtm for VR-Shell

[Peter Kasting, 2016-12-07 04:50:41]

LGTM

[Navid Zolghadr, 2016-12-07 15:33:23]

Description was changed from

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025
==========

to

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025

TBR=droger@chromium.org, gcasto@chromium.org
==========

[Navid Zolghadr, 2016-12-07 15:33:45]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-12-07 15:33:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
holte@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2510803003/#ps240001
(title: "Fix more compile errors in JNI files")

[commit-bot: I haz the power, 2016-12-07 15:34:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 15:46:00]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481124825274160,
"parent_rev": "5af1b7a8d63056de514edd6246acff186389658e", "commit_rev":
"4df3a6dd344933ce2f38b358b4a762385c594bc9"}

[commit-bot: I haz the power, 2016-12-07 15:46:26]

Description was changed from

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025

TBR=droger@chromium.org, gcasto@chromium.org
==========

to

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025

TBR=droger@chromium.org, gcasto@chromium.org
==========

[commit-bot: I haz the power, 2016-12-07 15:46:29]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-07 15:49:39]

Description was changed from

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025

TBR=droger@chromium.org, gcasto@chromium.org
==========

to

==========
Pass RapporService to content/browser/

Pass RapporService instance along with the
current page url to content/browser/renderer_host
to later use it in some metrics.

BUG=666025

TBR=droger@chromium.org, gcasto@chromium.org

Committed: https://crrev.com/d87a308d26eac76bcbcf06fc52e1bfa84d153bfb
Cr-Commit-Position: refs/heads/master@{#436962}
==========

[commit-bot: I haz the power, 2016-12-07 15:49:44]

Patchset 13 (id:??) landed as
https://crrev.com/d87a308d26eac76bcbcf06fc52e1bfa84d153bfb
Cr-Commit-Position: refs/heads/master@{#436962}