Bug 78831 - [Chromium] IndexedDB: Integrate with about:tracing
Summary: [Chromium] IndexedDB: Integrate with about:tracing
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: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-16 11:02 PST by Joshua Bell
Modified: 2012-02-22 10:42 PST (History)
4 users (show)

See Also:


Attachments
Patch (34.73 KB, patch)
2012-02-16 11:03 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (41.64 KB, patch)
2012-02-17 14:45 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (45.19 KB, patch)
2012-02-21 17:12 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (45.19 KB, patch)
2012-02-22 10:17 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-02-16 11:02:44 PST
[Chromium] IndexedDB: Integrate with about:tracing
Comment 1 Joshua Bell 2012-02-16 11:03:07 PST
Created attachment 127411 [details]
Patch
Comment 2 Joshua Bell 2012-02-17 14:45:38 PST
Created attachment 127660 [details]
Patch
Comment 3 David Grogan 2012-02-17 14:55:19 PST
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.
Comment 4 Joshua Bell 2012-02-17 15:01:23 PST
(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.
Comment 5 Nat Duca 2012-02-21 10:22:43 PST
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 6 Tony Chang 2012-02-21 12:09:08 PST
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.
Comment 7 Joshua Bell 2012-02-21 17:12:33 PST
Created attachment 128088 [details]
Patch
Comment 8 Joshua Bell 2012-02-21 17:31:16 PST
@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 9 Tony Chang 2012-02-22 10:00:27 PST
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.
Comment 10 Joshua Bell 2012-02-22 10:12:11 PST
(In reply to comment #9)
> 
> ((void)0) seems safer and what I see in Assertions.h.

Agreed, thanks.
Comment 11 Joshua Bell 2012-02-22 10:17:51 PST
Created attachment 128239 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-02-22 10:41:57 PST
Comment on attachment 128239 [details]
Patch for landing

Clearing flags on attachment: 128239

Committed r108520: <http://trac.webkit.org/changeset/108520>
Comment 13 WebKit Review Bot 2012-02-22 10:42:03 PST
All reviewed patches have been landed.  Closing bug.