Fix the testing::LogDisabler for the change r64883. See http://codereview.chr...

Fix the testing::LogDisabler for the change r64883. See http://codereview.chromium.org/4262001/show
The r64883 broke most unit tests in CEEE that relied on disabling logs while testing error paths.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64948

[hansl, 2010-11-03 18:01:11]

Please take a look.

brettw, akalin: This is following the change earlier today:
http://codereview.chromium.org/4262001

Thanks

[akalin, 2010-11-03 18:16:51]

LGTM, sorry for the breakage.  Is there a way I could have avoided it?

On 2010/11/03 18:01:11, hansl wrote:
> Please take a look.
> 
> brettw, akalin: This is following the change earlier today:
> http://codereview.chromium.org/4262001
> 
> Thanks

[hansl_g, 2010-11-03 18:18:24]

By asking gently  :P

CEEE isn't in the official waterfall yet, that's why you didn't see it. Better
knowledge in the interaction between ::testing and ::logging would probably have
raised the issue. Don't worry much.

Should I wait for brettw lgtm?

On 2010/11/03 18:16:51, akalin wrote:
> LGTM, sorry for the breakage.  Is there a way I could have avoided it?
> 
> On 2010/11/03 18:01:11, hansl wrote:
> > Please take a look.
> > 
> > brettw, akalin: This is following the change earlier today:
> > http://codereview.chromium.org/4262001
> > 
> > Thanks

[Jói, 2010-11-03 18:21:51]

LGTM

The nice thing here is that with some additions we could start asserting in unit
tests on certain logging behavior, which was my intent initially when I wrote
the LogDisabler. Thanks Hans!

Cheers,
Jói

[akalin, 2010-11-03 18:26:19]

On 2010/11/03 18:18:24, hansl_g wrote:
> By asking gently  :P
> 
> CEEE isn't in the official waterfall yet, that's why you didn't see it. Better
> knowledge in the interaction between ::testing and ::logging would probably
have
> raised the issue. Don't worry much.
> 
> Should I wait for brettw lgtm?

I don't think that's needed.

The LogDisabler may be useful in some other Chrome unit tests (e.g., the ipc one
that broke in my CL).  Is the LogDisabler meant to be used simply to stop
DCHECKs or is blocking *all* log messages needed?  If it's the former, then
maybe it could be moved to base/ to prevent future breakage.

[Jói, 2010-11-03 18:28:23]

The initial intent was just to disable DCHECKs (but ideally to be able
to set a flag that they happened, which unit tests could assert on -
that's not in yet but Hans's change seems to make that easily
possible). I agree we could move it to base once it's just DCHECKs.

Cheers,
Jói


On Wed, Nov 3, 2010 at 2:26 PM,  <akalin@chromium.org> wrote:
> On 2010/11/03 18:18:24, hansl_g wrote:
>>
>> By asking gently  :P
>
>> CEEE isn't in the official waterfall yet, that's why you didn't see it.
>> Better
>> knowledge in the interaction between ::testing and ::logging would
>> probably
>
> have
>>
>> raised the issue. Don't worry much.
>
>> Should I wait for brettw lgtm?
>
> I don't think that's needed.
>
> The LogDisabler may be useful in some other Chrome unit tests (e.g., the ipc
> one
> that broke in my CL).  Is the LogDisabler meant to be used simply to stop
> DCHECKs or is blocking *all* log messages needed?  If it's the former, then
> maybe it could be moved to base/ to prevent future breakage.
>
> http://codereview.chromium.org/4390001/show
>