Proxy private UMA pepper interface for out-of-process and NaCl plugins.

Proxy private UMA pepper interface for out-of-process and NaCl plugins.

BUG=317833
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246962

[bbudge, 2013-11-14 01:13:16]

High level comment: It might speed reviews to break out the IDL and C++ wrapper
into a separate CL.

Since you're changing the API, you should probably version it (0.1, and now
0.2). Also, it's preferable to generate the thunk code. I'm not sure how the
generator would handle an API like this consisting of 'static' or 'singleton'
functions.

[elijahtaylor1, 2013-11-27 01:07:56]

@yzshen:  PTAL.  This CL is the chrome side of the test case for the issue I was
telling you about, that different types of plugins (in-process, out-of-process,
nacl) only have access to certain host implementations.  I'm emailing you and
Bill a link to download the app I'm testing with since it's private.

If you use the RENDERER impl (chrome/renderer host) like I have enabled in the
latest patchset, in-process and out-of-process trusted plugins can use it, but
nacl plugins cannot.  If you use the BROWSER impl (chrome/browser host),
in-process plugins cannot use it, but out-of-process and nacl plugins can.

What is the path of least resistance here, to enable in-process plugins access
to the chrome/browser host, or allow nacl plugins access to the chrome/renderer
host?  I suspect it would be the former.  WDYT?

https://codereview.chromium.org/61643022/diff/80001/ppapi/proxy/uma_private_r...
File ppapi/proxy/uma_private_resource.cc (right):

https://codereview.chromium.org/61643022/diff/80001/ppapi/proxy/uma_private_r...
ppapi/proxy/uma_private_resource.cc:30: SendCreate(RENDERER,
PpapiHostMsg_UMA_Create());
you can now change all of these to BROWSER to switch to the browser impl in this
CL.  (obviously we'll strip out all implementations but one when we settle on
the right thing to do)

[elijahtaylor1, 2013-11-28 00:54:03]

@bbudge, yzshen: PTAL, the implementation now works (thanks Yuzhu!)

I'm going to get others to review the NaCl/PNaCl changes, so feel free to
ignore, I'd like to get the Pepper stuff straightened out first.

Sorry about bundling the C++ interface in here too, it was just much easier to
do it all at once and update the NaCl plugin to use it instead of doing two
changes to the NaCl plugin.

[yzshen1, 2013-12-04 19:32:51]

Mostly looks good, only some nits.

Thanks!

https://codereview.chromium.org/61643022/diff/120001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/chrome_renderer.g...
chrome/chrome_renderer.gypi:273: 'renderer/pepper/pepper_uma_renderer_host.h',
nit: you could probably call it pepper_uma_host to be consistent with others.
(pepper_flash_renderer_host has renderer in the name because it also has a
browser host.)

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.h:166: const GURL& url, const
std::set<std::string>& whitelist);
nit:
- wrong indent.
- please include <set>

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_renderer_host.cc (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:75: bool is_whitelisted =
please move this to be right above where it is used.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:95: int64_t sample,
Do you need to validate the inputs? (>0; min <= max; etc.)

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_renderer_host.h (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:11: #include
"base/files/file_path.h"
no need to have this one, right?

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:15: #include
"ppapi/host/resource_message_filter.h"
we don't need this one, right?

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:30:
PepperUMARendererHost(content::RendererPpapiHost* host, PP_Instance instance,
one parameter on each line, please (this is required for declaration and
definition, but not invocation)

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:32: 
redundant line.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:40: virtual
~PepperUMARendererHost();
It seems we don't have to make it a private method. Or you would like to make
sure we always delete the object with a pointer of ResourceHost?

https://codereview.chromium.org/61643022/diff/120001/content/renderer/pepper/...
File content/renderer/pepper/ppb_uma_private_impl.h (left):

https://codereview.chromium.org/61643022/diff/120001/content/renderer/pepper/...
content/renderer/pepper/ppb_uma_private_impl.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
weird, the file is not showed as deleted.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
File ppapi/cpp/private/uma_private.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:19: virtual ~UMAPrivate();
it doesn't require a virtual constructor, right?

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:21: int32_t HistogramCustomTimes(const
std::string& name,
It seems there is no need to return int32_t, now that the C interface returns
void.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
It seems a little bit weird to use const for these methods, semantically they do
change internal state.

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:108: void
Plugin::HistogramTimeSmall(const std::string& name,
Please keep the definitions in the same order as declarations in the .h file.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1214: uint32_t /* bucket count */)
nit: for consistency, please use bucket_count (and also line 1220).

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.cc (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:10: #include
"ppapi/shared_impl/host_resource.h"
this is not necessary, right?

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:12: #include "ppapi/thunk/enter.h"
ditto

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:16: std::string StringFromPPVar(PP_Var var)
{
const &, please.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:18: ppapi::StringVar::FromPPVar(var);
Please check NULL before using it.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:47: 
no need to have this line (all methods below)

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:53: min,
wrong indent (all methods below)

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.h:4: //
redundant line?

[bbudge, 2013-12-04 21:08:29]

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_renderer_host.cc (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:81: histogram.substr(0,
nacl_prefix.size()) == nacl_prefix) {
You could avoid some string creation by using find:
histogram.find("NaCl.") == 0

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:85: if (is_whitelisted &&
allowed_histograms_.count(HashHistogram(histogram)) > 0)
allowed_histograms_.find is clearer and potentially faster.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
File ppapi/cpp/private/uma_private.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
On 2013/12/04 19:32:51, yzshen1 wrote:
> It seems a little bit weird to use const for these methods, semantically they
do
> change internal state.

These are really static methods. Why not define them that way, taking an
InstanceHandle as the first parameter? See:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/dev/font...
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/network_...

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
File ppapi/thunk/ppb_uma_singleton_api.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
ppapi/thunk/ppb_uma_singleton_api.h:9: #include
"ppapi/shared_impl/tracked_callback.h"
I don't think you need this.

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
ppapi/thunk/ppb_uma_singleton_api.h:13: 
extra sp.

[elijahtaylor1, 2013-12-21 02:26:21]

Thanks for all the great feedback, sorry my turnaround time was really bad.

I'm OOO until the new year, so please only look at this CL before then if you
really need something to do :)

Otherwise, we'll pick this up in the new year!

https://codereview.chromium.org/61643022/diff/120001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/chrome_renderer.g...
chrome/chrome_renderer.gypi:273: 'renderer/pepper/pepper_uma_renderer_host.h',
On 2013/12/04 19:32:51, yzshen1 wrote:
> nit: you could probably call it pepper_uma_host to be consistent with others.
> (pepper_flash_renderer_host has renderer in the name because it also has a
> browser host.)

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.h:166: const GURL& url, const
std::set<std::string>& whitelist);
On 2013/12/04 19:32:51, yzshen1 wrote:
> nit:
> - wrong indent.
> - please include <set>

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_renderer_host.cc (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:75: bool is_whitelisted =
On 2013/12/04 19:32:51, yzshen1 wrote:
> please move this to be right above where it is used.

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:81: histogram.substr(0,
nacl_prefix.size()) == nacl_prefix) {
On 2013/12/04 21:08:30, bbudge1 wrote:
> You could avoid some string creation by using find:
> histogram.find("NaCl.") == 0

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:85: if (is_whitelisted &&
allowed_histograms_.count(HashHistogram(histogram)) > 0)
On 2013/12/04 21:08:30, bbudge1 wrote:
> allowed_histograms_.find is clearer and potentially faster.

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.cc:95: int64_t sample,
On 2013/12/04 19:32:51, yzshen1 wrote:
> Do you need to validate the inputs? (>0; min <= max; etc.)

Brought over a similar macro that was in the in-process impl before.  (Didn't
want to duplicate the checks in each function, and didn't want to make a
template function because the arg types are all over the place)

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_renderer_host.h (right):

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:11: #include
"base/files/file_path.h"
On 2013/12/04 19:32:51, yzshen1 wrote:
> no need to have this one, right?

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:15: #include
"ppapi/host/resource_message_filter.h"
On 2013/12/04 19:32:51, yzshen1 wrote:
> we don't need this one, right?

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:30:
PepperUMARendererHost(content::RendererPpapiHost* host, PP_Instance instance,
On 2013/12/04 19:32:51, yzshen1 wrote:
> one parameter on each line, please (this is required for declaration and
> definition, but not invocation)

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:32: 
On 2013/12/04 19:32:51, yzshen1 wrote:
> redundant line.

Done.

https://codereview.chromium.org/61643022/diff/120001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_renderer_host.h:40: virtual
~PepperUMARendererHost();
On 2013/12/04 19:32:51, yzshen1 wrote:
> It seems we don't have to make it a private method. Or you would like to make
> sure we always delete the object with a pointer of ResourceHost?

Done.

https://codereview.chromium.org/61643022/diff/120001/content/renderer/pepper/...
File content/renderer/pepper/ppb_uma_private_impl.h (left):

https://codereview.chromium.org/61643022/diff/120001/content/renderer/pepper/...
content/renderer/pepper/ppb_uma_private_impl.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2013/12/04 19:32:51, yzshen1 wrote:
> weird, the file is not showed as deleted.

Yeah, it's deleted in my git repo, hopefully a codereview problem only.  If it's
still there after submit I'll try again to remove them.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
File ppapi/cpp/private/uma_private.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:19: virtual ~UMAPrivate();
On 2013/12/04 19:32:51, yzshen1 wrote:
> it doesn't require a virtual constructor, right?

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:21: int32_t HistogramCustomTimes(const
std::string& name,
On 2013/12/04 19:32:51, yzshen1 wrote:
> It seems there is no need to return int32_t, now that the C interface returns
> void.

This was mostly to be able to return PP_ERROR_NOINTERFACE.  Do you think I
should just change to return; ?

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
On 2013/12/04 21:08:30, bbudge1 wrote:
> On 2013/12/04 19:32:51, yzshen1 wrote:
> > It seems a little bit weird to use const for these methods, semantically
they
> do
> > change internal state.
> 
> These are really static methods. Why not define them that way, taking an
> InstanceHandle as the first parameter? See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/dev/font...
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/network_...

I can do that, but I'm wondering if you would accept some pushback on this.  The
main reason I made this C++ class was to hold the instance to make it easier for
code using this interface.  If each method required an InstanceHandle, they
might as well just use the C interface and pass a PP_Instance to every method.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
On 2013/12/04 19:32:51, yzshen1 wrote:
> It seems a little bit weird to use const for these methods, semantically they
do
> change internal state.

They change internal state of chrome, but not of the class.  I can remove the
const if you want.

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:108: void
Plugin::HistogramTimeSmall(const std::string& name,
On 2013/12/04 19:32:51, yzshen1 wrote:
> Please keep the definitions in the same order as declarations in the .h file.

These Histogram functions were in the correct order (among the Histogram
functions), but I moved them down to above LoadNaClModuleCommon (and switched
that function with StartSelLdrOnMainThread for .h consistency).  There is not
wide agreement though between the order in the .h and .cc file for this class,
and did not want to reorder the whole file and confuse things.  Let me know if
this is sufficient or if I misunderstood something.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1214: uint32_t /* bucket count */)
On 2013/12/04 19:32:51, yzshen1 wrote:
> nit: for consistency, please use bucket_count (and also line 1220).

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.cc (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:10: #include
"ppapi/shared_impl/host_resource.h"
On 2013/12/04 19:32:51, yzshen1 wrote:
> this is not necessary, right?

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:12: #include "ppapi/thunk/enter.h"
On 2013/12/04 19:32:51, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:16: std::string StringFromPPVar(PP_Var var)
{
On 2013/12/04 19:32:51, yzshen1 wrote:
> const &, please.

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:18: ppapi::StringVar::FromPPVar(var);
On 2013/12/04 19:32:51, yzshen1 wrote:
> Please check NULL before using it.

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:47: 
On 2013/12/04 19:32:51, yzshen1 wrote:
> no need to have this line (all methods below)

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:53: min,
On 2013/12/04 19:32:51, yzshen1 wrote:
> wrong indent (all methods below)

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.h:4: //
On 2013/12/04 19:32:51, yzshen1 wrote:
> redundant line?

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
File ppapi/thunk/ppb_uma_singleton_api.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
ppapi/thunk/ppb_uma_singleton_api.h:9: #include
"ppapi/shared_impl/tracked_callback.h"
On 2013/12/04 21:08:30, bbudge1 wrote:
> I don't think you need this.

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/thunk/ppb_uma_sing...
ppapi/thunk/ppb_uma_singleton_api.h:13: 
On 2013/12/04 21:08:30, bbudge1 wrote:
> extra sp.

Done.

[elijahtaylor1, 2014-01-07 19:44:34]

PTAL

[yzshen1, 2014-01-08 00:26:23]

Only a few more nits.
Thanks!

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
File ppapi/cpp/private/uma_private.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:21: int32_t HistogramCustomTimes(const
std::string& name,
Maybe we can have a static bool IsAvailable() method, and just return void for
all the other methods.
We already have some classes that have an IsAvailable() method.

What do you think?

On 2013/12/21 02:26:21, elijahtaylor1 wrote:
> On 2013/12/04 19:32:51, yzshen1 wrote:
> > It seems there is no need to return int32_t, now that the C interface
returns
> > void.
> 
> This was mostly to be able to return PP_ERROR_NOINTERFACE.  Do you think I
> should just change to return; ?

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
I understand your point. I just think that would be more consistent with other
classes.

On 2013/12/21 02:26:21, elijahtaylor1 wrote:
> On 2013/12/04 19:32:51, yzshen1 wrote:
> > It seems a little bit weird to use const for these methods, semantically
they
> do
> > change internal state.
> 
> They change internal state of chrome, but not of the class.  I can remove the
> const if you want.

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:108: void
Plugin::HistogramTimeSmall(const std::string& name,
Yeah. It is good enough for me. Thanks!
On 2013/12/21 02:26:21, elijahtaylor1 wrote:
> On 2013/12/04 19:32:51, yzshen1 wrote:
> > Please keep the definitions in the same order as declarations in the .h
file.
> 
> These Histogram functions were in the correct order (among the Histogram
> functions), but I moved them down to above LoadNaClModuleCommon (and switched
> that function with StartSelLdrOnMainThread for .h consistency).  There is not
> wide agreement though between the order in the .h and .cc file for this class,
> and did not want to reorder the whole file and confuse things.  Let me know if
> this is sufficient or if I misunderstood something.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_host.h (right):

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit: Please consider changing to 2014 (here and other new files)

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:29: PP_Instance instance,
nit: Please either align with content::... on the previous line or move the
first param to a separate line.

https://codereview.chromium.org/61643022/diff/140001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.cc (right):

https://codereview.chromium.org/61643022/diff/140001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:17: CHECK(name_stringvar.get());
Maybe it is sufficient to do if (!...) return std::string()?

This |var| is from a public API, so I think it is okay to be a little bit
tolerant.

[bbudge, 2014-01-08 19:01:49]

Looks pretty good. Just nits.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/c...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/c...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:114: // time of
the corresponding instance's methods calls.  Currently these
nit: s/methods/method

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_host.h (right):

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:37: virtual ~PepperUMAHost();
It's more conventional for this to follow the ctor. These should also be grouped
together in the .cc file.

https://codereview.chromium.org/61643022/diff/140001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/61643022/diff/140001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:86: 
Did you intentionally move this here? It seems better to have it after all the
constant definitions, at the end of the anonymous namespace.

[elijahtaylor1, 2014-01-08 23:59:40]

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
File ppapi/cpp/private/uma_private.h (right):

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:21: int32_t HistogramCustomTimes(const
std::string& name,
On 2014/01/08 00:26:24, yzshen1 wrote:
> Maybe we can have a static bool IsAvailable() method, and just return void for
> all the other methods.
> We already have some classes that have an IsAvailable() method.
> 
> What do you think?
> 
> On 2013/12/21 02:26:21, elijahtaylor1 wrote:
> > On 2013/12/04 19:32:51, yzshen1 wrote:
> > > It seems there is no need to return int32_t, now that the C interface
> returns
> > > void.
> > 
> > This was mostly to be able to return PP_ERROR_NOINTERFACE.  Do you think I
> > should just change to return; ?
> 

Done.

https://codereview.chromium.org/61643022/diff/120001/ppapi/cpp/private/uma_pr...
ppapi/cpp/private/uma_private.h:25: uint32_t bucket_count) const;
On 2014/01/08 00:26:24, yzshen1 wrote:
> I understand your point. I just think that would be more consistent with other
> classes.
> 
> On 2013/12/21 02:26:21, elijahtaylor1 wrote:
> > On 2013/12/04 19:32:51, yzshen1 wrote:
> > > It seems a little bit weird to use const for these methods, semantically
> they
> > do
> > > change internal state.
> > 
> > They change internal state of chrome, but not of the class.  I can remove
the
> > const if you want.
> 

Done.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/c...
File chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/c...
chrome/renderer/pepper/chrome_renderer_pepper_host_factory.cc:114: // time of
the corresponding instance's methods calls.  Currently these
On 2014/01/08 19:01:49, bbudge wrote:
> nit: s/methods/method

Done.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pepper_uma_host.h (right):

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/01/08 00:26:24, yzshen1 wrote:
> nit: Please consider changing to 2014 (here and other new files)

Done.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:29: PP_Instance instance,
On 2014/01/08 00:26:24, yzshen1 wrote:
> nit: Please either align with content::... on the previous line or move the
> first param to a separate line.

Done.

https://codereview.chromium.org/61643022/diff/140001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pepper_uma_host.h:37: virtual ~PepperUMAHost();
On 2014/01/08 19:01:49, bbudge wrote:
> It's more conventional for this to follow the ctor. These should also be
grouped
> together in the .cc file.

Done.

https://codereview.chromium.org/61643022/diff/140001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/61643022/diff/140001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:86: 
On 2014/01/08 19:01:49, bbudge wrote:
> Did you intentionally move this here? It seems better to have it after all the
> constant definitions, at the end of the anonymous namespace.

I don't know how this ended up here.  Done.

https://codereview.chromium.org/61643022/diff/140001/ppapi/proxy/uma_private_...
File ppapi/proxy/uma_private_resource.cc (right):

https://codereview.chromium.org/61643022/diff/140001/ppapi/proxy/uma_private_...
ppapi/proxy/uma_private_resource.cc:17: CHECK(name_stringvar.get());
On 2014/01/08 00:26:24, yzshen1 wrote:
> Maybe it is sufficient to do if (!...) return std::string()?
> 
> This |var| is from a public API, so I think it is okay to be a little bit
> tolerant.

Done.

[elijahtaylor1, 2014-01-15 19:42:21]

PTAL

I've rebased to newer chrome since the version I was using previously was pretty
old, as well as addressed feedback in my last patchset.  Trybots are underway

I feel like this is close.  If you guys give the ok, I'll send out
security/owners/uma-team reviews.

[bbudge, 2014-01-16 18:08:54]

lgtm

[yzshen1, 2014-01-16 18:12:19]

lgtm

[elijahtaylor1, 2014-01-16 20:54:27]

need OWNERS approval:


darin@:

chrome/renderer/chrome_content_renderer_client.cc
chrome/renderer/chrome_content_renderer_client.h
content/content_renderer.gypi
native_client_sdk/src/libraries/ppapi_cpp_private/library.dsc


tsepez@:

IPC review for ppapi/proxy/ppapi_messages.h

[Alexei Svitkine (slow), 2014-01-16 22:12:46]

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_uma_host.cc (right):

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_uma_host.cc:33:
"D95DDB0F180CF797AD30E221D659A9E2B6404BC8"   // see crbug.com/317833
The bug doesn't seem to provide more context here. (What are these hashes and
why does this use hashes?)

I've cc'd myself on the bug, so if you prefer we can discuss there.

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_uma_host.cc:138: bucket_count,
We also have a metricsPrivate API that works similarly to this, for some
extensions.

There was a recent bug filed that incorrect use of the API can result in crashes
- see: https://code.google.com/p/chromium/issues/detail?id=334135

It was hit by two different versions of extensions (with different bucket
counts) running in the same browser. I suspect the same problem exists with
this. Since presumably different PPAPI plugins could be loaded and they may have
different versions, it can trigger the same problem .

I would prefer we not add any more entry points into the API until that's fixed.
Would you mind taking a look at what it would take to fix that? I suspect some
extra validation in FactoryGet()...

[elijahtaylor1, 2014-01-16 22:55:25]

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
File chrome/renderer/pepper/pepper_uma_host.cc (right):

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_uma_host.cc:33:
"D95DDB0F180CF797AD30E221D659A9E2B6404BC8"   // see crbug.com/317833
On 2014/01/16 22:12:47, Alexei Svitkine wrote:
> The bug doesn't seem to provide more context here. (What are these hashes and
> why does this use hashes?)
> 
> I've cc'd myself on the bug, so if you prefer we can discuss there.

Thanks for catching this.  I forgot to update the bug and there are indeed two
extra ones I was using early on but later abandoned.  Now removed.

https://codereview.chromium.org/61643022/diff/1180002/chrome/renderer/pepper/...
chrome/renderer/pepper/pepper_uma_host.cc:138: bucket_count,
On 2014/01/16 22:12:47, Alexei Svitkine wrote:
> We also have a metricsPrivate API that works similarly to this, for some
> extensions.
> 
> There was a recent bug filed that incorrect use of the API can result in
crashes
> - see: https://code.google.com/p/chromium/issues/detail?id=334135
> 
> It was hit by two different versions of extensions (with different bucket
> counts) running in the same browser. I suspect the same problem exists with
> this. Since presumably different PPAPI plugins could be loaded and they may
have
> different versions, it can trigger the same problem .
> 
> I would prefer we not add any more entry points into the API until that's
fixed.
> Would you mind taking a look at what it would take to fix that? I suspect some
> extra validation in FactoryGet()...

Right now, the only users of this API by policy are the NaCl/PNaCl in-process
plugins (who were already using it) and now one additional extension (there are
two listed above in the whitelist, but they are mutually exclusive), which is
not shipping to users.

We discussed this offline and you said it was ok to submit as-is and I'll take a
look at fixing the crash you mentioned.

[Tom Sepez, 2014-01-16 23:16:33]

messages LGTM

[Alexei Svitkine (slow), 2014-01-17 16:21:18]

LGTM

[elijahtaylor1, 2014-01-17 20:18:38]

@jam: I had darin as an reviewer for OWNER of these files but I think he's not
around yesterday and today.  Can you take a look?

chrome/renderer/chrome_content_renderer_client.cc
chrome/renderer/chrome_content_renderer_client.h
content/content_renderer.gypi

[binji, 2014-01-17 21:48:27]

native_client_sdk lgtm

[jam, 2014-01-18 02:03:04]

chrome lgtm with the one change

https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1321: const GURL& url, const
std::set<std::string>& whitelist) {
put these two lines in the one calling method. ChromeContentRendererClient is
just supposed to be the place where we implement content callbacks. if we have
static methods in there, it's because they're called by its implementation
methods and some other code.

[elijahtaylor1, 2014-01-22 20:15:03]

Comment for jam@

https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1321: const GURL& url, const
std::set<std::string>& whitelist) {
On 2014/01/18 02:03:05, jam wrote:
> put these two lines in the one calling method. ChromeContentRendererClient is
> just supposed to be the place where we implement content callbacks. if we have
> static methods in there, it's because they're called by its implementation
> methods and some other code.

I'm happy to move this code out of here, the reason I did it here was because it
has access to the extension dispatcher -> extension set.  Maybe you can help me,
I couldn't find an easy way to get an ExtensionSet* in a renderer process
generally.  Most of the code I've seen that deals with ExtensionSets is in the
browser.  Is there an obvious way to do this that I'm missing?

[elijahtaylor1, 2014-01-24 00:00:57]

On 2014/01/22 20:15:03, elijahtaylor1 wrote:
> Comment for jam@
> 
>
https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/61643022/diff/1700001/chrome/renderer/chrome_...
> chrome/renderer/chrome_content_renderer_client.cc:1321: const GURL& url, const
> std::set<std::string>& whitelist) {
> On 2014/01/18 02:03:05, jam wrote:
> > put these two lines in the one calling method. ChromeContentRendererClient
is
> > just supposed to be the place where we implement content callbacks. if we
have
> > static methods in there, it's because they're called by its implementation
> > methods and some other code.
> 
> I'm happy to move this code out of here, the reason I did it here was because
it
> has access to the extension dispatcher -> extension set.  Maybe you can help
me,
> I couldn't find an easy way to get an ExtensionSet* in a renderer process
> generally.  Most of the code I've seen that deals with ExtensionSets is in the
> browser.  Is there an obvious way to do this that I'm missing?

For posterity: I chatted with jam@ offline and he LGTM'd as is

[commit-bot: I haz the power, 2014-01-24 00:01:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/61643022/170...

[commit-bot: I haz the power, 2014-01-24 02:39:16]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-24 19:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/61643022/170...

[commit-bot: I haz the power, 2014-01-24 20:49:43]

Change committed as 246962