Add C++ Trace Event support to PPAPI/NaCl SDK

Add C++ Trace Event support to PPAPI/NaCl SDK

* Add trace_event_internal.h header to the NaCl SDK
* Add example to the NaCl SDK of how to use trace events

BUG=93839, 160562
TEST=manual (build nacl sdk, run example)

[elijahtaylor1, 2013-02-14 21:03:29]

brettw: please review PPAPI C++ changes (there is some potentially questionable
stuff in there with depending on base/ and having simple c++ functions instead
of a class like other C++ interfaces)

binji: please review NaCl SDK changes, new example.  The example is largely
cargo culted, please be critical of it.

[binji, 2013-02-14 22:01:28]

nacl_sdk stuff lgtm, though you should check with noelallen as well.

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
File native_client_sdk/src/examples/trace_events/index.html (right):

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/index.html:26: <li>"Run threaded
task" creates a new thread with and captures the length
with...?

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
File native_client_sdk/src/examples/trace_events/trace_events.cc (right):

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:25: namespace
trace_event_example {
we don't usually namespace the examples

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:37: int thread_num_;
move to end of class definition

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:40: explicit
TraceEventInstance(PP_Instance instance) : pp::Instance(instance),
I usually move the initializer list to the next line w/ 4 spaces, but OK

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:67:
pp::trace_events::SetThreadName("Main Pepper Thread");
what is the default name if this is not set?

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:82:
TRACE_EVENT_ASYNC_BEGIN0("ExampleCategory", "ExampleMessage", async_num_);
maybe a little more explanation of what this is doing... what is the 0?

[elijahtaylor1, 2013-02-14 23:38:10]

Thanks for the quick first review.

+noelallen for SDK signoff

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
File native_client_sdk/src/examples/trace_events/index.html (right):

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/index.html:26: <li>"Run threaded
task" creates a new thread with and captures the length
On 2013/02/14 22:01:28, binji wrote:
> with...?

Done.

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
File native_client_sdk/src/examples/trace_events/trace_events.cc (right):

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:25: namespace
trace_event_example {
On 2013/02/14 22:01:28, binji wrote:
> we don't usually namespace the examples

This was cribbed from hello_world_interactive.  Removed

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:37: int thread_num_;
On 2013/02/14 22:01:28, binji wrote:
> move to end of class definition

Done.

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:40: explicit
TraceEventInstance(PP_Instance instance) : pp::Instance(instance),
On 2013/02/14 22:01:28, binji wrote:
> I usually move the initializer list to the next line w/ 4 spaces, but OK

Again, cribbed from hello_world_interactive (where admittedly they only have the
base class in the initializer list)

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:67:
pp::trace_events::SetThreadName("Main Pepper Thread");
On 2013/02/14 22:01:28, binji wrote:
> what is the default name if this is not set?

Added comment

https://codereview.chromium.org/12212198/diff/1/native_client_sdk/src/example...
native_client_sdk/src/examples/trace_events/trace_events.cc:82:
TRACE_EVENT_ASYNC_BEGIN0("ExampleCategory", "ExampleMessage", async_num_);
On 2013/02/14 22:01:28, binji wrote:
> maybe a little more explanation of what this is doing... what is the 0?

Done.

[noelallen1, 2013-02-15 01:59:25]

LGTM+

Fix failures first.

[brettw, 2013-02-15 19:21:59]

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_dev.h
File ppapi/cpp/dev/trace_event_dev.h (right):

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:14: namespace pp {
Blank line before this.

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:15: namespace trace_events {
In other places, we've had a C++ class following the C naming convention (so
this would be Trace_Events) with static classes inside it, rather than a nested
namespace.

Also, the interface in C should be PPB_TraceEvent_Dev (no underscore between
Trace and Event). We don't have to fix it in this patch, it but might be nice.

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:31: unsigned long long id,
This should match the type on the C interface (uint64_t). Same below.

https://codereview.chromium.org/12212198/diff/19/ppapi/ppapi_cpp.gypi
File ppapi/ppapi_cpp.gypi (right):

https://codereview.chromium.org/12212198/diff/19/ppapi/ppapi_cpp.gypi#newcode53
ppapi/ppapi_cpp.gypi:53: '-Wno-long-long'],
You can remove this when you remove long long

[elijahtaylor1, 2013-02-20 01:09:50]

In the process of sorting out fallout from
(https://chromiumcodereview.appspot.com/11366109), please no reviews in the
meantime, just publishing drafts

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_dev.h
File ppapi/cpp/dev/trace_event_dev.h (right):

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:14: namespace pp {
On 2013/02/15 19:21:59, brettw wrote:
> Blank line before this.

Done.

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:15: namespace trace_events {
On 2013/02/15 19:21:59, brettw wrote:
> In other places, we've had a C++ class following the C naming convention (so
> this would be Trace_Events) with static classes inside it, rather than a
nested
> namespace.
> 
> Also, the interface in C should be PPB_TraceEvent_Dev (no underscore between
> Trace and Event). We don't have to fix it in this patch, it but might be nice.

Done.  Will roll a separate CL to rename Trace_Event -> TraceEvent for C and C++

https://codereview.chromium.org/12212198/diff/19/ppapi/cpp/dev/trace_event_de...
ppapi/cpp/dev/trace_event_dev.h:31: unsigned long long id,
On 2013/02/15 19:21:59, brettw wrote:
> This should match the type on the C interface (uint64_t). Same below.

Done.  I kept the references below the same because they represent the interface
that the TRACE_EVENT macros expect (though I removed the "threshold" arguments
which don't exist anymore).

https://codereview.chromium.org/12212198/diff/19/ppapi/ppapi_cpp.gypi
File ppapi/ppapi_cpp.gypi (right):

https://codereview.chromium.org/12212198/diff/19/ppapi/ppapi_cpp.gypi#newcode53
ppapi/ppapi_cpp.gypi:53: '-Wno-long-long'],
On 2013/02/15 19:21:59, brettw wrote:
> You can remove this when you remove long long

Actually I can't.  trace_events_internal.h still uses 'long long' throughout and
without -Wno-long-long there are a lot of warnings/errors.  Solutions include:

1) -Wno-long-long
2) Remove -pedantic
3) Add -std=c++0x

Preference?

[nduca, 2013-02-20 01:16:38]

You really should be running these kinds of changes by trace_event maintainers.
It doesn't have to me. scottmg, dsinclair, simonjam, all have experience in that
code and can provide useful feedback on the api.

[elijahtaylor1, 2013-02-20 01:45:34]

On 2013/02/20 01:16:38, nduca wrote:
> You really should be running these kinds of changes by trace_event
maintainers.
> It doesn't have to me. scottmg, dsinclair, simonjam, all have experience in
that
> code and can provide useful feedback on the api.

I agree.  I've sent out an email to everyone (including you) for clarification.