Add UMA private PPAPI interface for UMA logging from NaCl plugin

Add UMA private PPAPI interface for UMA logging from NaCl plugin

BUG= none
TEST= manual test of ppapi_tests, run UMA tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83622

[elijahtaylor (use chromium), 2011-04-27 22:15:05]

Sorry if I've cast too wide a net with ppapi reviewers, let me know if I should
eliminate/replace anyone.

The interface is definitely up for debate, this was basically the smallest set I
thought it could be while maintaining the full functionality that UMA provides.

[bbudge-google, 2011-04-27 23:03:54]

LGTM with a formatting nit, but you should add the owner, jam@chromium.org to
reviewers.

http://codereview.chromium.org/6903084/diff/4004/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/4004/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:19: int64_t min, int64_t max,
uint32_t bucket_count) {
I think the parameter formatting rule is all on one line, or if they don't fit,
on separate lines, with the exception that logically connected params can share
a line. So possibly...
   name,
   sample,
   min, max,
   bucket_count) {

[bbudge-google, 2011-04-28 12:48:16]

Since this is generally useful, perhaps it should be a trusted interface.
Private interfaces seem to belong to specific plugins like nacl, flash, or pdf.

[elijahtaylor (use chromium), 2011-04-28 18:13:13]

+jam

So I guess I don't know the difference between trusted/private.  Is this just a
logical separation for us or do the headers and interfaces get treated
differently for distribution or what?  Also, I was assuming this may be an
interface we eventually want to proxy on a per-extension basis in order to
gather stats on dogfood efforts, "trusted" sounds like it wouldn't be kosher to
proxy to the untrusted side.

On 2011/04/28 12:48:16, bbudge wrote:
> Since this is generally useful, perhaps it should be a trusted interface.
> Private interfaces seem to belong to specific plugins like nacl, flash, or
pdf.

[jam, 2011-04-28 18:20:02]

brettw can comment on this better.  the lines between private/trusted are a
little fuzzy. 

On 2011/04/28 18:13:13, elijahtaylor wrote:
> +jam
> 
> So I guess I don't know the difference between trusted/private.  Is this just
a
> logical separation for us or do the headers and interfaces get treated
> differently for distribution or what?  Also, I was assuming this may be an
> interface we eventually want to proxy on a per-extension basis in order to
> gather stats on dogfood efforts, "trusted" sounds like it wouldn't be kosher
to
> proxy to the untrusted side.
> 
> On 2011/04/28 12:48:16, bbudge wrote:
> > Since this is generally useful, perhaps it should be a trusted interface.
> > Private interfaces seem to belong to specific plugins like nacl, flash, or
> pdf.

[elijahtaylor (use chromium), 2011-04-28 19:14:46]

http://codereview.chromium.org/6903084/diff/4004/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/4004/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:19: int64_t min, int64_t max,
uint32_t bucket_count) {
On 2011/04/27 23:03:54, bbudge wrote:
> I think the parameter formatting rule is all on one line, or if they don't
fit,
> on separate lines, with the exception that logically connected params can
share
> a line. So possibly...
>    name,
>    sample,
>    min, max,
>    bucket_count) {

Done.

[brettw, 2011-04-28 20:15:23]

On Thu, Apr 28, 2011 at 11:20 AM,  <jam@chromium.org> wrote:
> brettw can comment on this better.  the lines between private/trusted are a
> little fuzzy.

Private = Plugin by us.
Trusted = Imagine there was a way for people to locally install
plugins & get extra permissions.

So private seems right for this.

Brett

[elijahtaylor (use chromium), 2011-04-29 19:05:58]

*ping*

Can someone look at this today possibly?  In particular I'm looking for an LGTM
from one of (darin,brettw,jam) before committing



On 2011/04/28 20:15:23, brettw wrote:
> On Thu, Apr 28, 2011 at 11:20 AM,  <mailto:jam@chromium.org> wrote:
> > brettw can comment on this better. &nbsp;the lines between private/trusted
are a
> > little fuzzy.
> 
> Private = Plugin by us.
> Trusted = Imagine there was a way for people to locally install
> plugins & get extra permissions.
> 
> So private seems right for this.
> 
> Brett

[brettw, 2011-04-29 21:05:04]

http://codereview.chromium.org/6903084/diff/2011/ppapi/c/private/ppb_uma_priv...
File ppapi/c/private/ppb_uma_private.h (right):

http://codereview.chromium.org/6903084/diff/2011/ppapi/c/private/ppb_uma_priv...
ppapi/c/private/ppb_uma_private.h:8: #include "ppapi/c/pp_stdint.h"
This should include pp_var.h also since you use it.

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:22: scoped_refptr<StringVar>
name_string(StringVar::FromPPVar(name));
You should NULL check these values (same below) in case somebody sends an
invalid string.

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:48:
base::LinearHistogram::FactoryGet(name_string->value(), 1, boundary_value,
This should use 4-space indents (same above).

[elijahtaylor (use chromium), 2011-04-29 21:52:07]

Thanks for the feedback, PTAL

http://codereview.chromium.org/6903084/diff/2011/ppapi/c/private/ppb_uma_priv...
File ppapi/c/private/ppb_uma_private.h (right):

http://codereview.chromium.org/6903084/diff/2011/ppapi/c/private/ppb_uma_priv...
ppapi/c/private/ppb_uma_private.h:8: #include "ppapi/c/pp_stdint.h"
On 2011/04/29 21:05:05, brettw wrote:
> This should include pp_var.h also since you use it.

Done.

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:22: scoped_refptr<StringVar>
name_string(StringVar::FromPPVar(name));
On 2011/04/29 21:05:05, brettw wrote:
> You should NULL check these values (same below) in case somebody sends an
> invalid string.

I noticed other badness that could happen with invalid args, so I created a
macro (don't hit me) to check for errors in the args and return (fail silently).
 I assume this is preferred to CHECK failures in ppb interfaces.

http://codereview.chromium.org/6903084/diff/2011/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:48:
base::LinearHistogram::FactoryGet(name_string->value(), 1, boundary_value,
On 2011/04/29 21:05:05, brettw wrote:
> This should use 4-space indents (same above).

Done.

[brettw, 2011-04-29 22:11:06]

http://codereview.chromium.org/6903084/diff/4018/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/4018/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:67: scoped_refptr<StringVar>
name_string(StringVar::FromPPVar(name));
You still need to check this pointer for NULL. The caller could be corrupt or
malicious and put a random ID into the Var for the string ID. This will return
NULL.

[elijahtaylor (use chromium), 2011-04-29 22:27:03]

http://codereview.chromium.org/6903084/diff/4018/webkit/plugins/ppapi/ppb_uma...
File webkit/plugins/ppapi/ppb_uma_private_impl.cc (right):

http://codereview.chromium.org/6903084/diff/4018/webkit/plugins/ppapi/ppb_uma...
webkit/plugins/ppapi/ppb_uma_private_impl.cc:67: scoped_refptr<StringVar>
name_string(StringVar::FromPPVar(name));
On 2011/04/29 22:11:06, brettw wrote:
> You still need to check this pointer for NULL. The caller could be corrupt or
> malicious and put a random ID into the Var for the string ID. This will return
> NULL.

Done.

[brettw, 2011-04-29 22:53:37]

LGTM