Summary: | [Chromium] IndexedDB: Integrate with about:tracing | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||
Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dgrogan, nduca, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Joshua Bell
2012-02-16 11:02:44 PST
Created attachment 127411 [details]
Patch
Created attachment 127660 [details]
Patch
Comment on attachment 127660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127660&action=review LGTM > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:41 > +#include "TraceEvent.h" Would it have been possible to do something here like #define IDB_TRACE(a, b) TRACE_EVENT0(a, b) #else #define IDB_TRACE(a, b) (void)0 and then use IDB_TRACE everywhere instead of #if PLATFORM(chromium) TRACE_EVENT0(....) #endif ? If so, I don't think you should go back and change it, mostly just wondering. (In reply to comment #3) > > and then use IDB_TRACE everywhere instead of #if PLATFORM(chromium) TRACE_EVENT0(....) #endif ? > > If so, I don't think you should go back and change it, mostly just wondering. Nat, any opinions? At this point I'm just copying the style used in other files. I agree it would be more succinct. I'd go further and have it take just the one argument so: #define IDB_TRACE(a) TRACE_EVENT0("IndexedDB", (a)) So callers can just use: IDB_TRACE("IDBObjectStore::put") Also, we could move the now 5-line #if...#endif preamble into a header file (IDBTracing.h) but I'm not sure how "webkitty" that is. Neat stuff, Josh! (In reply to comment #4) > #define IDB_TRACE(a) TRACE_EVENT0("IndexedDB", (a)) > > So callers can just use: IDB_TRACE("IDBObjectStore::put") > I certainly don't see a problem with it. I think this pokes at us needing to clean up the trace macro for overall webcore use, but thats a separate conversation for a separate time. Do what you want to make yourself smile, from my point of view. :) Comment on attachment 127660 [details]
Patch
OK. I agree that PLATFORM(CHROMIUM) seems like the wrong guard. Also, in the rest of webcore, I think we don't guard includes if it's easy to just have a stub header. This makes the #include lists more concise.
Created attachment 128088 [details]
Patch
@tony can you take another look? Guidance on the naming/formatting of the include file appreciated. (Also, I'm not sure why the bots are so unhappy with this patch. I'm not seeing anything obvious.) Comment on attachment 128088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128088&action=review > Source/WebCore/storage/IDBTracing.h:41 > +#define IDB_TRACE(a) (void)0 ((void)0) seems safer and what I see in Assertions.h. (In reply to comment #9) > > ((void)0) seems safer and what I see in Assertions.h. Agreed, thanks. Created attachment 128239 [details]
Patch for landing
Comment on attachment 128239 [details] Patch for landing Clearing flags on attachment: 128239 Committed r108520: <http://trac.webkit.org/changeset/108520> All reviewed patches have been landed. Closing bug. |