Avoid emptry tracing callbacks when possible.

base/trace_event/trace_event_impl.cc

Index: base/trace_event/trace_event_impl.cc
diff --git a/base/trace_event/trace_event_impl.cc b/base/trace_event/trace_event_impl.cc
index 5ae7fb2572f7b16653f0360a322f62298541829e..369d874266bae8ba2dcc7818cbee9c89ee944b13 100644
--- a/base/trace_event/trace_event_impl.cc
+++ b/base/trace_event/trace_event_impl.cc
@@ -1763,24 +1763,20 @@ void TraceLog::ConvertTraceEventsToTraceFormat(
 
   // The callback need to be called at least once even if there is no events
   // to let the caller know the completion of flush.
-  bool has_more_events = true;
-  do {
-    scoped_refptr<RefCountedString> json_events_str_ptr =
-        new RefCountedString();
-
-    while (json_events_str_ptr->size() < kTraceEventBufferSizeInBytes) {
-      const TraceBufferChunk* chunk = logged_events->NextChunk();
-      has_more_events = chunk != NULL;
-      if (!chunk)
-        break;
-      for (size_t j = 0; j < chunk->size(); ++j) {
-        if (json_events_str_ptr->size())
-          json_events_str_ptr->data().append(",\n");
-        chunk->GetEventAt(j)->AppendAsJSON(&(json_events_str_ptr->data()));
+  scoped_refptr<RefCountedString> json_events_str_ptr = new RefCountedString();
+  while (const TraceBufferChunk* chunk = logged_events->NextChunk()) {
+    for (size_t j = 0; j < chunk->size(); ++j) {
+      size_t size = json_events_str_ptr->size();
+      if (size > kTraceEventBufferSizeInBytes) {
+        flush_output_callback.Run(json_events_str_ptr, true);
+        json_events_str_ptr = new RefCountedString();
+      } else if (size) {
+        json_events_str_ptr->data().append(",\n");
       }
+      chunk->GetEventAt(j)->AppendAsJSON(&(json_events_str_ptr->data()));
     }
-    flush_output_callback.Run(json_events_str_ptr, has_more_events);
-  } while (has_more_events);
+  }
+  flush_output_callback.Run(json_events_str_ptr, false);

[dsinclair, 2015/06/11 13:54:39]

Doesn't this new code do the same as the old code?

If logged_events->NextChunk returns nullptr then we will flush_output_callback
with an empty json_events_str_ptr and false which was the same problem as
previously, no?

[hubbe, 2015/06/11 17:30:01]

The new code will only create an empty callback if there is no data at all,
which is consistent with the comment. The existing code can produce callbacks
like:

   DATA, true
   EMPTY, true
   DATA, true
   DATA, false

The comment doesn't explicitly say that won't happen, but it was not what I
expected, which is why the test broke.

[dsinclair, 2015/06/11 19:19:29]

Right, but I don't see how the old code could produce the EMPTY, true case with
more data afterwards? The inner while should keep us looping through the chunks
until either our buffer is full or we've processed them all. How do we end up
breaking out of the while when there are more chunks (in order for
has_more_events to be true) and we also haven't got size() > 
kTraceEventBufferSizeInBytes?

[hubbe, 2015/06/11 21:39:58]

Apparently, it's been too long since I thought about this, because my
description of the problem was not accurate. The sequence you get is:

  DATA, true
  ....
  DATA, true
  EMPTY, false

Because at the time we flush one buffer, we don't know that
it's the last one, so has_more_events is still true.

[dsinclair, 2015/06/12 13:48:35]

Ah, that makes a lot more sense, I can see how that would happen.

Thanks a lot for explaining this to me.

 }
 
 void TraceLog::FinishFlush(int generation) {