Add trace event Pepper API

Add trace event Pepper API

This facilitates adding trace data to chrome://tracing from plugins.

- broke out trace_event.h into trace_event.h/trace_event_internal.h
  for easier transplanting to plugin code by eliminating dependence on base/.
- inlined trace_event.cc methods (4 total) so the trace_event_internal
  implementation is contained in headers.
- added new PPB_TraceEvent_Dev interface (implemented entirely on the plugin side)

BUG=none
TEST=base unittests, manual for plugin testing


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

[elijahtaylor1, 2012-12-13 01:30:11]

PTAL.

brettw:  I added you since you are an owner of ppapi/ and base/.  Let me know if
there's someone else you want to review these if you're busy.

[jbauman, 2012-12-14 00:36:07]

https://codereview.chromium.org/11464005/diff/2001/base/debug/trace_event_int...
File base/debug/trace_event_internal.h (right):

https://codereview.chromium.org/11464005/diff/2001/base/debug/trace_event_int...
base/debug/trace_event_internal.h:917: TRACE_EVENT_API_ADD_TRACE_EVENT(
Hopefully this won't increase the chrome binary size too much.

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
File ppapi/api/dev/ppb_trace_event_dev.idl (right):

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
ppapi/api/dev/ppb_trace_event_dev.idl:26: mem_t GetCategoryEnabled([in]
PP_Instance instance,
Do we actually need the instance parameters on these? In practice they're not
really necessary for anything, and I guess you could also save on the cost of
entering the instance . I'm not sure how that would match the style of the other
pepper interfaces, though.

https://codereview.chromium.org/11464005/diff/2001/ppapi/shared_impl/ppb_inst...
File ppapi/shared_impl/ppb_instance_shared.cc (right):

https://codereview.chromium.org/11464005/diff/2001/ppapi/shared_impl/ppb_inst...
ppapi/shared_impl/ppb_instance_shared.cc:68: // management to make this work
quickly without requiring IPCs for every call,
This isn't true anymore.

[brettw, 2012-12-15 00:35:31]

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
File ppapi/api/dev/ppb_trace_event_dev.idl (right):

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
ppapi/api/dev/ppb_trace_event_dev.idl:26: mem_t GetCategoryEnabled([in]
PP_Instance instance,
I think all pepper interfaces need a PP_Instance or a PP_Resource.

Doing a pepper call involves a global lock and some map lookups. If this is too
slow for the API (like this is going to be called a lot on the inner loop), then
we probably need to come up with a less chatty interface (like provide a list of
enabled categories in one call, and a way to periodically provide a batch of
several events to the system, and then have the plugin code do all the low-level
management). What we do here may depend on how you intend this to be used.

[jbauman, 2012-12-15 12:25:53]

On 2012/12/15 00:35:31, brettw wrote:
>
https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
> File ppapi/api/dev/ppb_trace_event_dev.idl (right):
> 
>
https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
> ppapi/api/dev/ppb_trace_event_dev.idl:26: mem_t GetCategoryEnabled([in]
> PP_Instance instance,
> I think all pepper interfaces need a PP_Instance or a PP_Resource.
> 
> Doing a pepper call involves a global lock and some map lookups. If this is
too
> slow for the API (like this is going to be called a lot on the inner loop),
then
> we probably need to come up with a less chatty interface (like provide a list
of
> enabled categories in one call, and a way to periodically provide a batch of
> several events to the system, and then have the plugin code do all the
low-level
> management). What we do here may depend on how you intend this to be used.

Oh, I saw that some functions like PPB_ImageData's GetNativeImageDataFormat
don't use either of those, and in this case there's no real point to having an
instance, as it isn't actually used for anything. Hopefully the performance
won't matter in practice, as the tracing interface has been optimized to use the
smallest number of function calls possible.

[elijahtaylor1, 2012-12-18 20:40:31]

PTAL, addressed feedback

https://codereview.chromium.org/11464005/diff/2001/base/debug/trace_event_int...
File base/debug/trace_event_internal.h (right):

https://codereview.chromium.org/11464005/diff/2001/base/debug/trace_event_int...
base/debug/trace_event_internal.h:917: TRACE_EVENT_API_ADD_TRACE_EVENT(
On 2012/12/14 00:36:07, jbauman wrote:
> Hopefully this won't increase the chrome binary size too much.


Good point, I just measured and it looks like this adds about 50K to the
stripped release chrome binary on x86-32 linux.  How sensitive to changes of
this size are we?

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
File ppapi/api/dev/ppb_trace_event_dev.idl (right):

https://codereview.chromium.org/11464005/diff/2001/ppapi/api/dev/ppb_trace_ev...
ppapi/api/dev/ppb_trace_event_dev.idl:26: mem_t GetCategoryEnabled([in]
PP_Instance instance,
I'm not sure if I understand you Brett.  Why is it that all Pepper calls need an
instance or resource?  In the case of this interface, they don't interact with
any other part of pepper AFAIK, it's just that the code that does the tracing
lives in PPB_Instance_Shared.  PTAL at the revisions I made and tell me if that
is not kosher.


On 2012/12/15 00:35:31, brettw wrote:
> I think all pepper interfaces need a PP_Instance or a PP_Resource.
> 
> Doing a pepper call involves a global lock and some map lookups. If this is
too
> slow for the API (like this is going to be called a lot on the inner loop),
then
> we probably need to come up with a less chatty interface (like provide a list
of
> enabled categories in one call, and a way to periodically provide a batch of
> several events to the system, and then have the plugin code do all the
low-level
> management). What we do here may depend on how you intend this to be used.

https://codereview.chromium.org/11464005/diff/2001/ppapi/shared_impl/ppb_inst...
File ppapi/shared_impl/ppb_instance_shared.cc (right):

https://codereview.chromium.org/11464005/diff/2001/ppapi/shared_impl/ppb_inst...
ppapi/shared_impl/ppb_instance_shared.cc:68: // management to make this work
quickly without requiring IPCs for every call,
On 2012/12/14 00:36:07, jbauman wrote:
> This isn't true anymore.

Done.

[brettw, 2012-12-19 22:29:25]

I'm OK not taking PP_Instance in the interface. I think we can skip the thunk
file here and do what we did for
ppapi/shared_impl/private/net_address_private_impl.cc where the getter (with the
right name in the thunk namespace) is just implemented in the .cc file. The
thunk stuff is really just to dispatch to the right virtual C++ interface.

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
File ppapi/api/dev/ppb_trace_event_dev.idl (right):

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
ppapi/api/dev/ppb_trace_event_dev.idl:28: * Adds a trace event to the platform
tracing system.
Can this be documented more fully? These API files should be as well-defined as
possible, and not require looking at e.g. base to see what everything means.

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
ppapi/api/dev/ppb_trace_event_dev.idl:44: * Sets the thread name so it will show
up properly in chrome://tracing.
Can you put "calling thread" or something in here to specify which thread is
being set?

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
File ppapi/shared_impl/ppb_trace_event_shared.cc (right):

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:16: void*
PPB_Trace_Event_Shared::GetCategoryEnabled(const char* category_name) {
These should have "static" comments.

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:21: return
const_cast<void*>(reinterpret_cast<const void*>(
reinterpret_cast -> I think you can static_cast pointers to void*

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:37: (const unsigned
char*)category_enabled, name, id, num_args, arg_names,
C++ cast

[elijahtaylor1, 2012-12-21 00:59:15]

Thanks for the review, I moved things around as suggested and I think it's
looking much better now.

PTAL, sorry for the file renaming mid-CL

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
File ppapi/api/dev/ppb_trace_event_dev.idl (right):

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
ppapi/api/dev/ppb_trace_event_dev.idl:28: * Adds a trace event to the platform
tracing system.
On 2012/12/19 22:29:25, brettw wrote:
> Can this be documented more fully? These API files should be as well-defined
as
> possible, and not require looking at e.g. base to see what everything means.

Done.

I would like to include a copy of trace_event.h, etc. that will work out of the
box in the C++ pepper library, but would prefer to do that in another change
because it's not exactly pertinent to getting the interface in, and that change
will live more in the NaCl SDK.

https://codereview.chromium.org/11464005/diff/11001/ppapi/api/dev/ppb_trace_e...
ppapi/api/dev/ppb_trace_event_dev.idl:44: * Sets the thread name so it will show
up properly in chrome://tracing.
On 2012/12/19 22:29:25, brettw wrote:
> Can you put "calling thread" or something in here to specify which thread is
> being set?

Done.

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
File ppapi/shared_impl/ppb_trace_event_shared.cc (right):

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:16: void*
PPB_Trace_Event_Shared::GetCategoryEnabled(const char* category_name) {
On 2012/12/19 22:29:25, brettw wrote:
> These should have "static" comments.

Done.

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:21: return
const_cast<void*>(reinterpret_cast<const void*>(
On 2012/12/19 22:29:25, brettw wrote:
> reinterpret_cast -> I think you can static_cast pointers to void*

Done.

https://codereview.chromium.org/11464005/diff/11001/ppapi/shared_impl/ppb_tra...
ppapi/shared_impl/ppb_trace_event_shared.cc:37: (const unsigned
char*)category_enabled, name, id, num_args, arg_names,
On 2012/12/19 22:29:25, brettw wrote:
> C++ cast

Done.

[brettw, 2013-01-02 22:44:25]

lgtm

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h#...
base/debug/trace_event.h:57: #define TRACE_EVENT_API_CLASS_EXPORT BASE_EXPORT
Is it possible to #undef this stuff after including the internal.h file to
prevent macro pollution of other files.

https://codereview.chromium.org/11464005/diff/16001/ppapi/thunk/ppb_instance_...
File ppapi/thunk/ppb_instance_api.h (right):

https://codereview.chromium.org/11464005/diff/16001/ppapi/thunk/ppb_instance_...
ppapi/thunk/ppb_instance_api.h:12: #include "ppapi/c/dev/ppb_trace_event_dev.h"
I think you can revert this file.

[elijahtaylor1, 2013-01-03 22:11:35]

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h#...
base/debug/trace_event.h:57: #define TRACE_EVENT_API_CLASS_EXPORT BASE_EXPORT
On 2013/01/02 22:44:25, brettw wrote:
> Is it possible to #undef this stuff after including the internal.h file to
> prevent macro pollution of other files.

Done.

https://codereview.chromium.org/11464005/diff/16001/ppapi/thunk/ppb_instance_...
File ppapi/thunk/ppb_instance_api.h (right):

https://codereview.chromium.org/11464005/diff/16001/ppapi/thunk/ppb_instance_...
ppapi/thunk/ppb_instance_api.h:12: #include "ppapi/c/dev/ppb_trace_event_dev.h"
On 2013/01/02 22:44:25, brettw wrote:
> I think you can revert this file.

Done.

[elijahtaylor1, 2013-01-03 22:22:32]

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/11464005/diff/16001/base/debug/trace_event.h#...
base/debug/trace_event.h:57: #define TRACE_EVENT_API_CLASS_EXPORT BASE_EXPORT
On 2013/01/03 22:11:35, elijahtaylor1 wrote:
> On 2013/01/02 22:44:25, brettw wrote:
> > Is it possible to #undef this stuff after including the internal.h file to
> > prevent macro pollution of other files.
> 
> Done.

Clicked Done before and forgot to edit.  This was actually not possible, the
macros need to remain defined.

[brettw, 2013-01-03 22:23:10]

> Clicked Done before and forgot to edit.  This was actually not possible, the
> macros need to remain defined.

Too bad, oh well.

[commit-bot: I haz the power, 2013-01-04 21:17:11]

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

[commit-bot: I haz the power, 2013-01-04 23:37:47]

Change committed as 175233