Add a mode flag to the tracing framework.

Add a mode flag to the tracing framework.

The mode is provided when SetEnabled is called. Currently there
is only one mode which is the trace until buffer is full mode.

We will be adding a continuous tracing mode which this will support.

BUG=156025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184258

[dsinclair, 2013-02-19 23:02:32]

Nat,

Here is my first pass at adding the mode to SetEnabled(). Can you please take a
look to make sure it's inline with what you were thinking?

Thanks,
dan

[pfeldman, 2013-02-20 09:33:38]

https://codereview.chromium.org/12302036/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/12302036/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:71:
params->GetInteger(kTraceModeParam, &mode_);
You should check for it returning value here.
We also use symbolic names (string enums) for flags like this. You would end up
with switch here, but the protocol will get more stable.

[dsinclair, 2013-02-20 20:36:12]

https://codereview.chromium.org/12302036/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/12302036/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:71:
params->GetInteger(kTraceModeParam, &mode_);
Done.

I put the conversion in the trace_event_impl.cc code as I've used the same
string names coming from the trace_ui code.

[nduca, 2013-02-21 05:40:16]

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:173: enum TraceMode {
I think TraceMode -> TraceOptions?

This is an inner enum, so can it just be Options? Its already
TraceLog::-prefixed.

Think about things like "enable sampling" being added in there. E.g continuous |
sampling

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:174: UNTIL_FULL = 0
naming is kind of awks. RECORD_UNTIL_FULL, RECORD_CONTINUOUSLY

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:387: if (args->GetSize() == 3) {
would be less fragile if you wrote >= 3

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:391: mode =
base::debug::TraceLog::TraceModeFromString(mode_);
might check with pavel, but i think you need to do the string-to-enum conversion
in devtools. Once released, the names we use on the devtool protocol handler we
have to commit to being backcompat. That's not the case for tracing_ui, and not
obvious when you're just a dude hacking in base/. That is to say, lets do this
per-instance conversion to enum here and in devtools, rather than sharing code.

[pfeldman, 2013-02-21 05:49:23]

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:391: mode =
base::debug::TraceLog::TraceModeFromString(mode_);
Yep, you should have local conversion barrier in the devtools code.

[dsinclair, 2013-02-21 16:26:04]

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:173: enum TraceMode {
On 2013/02/21 05:40:16, nduca wrote:
> I think TraceMode -> TraceOptions?
> 
> This is an inner enum, so can it just be Options? Its already
> TraceLog::-prefixed.
> 
> Think about things like "enable sampling" being added in there. E.g continuous
|
> sampling

Done.

Turned this into a bit field as it looks like we'll want to enable multiple
items at the same time:

RECORD_CONTINUOUSLY | SAMPLING

we'll need to figure out what to do if the user says RECORD_CONTINUOUSLY |
RECORD_UNTIL_FULL but I think we can just go with continuous and leave it at
that.

https://codereview.chromium.org/12302036/diff/4001/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:174: UNTIL_FULL = 0
On 2013/02/21 05:40:16, nduca wrote:
> naming is kind of awks. RECORD_UNTIL_FULL, RECORD_CONTINUOUSLY

Done.

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:387: if (args->GetSize() == 3) {
On 2013/02/21 05:40:16, nduca wrote:
> would be less fragile if you wrote >= 3

Done.

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:391: mode =
base::debug::TraceLog::TraceModeFromString(mode_);
On 2013/02/21 05:49:24, pfeldman wrote:
> Yep, you should have local conversion barrier in the devtools code.

Done.

https://codereview.chromium.org/12302036/diff/4001/content/browser/tracing/tr...
content/browser/tracing/tracing_ui.cc:391: mode =
base::debug::TraceLog::TraceModeFromString(mode_);
On 2013/02/21 05:40:16, nduca wrote:
> might check with pavel, but i think you need to do the string-to-enum
conversion
> in devtools. Once released, the names we use on the devtool protocol handler
we
> have to commit to being backcompat. That's not the case for tracing_ui, and
not
> obvious when you're just a dude hacking in base/. That is to say, lets do this
> per-instance conversion to enum here and in devtools, rather than sharing
code.

Done.

I've made it a comma separated list, so devtools would receive
record-until-full,sampling and we'll split and turn into options from here.
(same with tracing_ui)

[pfeldman, 2013-02-21 16:33:05]

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_tracing_handler.cc:72: if (*iter ==
kRecordUntilFull) {
Do you need to trim it before comparison?

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_tracing_handler.cc:79: return ret;
Not sure I follow, but it sounds like it always returns RECORD_UNTIL_FULL no
matter what the input it?

[dsinclair, 2013-02-21 16:35:59]

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_tracing_handler.cc:72: if (*iter ==
kRecordUntilFull) {
On 2013/02/21 16:33:05, pfeldman wrote:
> Do you need to trim it before comparison?

SplitString will trim leading and trailing whitespace when it does the split.

https://codereview.chromium.org/12302036/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_tracing_handler.cc:79: return ret;
On 2013/02/21 16:33:05, pfeldman wrote:
> Not sure I follow, but it sounds like it always returns RECORD_UNTIL_FULL no
> matter what the input it?

It does at the moment. I have another patch in the works which will add a
RECORD_CONTINUOUS option and there is a third patch being worked on to add a
SAMPLING option as well.

This is just getting the foundation in place which is why it looks a bit weird
at the moment.

[pfeldman, 2013-02-21 16:45:44]

devtools lgtm

[dsinclair, 2013-02-22 15:10:08]

jar@ can you please take a look for base/ OWNERS.

phajdan.jr@ can you please take a look for chrome/ OWNERS.

nduca@ can you please take a look for components/tracing and
content/browser/tracing OWNERS.

agl@ can you please take a look for content/app OWNERS.

yfriedman@ can you please take a look for content/browser/android OWNERS.

joi@ can you please take a look for content/public/browser OWNERS.

Thanks everyone.
dan

[Jói, 2013-02-22 15:11:30]

LGTM for content/public with one nit.

https://codereview.chromium.org/12302036/diff/4002/content/public/browser/tra...
File content/public/browser/trace_controller.h (right):

https://codereview.chromium.org/12302036/diff/4002/content/public/browser/tra...
content/public/browser/trace_controller.h:39: // |mode| is the tracing mode
begin used.
begin -> being

[agl, 2013-02-22 15:15:03]

content/app LGTM.

[dsinclair, 2013-02-22 15:16:00]

https://codereview.chromium.org/12302036/diff/4002/content/public/browser/tra...
File content/public/browser/trace_controller.h (right):

https://codereview.chromium.org/12302036/diff/4002/content/public/browser/tra...
content/public/browser/trace_controller.h:39: // |mode| is the tracing mode
begin used.
On 2013/02/22 15:11:30, Jói wrote:
> begin -> being

Done.

[nduca, 2013-02-22 18:38:12]

You'll need to not passing around ints as options, but rather the Options type.
Use a typedef and pull it out of tracelog. If you find the static_cast at the
call site to be annoying, then we can always put predefined combinations in the
enum definition.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:214: int GetTraceOptions() { return
trace_options_; }
int -> Options

const

This is a trivial accessor, so it should probably be trace_options() intsead of
GetTraceOPtions.

[Yaron, 2013-02-22 18:48:52]

lgtm for content/browser/android

[dsinclair, 2013-02-22 19:16:02]

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:214: int GetTraceOptions() { return
trace_options_; }
On 2013/02/22 18:38:13, nduca wrote:
> int -> Options
> 
> const
> 
> This is a trivial accessor, so it should probably be trace_options() intsead
of
> GetTraceOPtions.

Done.

[nduca, 2013-02-22 19:17:37]

lgtm

[jar (doing other things), 2013-02-22 19:31:27]

Comments on Base for patch set 5

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:359: std::vector<std::string>::iterator iter;
nit: move into line 363, so it is next to first use.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:362: base::SplitString(options, ',', &split);
nit: declare closer to first use.  Probably push this line up to between line
358 and 359.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:366: }
nit: Should we have a DCHECK for other spellings here?

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:369: ret = RECORD_UNTIL_FULL;
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?

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: static int TraceOptionsFromString(const
std::string& str);
nit: perhaps it would be nice to return Options.  Truth is, you'll have OR of
several... but it might be clearer.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:197: int options);
nit: use Options rather than int throughout in arg list, and return types.

[nduca, 2013-02-22 19:35:49]

More options are coming the second this lands, so I would argue not too much is
needed for the ~1 day where there's only one value. :)

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

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:359: std::vector<std::string>::iterator iter;
On 2013/02/22 19:31:28, jar wrote:
> nit: move into line 363, so it is next to first use.

Done.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:362: base::SplitString(options, ',', &split);
On 2013/02/22 19:31:28, jar wrote:
> nit: declare closer to first use.  Probably push this line up to between line
> 358 and 359.

Done.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:366: }
On 2013/02/22 19:31:28, jar wrote:
> nit: Should we have a DCHECK for other spellings here?

Added a else NOTREACHED();

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:369: ret = RECORD_UNTIL_FULL;
On 2013/02/22 19:31:28, jar wrote:
> 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?

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.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:181: static int TraceOptionsFromString(const
std::string& str);
On 2013/02/22 19:31:28, jar wrote:
> nit: perhaps it would be nice to return Options.  Truth is, you'll have OR of
> several... but it might be clearer.

Done.

https://codereview.chromium.org/12302036/diff/9025/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:197: int options);
On 2013/02/22 19:31:28, jar wrote:
> nit: use Options rather than int throughout in arg list, and return types.

Done.

[jar (doing other things), 2013-02-22 20:15:57]

Patch set 7 base LGTM with the nit below.

If you are about to land more RECORD_UNTIL_* Options, then you can skip the
comment (nit).

https://codereview.chromium.org/12302036/diff/12023/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12302036/diff/12023/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:370: if (!(ret & RECORD_UNTIL_FULL))
nit: This still reads super strangly.  As written (with no comment) it says that
IF the bit is not set, then we should OR in the bit.  Inquiring minds would ask:
Why didn't you just OR in the bit??  What did you really mean to say here,
'cause no one would write code this way unless there was a typo.

Suggested comment:
// Check to see if any RECORD_UNTIL_* options are set, and if none, then provide
a default.  TODO: Remove this comment when we have more than one RECORD_UNTIL_*
flags and the code's structure is then sensible.

[dsinclair, 2013-02-22 20:21:13]

https://codereview.chromium.org/12302036/diff/12023/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/12302036/diff/12023/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:370: if (!(ret & RECORD_UNTIL_FULL))
On 2013/02/22 20:15:57, jar wrote:
> nit: This still reads super strangly.  As written (with no comment) it says
that
> IF the bit is not set, then we should OR in the bit.  Inquiring minds would
ask:
> Why didn't you just OR in the bit??  What did you really mean to say here,
> 'cause no one would write code this way unless there was a typo.
> 
> Suggested comment:
> // Check to see if any RECORD_UNTIL_* options are set, and if none, then
provide
> a default.  TODO: Remove this comment when we have more than one
RECORD_UNTIL_*
> flags and the code's structure is then sensible.
> 

Done.

[dsinclair, 2013-02-22 20:30:33]

jam@ can you please take a look for chrome/browser/automation OWNERS.

sky@ can you please take a look for chrome/test OWNERS.

Thanks,
dan

[jam, 2013-02-22 20:56:27]

On 2013/02/22 20:30:33, dsinclair wrote:
> jam@ can you please take a look for chrome/browser/automation OWNERS.
> 
> sky@ can you please take a look for chrome/test OWNERS.
lgtm for both
> 
> Thanks,
> dan

[commit-bot: I haz the power, 2013-02-22 20:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/12302036/18001

[commit-bot: I haz the power, 2013-02-22 20:58:21]

Failed to apply patch for base/debug/trace_event_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file base/debug/trace_event_unittest.cc
  Hunk #1 FAILED at 73.
  1 out of 15 hunks FAILED -- saving rejects to file
base/debug/trace_event_unittest.cc.rej

Patch:       base/debug/trace_event_unittest.cc
Index: base/debug/trace_event_unittest.cc
diff --git a/base/debug/trace_event_unittest.cc
b/base/debug/trace_event_unittest.cc
index
15e11a70f4829a10ded2606e6f52d1fe1e5e0e92..12e02531cb84134bf8582043b4306462c764e30c
100644
--- a/base/debug/trace_event_unittest.cc
+++ b/base/debug/trace_event_unittest.cc
@@ -73,7 +73,7 @@ class TraceEventTestFixture : public testing::Test {
 
   void BeginTrace() {
     event_watch_notification_ = 0;
-    TraceLog::GetInstance()->SetEnabled("*");
+    TraceLog::GetInstance()->SetEnabled("*", TraceLog::RECORD_UNTIL_FULL);
   }
 
   void EndTraceAndFlush() {
@@ -714,7 +714,7 @@ void HighResSleepForTraceTest(base::TimeDelta elapsed) {
 // Simple Test for emitting data and validating it was received.
 TEST_F(TraceEventTestFixture, DataCaptured) {
   ManualTestSetUp();
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
 
   TraceWithAllMacroVariants(NULL);
 
@@ -738,18 +738,18 @@ TEST_F(TraceEventTestFixture,
EnabledObserverFiresOnEnable) {
 
   EXPECT_CALL(observer, OnTraceLogWillEnable())
       .Times(1);
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
   testing::Mock::VerifyAndClear(&observer);
 
   // Cleanup.
   TraceLog::GetInstance()->RemoveEnabledStateObserver(&observer);
-  TraceLog::GetInstance()->SetEnabled(false);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
 }
 
 TEST_F(TraceEventTestFixture, EnabledObserverDoesntFireOnSecondEnable) {
   ManualTestSetUp();
 
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
 
   testing::StrictMock<MockEnabledStateChangedObserver> observer;
   TraceLog::GetInstance()->AddEnabledStateObserver(&observer);
@@ -758,20 +758,20 @@ TEST_F(TraceEventTestFixture,
EnabledObserverDoesntFireOnSecondEnable) {
       .Times(0);
   EXPECT_CALL(observer, OnTraceLogWillDisable())
       .Times(0);
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
   testing::Mock::VerifyAndClear(&observer);
 
   // Cleanup.
   TraceLog::GetInstance()->RemoveEnabledStateObserver(&observer);
-  TraceLog::GetInstance()->SetEnabled(false);
-  TraceLog::GetInstance()->SetEnabled(false);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
 }
 
 TEST_F(TraceEventTestFixture, EnabledObserverDoesntFireOnNestedDisable) {
   ManualTestSetUp();
 
-  TraceLog::GetInstance()->SetEnabled(true);
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
 
   testing::StrictMock<MockEnabledStateChangedObserver> observer;
   TraceLog::GetInstance()->AddEnabledStateObserver(&observer);
@@ -780,25 +780,25 @@ TEST_F(TraceEventTestFixture,
EnabledObserverDoesntFireOnNestedDisable) {
       .Times(0);
   EXPECT_CALL(observer, OnTraceLogWillDisable())
       .Times(0);
-  TraceLog::GetInstance()->SetEnabled(false);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
   testing::Mock::VerifyAndClear(&observer);
 
   // Cleanup.
   TraceLog::GetInstance()->RemoveEnabledStateObserver(&observer);
-  TraceLog::GetInstance()->SetEnabled(false);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
 }
 
 TEST_F(TraceEventTestFixture, EnabledObserverFiresOnDisable) {
   ManualTestSetUp();
 
-  TraceLog::GetInstance()->SetEnabled(true);
+  TraceLog::GetInstance()->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
 
   MockEnabledStateChangedObserver observer;
   TraceLog::GetInstance()->AddEnabledStateObserver(&observer);
 
   EXPECT_CALL(observer, OnTraceLogWillDisable())
       .Times(1);
-  TraceLog::GetInstance()->SetEnabled(false);
+  TraceLog::GetInstance()->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
   testing::Mock::VerifyAndClear(&observer);
 
   // Cleanup.
@@ -834,7 +834,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   Clear();
   included_categories.clear();
   included_categories.push_back("not_found823564786");
-  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories);
+  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("cat1", "name");
   TRACE_EVENT_INSTANT0("cat2", "name");
   EndTraceAndFlush();
@@ -844,7 +845,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   Clear();
   included_categories.clear();
   included_categories.push_back("inc");
-  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories);
+  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("inc", "name");
   TRACE_EVENT_INSTANT0("inc2", "name");
   EndTraceAndFlush();
@@ -856,7 +858,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   included_categories.clear();
   included_categories.push_back("inc_wildcard_*");
   included_categories.push_back("inc_wildchar_?_end");
-  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories);
+  TraceLog::GetInstance()->SetEnabled(included_categories, empty_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("inc_wildcard_abc", "included");
   TRACE_EVENT_INSTANT0("inc_wildcard_", "included");
   TRACE_EVENT_INSTANT0("inc_wildchar_x_end", "included");
@@ -875,7 +878,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   Clear();
   excluded_categories.clear();
   excluded_categories.push_back("not_found823564786");
-  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories);
+  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("cat1", "name");
   TRACE_EVENT_INSTANT0("cat2", "name");
   EndTraceAndFlush();
@@ -886,7 +890,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   Clear();
   excluded_categories.clear();
   excluded_categories.push_back("inc");
-  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories);
+  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("inc", "name");
   TRACE_EVENT_INSTANT0("inc2", "name");
   EndTraceAndFlush();
@@ -898,7 +903,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   excluded_categories.clear();
   excluded_categories.push_back("inc_wildcard_*");
   excluded_categories.push_back("inc_wildchar_?_end");
-  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories);
+  TraceLog::GetInstance()->SetEnabled(empty_categories, excluded_categories,
+                                      TraceLog::RECORD_UNTIL_FULL);
   TRACE_EVENT_INSTANT0("inc_wildcard_abc", "not_inc");
   TRACE_EVENT_INSTANT0("inc_wildcard_", "not_inc");
   TRACE_EVENT_INSTANT0("inc_wildchar_x_end", "not_inc");
@@ -1431,17 +1437,17 @@ TEST_F(TraceEventTestFixture, TraceEnableDisable) {
   ManualTestSetUp();
 
   TraceLog* trace_log = TraceLog::GetInstance();
-  trace_log->SetEnabled(std::string());
+  trace_log->SetEnabled(std::string(), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(trace_log->IsEnabled());
   trace_log->SetDisabled();
   EXPECT_FALSE(trace_log->IsEnabled());
 
-  trace_log->SetEnabled(true);
+  trace_log->SetEnabled(true, TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(trace_log->IsEnabled());
   const std::vector<std::string> empty;
-  trace_log->SetEnabled(empty, empty);
+  trace_log->SetEnabled(empty, empty, TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(trace_log->IsEnabled());
-  trace_log->SetEnabled(false);
+  trace_log->SetEnabled(false, TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(trace_log->IsEnabled());
   trace_log->SetDisabled();
   EXPECT_FALSE(trace_log->IsEnabled());
@@ -1451,14 +1457,14 @@ TEST_F(TraceEventTestFixture,
TraceCategoriesAfterNestedEnable) {
   ManualTestSetUp();
 
   TraceLog* trace_log = TraceLog::GetInstance();
-  trace_log->SetEnabled(std::string("foo,bar"));
+  trace_log->SetEnabled(std::string("foo,bar"), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("foo"));
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("bar"));
   EXPECT_FALSE(*trace_log->GetCategoryEnabled("baz"));
-  trace_log->SetEnabled(std::string("foo2"));
+  trace_log->SetEnabled(std::string("foo2"), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("foo2"));
   EXPECT_FALSE(*trace_log->GetCategoryEnabled("baz"));
-  trace_log->SetEnabled(std::string(""));
+  trace_log->SetEnabled(std::string(""), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("foo"));
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("baz"));
   trace_log->SetDisabled();
@@ -1467,10 +1473,10 @@ TEST_F(TraceEventTestFixture,
TraceCategoriesAfterNestedEnable) {
   EXPECT_FALSE(*trace_log->GetCategoryEnabled("foo"));
   EXPECT_FALSE(*trace_log->GetCategoryEnabled("baz"));
 
-  trace_log->SetEnabled(std::string("-foo,-bar"));
+  trace_log->SetEnabled(std::string("-foo,-bar"), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_FALSE(*trace_log->GetCategoryEnabled("foo"));
   EXPECT_TRUE(*trace_log->GetCategoryEnabled("baz"));
-  trace_log->SetEnabled(std::string("moo"));
+…
(message too large)

[commit-bot: I haz the power, 2013-02-22 21:08:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/12302036/7019

[commit-bot: I haz the power, 2013-02-23 02:09:55]

Change committed as 184258