Bug 76885 – [Chromium] Add chromium-style tracing support

• Component: New Bugs
• Status: RESOLVED FIXED

[John Bates]

[Chromium] Add chromium-style tracing support

[John Bates]

Created attachment 123667 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

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.

[John Bates]

Created attachment 123687 [details]
Patch

[Nat Duca]

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?

[Darin Fisher (:fishd, Google)]

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

[John Bates]

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

[John Bates]

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

[John Bates]

Created attachment 123825 [details]
Patch

[John Bates]

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

[John Bates]

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

[Darin Fisher (:fishd, Google)]

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

[John Bates]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

(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 :)

[Darin Fisher (:fishd, Google)]

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.

[John Bates]

Created attachment 124007 [details]
Patch

[John Bates]

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.

[John Bates]

Created attachment 124021 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[James Robinson]

(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

[John Bates]

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

[John Bates]

Created attachment 124170 [details]
Patch

[John Bates]

Created attachment 124221 [details]
Patch

[John Bates]

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.

[John Bates]

Created attachment 124224 [details]
Patch

[John Bates]

Comment on attachment 124224 [details]
Patch

Voiding unused parameters to be pedantic. Sorry for the patch spam!

[John Bates]

jamesr, darin -- final R / CQ?

[James Robinson]

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

[John Bates]

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.

[John Bates]

Created attachment 124353 [details]
Patch

[John Bates]

Created attachment 124599 [details]
Patch

[John Bates]

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

[WebKit Review Bot]

Comment on attachment 124599 [details]
Patch

Clearing flags on attachment: 124599

Committed r106382: <http://trac.webkit.org/changeset/106382>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[John Bates]

Reopen after revert to fix crash in chromium unit_tests.

[John Bates]

Created attachment 125429 [details]
Patch

[John Bates]

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.

[John Bates]

Comment on attachment 124599 [details]
Patch

recommitting now that chromium tests have been fixed.

[WebKit Review Bot]

Comment on attachment 124599 [details]
Patch

Rejecting attachment 124599 [details] from review queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 124599 [details]
Patch

Rejecting attachment 124599 [details] from commit-queue.

[e-mail] 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.

[John Bates]

Created attachment 126338 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126338 [details]
Patch

Clearing flags on attachment: 126338

Committed r107291: <http://trac.webkit.org/changeset/107291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.