Add a mode flag to the tracing framework.

base/debug/trace_event_impl.cc

Index: base/debug/trace_event_impl.cc
diff --git a/base/debug/trace_event_impl.cc b/base/debug/trace_event_impl.cc
index cbcf54b81c6aac08b08d5f71fb6796f6869b84de..481d5e14679b28ba79e0c7f197fd972289bbeb8a 100644
--- a/base/debug/trace_event_impl.cc
+++ b/base/debug/trace_event_impl.cc
@@ -14,6 +14,7 @@
 #include "base/memory/singleton.h"
 #include "base/process_util.h"
 #include "base/stl_util.h"
+#include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/strings/string_tokenizer.h"
@@ -72,6 +73,8 @@ int g_category_index = 3; // skip initial 3 categories
 LazyInstance<ThreadLocalPointer<const char> >::Leaky
     g_current_thread_name = LAZY_INSTANCE_INITIALIZER;
 
+const char kRecordUntilFull[] = "record-until-full";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -348,10 +351,31 @@ TraceLog* TraceLog::GetInstance() {
   return Singleton<TraceLog, StaticMemorySingletonTraits<TraceLog> >::get();
 }
 
+// static
+// Note, if you add more options here you also need to update:
+// content/browser/devtools/devtools_tracing_handler:TraceOptionsFromString
+int TraceLog::TraceOptionsFromString(const std::string& options) {
+  std::vector<std::string> split;
+  std::vector<std::string>::iterator iter;

[jar (doing other things), 2013/02/22 19:31:28]

nit: move into line 363, so it is next to first use.

[dsinclair, 2013/02/22 20:01:42]

Done.

+  int ret = 0;
+
+  base::SplitString(options, ',', &split);

[jar (doing other things), 2013/02/22 19:31:28]

nit: declare closer to first use.  Probably push this line up to between line
358 and 359.

[dsinclair, 2013/02/22 20:01:42]

Done.

+  for (iter = split.begin(); iter != split.end(); ++iter) {
+    if (*iter == kRecordUntilFull) {
+      ret |= RECORD_UNTIL_FULL;
+    }

[jar (doing other things), 2013/02/22 19:31:28]

nit: Should we have a DCHECK for other spellings here?

[dsinclair, 2013/02/22 20:01:42]

Added a else NOTREACHED();

+  }
+  if (ret == 0)
+    ret = RECORD_UNTIL_FULL;

[jar (doing other things), 2013/02/22 19:31:28]

nit: Add comment: // Default when no options are specified.

The code is pretty simple, but yet confusing, since with only one option, this
assignment always happens one way or the other (line 365):-/.  It may help
clarify when you add a DCHECK for other spellings, but really this is forward
looking code, anticipating that there will be other options scanned for.  

How about adding a comment between line 367 and 368:
// Anticipating a collection of option, we provide a baseline default.

The weird thing is that you can presumably have a bunch of OR'ed options.  The
fact that they are ORed suggests you can have one, and not another.  As a
result, it is surprising that you have a default to one, when there are no
options specified.  I would (for example) have expected that if some subset of
options were not selected, *then* you would OR in this default.

Can you help clarify what the intent is, so that folks will understand how to
modify this when other options are added?

[dsinclair, 2013/02/22 20:01:42]

Skipping the comment as there is already a patch waiting to build on this when
it gets committed.

I've updated the ret == 0 check to be a !(ret & RECORD_UNTIL_FULL) which will
get expended when we have other recording options that could get set.

+
+  return ret;
+}
+
 TraceLog::TraceLog()
     : enable_count_(0),
       dispatching_to_observer_list_(false),
-      watch_category_(NULL) {
+      watch_category_(NULL),
+      trace_options_(RECORD_UNTIL_FULL) {
   // Trace is enabled or disabled on one thread while other threads are
   // accessing the enabled flag. We don't care whether edge-case events are
   // traced or not, so we allow races on the enabled flag to keep the trace
@@ -478,10 +502,16 @@ void TraceLog::GetKnownCategories(std::vector<std::string>* categories) {
 }
 
 void TraceLog::SetEnabled(const std::vector<std::string>& included_categories,
-                          const std::vector<std::string>& excluded_categories) {
+                          const std::vector<std::string>& excluded_categories,
+                          int options) {
   AutoLock lock(lock_);
 
   if (enable_count_++ > 0) {
+    if (options != trace_options_) {
+      DLOG(ERROR) << "Attemting to re-enable tracing with a different "
+                  << "set of options.";
+    }
+
     // Tracing is already enabled, so just merge in enabled categories.
     // We only expand the set of enabled categories upon nested SetEnable().
     if (!included_categories_.empty() && !included_categories.empty()) {
@@ -497,6 +527,7 @@ void TraceLog::SetEnabled(const std::vector<std::string>& included_categories,
     }
     return;
   }
+  trace_options_ = options;
 
   if (dispatching_to_observer_list_) {
     DLOG(ERROR) <<
@@ -520,7 +551,7 @@ void TraceLog::SetEnabled(const std::vector<std::string>& included_categories,
     EnableMatchingCategories(excluded_categories_, 0, CATEGORY_ENABLED);
 }
 
-void TraceLog::SetEnabled(const std::string& categories) {
+void TraceLog::SetEnabled(const std::string& categories, int options) {
   std::vector<std::string> included, excluded;
   // Tokenize list of categories, delimited by ','.
   StringTokenizer tokens(categories, ",");
@@ -538,7 +569,7 @@ void TraceLog::SetEnabled(const std::string& categories) {
     else
       excluded.push_back(category);
   }
-  SetEnabled(included, excluded);
+  SetEnabled(included, excluded, options);
 }
 
 void TraceLog::GetEnabledTraceCategories(
@@ -577,9 +608,9 @@ void TraceLog::SetDisabled() {
   AddThreadNameMetadataEvents();
 }
 
-void TraceLog::SetEnabled(bool enabled) {
+void TraceLog::SetEnabled(bool enabled, int options) {
   if (enabled)
-    SetEnabled(std::vector<std::string>(), std::vector<std::string>());
+    SetEnabled(std::vector<std::string>(), std::vector<std::string>(), options);
   else
     SetDisabled();
 }