Simplify file tracing end events.

content/browser/tracing/file_tracing_provider_impl.cc

Index: content/browser/tracing/file_tracing_provider_impl.cc
diff --git a/content/browser/tracing/file_tracing_provider_impl.cc b/content/browser/tracing/file_tracing_provider_impl.cc
index 21fcf07b5d908df74fb5ebc301caf035904cd97f..c9d6dee08fea77a705dcfbb9da83a4854a567bcb 100644
--- a/content/browser/tracing/file_tracing_provider_impl.cc
+++ b/content/browser/tracing/file_tracing_provider_impl.cc
@@ -30,26 +30,36 @@ void FileTracingProviderImpl::FileTracingDisable(void* id) {
       kFileTracingEventCategoryGroup, FILE_TRACING_PREFIX, id);
 }
 
+#define TRACE_VARIED_PARAMS(BEGIN_OR_END) \

[dsinclair, 2015/06/08 13:05:38]

I'd prefer not to do this. It makes the code a lot harder to follow. Why not
just always log size, even if it's zero? Or, if it's zero, log as -1 or
something so we know it was unknown.

[Dan Beam, 2015/06/08 18:42:54]

making the code noisier keeps the UI cleaner, but I'm fine to just change this
all to

  TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)

if you want there to be a lot of cruft in these events

+    const bool has_path = !path.empty(); \
+    const bool has_size = size > 0; \
+    if (has_path && has_size) { \
+      TRACE_EVENT_NESTABLE_ASYNC_##BEGIN_OR_END##2( \
+          kFileTracingEventCategoryGroup, name, id, \
+          "path", path.AsUTF8Unsafe(), "size", size); \
+    } else if (has_path) { \
+      TRACE_EVENT_NESTABLE_ASYNC_##BEGIN_OR_END##1( \
+          kFileTracingEventCategoryGroup, name, id, \
+          "path", path.AsUTF8Unsafe()); \
+    } else if (has_size) { \
+      TRACE_EVENT_NESTABLE_ASYNC_##BEGIN_OR_END##1( \
+          kFileTracingEventCategoryGroup, name, id, \
+          "size", size); \
+    } else { \
+      TRACE_EVENT_NESTABLE_ASYNC_##BEGIN_OR_END##0( \
+          kFileTracingEventCategoryGroup, name, id); \
+    }
+
 void FileTracingProviderImpl::FileTracingEventBegin(
     const char* name, void* id, const base::FilePath& path, int64 size) {
-  if (size) {
-    TRACE_EVENT_NESTABLE_ASYNC_BEGIN2(kFileTracingEventCategoryGroup, name, id,
-        "path", path.AsUTF8Unsafe(), "size", size);
-  } else {
-    TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(kFileTracingEventCategoryGroup, name, id,
-        "path", path.AsUTF8Unsafe());
-  }
+  TRACE_VARIED_PARAMS(BEGIN);

[oystein (OOO til 10th of July), 2015/06/05 22:06:25]

The macro is cool, but I'm not really convinced it's worth it :). Is it because
you suspect this'll grow to include even more parameters?

Also, do you even need to pass the same arguments to _END? I assume they don't
change. What happens if you just use TRACE_EVENT_NESTABLE_ASYNC_END0? 

[Dan Beam, 2015/06/06 01:57:25]

I guess I could, but what happens if the params change between beginning and
end?  Should I just remove the params for End()?

I agree that the code doesn't seem to care, it's just looking for ID.

[oystein (OOO til 10th of July), 2015/06/08 18:53:47]

Hmm... In what situations would the params change? From what I can tell (I may
be missing something) these things are only set on initialization. If so I would
just include the params in the _BEGIN(X) and use _END0 without them (removing
the need for the macro too).

[Dan Beam, 2015/06/08 19:22:28]

The current implementation wouldn't, but this level of the code doesn't
know/can't guarantee that.  It'd also be nice to eventually log the actual
amount of bytes read instead of the maximum allowed (e.g you ask for 4096 bytes
but there's only 1 left).

 }
 
 void FileTracingProviderImpl::FileTracingEventEnd(
     const char* name, void* id, const base::FilePath& path, int64 size) {
-  if (size) {
-    TRACE_EVENT_NESTABLE_ASYNC_END2(kFileTracingEventCategoryGroup, name, id,
-        "path", path.AsUTF8Unsafe(), "size", size);
-  } else {
-    TRACE_EVENT_NESTABLE_ASYNC_END1(kFileTracingEventCategoryGroup, name, id,
-        "path", path.AsUTF8Unsafe());
-  }
+  TRACE_VARIED_PARAMS(END);
 }
 
+#undef TRACE_VARIED_PARAMS
+
 }  // namespace content