Bug 76885 - [Chromium] Add chromium-style tracing support
Summary: [Chromium] Add chromium-style tracing support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Bates
URL:
Keywords:
Depends on: 77571
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 17:14 PST by John Bates
Modified: 2012-02-09 14:18 PST (History)
8 users (show)

See Also:


Attachments
Patch (45.42 KB, patch)
2012-01-23 17:18 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (48.87 KB, patch)
2012-01-23 18:22 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (48.54 KB, patch)
2012-01-24 15:31 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (46.95 KB, patch)
2012-01-25 14:05 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (53.41 KB, patch)
2012-01-25 15:04 PST, John Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (58.37 KB, application/zip)
2012-01-25 15:42 PST, WebKit Review Bot
no flags Details
Patch (53.37 KB, patch)
2012-01-26 13:28 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (52.76 KB, patch)
2012-01-26 17:21 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (52.89 KB, patch)
2012-01-26 17:35 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (52.85 KB, patch)
2012-01-27 12:43 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (54.31 KB, patch)
2012-01-30 14:22 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (54.48 KB, patch)
2012-02-03 15:08 PST, John Bates
no flags Details | Formatted Diff | Diff
Patch (54.34 KB, patch)
2012-02-09 11:36 PST, John Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bates 2012-01-23 17:14:59 PST
[Chromium] Add chromium-style tracing support
Comment 1 John Bates 2012-01-23 17:18:46 PST
Created attachment 123667 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-23 17:21:00 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 James Robinson 2012-01-23 17:38:22 PST
Comment on attachment 123667 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:246
> +    virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; }
> +    virtual int addTraceEvent(

these are the public WK APIs that are being exported (and that anyone implementing support for the WebKit API will need to implement), so they need to be documented here.
Comment 4 John Bates 2012-01-23 18:22:24 PST
Created attachment 123687 [details]
Patch
Comment 5 Nat Duca 2012-01-23 18:43:54 PST
Comment on attachment 123687 [details]
Patch

Is the use of the internal namespace buying us anything? I'd rather we just set up the names so that they're not going to obviously clash. We talked about this before though so not a blocker from my point of view.

How are you going to switch over the old instrumentation points to this one? Does this even compile?
Comment 6 Darin Fisher (:fishd, Google) 2012-01-23 22:11:46 PST
Comment on attachment 123687 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:250
> +    virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; }

sorry, i can't help but nit-pick the name of this function.  it confuses me everytime
I read it.

perhaps better named:  "const bool* getTraceCategoryEnabledFlag(const char* categoryName)"

 - what's the point of returning the address of an unsigned char over a bool?
 - it seems like it should have the word "Trace" in it somewhere for namespace reasons.
 - a "Flag" suffix might make the function name a little clearer--dunno.

Everytime I read "getCategoryEnabled" I think it is going to tell me right now if the category
is enabled, but in reality this is a query for the address of a flag that I can later dereference
to see if the category is enabled.

another idea, maybe too crazy, would be to implement a small class to wrap the flag:

  class WebTraceCategory {
  public:
      ...
      bool isEnabled() const { *m_flag != 0; }
  private:
      const unsigned char* m_flag;
  };

Then getCategoryEnabled could be morphed into "WebTraceCategory getTraceCategory(const char* categoryName)"

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:274
> +    virtual int addTraceEvent(

had to you considered creating a struct for all these params?

  WebTraceEvent?

if you went with my suggestion of WebTraceCategory, then you could also
just make this be a virtual method on WebTraceCategory since you already
having to pass the "categoryEnabled" flag to this function.

also, if you went with WebTraceCategory::addEvent, then you'd be able to
document the addEvent function not in WebKitPlatformSupport.h.

perhaps some downsides to introducing classes like these:

 - don't have it on the chromium side
 - don't have it on the webcore side
 - would be awkward to have it only in the webkit API and not elsewhere

hmmm...
Comment 7 John Bates 2012-01-24 10:07:08 PST
(In reply to comment #5)
> (From update of attachment 123687 [details])
> Is the use of the internal namespace buying us anything? 

Yeah, I think so. It buys us less WebCore namespace clutter with things that should never be used outside of this header. This follows the original design in this header where the ScopedTracer class was in the internal namespace.

>I'd rather we just set up the names so that they're not going to obviously clash. 

How about we mangle the names a bit more but still keep them in internal to avoid cluttering the WebCore namespace?

> How are you going to switch over the old instrumentation points to this one? Does this even compile?

Yes, it does compile and run. Notice the old TRACE_EVENT macro that has been changed to use the new style. However, a followup patch needs to clean up all instances of TRACE_EVENT so that we're not wasting arguments on 0 and 'this', etc. We could also optionally switch to variadic TRACE_EVENT at that point.
Comment 8 John Bates 2012-01-24 10:45:14 PST
(In reply to comment #6)
> (From update of attachment 123687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123687&action=review
> 
> > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:250
> > +    virtual const unsigned char* getCategoryEnabled(const char* categoryName) { return 0; }
> 
> sorry, i can't help but nit-pick the name of this function.  it confuses me everytime
> I read it.

Me too, it was left over API naming that I forgot to fix up -- I'll fix it now.

> 
> perhaps better named:  "const bool* getTraceCategoryEnabledFlag(const char* categoryName)"
>  - what's the point of returning the address of an unsigned char over a bool?

Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.)

>  - it seems like it should have the word "Trace" in it somewhere for namespace reasons.
>  - a "Flag" suffix might make the function name a little clearer--dunno.

Sounds good, will go with getTraceCategoryEnabledFlag.

> 
> Everytime I read "getCategoryEnabled" I think it is going to tell me right now if the category
> is enabled, but in reality this is a query for the address of a flag that I can later dereference
> to see if the category is enabled.
> 
> another idea, maybe too crazy, would be to implement a small class to wrap the flag:
> 
>   class WebTraceCategory {
>   public:
>       ...
>       bool isEnabled() const { *m_flag != 0; }
>   private:
>       const unsigned char* m_flag;
>   };
> 
> Then getCategoryEnabled could be morphed into "WebTraceCategory getTraceCategory(const char* categoryName)"
> 
> > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:274
> > +    virtual int addTraceEvent(
> 
> had to you considered creating a struct for all these params?

One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower.

> 
>   WebTraceEvent?
> 
> if you went with my suggestion of WebTraceCategory, then you could also
> just make this be a virtual method on WebTraceCategory since you already
> having to pass the "categoryEnabled" flag to this function.
> 
> also, if you went with WebTraceCategory::addEvent, then you'd be able to
> document the addEvent function not in WebKitPlatformSupport.h.
> 
> perhaps some downsides to introducing classes like these:
> 
>  - don't have it on the chromium side
>  - don't have it on the webcore side
>  - would be awkward to have it only in the webkit API and not elsewhere
> 
> hmmm...

Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value).
Comment 9 John Bates 2012-01-24 15:31:01 PST
Created attachment 123825 [details]
Patch
Comment 10 John Bates 2012-01-24 15:35:13 PST
(In reply to comment #9)
> Created an attachment (id=123825) [details]
> Patch

I've updated the API wording and moved from the internal namespace to WebCore::TraceEvent to give the tracing symbols a home of their own. PTAL.
Comment 11 John Bates 2012-01-24 15:36:01 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=123825) [details] [details]
> > Patch
> 
> I've updated the API wording and moved from the internal namespace to WebCore::TraceEvent to give the tracing symbols a home of their own. PTAL.

Note this won't compile until the chromium-side CL has landed: https://chromiumcodereview.appspot.com/9234007/
Comment 12 Darin Fisher (:fishd, Google) 2012-01-24 17:15:57 PST
(In reply to comment #8)
> > perhaps better named:  "const bool* getTraceCategoryEnabledFlag(const char* categoryName)"
> >  - what's the point of returning the address of an unsigned char over a bool?
> 
> Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.)

It doesn't seem like we should worry about C-compat, especially in the context of WebKit.  My proposal really was to just wrap the char pointer with a class.  The generated code would be no different than what you get by using the char pointer directly.  I was only proposing some syntactic sugar to make the API cleaner :)


> One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower.

Multiple definitions of the same struct is definitely annoying to maintain.  I wouldn't be concerned about the cost of copying structs.  Passing a struct by value is the same as passing a set of parameters each by value.  You should get nearly the same code generated.


> Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value).

I wasn't proposing an additional virtual calls.  My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory.
Comment 13 John Bates 2012-01-24 18:25:54 PST
(In reply to comment #12)
> (In reply to comment #8)
> > > perhaps better named:  "const bool* getTraceCategoryEnabledFlag(const char* categoryName)"
> > >  - what's the point of returning the address of an unsigned char over a bool?
> > 
> > Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.)
> 
> It doesn't seem like we should worry about C-compat, especially in the context of WebKit.  My proposal really was to just wrap the char pointer with a class.  The generated code would be no different than what you get by using the char pointer directly.  I was only proposing some syntactic sugar to make the API cleaner :)

I see. I guess this API has become a bit monstrous in appearance because it's never called by user code. It's only called by the macros in TraceEvent.h. The main goal for the macros is to keep them somewhat in sync with chromium's versions in trace_event.h so that the behavior stays the same.

> 
> 
> > One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower.
> 
> Multiple definitions of the same struct is definitely annoying to maintain.  I wouldn't be concerned about the cost of copying structs.  Passing a struct by value is the same as passing a set of parameters each by value.  You should get nearly the same code generated.

Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame.

> 
> 
> > Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value).
> 
> I wasn't proposing an additional virtual calls.  My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory.

I think I understand now. Is the goal to simplify the API of WebKitPlatformSupport or to provide syntactic sugar for the code that calls WebKitPlatformSupport? (The latter would be limited to the TraceEvent macros since they are the only callers.)
Comment 14 Darin Fisher (:fishd, Google) 2012-01-24 22:05:20 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #8)
> > > > perhaps better named:  "const bool* getTraceCategoryEnabledFlag(const char* categoryName)"
> > > >  - what's the point of returning the address of an unsigned char over a bool?
> > > 
> > > Some discussion of this took place on the chromium-side CL, but essentially C-compatibility in the unlikely event that we want to trace from a C third-party library. (tl;dr There were also unsubstantiated claims that this pointer could point to garbage in some cases, which would mean unsigned char != 0 is more spec compliant than bool == true, but that's being really pedantic. Also it should never be garbage thanks to cache coherency as discussed in the chromium CL http://codereview.chromium.org/9155024/.)
> > 
> > It doesn't seem like we should worry about C-compat, especially in the context of WebKit.  My proposal really was to just wrap the char pointer with a class.  The generated code would be no different than what you get by using the char pointer directly.  I was only proposing some syntactic sugar to make the API cleaner :)
> 
> I see. I guess this API has become a bit monstrous in appearance because it's never called by user code. It's only called by the macros in TraceEvent.h. The main goal for the macros is to keep them somewhat in sync with chromium's versions in trace_event.h so that the behavior stays the same.

Yeah...


> > > One of the design constraints was to avoid structs altogether in this API so that we didn't end up with structs defined in multiple libraries that could potentially diverge. Since the third party libraries can't include headers from chromium/base and base can't include from a third party, this seemed to be the best solution. Another design constraint is performance, so if we went with a struct and then had to copy from each third party variant to the base struct it would be a bit slower.
> > 
> > Multiple definitions of the same struct is definitely annoying to maintain.  I wouldn't be concerned about the cost of copying structs.  Passing a struct by value is the same as passing a set of parameters each by value.  You should get nearly the same code generated.
> 
> Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame.

that optimization should apply regardless of how you structure the parameters passed on the stack, right?  pass by struct or pass by N variables... same thing, no?


> > > Yeah, there were some hairy design constraints. It's also nice to avoid the virtual call to get the enabled state for optimal performance when tracing is disabled (where it currently only needs to fetch the unsigned char value).
> > 
> > I wasn't proposing an additional virtual calls.  My proposal was to move the addTraceEvent virtual call from WebKitPlatformSupport to WebTraceCategory.
> 
> I think I understand now. Is the goal to simplify the API of WebKitPlatformSupport or to provide syntactic sugar for the code that calls WebKitPlatformSupport? (The latter would be limited to the TraceEvent macros since they are the only callers.

I am mostly motivated by wanting to compartmentalize, but also when I see a function with a zillion mysterious parameters, it makes me feel a bit sad :)

That said, I don't think you need to necessarily take my suggestions.  I just wanted to share in case it spurned some thoughts about how to improve things.  It is fine to proceed as-is so that folks can start benefiting from tracing soon.
Comment 15 Darin Fisher (:fishd, Google) 2012-01-24 22:15:26 PST
(In reply to comment #14)
> > Good point. I was hoping the compiler could optimize those copies when they are passed through multiple leaf-function calls with the same parameters by sharing the stack frame.
> 
> that optimization should apply regardless of how you structure the parameters passed on the stack, right?  pass by struct or pass by N variables... same thing, no?

I get it now.  The structs at each level would be different types, and while we might attempt to keep them in-sync, if they ever drifted, then it would neutralize any chance of such a "pass through" optimization.  Got it.

OK, so you've convinced that we should have a function with a zillion parameters :)
Comment 16 Darin Fisher (:fishd, Google) 2012-01-24 22:23:02 PST
Comment on attachment 123825 [details]
Patch

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

> Source/WebCore/platform/chromium/PlatformSupport.h:424
> +                             const unsigned char* category_enabled,

nit: category_enabled_flag

> Source/WebCore/platform/chromium/TraceEvent.h:570
> +const int kZeroNumArgs = 0;

nit: webkit style for constants is still to use variable naming style: zeroNumArgs, etc.

> Source/WebCore/platform/chromium/TraceEvent.h:571
> +const int kNoThreshholdBeginId = -1;

nit: "hh" -> "h" ??

> Source/WebCore/platform/chromium/TraceEvent.h:791
> +    void Initialize(const unsigned char* categoryEnabled,

nit: webkit style has function names starting with a lowercase letter.
Comment 17 John Bates 2012-01-25 14:05:59 PST
Created attachment 124007 [details]
Patch
Comment 18 John Bates 2012-01-25 14:09:07 PST
Comment on attachment 123825 [details]
Patch

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

One more thing to do -- remove the old unused tracing API (traceEventBegin, etc).

>> Source/WebCore/platform/chromium/PlatformSupport.h:424
>> +                             const unsigned char* category_enabled,
> 
> nit: category_enabled_flag

Done.

>> Source/WebCore/platform/chromium/TraceEvent.h:570
>> +const int kZeroNumArgs = 0;
> 
> nit: webkit style for constants is still to use variable naming style: zeroNumArgs, etc.

Done.

>> Source/WebCore/platform/chromium/TraceEvent.h:571
>> +const int kNoThreshholdBeginId = -1;
> 
> nit: "hh" -> "h" ??

Done.

>> Source/WebCore/platform/chromium/TraceEvent.h:791
>> +    void Initialize(const unsigned char* categoryEnabled,
> 
> nit: webkit style has function names starting with a lowercase letter.

Done.
Comment 19 John Bates 2012-01-25 15:04:37 PST
Created attachment 124021 [details]
Patch
Comment 20 WebKit Review Bot 2012-01-25 15:42:07 PST
Comment on attachment 124021 [details]
Patch

Attachment 124021 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11191070

New failing tests:
animations/animation-controller-drt-api.html
animations/3d/transform-origin-vs-functions.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
accessibility/adjacent-continuations-cause-assertion-failure.html
animations/additive-transform-animations.html
animations/3d/change-transform-in-end-event.html
animations/3d/replace-filling-transform.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-direction-normal.html
accessibility/aria-checkbox-sends-notification.html
animations/animation-add-events-in-handler.html
Comment 21 WebKit Review Bot 2012-01-25 15:42:11 PST
Created attachment 124025 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 22 James Robinson 2012-01-25 15:45:48 PST
(In reply to comment #20)
> (From update of attachment 124021 [details])
> Attachment 124021 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11191070
> 
> New failing tests:
> animations/animation-controller-drt-api.html
> animations/3d/transform-origin-vs-functions.html
> accessibility/aria-checkbox-checked.html
> http/tests/appcache/access-via-redirect.php
> http/tests/appcache/auth.html
> animations/animation-css-rule-types.html
> animations/3d/matrix-transform-type-animation.html
> animations/3d/state-at-end-event-transform.html
> http/tests/appcache/credential-url.html
> http/tests/appcache/crash-when-navigating-away-then-back.html
> accessibility/anonymous-render-block-in-continuation-causes-crash.html
> accessibility/adjacent-continuations-cause-assertion-failure.html
> animations/additive-transform-animations.html
> animations/3d/change-transform-in-end-event.html
> animations/3d/replace-filling-transform.html
> http/tests/appcache/cyrillic-uri.html
> accessibility/anchor-linked-anonymous-block-crash.html
> animations/animation-direction-normal.html
> accessibility/aria-checkbox-sends-notification.html
> animations/animation-add-events-in-handler.html

Looks like every test that hits a TRACE_EVENT crashes
Comment 23 John Bates 2012-01-25 15:59:14 PST
(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 124021 [details] [details])
> > Attachment 124021 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11191070
> > 
> > New failing tests:
> > animations/animation-controller-drt-api.html
> > animations/3d/transform-origin-vs-functions.html
> > accessibility/aria-checkbox-checked.html
> > http/tests/appcache/access-via-redirect.php
> > http/tests/appcache/auth.html
> > animations/animation-css-rule-types.html
> > animations/3d/matrix-transform-type-animation.html
> > animations/3d/state-at-end-event-transform.html
> > http/tests/appcache/credential-url.html
> > http/tests/appcache/crash-when-navigating-away-then-back.html
> > accessibility/anonymous-render-block-in-continuation-causes-crash.html
> > accessibility/adjacent-continuations-cause-assertion-failure.html
> > animations/additive-transform-animations.html
> > animations/3d/change-transform-in-end-event.html
> > animations/3d/replace-filling-transform.html
> > http/tests/appcache/cyrillic-uri.html
> > accessibility/anchor-linked-anonymous-block-crash.html
> > animations/animation-direction-normal.html
> > accessibility/aria-checkbox-sends-notification.html
> > animations/animation-add-events-in-handler.html
> 
> Looks like every test that hits a TRACE_EVENT crashes

The chromium rev needs to be rolled past 118999 before this will run.
Comment 24 John Bates 2012-01-26 13:28:49 PST
Created attachment 124170 [details]
Patch
Comment 25 John Bates 2012-01-26 17:21:22 PST
Created attachment 124221 [details]
Patch
Comment 26 John Bates 2012-01-26 17:23:18 PST
Comment on attachment 124221 [details]
Patch

I removed the manglePointer API, because it's simpler and cleaner to do that through the flags parameter of addTraceEvent. Everything else is the same -- should be ready to commit.
Comment 27 John Bates 2012-01-26 17:35:29 PST
Created attachment 124224 [details]
Patch
Comment 28 John Bates 2012-01-26 17:36:27 PST
Comment on attachment 124224 [details]
Patch

Voiding unused parameters to be pedantic. Sorry for the patch spam!
Comment 29 John Bates 2012-01-27 10:09:03 PST
jamesr, darin -- final R / CQ?
Comment 30 James Robinson 2012-01-27 11:09:27 PST
Comment on attachment 124224 [details]
Patch

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

API is up to Darin. Left some comments.

> Source/WebCore/bindings/v8/V8Proxy.cpp:342
> +        TRACE_EVENT_BEGIN1("v8", "v8.compile", "url", source.url().string().ascii());

will the argument only be evaluated when tracing is active? source.url().string().ascii() is going to do at least one copy of the string data. even if it's only active when tracing is enabled this seems pretty heavyweight to do on every evaluate

> Source/WebCore/page/Console.cpp:302
> +    TRACE_EVENT_COPY_BEGIN0("webkit", title.utf8().data());

title.utf8() is going to do a conversion and string copy. i guess it was before, though
Comment 31 John Bates 2012-01-27 12:38:02 PST
Comment on attachment 124224 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8Proxy.cpp:342
>> +        TRACE_EVENT_BEGIN1("v8", "v8.compile", "url", source.url().string().ascii());
> 
> will the argument only be evaluated when tracing is active? source.url().string().ascii() is going to do at least one copy of the string data. even if it's only active when tracing is enabled this seems pretty heavyweight to do on every evaluate

It's only evaluated when tracing is active, but even so I'll remove it since this is a hot code path.

>> Source/WebCore/page/Console.cpp:302
>> +    TRACE_EVENT_COPY_BEGIN0("webkit", title.utf8().data());
> 
> title.utf8() is going to do a conversion and string copy. i guess it was before, though

Yeah, same behavior as before where this arg is only evaluated when tracing is enabled.
Comment 32 John Bates 2012-01-27 12:43:04 PST
Created attachment 124353 [details]
Patch
Comment 33 John Bates 2012-01-30 14:22:54 PST
Created attachment 124599 [details]
Patch
Comment 34 John Bates 2012-01-30 14:29:42 PST
(In reply to comment #33)
> Created an attachment (id=124599) [details]
> Patch

The latest patch adds more detail to the documentation of the new tracing API in WebKitPlatformSupport.h so that embedders know exactly what to expect.
Comment 35 WebKit Review Bot 2012-01-31 13:03:03 PST
Comment on attachment 124599 [details]
Patch

Clearing flags on attachment: 124599

Committed r106382: <http://trac.webkit.org/changeset/106382>
Comment 36 WebKit Review Bot 2012-01-31 13:03:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 John Bates 2012-02-03 14:48:22 PST
Reopen after revert to fix crash in chromium unit_tests.
Comment 38 John Bates 2012-02-03 15:08:13 PST
Created attachment 125429 [details]
Patch
Comment 39 John Bates 2012-02-03 15:13:18 PST
Comment on attachment 125429 [details]
Patch

I hadn't realized there were chromium tests that ran with the default implementation of WebKitPlatformSupport. The latest patch makes the tracing macros run in a disabled state when the default API in WebKitPlatformSupport is used.
Comment 40 John Bates 2012-02-07 10:47:54 PST
Comment on attachment 124599 [details]
Patch

recommitting now that chromium tests have been fixed.
Comment 41 WebKit Review Bot 2012-02-07 10:48:14 PST
Comment on attachment 124599 [details]
Patch

Rejecting attachment 124599 [details] from review queue.

jbates@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 42 WebKit Review Bot 2012-02-07 10:49:03 PST
Comment on attachment 124599 [details]
Patch

Rejecting attachment 124599 [details] from commit-queue.

jbates@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 43 John Bates 2012-02-09 11:36:07 PST
Created attachment 126338 [details]
Patch
Comment 44 WebKit Review Bot 2012-02-09 14:18:41 PST
Comment on attachment 126338 [details]
Patch

Clearing flags on attachment: 126338

Committed r107291: <http://trac.webkit.org/changeset/107291>
Comment 45 WebKit Review Bot 2012-02-09 14:18:49 PST
All reviewed patches have been landed.  Closing bug.