Enable trace with multiple categories with any one category ON

base/trace_event/trace_event_impl.cc

 // Copyright (c) 2012 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.
 
 #include "base/trace_event/trace_event_impl.h"
 
 #include <algorithm>
 
 #include "base/base_switches.h"
 #include "base/bind.h"
@@ skipping 2472 matching lines @@
     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();
+  bool had_category_group_enabled = true;

[dsinclair, 2015/03/03 14:54:05]

This flag is backwards to what we're checking and makes the logic hard to
follow. Can you rename it to had_category_group_disabled and update the logic
below? This check is running through the explicitly disabled categories.

Actually, can we call it 'all_category_groups_disabled' I think that would be
the clearest. Then as soon as something doesn't match in the loops below, you
can set it to true.

You then don't need the final if (!had_category_group_enabled) you can put it
right into the return as:

return !all_category_groups_disabled || (included_.empty() &&
had_enabled_by_default)

   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 (MatchPattern(category_group_token.c_str(), ci->c_str())) {
+        // Current token of category_group_name is present in excluded_ list.

[dsinclair, 2015/03/03 14:54:05]

Nit: remove space between excluded_ and list.

+        // Flag the exclusion and proceed further to check if any of the
+        // remaining categories of category_group_name is not present in the
+        // excluded_ list.
+        had_category_group_enabled = false;
+        break;
+      } else {

[dsinclair, 2015/03/03 14:54:05]

Since the if () has a break, you don't need else here. We can only get here if
the if didn't execute.

+        // One of the category of category_group_name is not present in
+        // excluded_ list. So, it has to be included_ list. Enable the
+        // category_group_name for recording.
+        had_category_group_enabled = true;
+      }
     }
+    // One of the categories present in category_group_name is not present in
+    // excluded_ list. Implies this category_group_name group can be enabled
+    // for recording, since one of its groups is enabled for recording.
+    if (had_category_group_enabled)
+      break;
   }
+  // None of the categories part of category_group_name are enabled.
+  // Don't allow recording of this category_group_name.
+  if (!had_category_group_enabled)
+    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
@@ skipping 77 matching lines @@
 }
 
 ScopedTraceBinaryEfficient::~ScopedTraceBinaryEfficient() {
   if (*category_group_enabled_) {
     TRACE_EVENT_API_UPDATE_TRACE_EVENT_DURATION(category_group_enabled_,
                                                 name_, event_handle_);
   }
 }
 
 }  // namespace trace_event_internal