[Startup Tracing] The TraceConfig class [STALE]

base/trace_event/trace_event_impl.h

Index: base/trace_event/trace_event_impl.h
diff --git a/base/trace_event/trace_event_impl.h b/base/trace_event/trace_event_impl.h
index 50d33ca7da9129a62f1a4518381bb6bce4898033..6ee94f6167a5e03af112bcafb8031f3bcb478eea 100644
--- a/base/trace_event/trace_event_impl.h
+++ b/base/trace_event/trace_event_impl.h
@@ -24,6 +24,7 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_local.h"
+#include "base/values.h"
 
 // Older style trace macros with explicit id and extra data
 // Only these macros result in publishing data to ETW as currently implemented.
@@ -53,6 +54,8 @@ class MessageLoop;
 
 namespace trace_event {
 
+class TraceConfig;
+
 // For any argument of type TRACE_VALUE_TYPE_CONVERTABLE the provided
 // class must implement this interface.
 class BASE_EXPORT ConvertableToTraceFormat
@@ -342,6 +345,7 @@ class BASE_EXPORT CategoryFilter {
 
  private:
   FRIEND_TEST_ALL_PREFIXES(TraceEventTestFixture, CategoryFilter);
+  friend class TraceConfig;

[dsinclair, 2015/05/08 17:44:50]

Can we just merge this straight into TraceConfig without the indirection step?

[Zhen Wang, 2015/05/13 00:01:11]

Many places are using CategoryFilter. Do you mean just update all the
implementations with CategoryFilter by TraceConfig directly in this CL? I planed
to do them in 2 steps. Doing them in one step leads to a much longer CL.

 
   // Returns true if category is enable according to this filter.
   bool IsCategoryEnabled(const char* category_name) const;
@@ -356,10 +360,7 @@ class BASE_EXPORT CategoryFilter {
   void WriteString(const StringList& delays, std::string* out) const;
   bool HasIncludedPatterns() const;
 
-  StringList included_;
-  StringList disabled_;
-  StringList excluded_;
-  StringList delays_;
+  scoped_ptr<TraceConfig> config_;

[nednguyen, 2015/05/11 17:59:28]

Why not just make this a value instead of scoped_ptr?

[Zhen Wang, 2015/05/13 00:01:11]

There will be extra copies in the constructors, right? Or that does not matter
much?

[nednguyen, 2015/05/13 18:16:31]

Chatted offline. It doesn't matter much & the compiler probably will do the
right thing for you.

[Zhen Wang, 2015/05/13 20:59:16]

Done.

 };
 
 class TraceSamplingThread;
@@ -422,6 +423,95 @@ struct BASE_EXPORT TraceOptions {
   bool enable_systrace;
 };
 
+class BASE_EXPORT TraceConfig {
+ public:
+  typedef std::vector<std::string> StringList;

[dsinclair, 2015/05/08 17:44:49]

Is this worth typedef'ing?

[Zhen Wang, 2015/05/13 00:01:11]

I was following the style used in CategoryFilter, so that it is consistent.

+
+  // The default trace config, used when none is provided.
+  // Allows all categories through, except if they end in the suffix 'Debug' or

[dsinclair, 2015/05/08 17:44:49]

Is this true? We don't want to allow disable-by-default- through by default ....

[Zhen Wang, 2015/05/13 00:01:11]

This is converted from kDefaultCategoryFilterString. See above in
CategoryFilter. Now kDefaultTraceConfigString is removed as we will use
InitializeDefault().

+  // 'Test'.
+  static const char kDefaultTraceConfigString[];

[dsinclair, 2015/05/08 17:44:49]

Does this need to be in the public header? Can it be moved to an anonymous
namespace inside the cc file?

[Zhen Wang, 2015/05/13 00:01:11]

This is removed now as we will use InitializeDefault().

+
+  TraceConfig();
+
+  // Create TraceConfig object from CategoryFilter and TraceOptions.
+  TraceConfig(const CategoryFilter& cf, const TraceOptions& options);
+
+  // Create TraceConfig object from category filter and trace options strings.
+  TraceConfig(const std::string& category_filter_string,
+              const std::string& trace_options_string);
+
+  // |config_string| is a JSON formated string, containing both category filters

[dsinclair, 2015/05/08 17:44:49]

Can you add a similar comment to the above here:

Create TraceConfig object from JSON encoded trace config string.

[Zhen Wang, 2015/05/13 00:01:11]

Done.

+  // and trace options.
+  TraceConfig(const std::string& config_string);

[dsinclair, 2015/05/08 17:44:50]

nit: explicit

[Zhen Wang, 2015/05/13 00:01:11]

Done.

+
+  TraceConfig(const TraceConfig& tc);

[dsinclair, 2015/05/08 17:44:50]

nit: explicit

[Zhen Wang, 2015/05/13 00:01:10]

Done.

+
+  ~TraceConfig();
+
+  TraceConfig& operator=(const TraceConfig& rhs);

[dsinclair, 2015/05/08 17:44:49]

Do we use copy and assign for this?

[Zhen Wang, 2015/05/13 00:01:11]

Not now. But it may be used later when updating the implementations with
CategoryFilter to TraceConfig.

[dsinclair, 2015/05/21 13:42:07]

In that case, let's add the DISALLOW for now and we can remove it later if we
determine we need it.

[Zhen Wang, 2015/05/22 23:38:04]

COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove
them when cleaning all CategoryFilter implementations in the next CL.

[dsinclair, 2015/05/25 13:36:02]

I'm confused, above you said they aren't used? So, are they currently used or
not?

+
+  // Return a list of the synthetic delays specified in this category filter.
+  const StringList& GetSyntheticDelayValues() const;
+
+  // Convert TraceConfig to CategoryFilter.
+  void ToCategoryFilter(CategoryFilter& cf) const;

[dsinclair, 2015/05/08 17:44:49]

Are these ToXXX methods only for tests? I don't see them being called anywhere.
I'd prefer if we could get rid of them as it's making the public interfact of
this class a lot bigger.

Or, get rid of the bulk of them and just have toString

[Zhen Wang, 2015/05/13 00:01:11]

Removed now.

+
+  // Convert TraceConfig to TraceOptions.
+  void ToTraceOptions(TraceOptions& options) const;
+
+  // Convert TraceConfig to the dict representation of the TraceConfig.
+  void ToDict(base::DictionaryValue& dict) const;

[nednguyen, 2015/05/11 17:59:28]

Do we actually need this API?

[Zhen Wang, 2015/05/13 00:01:11]

I think this can be used by slow report.

[dsinclair, 2015/05/21 13:42:07]

Can we determine if it is or isn't? I'd prefer to leave it out of it isn't
needed.

[Zhen Wang, 2015/05/22 23:38:04]

Fadi has confirmed that this is not needed in the 1st stage of Deep Report.

Since it is used by ToString(). I moved it to private.

+
+  // Writes the string representation of the TraceConfig. The string is JSON
+  // formatted.
+  std::string ToString() const;
+
+  // Write the string representation of the CategoryFilter part.
+  std::string ToCategoryFilterString() const;
+
+  // Write the string representation of the TraceOptions part.
+  std::string ToTraceOptionsString() const;
+
+  // Returns true if at least one category in the list is enabled by this

[dsinclair, 2015/05/08 17:44:49]

How does this interact with disabled categories? If I do "foo,-bar" and
TRACE("foo,bar") will it be considered enabled?

[Zhen Wang, 2015/05/13 00:01:11]

Yes, it will be considered enabled. This comes from the original implementation
in CategoryFilter. See 
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

And I add one more check for this in TraceConfigTest.TraceConfigFromValidString:
EXPECT_TRUE(tc.IsCategoryGroupEnabled("included,excluded"));

+  // trace config.
+  bool IsCategoryGroupEnabled(const char* category_group) const;
+
+  // Merges config with the current TraceConfig
+  void Merge(const TraceConfig& config);

[dsinclair, 2015/05/08 17:44:49]

Will this go away when CategoryFilter is removed? Or will this have to live on?

[Zhen Wang, 2015/05/13 00:01:11]

This will live on. It is used by TraceLog. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

+
+  void Clear();

[dsinclair, 2015/05/08 17:44:50]

Is this needed? Or just for tests? If for tests, delete it and just create a new
object.

[Zhen Wang, 2015/05/13 00:01:11]

Clear() will be used by TraceLog. See
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(TraceConfigTest, TraceConfigFromAndToString);
+  FRIEND_TEST_ALL_PREFIXES(TraceConfigTest, TraceConfig);
+  friend class CategoryFilter;
+
+  void Initialize(const std::string& config_string);
+  void SetCategoriesFromList(StringList& categories,
+                             const base::ListValue& list);
+  void AddCategoryToDict(base::DictionaryValue& dict,
+                         const char* param,
+                         const StringList& categories) const;
+
+  // Returns true if category is enable according to this trace config.
+  bool IsCategoryEnabled(const char* category_name) const;
+
+  static bool IsEmptyOrContainsLeadingOrTrailingWhitespace(
+      const std::string& str);
+
+  bool HasIncludedPatterns() const;
+
+  TraceRecordMode record_mode_;
+  bool enable_sampling_;
+  bool enable_systrace_;

[dsinclair, 2015/05/08 17:44:50]

bool enable_sampling_ : 1;
bool enable_systrace_ : 1;

[Zhen Wang, 2015/05/13 00:01:11]

Those two members will be used in TraceConfig::Initialize:

dict->GetBoolean(kEnableSamplingParam, &enable_sampling_)
dict->GetBoolean(kEnableSystraceParam, &enable_systrace_)

Specify them as bit field will make them not addressable.

[dsinclair, 2015/05/21 13:42:07]

Sure, but we can also easily change the initialize call to write into a temp
boolean and then assign the value.

We're using more space then needed here, although it isn't a lot, but why not
save the extra byte?

[Zhen Wang, 2015/05/22 23:38:04]

Done.

+
+  StringList included_categories_;
+  StringList disabled_categories_;
+  StringList excluded_categories_;
+  StringList synthetic_delays_;
+};

[dsinclair, 2015/05/08 17:44:50]

Should this have a DISALLOW_COPY_AND_ASSIGN?

[Zhen Wang, 2015/05/13 00:01:10]

Existing CategoryFilter has copy constructor and operator= function defined. So
TraceConfig will also need them.

[dsinclair, 2015/05/21 13:42:07]

If it isn't needed let, let's put in the DISALLOW. We can remove it in the
future if needed, but it's possible CategoryFilter just never had one.

[Zhen Wang, 2015/05/22 23:38:04]

COPY and ASSIGN are needed to implement CategoryFilter for now. I can remove
them when cleaning all CategoryFilter implementations in the next CL.

+
 struct BASE_EXPORT TraceLogStatus {
   TraceLogStatus();
   ~TraceLogStatus();