Bug 85399 – [Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

[Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly

[Mark Pilgrim (Google)]

Created attachment 139854 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 139854 [details]
Patch

Attachment 139854 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12612021

[Build Bot]

Comment on attachment 139854 [details]
Patch

Attachment 139854 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12605014

[Mark Pilgrim (Google)]

Created attachment 139858 [details]
Patch

[Mark Pilgrim (Google)]

It helps when you "svn add" the new files you're adding to WebKit...

[Build Bot]

Comment on attachment 139858 [details]
Patch

Attachment 139858 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12545028

[Mark Pilgrim (Google)]

Created attachment 139865 [details]
Patch

[Mark Pilgrim (Google)]

Fixed my error in xcodeproj

[Build Bot]

Comment on attachment 139865 [details]
Patch

Attachment 139865 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12606023

[Mark Pilgrim (Google)]

Created attachment 139877 [details]
Patch

[Mark Pilgrim (Google)]

Fixed unused parameter warnings.

[John Bates]

Comment on attachment 139877 [details]
Patch

LGTM

[Mark Pilgrim (Google)]

Can I get a review on this?

[Eric Seidel (no email)]

Comment on attachment 139877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139877&action=review

> Source/WebCore/GNUmakefile.list.am:3583
> -	Source/WebCore/platform/StatsCounter.h \
> +        Source/WebCore/platform/StatsCounter.h \

This looks wrong.

> Source/WebCore/GNUmakefile.list.am:3658
> +        Source/WebCore/platform/TraceEventSupport.cpp \
> +        Source/WebCore/platform/TraceEventSupport.h \

This too.

> Source/WebCore/platform/TraceEventSupport.h:49
> +    static int addTraceEvent(char,
> +                             const unsigned char*,
> +                             const char*,
> +                             unsigned long long,
> +                             int,
> +                             const char**,
> +                             const unsigned char*,
> +                             const unsigned long long*,
> +                             int,
> +                             long long,
> +                             unsigned char);

Named args would help, alot.

[Mark Pilgrim (Google)]

(In reply to comment #14)
> (From update of attachment 139877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139877&action=review
> 
> > Source/WebCore/GNUmakefile.list.am:3583
> > -	Source/WebCore/platform/StatsCounter.h \
> > +        Source/WebCore/platform/StatsCounter.h \
> 
> This looks wrong.

Will fix the spacing.

> 
> > Source/WebCore/GNUmakefile.list.am:3658
> > +        Source/WebCore/platform/TraceEventSupport.cpp \
> > +        Source/WebCore/platform/TraceEventSupport.h \
> 
> This too.

Why? We're adding these two files to WebCore/platform/.

> 
> > Source/WebCore/platform/TraceEventSupport.h:49
> > +    static int addTraceEvent(char,
> > +                             const unsigned char*,
> > +                             const char*,
> > +                             unsigned long long,
> > +                             int,
> > +                             const char**,
> > +                             const unsigned char*,
> > +                             const unsigned long long*,
> > +                             int,
> > +                             long long,
> > +                             unsigned char);
> 
> Named args would help, alot.

Named args break the Mac build (unused parameters, warnings treated as errors).

[Adam Barth]

Comment on attachment 139877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139877&action=review

>>> Source/WebCore/GNUmakefile.list.am:3658
>>> +        Source/WebCore/platform/TraceEventSupport.h \
>> 
>> This too.
> 
> Why? We're adding these two files to WebCore/platform/.

I think there's just a tab/space intent issue.

>>> Source/WebCore/platform/TraceEventSupport.h:49
>>> +                             unsigned char);
>> 
>> Named args would help, alot.
> 
> Named args break the Mac build (unused parameters, warnings treated as errors).

You should be able to declare them in the header.  It's only in the implementation that unused parameter names cause problems.

[Mark Pilgrim (Google)]

Created attachment 139939 [details]
Patch

[Mark Pilgrim (Google)]

OK, spacing issues fixed and header file parameter names restored.

[Adam Barth]

Comment on attachment 139939 [details]
Patch

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".

[Mark Pilgrim (Google)]

jbates, can you comment on Adam's renaming proposal?

[John Bates]

(In reply to comment #19)
> (From update of attachment 139939 [details])
> 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".

These names match up with chromium APIs and type names so it would be best for consistency to keep them as is if it's not too much of an issue.

[Mark Pilgrim (Google)]

Comment on attachment 139939 [details]
Patch

Following up on jbates' comment, I feel we should just land this as-is. I just checked and the patch still applies cleanly to ToT.

[Adam Barth]

Comment on attachment 139939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139939&action=review

>>> Source/WebCore/platform/TraceEventSupport.h:36
>>> +class TraceEventSupport {
>> 
>> TraceEventSupport seems overly wordy.  How about EventTracer ?
> 
> These names match up with chromium APIs and type names so it would be best for consistency to keep them as is if it's not too much of an issue.

Ok.  Let's keep the getTraceCategoryEnabledFlag and addTraceEvent names for now.  They're not great names because they use weak verbs like "get" and "add", but that's ok for now.  However, the TraceEventSupport name doesn't appear to be used previously.  Perhaps we should pick a better than for that?  EventTracer seems more like the name of an object that traces events.

[Nat Duca]

Adam, does "push" constitute a verb that you consider weak?

[Adam Barth]

> Adam, does "push" constitute a verb that you consider weak?

Push can be a good verb, especially when you're working with a stack or a queue.  In the case here, the work "trace" is already in the names, but it's used as an adjective when it could be a great verb.  None of these things are really big deals though.

[Mark Pilgrim (Google)]

Created attachment 141227 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 141227 [details]
Patch

OK, TraceEventSupport is now called EventTracer.

[Adam Barth]

Comment on attachment 141227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141227&action=review

> Source/WebCore/platform/EventTracer.cpp:51
> +int EventTracer::addTraceEvent(char,
> +                                     const unsigned char*,
> +                                     const char*,
> +                                     unsigned long long,
> +                                     int,
> +                                     const char**,
> +                                     const unsigned char*,
> +                                     const unsigned long long*,
> +                                     int,
> +                                     long long,
> +                                     unsigned char)

This argument list looks a bit silly, but I'm not sure it matters too much.  I might have combined these all into one line.  (Not worth uploading the patch again.)

> Source/WebCore/platform/chromium/EventTracerChromium.cpp:44
> +int EventTracer::addTraceEvent(char phase,
> +                                     const unsigned char* categoryEnabledFlag,

The indents should probably line up.

[WebKit Review Bot]

Comment on attachment 141227 [details]
Patch

Clearing flags on attachment: 141227

Committed r116690: <http://trac.webkit.org/changeset/116690>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.