Bug 85399

Created attachment 139854 [details] Patch

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

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

Created attachment 139858 [details] Patch

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

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

Created attachment 139865 [details] Patch

Fixed my error in xcodeproj

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

Created attachment 139877 [details] Patch

Fixed unused parameter warnings.

Comment on attachment 139877 [details] Patch LGTM

Can I get a review on this?

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.

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

Created attachment 139939 [details] Patch

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

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

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

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

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

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

Created attachment 141227 [details] Patch

Comment on attachment 141227 [details] Patch OK, TraceEventSupport is now called EventTracer.

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 on attachment 141227 [details] Patch Clearing flags on attachment: 141227 Committed r116690: <http://trac.webkit.org/changeset/116690>

All reviewed patches have been landed. Closing bug.