[webkit-reviews] review granted: [Bug 85399] [Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly : [Attachment 139939] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 2 21:36:53 PDT 2012


Adam Barth <abarth at webkit.org> has granted Mark Pilgrim (Google)
<pilgrim at chromium.org>'s request for review:
Bug 85399: [Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag
directly
https://bugs.webkit.org/show_bug.cgi?id=85399

Attachment 139939: Patch
https://bugs.webkit.org/attachment.cgi?id=139939&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139939&action=review


This looks fine, but there are some name nit picks below.  I know you're just
using the names that already exist, but we might want to take this opportunity
to improve them slightly.

> Source/WebCore/platform/TraceEventSupport.h:36
> +class TraceEventSupport {

TraceEventSupport seems overly wordy.  How about EventTracer ?

> Source/WebCore/platform/TraceEventSupport.h:38
> +    static const unsigned char* getTraceCategoryEnabledFlag(const char*);

What does getTraceCategoryEnabledFlag mean?  Maybe... 
traceCategoryForEnableFlag ?

> Source/WebCore/platform/TraceEventSupport.h:39
> +    static int addTraceEvent(char phase,

addTraceEvent -> traceEvent ?

Generally, we prefer strong verbs like "trace" over weak verbs like "add".


More information about the webkit-reviews mailing list