StackSamplingProfiler clean up

base/profiler/native_stack_sampler.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_PROFILER_NATIVE_STACK_SAMPLER_H_
+#define BASE_PROFILER_NATIVE_STACK_SAMPLER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/profiler/stack_sampling_profiler.h"
+#include "base/threading/platform_thread.h"
+
+namespace base {
+
+// NativeStackSampler is an implementation detail of StackSamplingProfiler. It
+// abstracts the native implementation required to record a stack sample for a
+// given thread.
+class NativeStackSampler {
+ public:
+  virtual ~NativeStackSampler();
+
+  // Creates a stack sampler that records samples for |thread_handle|. Returns
+  // null if this platform does not support stack sampling.
+  static scoped_ptr<NativeStackSampler> Create(PlatformThreadId thread_id);
+
+  // The following functions are all called on the SamplingThread (not the

[Peter Kasting, 2015/03/31 01:51:25]

You went back to using "SamplingThread" here instead of the previous "thread
used for sampling", which I thought was clearer, especially as "SamplingThread"
isn't otherwise used in this file.

Was it not correct to say that these functions are called by the
StackSamplingProfiler?  I thought that information was useful.

[Mike Wittman, 2015/03/31 19:29:41]

Sorry, changed back to "thread used for sampling".
That seems misleading to me, since they're not called directly by the profiler,
only by the SamplingThread during its independent execution.

[Peter Kasting, 2015/04/01 06:16:40]

Oh.  You could say they're called by the SamplingThread then.  Whatever the
correct concrete classname is, having the specificity here seems helpful for
readers trying to piece together who's calling what why.

[Mike Wittman, 2015/04/01 19:08:23]

Done.

+  // thread being sampled).
+
+  // Notifies the sampler that we're starting to record a new profile.

[Peter Kasting, 2015/03/31 01:51:25]

I wonder... should this API simply pass in a pointer to the profile's modules,
instead of the profile itself?  That's the only thing the native sampler should
care about, and giving the sampler access to a non-const profile means in theory
it could muck with the sample list directly.  Not only do we not want that, it
obscures the intent of this API, which is basically "we'll provide you with a
module list to update, you provide us with Samples on request".

At the very least this function deserves commentary about what the sampler is
intended to do with the provided profile, but if the API can be made more
focused, so much the better.  This also theoretically makes testing nicer since
test code could just pass a module vector instead of needing to construct a full
profile object.  (In reality, the practical benefit of that is minimal.)

[Mike Wittman, 2015/03/31 19:29:41]

I was on the fence about this when creating this interface... Passing modules is
narrower, but passing the profile makes the interface a little less surprising
at first glance. I don't have a strong preference, so I'll change to modules.

+  virtual void ProfileRecordingStarting(
+      StackSamplingProfiler::CallStackProfile* profile) = 0;
+
+  // Records a stack sample.

[Peter Kasting, 2015/03/31 01:51:25]

Nit: "... to |sample|."

[Mike Wittman, 2015/03/31 19:29:41]

Done.

+  virtual void RecordStackSample(StackSamplingProfiler::Sample* sample) = 0;
+
+  // Notifies the sampler that we've stopped recording the current
+  // profile.
+  virtual void ProfileRecordingStopped() = 0;
+
+ protected:
+  NativeStackSampler();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NativeStackSampler);
+};
+
+}  // namespace base
+
+#endif  // BASE_PROFILER_NATIVE_STACK_SAMPLER_H_
+