Bug 78831 – [Chromium] IndexedDB: Integrate with about:tracing

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

[Chromium] IndexedDB: Integrate with about:tracing

[Joshua Bell]

Created attachment 127411 [details]
Patch

[Joshua Bell]

Created attachment 127660 [details]
Patch

[David Grogan]

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.

[Joshua Bell]

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

[Nat Duca]

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

[Tony Chang]

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.

[Joshua Bell]

Created attachment 128088 [details]
Patch

[Joshua Bell]

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

[Tony Chang]

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.

[Joshua Bell]

(In reply to comment #9)
> 
> ((void)0) seems safer and what I see in Assertions.h.

Agreed, thanks.

[Joshua Bell]

Created attachment 128239 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128239 [details]
Patch for landing

Clearing flags on attachment: 128239

Committed r108520: <http://trac.webkit.org/changeset/108520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.