Bug 70618 - [chromium] Add isTraceEventEnabledForCategory to PlatformSupport.
Summary: [chromium] Add isTraceEventEnabledForCategory to PlatformSupport.
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: Nat Duca
URL:
Keywords:
Depends on:
Blocks: 70620
  Show dependency treegraph
 
Reported: 2011-10-21 10:46 PDT by Nat Duca
Modified: 2011-10-24 14:19 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.59 KB, patch)
2011-10-21 10:47 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Blanket enable instead of per-category (3.44 KB, patch)
2011-10-21 13:33 PDT, Nat Duca
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-10-21 10:46:14 PDT
[chromium] Add isTraceEventEnabledForCategory to PlatformSupport.
Comment 1 Nat Duca 2011-10-21 10:47:07 PDT
Created attachment 111985 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-21 10:50:08 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Nat Duca 2011-10-21 13:33:18 PDT
Created attachment 112011 [details]
Blanket enable instead of per-category
Comment 4 Darin Fisher (:fishd, Google) 2011-10-23 23:27:21 PDT
Comment on attachment 112011 [details]
Blanket enable instead of per-category

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

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:233
> +    virtual bool isTraceEventEnabled() const { return true; }

actually, are you sure you don't want to invert this?  it seems better to push
settings down into webkit instead of making webkit ask the embedder each and
every time via a virtual function call.  most other things like this work by
pushing a setting down into WebKit.  see for example WebKit::setLayoutTestMode
or WebKit::WebSecurityPolicy.

perhaps this could be a method in WebKit.h?  setTraceEventEnabled(bool)?

it seems like a corresponding method could exist in TraceEvent.h.
Comment 5 Nat Duca 2011-10-24 14:05:02 PDT
You're totally right about inverting it. I investigated this a bit further and I think I know how we should do it. Unfortunately, doing that inversion means we also have to switch from having traceBegin/traceEnd to the addTraceEvent style. I filed a bug to do this, and I'm going to beg jbates to do it. :)

https://bugs.webkit.org/show_bug.cgi?id=70761
Comment 6 Nat Duca 2011-10-24 14:19:26 PDT
Committed r98282: <http://trac.webkit.org/changeset/98282>