initial import of Chrome's trace_event into skia framework

include/utils/SkEventTracer.h

+/*
+ * Copyright (C) 2014 Google Inc. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef EventTracer_h

[mtklein, 2014/01/29 20:57:55]

SkEventTracer_DEFINED

[humper, 2014/01/29 21:29:09]

Done.

+#define EventTracer_h
+
+#include "SkTypes.h"
+

[mtklein, 2014/01/29 20:57:55]

I'm getting the feeling users aren't supposed to even look at this file.  If
that's so, can you add a big comment that points them over to the macros they
want to be using?

[humper, 2014/01/29 21:29:09]

Sure -- this isn't a standard skia user-facing API, but Chrome will call it.  I 
can add a comment to the effect that this is likely not the place that people
want to be looking.

+// This will mark the trace event as disabled by default. The user will need
+// to explicitly enable the event.
+#define TRACE_DISABLED_BY_DEFAULT(name) "disabled-by-default-" name

[mtklein, 2014/01/29 20:57:55]

Do we not double-define this?  Also in SkTraceEvent.

[humper, 2014/01/29 21:29:09]

Done.

+
+namespace skia {

[mtklein, 2014/01/29 20:57:55]

I don't mind namespace namespacing or Sk namespacing, but doing both seems
redundant.

[humper, 2014/01/29 21:29:09]

I'll remove the namespaces.

+namespace tracing {
+
+typedef uint32_t TraceEventHandle;
+
+// FIXME: Make these global variables thread-safe. Make a value update atomic.
+SK_API extern long* traceSamplingState[3];

[mtklein, 2014/01/29 20:57:55]

I don't see where we need this here?

[humper, 2014/01/29 21:29:09]

Deleted.  I'll reintroduce it when I tackle the part of the tracing system that
uses it, in a later phase of this integration.

+
+class SK_API SkEventTracer {
+public:
+
+    static SkEventTracer* GetInstance();
+
+    static void SetInstance(SkEventTracer* tracer) { 

[mtklein, 2014/01/29 20:57:55]

Can this be private?  Who calls it?

[humper, 2014/01/29 21:29:09]

Chrome will call it.

+        SkDELETE(SkEventTracer::_instance);
+        SkEventTracer::_instance = tracer;
+    }
+
+    virtual ~SkEventTracer() { SkDELETE(SkEventTracer::_instance); }

[mtklein, 2014/01/29 20:57:55]

Does this ever get called?  Is it not an infinite deletion loop?

You can pass another void(*)(void) to SkOnce as the fourth argument to register
it to be called at process exit, if you want to clean up the global.

[humper, 2014/01/29 21:29:09]

Good catch.  Will move to SkOnce.

+
+    // The pointer returned from GetCategoryGroupEnabled() points to a
+    // value with zero or more of the following bits. Used in this class only.
+    // The TRACE_EVENT macros should only use the value as a bool.
+    // These values must be in sync with macro values in trace_event.h in chromium.
+    enum CategoryGroupEnabledFlags {

[mtklein, 2014/01/29 20:57:55]

We'd normally call these kEnabledForRecording_CategoryGroupEnabledFlags, etc.

Is there any particular reason to keep them LIKE_THIS?

[humper, 2014/01/29 21:29:09]

As long as the numbers match Chromium, we can call them whatever we want.  I'll
clean them up.

+        // Category group enabled for the recording mode.
+        ENABLED_FOR_RECORDING = 1 << 0,
+        // Category group enabled for the monitoring mode.
+        ENABLED_FOR_MONITORING = 1 << 1,
+        // Category group enabled by SetEventCallbackEnabled().
+        ENABLED_FOR_EVENT_CALLBACK = 1 << 2,
+    };
+
+    virtual const unsigned char* GetCategoryGroupEnabled(const char* name) = 0;

[mtklein, 2014/01/29 20:57:55]

So there's one global SkEventTracer that's an interface to one of several
concrete types?

Can you make an empty protected constructor for SkEventTracer?

[mtklein, 2014/01/29 20:57:55]

These methods all want to be firstLetterLowercase.

[humper, 2014/01/29 21:29:09]

Sure.  is that necessary since SkEventTracer is pure virtual?

[humper, 2014/01/29 21:29:09]

Done.

+    virtual const char* GetCategoryGroupName(
+      const unsigned char* category_group_enabled) = 0;
+    
+    virtual TraceEventHandle 
+        AddTraceEvent(char phase,

[mtklein, 2014/01/29 20:57:55]

I'm getting a little thrown by the types here

some places we have const char*, some const unsigned char*, some long, some
unsigned long long, etc.

Where the type matters and it's not semantically an ASCII character or a string
(char or const char*), can you use the types from stdint instead? (uint8_t,
int64_t, uint64_t)?

[humper, 2014/01/29 21:29:09]

Done.

+                      const unsigned char* categoryEnabledFlag,
+                      const char* name,
+                      unsigned long long id,
+                      int numArgs,
+                      const char** argNames,
+                      const unsigned char* argTypes,
+                      const unsigned long long* argValues,
+                      unsigned char flags) = 0;
+    
+    virtual void 
+        UpdateTraceEventDuration(const unsigned char* categoryEnabledFlag, 
+                                 const char* name, 
+                                 tracing::TraceEventHandle) = 0;
+private:
+    static SkEventTracer *_instance;

[mtklein, 2014/01/29 20:57:55]

->

static SkEventTracer* fInstance;

or

static SkEventTracer* gInstance; ?

[humper, 2014/01/29 21:29:09]

Done.

+};
+
+} // namespace tracing
+} // namespace skia
+
+#endif // EventTracer_h