Bug 85399 - [Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly
Summary: [Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
Depends on:
Blocks: 82948
  Show dependency treegraph
 
Reported: 2012-05-02 12:01 PDT by Mark Pilgrim (Google)
Modified: 2012-05-10 14:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (17.06 KB, patch)
2012-05-02 12:05 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (25.96 KB, patch)
2012-05-02 12:25 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (25.96 KB, patch)
2012-05-02 13:06 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (25.73 KB, patch)
2012-05-02 13:50 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (25.42 KB, patch)
2012-05-02 19:09 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (23.96 KB, patch)
2012-05-10 12:38 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-05-02 12:01:52 PDT
[Chromium] Call addTraceEvent and getTraceCategoryEnabledFlag directly
Comment 1 Mark Pilgrim (Google) 2012-05-02 12:05:05 PDT
Created attachment 139854 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-05-02 12:18:00 PDT
Comment on attachment 139854 [details]
Patch

Attachment 139854 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12612021
Comment 3 Build Bot 2012-05-02 12:24:46 PDT
Comment on attachment 139854 [details]
Patch

Attachment 139854 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12605014
Comment 4 Mark Pilgrim (Google) 2012-05-02 12:25:24 PDT
Created attachment 139858 [details]
Patch
Comment 5 Mark Pilgrim (Google) 2012-05-02 12:26:18 PDT
It helps when you "svn add" the new files you're adding to WebKit...
Comment 6 Build Bot 2012-05-02 12:56:14 PDT
Comment on attachment 139858 [details]
Patch

Attachment 139858 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12545028
Comment 7 Mark Pilgrim (Google) 2012-05-02 13:06:48 PDT
Created attachment 139865 [details]
Patch
Comment 8 Mark Pilgrim (Google) 2012-05-02 13:07:43 PDT
Fixed my error in xcodeproj
Comment 9 Build Bot 2012-05-02 13:45:41 PDT
Comment on attachment 139865 [details]
Patch

Attachment 139865 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12606023
Comment 10 Mark Pilgrim (Google) 2012-05-02 13:50:24 PDT
Created attachment 139877 [details]
Patch
Comment 11 Mark Pilgrim (Google) 2012-05-02 13:50:58 PDT
Fixed unused parameter warnings.
Comment 12 John Bates 2012-05-02 14:13:24 PDT
Comment on attachment 139877 [details]
Patch

LGTM
Comment 13 Mark Pilgrim (Google) 2012-05-02 18:00:05 PDT
Can I get a review on this?
Comment 14 Eric Seidel (no email) 2012-05-02 18:30:59 PDT
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.
Comment 15 Mark Pilgrim (Google) 2012-05-02 18:42:30 PDT
(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).
Comment 16 Adam Barth 2012-05-02 18:59:28 PDT
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.
Comment 17 Mark Pilgrim (Google) 2012-05-02 19:09:29 PDT
Created attachment 139939 [details]
Patch
Comment 18 Mark Pilgrim (Google) 2012-05-02 19:11:39 PDT
OK, spacing issues fixed and header file parameter names restored.
Comment 19 Adam Barth 2012-05-02 21:36:53 PDT
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".
Comment 20 Mark Pilgrim (Google) 2012-05-03 05:35:23 PDT
jbates, can you comment on Adam's renaming proposal?
Comment 21 John Bates 2012-05-03 19:08:27 PDT
(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.
Comment 22 Mark Pilgrim (Google) 2012-05-09 07:48:06 PDT
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.
Comment 23 Adam Barth 2012-05-09 09:12:12 PDT
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.
Comment 24 Nat Duca 2012-05-09 10:02:54 PDT
Adam, does "push" constitute a verb that you consider weak?
Comment 25 Adam Barth 2012-05-09 10:11:49 PDT
> 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.
Comment 26 Mark Pilgrim (Google) 2012-05-10 12:38:26 PDT
Created attachment 141227 [details]
Patch
Comment 27 Mark Pilgrim (Google) 2012-05-10 12:38:53 PDT
Comment on attachment 141227 [details]
Patch

OK, TraceEventSupport is now called EventTracer.
Comment 28 Adam Barth 2012-05-10 13:06:42 PDT
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.
Comment 29 WebKit Review Bot 2012-05-10 14:51:17 PDT
Comment on attachment 141227 [details]
Patch

Clearing flags on attachment: 141227

Committed r116690: <http://trac.webkit.org/changeset/116690>
Comment 30 WebKit Review Bot 2012-05-10 14:51:24 PDT
All reviewed patches have been landed.  Closing bug.