Add browser_test to check that NaCl is logging UMA samples correctly.

Add browser_test to check that NaCl is logging UMA samples correctly.

This change required adding an include to plugin_error.h to ensure
NACL_DISALLOW_COPY_AND_ASSIGN was defined.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2959
TEST= browser_tests --gtest_filter=NaClBrowserTest*.UMALoadTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154579

[Nick Bray (chromium), 2012-08-20 22:22:39]

mseaborn as primary reviewer
jar as UMA stakeholder.

[Mark Seaborn, 2012-08-21 19:41:37]

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS
File chrome/test/nacl/DEPS (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS#newcode3
chrome/test/nacl/DEPS:3: "+native_client",
Maybe list the specific header here, i.e.
"native_client/src/trusted/service_runtime/nacl_error_code.h",
and add a TODO to clean up this dependency and reference
https://code.google.com/p/nativeclient/issues/detail?id=2832.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:320: class HistogramHelper {
Should this all be in a separate file to be more obviously findable?  The UMA
test is fairly big.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:332: MessageLoop::current(), callback,
Should you be creating a new message loop object so that you're not modifying a
global when you call Quit()?

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:333:
base::TimeDelta::FromMilliseconds(60000));
Comment: "give up after 60s, which anyway is longer than the brower tests'
current timeout of ???s".

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:339: void ExpectUniqueSample(const
std::string &name, size_t bucket,
"&" spacing style.  'bucket' -> 'bucket_id', perhaps?

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:340: base::Histogram::Count count) {
'count' -> 'expected_count', just for clarity?  Same below.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:352: void ExpectTotalCount(const
std::string &name, base::Histogram::Count count) {
"&" spacing style.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:386: // TODO(ncbray) create multiple
variants via macros.
Multiple variants to do what?

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:411:
histograms.ExpectTotalCount("NaCl.Perf.Size.Nexe", 1);
Can you add a comment saying something like:

This test is a replacement for native_client/tests/pyauto_nacl/nacl_uma.py. 
TODO(ncbray): Convert the rest of that script, including [insert examples or
comprehensive list :-) ...]

http://codereview.chromium.org/10825462/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

http://codereview.chromium.org/10825462/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin_error.h:16: #include
"native_client/src/include/nacl_macros.h"
Is this change fixing #include-what-use-you?  If so, note that in the commit
message.

[jar (doing other things), 2012-08-23 00:35:39]

Just some nits that I noticed.

https://chromiumcodereview.appspot.com/10825462/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10825462/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:328: &HistogramHelper::FetchCallback,
nit: move 328 to end of 327.  For function calls, no reason to  wrap like this. 
You can also align 329 start with the paren.

https://chromiumcodereview.appspot.com/10825462/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:339: void ExpectUniqueSample(const
std::string &name, size_t bucket,
nit: For function definitions... one arg per line (align at paren is best).

https://chromiumcodereview.appspot.com/10825462/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:342: ASSERT_NE((base::Histogram*)NULL,
histogram) << "Histogram \"" << name <<
nit: avoid C style cast (here and below). Best is static_cast, but you may need
reinterpret_cast here.

https://chromiumcodereview.appspot.com/10825462/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:371: base::Histogram::SampleSet& samples) {
nit: prefer aligning args with paren in definition.

[Nick Bray (chromium), 2012-08-24 18:17:47]

PTAL

At the moment this test will time out.  I am looping back and trying to sort
that out in another CL.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS
File chrome/test/nacl/DEPS (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS#newcode3
chrome/test/nacl/DEPS:3: "+native_client",
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Maybe list the specific header here, i.e.
> "native_client/src/trusted/service_runtime/nacl_error_code.h",
> and add a TODO to clean up this dependency and reference
> https://code.google.com/p/nativeclient/issues/detail?id=2832.

1) I didn't want to list specific files because this will change.

2) I don't see a cleaner way to do this, so I don't see how it could be cleaned
up.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:320: class HistogramHelper {
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Should this all be in a separate file to be more obviously findable?  The UMA
> test is fairly big.

I agree, but I was planning to do structural reforms later.  The bigger question
is how to set things up so we can easily instantiate multiple variants of the
same test (newlib, glibc, pnacl) and be able to define tests in different files.
 This will require some polishing.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:328: &HistogramHelper::FetchCallback,
On 2012/08/23 00:35:39, jar wrote:
> nit: move 328 to end of 327.  For function calls, no reason to  wrap like
this. 
> You can also align 329 start with the paren.

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:332: MessageLoop::current(), callback,
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Should you be creating a new message loop object so that you're not modifying
a
> global when you call Quit()?

As far as I know, this is idiomatic for browser_tests.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:333:
base::TimeDelta::FromMilliseconds(60000));
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Comment: "give up after 60s, which anyway is longer than the brower tests'
> current timeout of ???s".

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:340: base::Histogram::Count count) {
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> 'count' -> 'expected_count', just for clarity?  Same below.

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:342: ASSERT_NE((base::Histogram*)NULL,
histogram) << "Histogram \"" << name <<
On 2012/08/23 00:35:39, jar wrote:
> nit: avoid C style cast (here and below). Best is static_cast, but you may
need
> reinterpret_cast here.

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:352: void ExpectTotalCount(const
std::string &name, base::Histogram::Count count) {
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> "&" spacing style.

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:371: base::Histogram::SampleSet& samples) {
On 2012/08/23 00:35:39, jar wrote:
> nit: prefer aligning args with paren in definition.

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:386: // TODO(ncbray) create multiple
variants via macros.
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Multiple variants to do what?

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:411:
histograms.ExpectTotalCount("NaCl.Perf.Size.Nexe", 1);
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Can you add a comment saying something like:
> 
> This test is a replacement for native_client/tests/pyauto_nacl/nacl_uma.py. 
> TODO(ncbray): Convert the rest of that script, including [insert examples or
> comprehensive list :-) ...]

Done.

http://codereview.chromium.org/10825462/diff/1/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

http://codereview.chromium.org/10825462/diff/1/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/plugin_error.h:16: #include
"native_client/src/include/nacl_macros.h"
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> Is this change fixing #include-what-use-you?  If so, note that in the commit
> message.

Done.

[Mark Seaborn, 2012-08-24 19:30:42]

LGTM with changes below

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS
File chrome/test/nacl/DEPS (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS#newcode3
chrome/test/nacl/DEPS:3: "+native_client",
On 2012/08/24 18:17:48, Nick Bray (chromium) wrote:
> On 2012/08/21 19:41:37, Mark Seaborn wrote:
> > Maybe list the specific header here, i.e.
> > "native_client/src/trusted/service_runtime/nacl_error_code.h",
> > and add a TODO to clean up this dependency and reference
> > https://code.google.com/p/nativeclient/issues/detail?id=2832.
> 
> 1) I didn't want to list specific files because this will change.
> 
> 2) I don't see a cleaner way to do this, so I don't see how it could be
cleaned
> up.

As we discussed, please read the issue I linked, which explains what I mean. :-)

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:339: void ExpectUniqueSample(const
std::string &name, size_t bucket,
On 2012/08/21 19:41:37, Mark Seaborn wrote:
> 'bucket' -> 'bucket_id', perhaps?

Ping.

http://codereview.chromium.org/10825462/diff/8001/chrome/test/nacl/nacl_brows...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/8001/chrome/test/nacl/nacl_brows...
chrome/test/nacl/nacl_browsertest.cc:330: // TODO(ncbray) convert the rest of
nacl_uma.py and test validation failiures
Clarify that nacl_uma.py is from the native_client repo?  Typo: "failures"

[Nick Bray (chromium), 2012-08-24 21:13:56]

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS
File chrome/test/nacl/DEPS (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/DEPS#newcode3
chrome/test/nacl/DEPS:3: "+native_client",
On 2012/08/24 19:30:42, Mark Seaborn wrote:
> On 2012/08/24 18:17:48, Nick Bray (chromium) wrote:
> > On 2012/08/21 19:41:37, Mark Seaborn wrote:
> > > Maybe list the specific header here, i.e.
> > > "native_client/src/trusted/service_runtime/nacl_error_code.h",
> > > and add a TODO to clean up this dependency and reference
> > > https://code.google.com/p/nativeclient/issues/detail?id=2832.
> > 
> > 1) I didn't want to list specific files because this will change.
> > 
> > 2) I don't see a cleaner way to do this, so I don't see how it could be
> cleaned
> > up.
> 
> As we discussed, please read the issue I linked, which explains what I mean.
:-)

Done.

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/1/chrome/test/nacl/nacl_browsert...
chrome/test/nacl/nacl_browsertest.cc:339: void ExpectUniqueSample(const
std::string &name, size_t bucket,
On 2012/08/24 19:30:42, Mark Seaborn wrote:
> On 2012/08/21 19:41:37, Mark Seaborn wrote:
> > 'bucket' -> 'bucket_id', perhaps?
> 
> Ping.

Done.

http://codereview.chromium.org/10825462/diff/8001/chrome/test/nacl/nacl_brows...
File chrome/test/nacl/nacl_browsertest.cc (right):

http://codereview.chromium.org/10825462/diff/8001/chrome/test/nacl/nacl_brows...
chrome/test/nacl/nacl_browsertest.cc:330: // TODO(ncbray) convert the rest of
nacl_uma.py and test validation failiures
On 2012/08/24 19:30:42, Mark Seaborn wrote:
> Clarify that nacl_uma.py is from the native_client repo?  Typo: "failures"

Done.

[commit-bot: I haz the power, 2012-08-31 21:51:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/10825462/17001

[commit-bot: I haz the power, 2012-09-01 02:30:37]

Change committed as 154579