Add simple pepper plugin telemetry test

Add simple pepper plugin telemetry test

A simple touch drawing pepper plugin is created using pnacl.
A telemetry test page simple_pepper_plugin.html is added, which injects
touch moves events to the plugin. Smoothness and input latency metrics
are collected from this test.

BUG=355719
TEST=./run_measurement --browser=cros-chrome --remote=XXX scheduler.tough_pepper_cases
     --profiler=trace
     In the trace there is latency info for the plugin touch events.

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

[Yufeng Shen (Slow to review), 2014-05-29 00:04:37]

David, the touch_drawing_plugin.pexe is generated from source
 touch_drawing_plugin.cc. I am not sure where is best to put the source, so I
just put it at ppapi/examples/input/. Let me know if you have any concern.

[Rick Byers, 2014-05-29 03:28:08]

This seems reasonable to me, thanks!  But I'm not very familiar with pepper
plugins - I defer to dmichael to review the plugin code.

https://codereview.chromium.org/300413002/diff/1/tools/perf/page_sets/tough_s...
File tools/perf/page_sets/tough_scheduling_cases/simple_pepper_plugin.html
(right):

https://codereview.chromium.org/300413002/diff/1/tools/perf/page_sets/tough_s...
tools/perf/page_sets/tough_scheduling_cases/simple_pepper_plugin.html:30:
listener.addEventListener('load', moduleDidLoad, true);
you could put the load handler directly on touch_drawing_plugin instead, right? 
This 'listener' div seems superfluous...

[dmichael (off chromium), 2014-05-29 16:28:03]

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
File ppapi/examples/input/touch_drawing_plugin.cc (right):

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I think it's preferred to leave off the "(c)". Also, please update the year.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:3: // found in the LICENSE file.
This file doesn't seem to belong in ppapi/examples.
- you didn't add it to the ppapi/ppapi_tests.gypi file, so it won't actually be
built
- Even if you do, it won't be built as a .pexe
- It uses a Private API that is really only there for your latency test.

Can you move it closer to the test .pexe? And would it make sense to either have
a build rule for it (to make sure it doesn't bitrot) or at the very least have
some instructions for reproducing the .pexe, in case this file needs to be
changed later?

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:12: #include "ppapi/cpp/logging.h"
You don't seem to be using this?

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:26: uint32_t color) {
nit: params in a function declaration or definition should be all on one line if
they fit, or each have their own line if they don'w.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:32: x++)
nit: since the "for" covers multiple lines, we would usually use curly braces,
even though it doesn't change the meaning.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:39: MyInstance(PP_Instance
instance)
nit: explicit

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:44:
pp::InputEventPrivate::StartTrackingLatency(pp::InstanceHandle(instance));
If this stays as an example in ppapi/examples, you should definitely add some
comments around usage of InputEventPrivate. I.e., the fact that the API is only
for measuring latency in Chromium, and isn't intended for use by third-party
plugins.

In any case, I think it would make sense to have a big comment near the top of
this file explaining what this plugin does.

[Yufeng Shen (Slow to review), 2014-05-29 18:29:20]

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
File ppapi/examples/input/touch_drawing_plugin.cc (right):

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2014/05/29 16:28:04, dmichael wrote:
> I think it's preferred to leave off the "(c)". Also, please update the year.

Done.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:3: // found in the LICENSE file.
On 2014/05/29 16:28:04, dmichael wrote:
> This file doesn't seem to belong in ppapi/examples.
> - you didn't add it to the ppapi/ppapi_tests.gypi file, so it won't actually
be
> built
> - Even if you do, it won't be built as a .pexe
> - It uses a Private API that is really only there for your latency test.
> 
> Can you move it closer to the test .pexe? And would it make sense to either
have
> a build rule for it (to make sure it doesn't bitrot) or at the very least have
> some instructions for reproducing the .pexe, in case this file needs to be
> changed later?

I moved the source to
tools/perf/page_sets/tough_scheduling_cases/extra_data
to be together with the pexe

and also added a README and a Makefile

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:12: #include "ppapi/cpp/logging.h"
On 2014/05/29 16:28:04, dmichael wrote:
> You don't seem to be using this?

Done.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:26: uint32_t color) {
On 2014/05/29 16:28:04, dmichael wrote:
> nit: params in a function declaration or definition should be all on one line
if
> they fit, or each have their own line if they don'w.

Done.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:32: x++)
On 2014/05/29 16:28:04, dmichael wrote:
> nit: since the "for" covers multiple lines, we would usually use curly braces,
> even though it doesn't change the meaning.

Done.

https://codereview.chromium.org/300413002/diff/1/ppapi/examples/input/touch_d...
ppapi/examples/input/touch_drawing_plugin.cc:39: MyInstance(PP_Instance
instance)
On 2014/05/29 16:28:04, dmichael wrote:
> nit: explicit

Done.

https://codereview.chromium.org/300413002/diff/1/tools/perf/page_sets/tough_s...
File tools/perf/page_sets/tough_scheduling_cases/simple_pepper_plugin.html
(right):

https://codereview.chromium.org/300413002/diff/1/tools/perf/page_sets/tough_s...
tools/perf/page_sets/tough_scheduling_cases/simple_pepper_plugin.html:30:
listener.addEventListener('load', moduleDidLoad, true);
On 2014/05/29 03:28:09, Rick Byers wrote:
> you could put the load handler directly on touch_drawing_plugin instead,
right? 
> This 'listener' div seems superfluous...

Done.

[dmichael (off chromium), 2014-05-29 18:42:38]

lgtm from a pepper perspective, though you don't need my review anymore ;-)

[nduca, 2014-06-02 20:40:17]

https://codereview.chromium.org/300413002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_scheduling_cases.py (right):

https://codereview.chromium.org/300413002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_scheduling_cases.py:330: def RunSmoothness(self,
action_runner):
can you ask nednguyen how to do this right? Two requirements:
1) if '--enable-pepper-testing' is not in the commandline for the browser, then
this page should fail
2) if the browser doesn't support pepper, eg android, then this page should fail

[nduca, 2014-06-02 20:41:17]

lgtm for me, but please get lg from nednguyen for committing

[nednguyen, 2014-06-02 20:47:33]

On 2014/06/02 20:41:17, nduca wrote:
> lgtm for me, but please get lg from nednguyen for committing

To specify that the page will only be run when browser supports pepper, you can
add s.t like HasPepperSupport to browser_info and implement CanRunOnBrowser(...)
method for Page21. Tony concerns about branching at page level instead of
page_set level, so I would also ask for his opinion.

[tonyg, 2014-06-05 01:06:57]

On 2014/06/02 20:47:33, nednguyen wrote:
> On 2014/06/02 20:41:17, nduca wrote:
> > lgtm for me, but please get lg from nednguyen for committing
> 
> To specify that the page will only be run when browser supports pepper, you
can
> add s.t like HasPepperSupport to browser_info and implement
CanRunOnBrowser(...)
> method for Page21. Tony concerns about branching at page level instead of
> page_set level, so I would also ask for his opinion.

I think it is a nice property to have a benchmark mean the same thing across
platforms. If we branch at the page level, then the averages that the benchmarks
report are different for reasons other than the performance of that platform.

Is seems like if we have one specific page for pepper, we could make a
tough_pepper_cases.py PageSet, then a scheduler.tough_pepper_cases Benchmark.

[nednguyen, 2014-06-05 01:34:52]

On 2014/06/05 01:06:57, tonyg wrote:
> On 2014/06/02 20:47:33, nednguyen wrote:
> > On 2014/06/02 20:41:17, nduca wrote:
> > > lgtm for me, but please get lg from nednguyen for committing
> > 
> > To specify that the page will only be run when browser supports pepper, you
> can
> > add s.t like HasPepperSupport to browser_info and implement
> CanRunOnBrowser(...)
> > method for Page21. Tony concerns about branching at page level instead of
> > page_set level, so I would also ask for his opinion.
> 
> I think it is a nice property to have a benchmark mean the same thing across
> platforms. If we branch at the page level, then the averages that the
benchmarks
> report are different for reasons other than the performance of that platform.

I agree. Maybe I should pull that CanRunOnBrowser to be a property of benchmark
instead of page then.

> Is seems like if we have one specific page for pepper, we could make a
> tough_pepper_cases.py PageSet, then a scheduler.tough_pepper_cases Benchmark.

[Yufeng Shen (Slow to review), 2014-06-05 18:23:16]

A new page_set tough_pepper_cases and benchmark scheduler.tough_pepper_cases 
are created.


Ned, Tony, PTAL.

[nednguyen, 2014-06-06 01:56:38]

https://codereview.chromium.org/300413002/diff/50001/tools/perf/benchmarks/sc...
File tools/perf/benchmarks/scheduler.py (right):

https://codereview.chromium.org/300413002/diff/50001/tools/perf/benchmarks/sc...
tools/perf/benchmarks/scheduler.py:24: page_set =
'page_sets/tough_pepper_cases.py'
This should be tough_pepper_cases.ToughPepperCases with Dave's change in
https://codereview.chromium.org/311193003/

https://codereview.chromium.org/300413002/diff/50001/tools/perf/benchmarks/sc...
tools/perf/benchmarks/scheduler.py:28:
options.AppendExtraBrowserArgs('--enable-pepper-testing')
This will be enough for the bot since it will pick up your pepper plugin change,
however this may fail on local machine which doesn't have the TOT chromium
build. 

@Nat, Tony: do we care about the second case? If so, I suggest you implement the
CanRunOnBrowser for your tough_pepper_cases page set.

[Yufeng Shen (Slow to review), 2014-06-06 16:15:19]

https://codereview.chromium.org/300413002/diff/50001/tools/perf/benchmarks/sc...
File tools/perf/benchmarks/scheduler.py (right):

https://codereview.chromium.org/300413002/diff/50001/tools/perf/benchmarks/sc...
tools/perf/benchmarks/scheduler.py:28:
options.AppendExtraBrowserArgs('--enable-pepper-testing')
On 2014/06/06 01:56:37, nednguyen wrote:
> This will be enough for the bot since it will pick up your pepper plugin
change,
> however this may fail on local machine which doesn't have the TOT chromium
> build. 
> 
> @Nat, Tony: do we care about the second case? If so, I suggest you implement
the
> CanRunOnBrowser for your tough_pepper_cases page set. 

@Tony: this new page_set meant for measuring the mean_input_event_latency of a
simple pepper plugin. mean_input_event_latency is available only when the
chromium
build has my recent change. But the test itself will still run on old chrome
build, only that this specific metric won't be reported. Do we still need
CanRunOnBrowser() to guard against running this benchmark on old chrome build ?
If we really need to, can we use chrome revision checking in CanRunOnBrowser() ?

[nduca, 2014-06-10 18:00:38]

Yes you need to make it not run on old browsers.

[nduca, 2014-06-10 18:06:46]

Actually, we don't expose chrome_branch_number publically --- its only on the
backends. Lets keep it that way for now.

Make sure it doesn't crash. If it doesn't crash, then lgtm and you can land.
Sorry for the confusion.

[Yufeng Shen (Slow to review), 2014-06-10 18:29:44]

On 2014/06/10 18:06:46, nduca wrote:
> Actually, we don't expose chrome_branch_number publically --- its only on the
> backends. Lets keep it that way for now.
> 
> Make sure it doesn't crash. If it doesn't crash, then lgtm and you can land.
> Sorry for the confusion.

Yes, it does not crash on old browsers, benchmark still runs and normal
rendering
metrics are still reported, only the new metric mean_input_event_latency won't
gets
reported.

I am going to land this.  Ned/Tony, if you still think we should totally skip
the
benchmark for old browsers, lets do it in a follow up patch.

[Yufeng Shen (Slow to review), 2014-06-10 18:30:00]

The CQ bit was checked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2014-06-10 18:30:38]

The CQ bit was unchecked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2014-06-10 18:40:15]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2014-06-10 18:42:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miletus@chromium.org/300413002/90001

[commit-bot: I haz the power, 2014-06-11 04:44:46]

Change committed as 276257