[tracing] Turn StackFrame into struct.

base/trace_event/heap_profiler_allocation_context_tracker.cc

Index: base/trace_event/heap_profiler_allocation_context_tracker.cc
diff --git a/base/trace_event/heap_profiler_allocation_context_tracker.cc b/base/trace_event/heap_profiler_allocation_context_tracker.cc
index 1fc8bc008a522419f7b1eb454c9b45a6d8c739b0..f27ab20e836bb78953527b1e49dcf4740c30a55d 100644
--- a/base/trace_event/heap_profiler_allocation_context_tracker.cc
+++ b/base/trace_event/heap_profiler_allocation_context_tracker.cc
@@ -75,17 +75,18 @@ void AllocationContextTracker::SetCaptureEnabled(bool enabled) {
   subtle::Release_Store(&capture_enabled_, enabled);
 }
 
-void AllocationContextTracker::PushPseudoStackFrame(StackFrame frame) {
+void AllocationContextTracker::PushPseudoStackFrame(
+    const char* trace_event_name) {
   // Impose a limit on the height to verify that every push is popped, because
   // in practice the pseudo stack never grows higher than ~20 frames.
   if (pseudo_stack_.size() < kMaxStackDepth)
-    pseudo_stack_.push_back(frame);
+    pseudo_stack_.push_back(trace_event_name);
   else
     NOTREACHED();
 }
 
-// static
-void AllocationContextTracker::PopPseudoStackFrame(StackFrame frame) {
+void AllocationContextTracker::PopPseudoStackFrame(
+    const char* trace_event_name) {
   // Guard for stack underflow. If tracing was started with a TRACE_EVENT in
   // scope, the frame was never pushed, so it is possible that pop is called
   // on an empty stack.
@@ -95,7 +96,7 @@ void AllocationContextTracker::PopPseudoStackFrame(StackFrame frame) {
   // Assert that pushes and pops are nested correctly. This DCHECK can be
   // hit if some TRACE_EVENT macro is unbalanced (a TRACE_EVENT_END* call
   // without a corresponding TRACE_EVENT_BEGIN).
-  DCHECK_EQ(frame, pseudo_stack_.back())
+  DCHECK_EQ(trace_event_name, pseudo_stack_.back())
       << "Encountered an unmatched TRACE_EVENT_END";
 
   pseudo_stack_.pop_back();
@@ -121,25 +122,22 @@ AllocationContext AllocationContextTracker::GetContextSnapshot() {
 
   // Fill the backtrace.
   {
-    auto src = pseudo_stack_.begin();
-    auto dst = std::begin(ctx.backtrace.frames);
-    auto src_end = pseudo_stack_.end();
-    auto dst_end = std::end(ctx.backtrace.frames);
+    auto backtrace = std::begin(ctx.backtrace.frames);
+    auto backtrace_end = std::end(ctx.backtrace.frames);
 
-    // Add the thread name as the first enrty in the backtrace.
+    // Add the thread name as the first entry

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

nit: missing period at end of comment.

     if (thread_name_) {

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

nit: you can drop the braces at this point :)

[Dmitry Skiba, 2016/04/15 07:01:40]

I would prefer not to, unless some style guide says I must :) I really don't
understand why NOT having braces around one liners is allowed at all, given
Apple's 'goto fail' bug.

[Primiano Tucci (use gerrit), 2016/04/15 13:47:47]

There is no must, indeed. my rationale is: unnecessary braces increase number of
lines. Longer files are harder to scroll and read :)
up to you really

-      DCHECK(dst < dst_end);
-      *dst = thread_name_;
-      ++dst;
+      *backtrace++ = StackFrame::FromThreadName(thread_name_);
     }
 
-    // Copy as much of the bottom of the pseudo stack into the backtrace as
-    // possible.
-    for (; src != src_end && dst != dst_end; src++, dst++)
-      *dst = *src;
+    for (const auto& event_name: pseudo_stack_) {

[Primiano Tucci (use gerrit), 2016/04/14 10:15:33]

I think this is regressing w.r.t the previous logic and can have out-of-bound
errors.
If pseudo_stack.size() >= kMaxFrameCount and you pushed a thread, you will start
writing past the end of bakctrace.
I quite liked the way the previous for loop was written, and that was definitely
safer :)

[Dmitry Skiba, 2016/04/15 07:01:39]

Hmm, thread is pushed first (at the top of the function), when there surely is a
space. This loop checks that we have space for at least one element (backtrace
!= backtrace_end), and then pushes one more element. So I don't see how it can
cause out-of-bound errors.

[Primiano Tucci (use gerrit), 2016/04/15 13:47:47]

Oh you are right, I completelly missed the  if(backtrace == backtrace_end) part

+      if (backtrace == backtrace_end) {
+        break;
+      }
+      *backtrace++ = StackFrame::FromTraceEventName(event_name);
+    }
 
-    // If there is room for more, fill the remaining slots with empty frames.
-    std::fill(dst, dst_end, nullptr);
+    ctx.backtrace.frame_count = backtrace - std::begin(ctx.backtrace.frames);
   }
 
   // TODO(ssid): Fix crbug.com/594803 to add file name as 3rd dimension