Tracing: make filtering logic more intuitive for multiple categories

This changes the way category filter processes multi-categories groups.
Previously, adding a disabled-by-default- category to a regular category
would prevent the event from matching filter if only regular category
is enabled. This is somewhat counter-intuitive, the expected behavior
is that each category is processed by the filter independently. The new
logic is:

1. if at least one category in event category group matches at
least one category enabled in the filter, the event passes;

2. if at least one category in event category group matches at least
one category explicitly excluded in the filter, the event is
filtered out;

3. the event is passed by default, if included categories list is empty
and event has at least one category that is not disabled-by-default-.

Note no existing tests were modified (though I'm not sure if (2) should
be the way it is).

BUG=

Review URL: https://codereview.chromium.org/788243003

Cr-Commit-Position: refs/heads/master@{#308136}

3 files changed, 63 insertions, 34 deletions

diff --git a/base/debug/trace_event_impl.cc b/base/debug/trace_event_impl.cc
index 1a64eb0..4591286 100644
--- a/base/debug/trace_event_impl.cc
+++ b/base/debug/trace_event_impl.cc
@@ -2358,24 +2358,6 @@ bool CategoryFilter::IsEmptyOrContainsLeadingOrTrailingWhitespace(
           str.at(str.length() - 1) == ' ';
 }
 
-bool CategoryFilter::DoesCategoryGroupContainCategory(
-    const char* category_group,
-    const char* category) const {
-  DCHECK(category);
-  CStringTokenizer category_group_tokens(category_group,
-                          category_group + strlen(category_group), ",");
-  while (category_group_tokens.GetNext()) {
-    std::string category_group_token = category_group_tokens.token();
-    // Don't allow empty tokens, nor tokens with leading or trailing space.
-    DCHECK(!CategoryFilter::IsEmptyOrContainsLeadingOrTrailingWhitespace(
-        category_group_token))
-        << "Disallowed category string";
-    if (MatchPattern(category_group_token.c_str(), category))
-      return true;
-  }
-  return false;
-}
-
 CategoryFilter::CategoryFilter(const std::string& filter_string) {
   if (!filter_string.empty())
     Initialize(filter_string);
@@ -2483,30 +2465,61 @@ bool CategoryFilter::IsCategoryGroupEnabled(
     const char* category_group_name) const {
   // TraceLog should call this method only as  part of enabling/disabling
   // categories.
+
+  bool had_enabled_by_default = false;
+  DCHECK(category_group_name);
+  CStringTokenizer category_group_tokens(
+      category_group_name, category_group_name + strlen(category_group_name),
+      ",");
+  while (category_group_tokens.GetNext()) {
+    std::string category_group_token = category_group_tokens.token();
+    // Don't allow empty tokens, nor tokens with leading or trailing space.
+    DCHECK(!CategoryFilter::IsEmptyOrContainsLeadingOrTrailingWhitespace(
+               category_group_token))
+        << "Disallowed category string";
+    if (IsCategoryEnabled(category_group_token.c_str())) {
+      return true;
+    }
+    if (!MatchPattern(category_group_token.c_str(),
+                      TRACE_DISABLED_BY_DEFAULT("*")))
+      had_enabled_by_default = true;
+  }
+  // Do a second pass to check for explicitly disabled categories
+  // (those explicitly enabled have priority due to first pass).
+  category_group_tokens.Reset();
+  while (category_group_tokens.GetNext()) {
+    std::string category_group_token = category_group_tokens.token();
+    for (StringList::const_iterator ci = excluded_.begin();
+         ci != excluded_.end(); ++ci) {
+      if (MatchPattern(category_group_token.c_str(), ci->c_str()))
+        return false;
+    }
+  }
+  // If the category group is not excluded, and there are no included patterns
+  // we consider this category group enabled, as long as it had categories
+  // other than disabled-by-default.
+  return included_.empty() && had_enabled_by_default;
+}
+
+bool CategoryFilter::IsCategoryEnabled(const char* category_name) const {
   StringList::const_iterator ci;
 
   // Check the disabled- filters and the disabled-* wildcard first so that a
   // "*" filter does not include the disabled.
   for (ci = disabled_.begin(); ci != disabled_.end(); ++ci) {
-    if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
+    if (MatchPattern(category_name, ci->c_str()))
       return true;
   }
-  if (DoesCategoryGroupContainCategory(category_group_name,
-                                       TRACE_DISABLED_BY_DEFAULT("*")))
+
+  if (MatchPattern(category_name, TRACE_DISABLED_BY_DEFAULT("*")))
     return false;
 
   for (ci = included_.begin(); ci != included_.end(); ++ci) {
-    if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
+    if (MatchPattern(category_name, ci->c_str()))
       return true;
   }
 
-  for (ci = excluded_.begin(); ci != excluded_.end(); ++ci) {
-    if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
-      return false;
-  }
-  // If the category group is not excluded, and there are no included patterns
-  // we consider this pattern enabled.
-  return included_.empty();
+  return false;
 }
 
 bool CategoryFilter::HasIncludedPatterns() const {
diff --git a/base/debug/trace_event_impl.h b/base/debug/trace_event_impl.h
index ce2e017..c80826c 100644
--- a/base/debug/trace_event_impl.h
+++ b/base/debug/trace_event_impl.h
@@ -320,8 +320,8 @@ class BASE_EXPORT CategoryFilter {
   // categories are distinguished from included categories by the prefix '-'.
   std::string ToString() const;
 
-  // Determines whether category group would be enabled or
-  // disabled by this category filter.
+  // Returns true if at least one category in the list is enabled by this
+  // category filter.
   bool IsCategoryGroupEnabled(const char* category_group) const;
 
   // Return a list of the synthetic delays specified in this category filter.
@@ -341,6 +341,9 @@ class BASE_EXPORT CategoryFilter {
  private:
   FRIEND_TEST_ALL_PREFIXES(TraceEventTestFixture, CategoryFilter);
 
+  // Returns true if category is enable according to this filter.
+  bool IsCategoryEnabled(const char* category_name) const;
+
   static bool IsEmptyOrContainsLeadingOrTrailingWhitespace(
       const std::string& str);
 
@@ -351,9 +354,6 @@ class BASE_EXPORT CategoryFilter {
   void WriteString(const StringList& delays, std::string* out) const;
   bool HasIncludedPatterns() const;
 
-  bool DoesCategoryGroupContainCategory(const char* category_group,
-                                        const char* category) const;
-
   StringList included_;
   StringList disabled_;
   StringList excluded_;
diff --git a/base/debug/trace_event_unittest.cc b/base/debug/trace_event_unittest.cc
index 0904954..85cf4eb 100644
--- a/base/debug/trace_event_unittest.cc
+++ b/base/debug/trace_event_unittest.cc
@@ -1615,6 +1615,22 @@ TEST_F(TraceEventTestFixture, DisabledCategories) {
     EXPECT_FIND_("disabled-by-default-cc");
     EXPECT_FIND_("other_included");
   }
+
+  Clear();
+
+  BeginSpecificTrace("other_included");
+  TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("cc") ",other_included",
+                       "first", TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0("other_included," TRACE_DISABLED_BY_DEFAULT("cc"),
+                       "second", TRACE_EVENT_SCOPE_THREAD);
+  EndTraceAndFlush();
+
+  {
+    const DictionaryValue* item = NULL;
+    ListValue& trace_parsed = trace_parsed_;
+    EXPECT_FIND_("disabled-by-default-cc,other_included");
+    EXPECT_FIND_("other_included,disabled-by-default-cc");
+  }
 }
 
 TEST_F(TraceEventTestFixture, NormallyNoDeepCopy) {