New test infrastructure for producing verbose logs in failing tests.

New test infrastructure for producing verbose logs in failing tests.

This infrastructure allows Google Test-based tests to opt-in to some magic that results in extremely verbose logs appearing in test output for only those tests that fail.  This includes all LOG() messages generated by chrome.exe, npchrome_frame.exe, and the test executable itself, as well as all TRACE_EVENT_*_ETW events.  The interesting entrypoints are in test_log_collector_win.h, file_logger_win.h, and log_file_printer_win.h.

BUG=none
TEST=none

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

[grt (UTC plus 2), 2012-03-02 18:30:54]

Hi guys,

Would you two take a look at this CL, which contains the general-purpose logging
stuff that I hope will help us squash Chrome Frame test flakiness?  I'd like to
get your sign-off before embarking on OWNERS approval in the relevant places. 
Thanks.

[erikwright (departed), 2012-03-02 20:47:59]

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:26: // From chrome_tab.cc:
{0562BFC3-2550-45b4-BD8E-A310583D3A6F}
I would argue against declaring these here, and against this class having
default providers.

Make the definitions suitably accessible, and then simply expect the client to
supply the list they're interested in.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:131:
::SetEnvironmentVariable(chrome_etw_logging.c_str(), L"1");
Should this also be reverted? If not, why not?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
I would appreciate further comments as to how this actually works. Is this a
standard Windows thing? What part of Chrome is reacting to it?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:173: scan != end; ++scan) {
Is it actually considered costly to just inline the call to
provider_names_.end()?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.h:24: class FileLogger {
I wonder if you really need separate steps for constructing, Initializing,
Starting, Stopping, Uninitializing, and destroying?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
File chrome/test/base/test_log_collector_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.h:25: // Installs the test log collector
into |unit_test|.  The collector cannot be
I infer that testing::UnitTest does not map to a single test case as I know it?
(i.e., one instance of TEST(...) {}?).

If I'm right, can you call that out here. If I'm wrong, I really don't
understand this because I assume it is installed for the life of a test
executable.

[grt (UTC plus 2), 2012-03-02 21:22:43]

Thanks for the comments.  Responses below.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:26: // From chrome_tab.cc:
{0562BFC3-2550-45b4-BD8E-A310583D3A6F}
On 2012/03/02 20:47:59, erikwright wrote:
> I would argue against declaring these here, and against this class having
> default providers.
> 
> Make the definitions suitably accessible, and then simply expect the client to
> supply the list they're interested in.

My argument is that these are the only three providers.  I think that the common
(perhaps only) case is that users would want to listen to all three.  Even if
they only care about the test and Chrome (and not CF), I'd go in favor of the
simple interface.  I put the extra Initialize method in just in case there was a
fringe user, or if someone wanted to write a unit test for this test code.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:131:
::SetEnvironmentVariable(chrome_etw_logging.c_str(), L"1");
On 2012/03/02 20:47:59, erikwright wrote:
> Should this also be reverted? If not, why not?

It's more work.  The change won't persist beyond the lifetime of this process. 
I can understand the purity and symmetry of doing so, but I don't think there's
any harm in leaving it there.  The extra steps are taken to conditionally undo
the registry change since that's a persistent system-wide modification.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
On 2012/03/02 20:47:59, erikwright wrote:
> I would appreciate further comments as to how this actually works. Is this a
> standard Windows thing? What part of Chrome is reacting to it?

Comment added.  It's a standard Windows thing
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682653(v=vs.85).aspx);
no part of Chrome would react to it, to my knowledge.  The idea is that we want
the environment for IE and Chrome to have the env var.  The hope is that after
the broadcast, the Windows shell and any other relevant parts of the system have
picked up the new env var.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:173: scan != end; ++scan) {
On 2012/03/02 20:47:59, erikwright wrote:
> Is it actually considered costly to just inline the call to
> provider_names_.end()?

For a release build, no.  For a debug build it'll create/destroy an iterator
each time through the loop.  Old habits die hard...

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.h:24: class FileLogger {
On 2012/03/02 20:47:59, erikwright wrote:
> I wonder if you really need separate steps for constructing, Initializing,
> Starting, Stopping, Uninitializing, and destroying?

Yup, the collector does.  Initialize happens just before any tests run (it
doesn't happen, for example, if you just --gtest_list_tests).  Starting happens
at the beginning of a single test.  Stopping happens at the end of a single
tests.  There's a start/stop pair for each test.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
File chrome/test/base/test_log_collector_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.h:25: // Installs the test log collector
into |unit_test|.  The collector cannot be
On 2012/03/02 20:47:59, erikwright wrote:
> I infer that testing::UnitTest does not map to a single test case as I know
it?
> (i.e., one instance of TEST(...) {}?).

Correct.  testing::UnitTest is the process-wide singleton that owns all test
cases and all tests.  Since UnitTest is a singleton, it's actually impossible to
try to install the collector into two different unit tests, so I've removed that
bit from the comment.

> If I'm right, can you call that out here.

See the comment at the top of the file.  Is that not sufficient?

> If I'm wrong, I really don't
> understand this because I assume it is installed for the life of a test
> executable.

I thought that was implied in the comment at the top; is it not clear?

[robertshield, 2012-03-02 22:37:41]

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:94: *out_ << reinterpret_cast<const
char*>(event->MofData);
Is MofData guaranteed to be null terminated or should you use MofLength here?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:139: &value) != ERROR_SUCCESS &&
do you need to check value here?

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
Did you test this against IE8/9 in protected mode? I vaguely recall that env
vars weren't always propagated when IE spawned child processes (which might be
why you added this), so this I suppose will have to be handled by IE as well for
CF logging to be caught.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:208: CHECK(!g_is_initialized);
If these are initialized on different threads, you could still have two of them
initialized without hitting this CHECK.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
File chrome/test/base/test_log_collector_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.cc:219:
std::replace(log_file_name.begin(), log_file_name.end(), '/', '_');
can test names contain other invalid characters (\ / : * ? " < > | according to
http://support.microsoft.com/kb/177506)

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.cc:272: (void) log_temp_dir_.Delete();
(void)? I guess you did this because Delete() is marked with 
WARN_UNUSED_RESULT, in which case you want ignore_result().

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:47: const HKEY kEnvironmentRoot =
HKEY_LOCAL_MACHINE;
I take it that this can only be used for tests running as admin, not the
user-level tests?

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:218: ConfigureChromeEtwLogging();
Make this return a value and log if it fails?

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:23: // time.
Consider adding a note here that this requires admin privileges and not to call
it if writing a test that runs without them.

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:34: // if it is not present.
* The variable CHROME_ETW_LOGGING=1 is added to the system-wide environment if
it is not present.

[erikwright (departed), 2012-03-03 02:04:56]

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
File chrome/test/base/file_logger_win.cc (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.cc:26: // From chrome_tab.cc:
{0562BFC3-2550-45b4-BD8E-A310583D3A6F}
On 2012/03/02 21:22:43, grt wrote:
> On 2012/03/02 20:47:59, erikwright wrote:
> > I would argue against declaring these here, and against this class having
> > default providers.
> > 
> > Make the definitions suitably accessible, and then simply expect the client
to
> > supply the list they're interested in.
> 
> My argument is that these are the only three providers.  I think that the
common
> (perhaps only) case is that users would want to listen to all three.  Even if
> they only care about the test and Chrome (and not CF), I'd go in favor of the
> simple interface.  I put the extra Initialize method in just in case there was
a
> fringe user, or if someone wanted to write a unit test for this test code.

I'll explain my reasoning, but leave it at that. I don't care too much either
way.

The main source of smell for me is the duplication of these values here. So I
wondered whether they could be included from elsewhere, but I realized that this
code here could not really have a dependency on chrome_frame. The only way to
neither duplicate nor add an inappropriate dependency would be to take them out.

Two other options present themselves:

1) Move this code higher up, to chrome_frame/test, for example. Of course, then
it would not be reusable by other teams.
2) Make a base class that does not include the Chrome Frame provider, and then
put a subclass in chrome_frame/test

The 3rd option of leaving it as is is OK too.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
On 2012/03/02 21:22:43, grt wrote:
> On 2012/03/02 20:47:59, erikwright wrote:
> > I would appreciate further comments as to how this actually works. Is this a
> > standard Windows thing? What part of Chrome is reacting to it?
> 
> Comment added.  It's a standard Windows thing
>
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682653%28v=vs.85%29...;
> no part of Chrome would react to it, to my knowledge.  The idea is that we
want
> the environment for IE and Chrome to have the env var.  The hope is that after
> the broadcast, the Windows shell and any other relevant parts of the system
have
> picked up the new env var.

So I gather that this class can only catch log messages from processes that are
not yet started, correct? If so, I would suggest including that in comments.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_win.h
File chrome/test/base/file_logger_win.h (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.h:24: class FileLogger {
On 2012/03/02 21:22:43, grt wrote:
> On 2012/03/02 20:47:59, erikwright wrote:
> > I wonder if you really need separate steps for constructing, Initializing,
> > Starting, Stopping, Uninitializing, and destroying?
> 
> Yup, the collector does.  Initialize happens just before any tests run (it
> doesn't happen, for example, if you just --gtest_list_tests).  Starting
happens
> at the beginning of a single test.  Stopping happens at the end of a single
> tests.  There's a start/stop pair for each test.

OK. One thing to consider is whether you simplify the logic of this class by
merging initialization/construction and uninitialization/destruction. Then
replace member instances with scoped_ptr instances.

It's up to you, but I find that removing state is always positive.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/test_log_colle...
File chrome/test/base/test_log_collector_win.h (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/test_log_colle...
chrome/test/base/test_log_collector_win.h:25: // Installs the test log collector
into |unit_test|.  The collector cannot be
On 2012/03/02 21:22:43, grt wrote:
> On 2012/03/02 20:47:59, erikwright wrote:
> > I infer that testing::UnitTest does not map to a single test case as I know
> it?
> > (i.e., one instance of TEST(...) {}?).
> 
> Correct.  testing::UnitTest is the process-wide singleton that owns all test
> cases and all tests.  Since UnitTest is a singleton, it's actually impossible
to
> try to install the collector into two different unit tests, so I've removed
that
> bit from the comment.
> 
> > If I'm right, can you call that out here.
> 
> See the comment at the top of the file.  Is that not sufficient?
> 
> > If I'm wrong, I really don't
> > understand this because I assume it is installed for the life of a test
> > executable.
> 
> I thought that was implied in the comment at the top; is it not clear?

The comment at the top was relatively clear. It did, after all, allow me to
infer that testing::UnitTest must represent the full test suite. I would have to
do more investigation to figure out how to get an instance of testing::UnitTest,
I suppose. I would suggest perhaps adding something along the lines of

// testing::UnitTest is the process-wide singleton that owns all test cases. It
can be retrieved via testing::UnitTest::GetInstance()

[grt (UTC plus 2), 2012-03-03 02:12:43]

Thanks, Robert.  PTAL.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:94: *out_ << reinterpret_cast<const
char*>(event->MofData);
On 2012/03/02 22:37:41, robertshield wrote:
> Is MofData guaranteed to be null terminated or should you use MofLength here?

The terminator is always added by loggin_win.cc.  It's better not to blow up if
it's missing for some reason, so I've made it safe.  Thanks for pointing this
out.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:139: &value) != ERROR_SUCCESS &&
On 2012/03/02 22:37:41, robertshield wrote:
> do you need to check value here?

Nope, Chrome doesn't care.  I've added a comment.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
On 2012/03/02 22:37:41, robertshield wrote:
> Did you test this against IE8/9 in protected mode?

I've tested against IE8, but I didn't do anything special for protected mode. 
How do I do that?

> I vaguely recall that env
> vars weren't always propagated when IE spawned child processes (which might be
> why you added this), so this I suppose will have to be handled by IE as well
for
> CF logging to be caught.

IE shouldn't be running when this happens (or if it is, it'll be killed shortly
thereafter).  The question is whether or not IE is handed the system env vars by
whatever fires it up, and whether or not it passes them on to child processes it
spawns.  I'll try it out with IE8.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:208: CHECK(!g_is_initialized);
On 2012/03/02 22:37:41, robertshield wrote:
> If these are initialized on different threads, you could still have two of
them
> initialized without hitting this CHECK.

The documentation for the class says only one instance may be initialized at a
time.  This is a feeble attempt at enforcing that so that unexpected things
don't happen.  I'll add a note that the class isn't threadsafe in any way,
shape, or form.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
File chrome/test/base/test_log_collector_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.cc:219:
std::replace(log_file_name.begin(), log_file_name.end(), '/', '_');
On 2012/03/02 22:37:41, robertshield wrote:
> can test names contain other invalid characters (\ / : * ? " < > | according
to
> http://support.microsoft.com/kb/177506)

Nope, they're identifiers in the C/C++ sense.

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/test_l...
chrome/test/base/test_log_collector_win.cc:272: (void) log_temp_dir_.Delete();
On 2012/03/02 22:37:41, robertshield wrote:
> (void)? I guess you did this because Delete() is marked with 
> WARN_UNUSED_RESULT, in which case you want ignore_result().

Awesome, thanks!

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:47: const HKEY kEnvironmentRoot =
HKEY_LOCAL_MACHINE;
On 2012/03/02 22:37:41, robertshield wrote:
> I take it that this can only be used for tests running as admin, not the
> user-level tests?

Yeah, firing up an ETW session requires admin, as does the environment
twiddling.

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:218: ConfigureChromeEtwLogging();
On 2012/03/02 22:37:41, robertshield wrote:
> Make this return a value and log if it fails?

I've added some logging to the implementation.  WDYT?

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:23: // time.
On 2012/03/02 22:37:41, robertshield wrote:
> Consider adding a note here that this requires admin privileges and not to
call
> it if writing a test that runs without them.

Done.

https://chromiumcodereview.appspot.com/9584017/diff/3001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:34: // if it is not present.
On 2012/03/02 22:37:41, robertshield wrote:
> * The variable CHROME_ETW_LOGGING=1 is added to the system-wide environment if
> it is not present.

Done.

[grt (UTC plus 2), 2012-03-03 02:31:58]

Thanks for the great comments, Erik.  PTAL.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
File chrome/test/base/file_logger_win.cc (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.cc:26: // From chrome_tab.cc:
{0562BFC3-2550-45b4-BD8E-A310583D3A6F}
On 2012/03/03 02:04:56, erikwright wrote:
> On 2012/03/02 21:22:43, grt wrote:
> > On 2012/03/02 20:47:59, erikwright wrote:
> > > I would argue against declaring these here, and against this class having
> > > default providers.
> > > 
> > > Make the definitions suitably accessible, and then simply expect the
client
> to
> > > supply the list they're interested in.
> > 
> > My argument is that these are the only three providers.  I think that the
> common
> > (perhaps only) case is that users would want to listen to all three.  Even
if
> > they only care about the test and Chrome (and not CF), I'd go in favor of
the
> > simple interface.  I put the extra Initialize method in just in case there
was
> a
> > fringe user, or if someone wanted to write a unit test for this test code.
> 
> I'll explain my reasoning, but leave it at that. I don't care too much either
> way.
> 
> The main source of smell for me is the duplication of these values here. So I
> wondered whether they could be included from elsewhere, but I realized that
this
> code here could not really have a dependency on chrome_frame. The only way to
> neither duplicate nor add an inappropriate dependency would be to take them
out.

Understood.  Duplicating them here made me a bit uneasy as well, but beer and
the fact that it's test infrastructure helped me feel better.  Not that test
code should necessarily be held to a lower standard...  Okay, maybe I'm taking
the easy way out.

> Two other options present themselves:
> 
> 1) Move this code higher up, to chrome_frame/test, for example. Of course,
then
> it would not be reusable by other teams.

Yeah, yuk.

> 2) Make a base class that does not include the Chrome Frame provider, and then
> put a subclass in chrome_frame/test

Hmm.  I'll think on that.  I'd like to keep it as drop-dead simple for consumers
as possible.

> The 3rd option of leaving it as is is OK too.

Another would be to put all of the constants in a central place in chrome/common
somewhere.  That'd be accessible to this, Chrome, and CF.  I'll think on it.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
On 2012/03/03 02:04:56, erikwright wrote:
> On 2012/03/02 21:22:43, grt wrote:
> > On 2012/03/02 20:47:59, erikwright wrote:
> > > I would appreciate further comments as to how this actually works. Is this
a
> > > standard Windows thing? What part of Chrome is reacting to it?
> > 
> > Comment added.  It's a standard Windows thing
> >
>
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682653%2528v=vs.85%...;
> > no part of Chrome would react to it, to my knowledge.  The idea is that we
> want
> > the environment for IE and Chrome to have the env var.  The hope is that
after
> > the broadcast, the Windows shell and any other relevant parts of the system
> have
> > picked up the new env var.
> 
> So I gather that this class can only catch log messages from processes that
are
> not yet started, correct? If so, I would suggest including that in comments.

Not necessarily.  If you already had that var in your environment (because
you've installed Sawbuck, for example), then it would work for running procs as
well.  I'll add a comment nonetheless.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_win.h
File chrome/test/base/file_logger_win.h (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/file_logger_wi...
chrome/test/base/file_logger_win.h:24: class FileLogger {
On 2012/03/03 02:04:56, erikwright wrote:
> On 2012/03/02 21:22:43, grt wrote:
> > On 2012/03/02 20:47:59, erikwright wrote:
> > > I wonder if you really need separate steps for constructing, Initializing,
> > > Starting, Stopping, Uninitializing, and destroying?
> > 
> > Yup, the collector does.  Initialize happens just before any tests run (it
> > doesn't happen, for example, if you just --gtest_list_tests).  Starting
> happens
> > at the beginning of a single test.  Stopping happens at the end of a single
> > tests.  There's a start/stop pair for each test.
> 
> OK. One thing to consider is whether you simplify the logic of this class by
> merging initialization/construction and uninitialization/destruction. Then
> replace member instances with scoped_ptr instances.
> 
> It's up to you, but I find that removing state is always positive.

That's a good point.  I understand where you're coming from.  Since Initialize
does significant work (there's considerable overhead to doing the env var
thing), moving it into the ctor goes against the Google C++ style guide.

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/test_log_colle...
File chrome/test/base/test_log_collector_win.h (right):

http://codereview.chromium.org/9584017/diff/1/chrome/test/base/test_log_colle...
chrome/test/base/test_log_collector_win.h:25: // Installs the test log collector
into |unit_test|.  The collector cannot be
On 2012/03/03 02:04:56, erikwright wrote:
> On 2012/03/02 21:22:43, grt wrote:
> > On 2012/03/02 20:47:59, erikwright wrote:
> > > I infer that testing::UnitTest does not map to a single test case as I
know
> > it?
> > > (i.e., one instance of TEST(...) {}?).
> > 
> > Correct.  testing::UnitTest is the process-wide singleton that owns all test
> > cases and all tests.  Since UnitTest is a singleton, it's actually
impossible
> to
> > try to install the collector into two different unit tests, so I've removed
> that
> > bit from the comment.
> > 
> > > If I'm right, can you call that out here.
> > 
> > See the comment at the top of the file.  Is that not sufficient?
> > 
> > > If I'm wrong, I really don't
> > > understand this because I assume it is installed for the life of a test
> > > executable.
> > 
> > I thought that was implied in the comment at the top; is it not clear?
> 
> The comment at the top was relatively clear. It did, after all, allow me to
> infer that testing::UnitTest must represent the full test suite. I would have
to
> do more investigation to figure out how to get an instance of
testing::UnitTest,
> I suppose. I would suggest perhaps adding something along the lines of
> 
> // testing::UnitTest is the process-wide singleton that owns all test cases.
It
> can be retrieved via testing::UnitTest::GetInstance()

I've had my head so far up Google Test's behind that I'd forgotten that I used
to not know about these details.  Thanks for the reminder.  I've added a little
more comment.

[grt (UTC plus 2), 2012-03-05 18:25:41]

Gents,

This update includes support for capturing/dumping trace events (previously only
log messages).  PTAL.

[robertshield, 2012-03-05 19:20:53]

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/1/chrome/test/base/file_l...
chrome/test/base/file_logger_win.cc:144: SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE, 0,
On 2012/03/03 02:12:43, grt wrote:
> On 2012/03/02 22:37:41, robertshield wrote:
> > Did you test this against IE8/9 in protected mode?
> 
> I've tested against IE8, but I didn't do anything special for protected mode. 
> How do I do that?

I just meant with IE8/9 in multi-process mode (the default), verify that either
the env var is handed off to *new* low-integrity processes, or that those low
integrity processes respond as desired to the WM_SETTINGCHANGE (they probably
do).

> 
> > I vaguely recall that env
> > vars weren't always propagated when IE spawned child processes (which might
be
> > why you added this), so this I suppose will have to be handled by IE as well
> for
> > CF logging to be caught.
> 
> IE shouldn't be running when this happens (or if it is, it'll be killed
shortly
> thereafter).  The question is whether or not IE is handed the system env vars
by
> whatever fires it up, and whether or not it passes them on to child processes
it
> spawns.  I'll try it out with IE8.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:10: #include <objbase.h>  // for
StringFromGUID2
any reason for the comment?

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:59: };
COMPILE_ASSERT on the arraysize vs the enum in the header.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:69: void WriteTimeStamp(LARGE_INTEGER
time_stamp, std::ostream& out) {
don't use non-const references

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:85: coarse_value.QuadPart =
time_stamp.QuadPart - coarse_value.QuadPart;
consider mentioning that ULARGE_INTEGER is a union and that doing this again
sets the LowPart. This might not be obvious given the way coarse_value is reused
here.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:160: : public
base::win::EtwTraceConsumerBase<TraceConsumer<ProcessEventFn> > {
template abuse alert.. template abuse alert.. 

It looks like this is only used in one place? If so, this is overly complicated.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:544: uint8 type) {
IMO this is needlessly complicated. Returning a factory here doesn't provide a
significant benefit to just new'ing up the right type of MofData and returning
it when I look at how this actually gets used.

You'll probably have cause to disagree with me though :-)

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:22: // initialized at a time.  This class is
not thread safe.  This class uses
Place the "not thread safe" message on a separate line, or make it more visible
in some other way.

[erikwright (departed), 2012-03-05 19:55:30]

My main comment is that file_logger_win.cc has a whole lot of utility code in it
that looks rather complicated.

I would prefer to see this broken down into a number of smaller files for
readability and, possibly, to facilitate writing unit tests for these classes.

I have not reviewed the MOF-parsing code for actual correctness or design
(partially as a consequence of the above).

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:101: // corresponding severity.
I suspect this corresponds to an inverse switch statement somewhere. If so, can
you provide a reference?

Would it be appropriate to expose two-way conversion functions at that point?

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:122: void WriteSeverity(logging::LogSeverity
severity, std::ostream& out) {
This is how base/logging.h does it:

http://code.google.com/codesearch#OAMlx_jo-ck/src/base/logging.cc&l=74

Might be a nice TODO to expose & reuse that functionality for severity >= 0?

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:168: // A parser for Mof data found in an
EVENT_TRACE object as formatted by
I find it shocking that this needs to be written but I couldn't find an existing
implementation. It does seem like it might be similar or a replacement of this
code in Syzygy:

http://code.google.com/p/sawbuck/source/search?q=MOF+file%3A%5C.cc&origq=MOF+...

Perhaps if one or the other got pushed down to base/ at some point they could
share code.

In any case, this code is complicated enough that I think it should live in a
separate file.

[robertshield, 2012-03-05 19:57:07]

http://codereview.chromium.org/9584017/diff/8001/chrome/test/base/file_logger...
File chrome/test/base/file_logger_win.cc (right):

http://codereview.chromium.org/9584017/diff/8001/chrome/test/base/file_logger...
chrome/test/base/file_logger_win.cc:160: : public
base::win::EtwTraceConsumerBase<TraceConsumer<ProcessEventFn> > {
On 2012/03/05 19:20:54, robertshield wrote:
> template abuse alert.. template abuse alert.. 
> 
> It looks like this is only used in one place? If so, this is overly
complicated.

Taking a look at the implementation of EtwTraceConsumerBase, I see why you did
this, and it's actually quite neat. I retract my previous mutterings.

[grt (UTC plus 2), 2012-03-06 03:24:56]

Thanks, Robert.  I've responded to your comments here.  Erik: yours are more
far-reaching (and appreciated), so I'll get to them tomorrow.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:10: #include <objbase.h>  // for
StringFromGUID2
On 2012/03/05 19:20:54, robertshield wrote:
> any reason for the comment?

Not really.  Removed.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:59: };
On 2012/03/05 19:20:54, robertshield wrote:
> COMPILE_ASSERT on the arraysize vs the enum in the header.

Kinda done.  I checked it against the bitfield of all providers.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:69: void WriteTimeStamp(LARGE_INTEGER
time_stamp, std::ostream& out) {
On 2012/03/05 19:20:54, robertshield wrote:
> don't use non-const references

In the case of streams, I think it makes more sense to use a non-const ref than
a pointer.  Although there are some hits for the latter in the codebase, the
former seems much more prevalent.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:85: coarse_value.QuadPart =
time_stamp.QuadPart - coarse_value.QuadPart;
On 2012/03/05 19:20:54, robertshield wrote:
> consider mentioning that ULARGE_INTEGER is a union and that doing this again
> sets the LowPart. This might not be obvious given the way coarse_value is
reused
> here.

Switched to using base/time.h's facilities, giving up on the idea of sub-ms
precision.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:160: : public
base::win::EtwTraceConsumerBase<TraceConsumer<ProcessEventFn> > {
On 2012/03/05 19:57:07, robertshield wrote:
> On 2012/03/05 19:20:54, robertshield wrote:
> > template abuse alert.. template abuse alert.. 
> > 
> > It looks like this is only used in one place? If so, this is overly
> complicated.
> 
> Taking a look at the implementation of EtwTraceConsumerBase, I see why you did
> this, and it's actually quite neat. I retract my previous mutterings.

Thanks.  I didn't like the other obvious options (having LogDumper derive from
EtwTraceConsumerBase, using "friend" so this could delegate to the active
LogDumper instance, or passing a function pointer at runtime).  I kinda like
this since the compiler can, in theory, peel away a level of indirection at
compile-time.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.cc:544: uint8 type) {
On 2012/03/05 19:20:54, robertshield wrote:
> IMO this is needlessly complicated. Returning a factory here doesn't provide a
> significant benefit to just new'ing up the right type of MofData and returning
> it when I look at how this actually gets used.
> 
> You'll probably have cause to disagree with me though :-)

No, I agree wholeheartedly.  This was left behind after other simplifications. 
Thanks for pointing it out.  I've removed the contagion.

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/8001/chrome/test/base/fil...
chrome/test/base/file_logger_win.h:22: // initialized at a time.  This class is
not thread safe.  This class uses
On 2012/03/05 19:20:54, robertshield wrote:
> Place the "not thread safe" message on a separate line, or make it more
visible
> in some other way.

Done.

[grt (UTC plus 2), 2012-03-08 21:37:06]

Hi guys,  I pulled the parsing and printing stuff out of the logging parts, and
simplified them a great deal in the process.  Still no unit tests, but the code
is probably more readable now.  PTAL.

https://chromiumcodereview.appspot.com/9584017/diff/21001/chrome/test/base/fi...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/21001/chrome/test/base/fi...
chrome/test/base/file_logger_win.h:22: //   be initialized at a time, and only
one file may be dumped at a time.
oops, i need to remove reference to dumping here.
also, the comments here should point to the new reader/printer classes

[grt (UTC plus 2), 2012-03-09 15:43:26]

Hi gang,

This CL is ready for review.  I'd like to land it soonish so I can enable this
logging in chrome_frame_tests.exe.

willchan - OWNERS approval for base (export one symbol from base.dll, and fix an
old bug in the ETW provider)
sky - OWNERS approval for chrome/test/*
robertshield - Windows guru familiar with the motivation

Please suggest an alternate reviewer if you'd like to absolve yourself of this. 
Thanks,

Greg

[grt (UTC plus 2), 2012-03-09 16:37:27]

I just added the missing test file in patch set 10.  It's otherwise identical to
patch set 9.  Apologies for the confusion.

[robertshield, 2012-03-09 16:58:04]

https://chromiumcodereview.appspot.com/9584017/diff/26016/base/win/event_trac...
File base/win/event_trace_provider.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/base/win/event_trac...
base/win/event_trace_provider.cc:89: DisableEvents();
if session_handle_ is null, the level and flags no longer get reset, is this
intended?

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:25: bool g_is_initialized = false;
Consider making this a static member of FileLogger, it makes the symbol slightly
easier to notice and inspect in the debugger. Failing that please rename it to
g_file_logger_is_initialized.

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:65: unsigned int
kBroadcastTimeoutMilliseconds = 2 * 1000;
const?

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/te...
File chrome/test/base/test_log_collector_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/te...
chrome/test/base/test_log_collector_win.cc:10: #include <ios>  // For std::hex
std::hex does not appear in this file.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
File chrome/test/base/log_file_reader_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.h:60: intptr_t id,
intptr_t seems like an odd type for an ID.

[sky, 2012-03-09 17:17:45]

Pawel, will you have time to review this?

  -Scott

[grt (UTC plus 2), 2012-03-09 17:55:25]

Thanks for the comments.  PTAL.

Pawel et al.:

For an idea of what this brings us, see the output for
FullTabDownloadTest.CF_DownloadFileFromPost in
http://build.chromium.org/p/tryserver.chromium/builders/win_cf/builds/90/step....
 This has helped a lot in tracking down flaky Chrome Frame tests.  I have some
other test enhancements that will be coming in future CLs (like the in-line
stack dump you see in that same log file).

https://chromiumcodereview.appspot.com/9584017/diff/26016/base/win/event_trac...
File base/win/event_trace_provider.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/base/win/event_trac...
base/win/event_trace_provider.cc:89: DisableEvents();
On 2012/03/09 16:58:04, robertshield wrote:
> if session_handle_ is null, the level and flags no longer get reset, is this
> intended?

I don't see a way for session_handle_ to be NULL and the flags/level to not be
zero.  That aside, if session_handle_ is NULL, the level and flags are
meaningless.

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:25: bool g_is_initialized = false;
On 2012/03/09 16:58:04, robertshield wrote:
> Consider making this a static member of FileLogger, it makes the symbol
slightly
> easier to notice and inspect in the debugger. Failing that please rename it to
> g_file_logger_is_initialized.

Done.

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:65: unsigned int
kBroadcastTimeoutMilliseconds = 2 * 1000;
On 2012/03/09 16:58:04, robertshield wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/te...
File chrome/test/base/test_log_collector_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26016/chrome/test/base/te...
chrome/test/base/test_log_collector_win.cc:10: #include <ios>  // For std::hex
On 2012/03/09 16:58:04, robertshield wrote:
> std::hex does not appear in this file.

I never said it did.  :-)  The C++ spec says that you include <ios> to get
std::hex.  Whether the implementation of the spec puts it in some other file
included by that or not is up to them.

In any case, I've removed the comment since it seems to have added rather than
removed confusion.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
File chrome/test/base/log_file_reader_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.h:60: intptr_t id,
On 2012/03/09 16:58:04, robertshield wrote:
> intptr_t seems like an odd type for an ID.

Indeed.  a void* is used in the creation of a trace event.  Reading this value
as a pointer sometime later doesn't make sense to me, whereas treating it as a
pointer-sized int does.  WDYT?

[Paweł Hajdan Jr., 2012-03-09 18:12:11]

Please put this nice new thing in chrome/test/logging (feel free to add yourself
or anyone else to OWNERS).

I can't read Windows-specific code very well, so just a few comments.

Otherwise anything that helps fix problems looks good.

That said, LGTM.

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:63: const unsigned int
kBroadcastTimeoutMilliseconds = 2 * 1000;
Why not base/test/test_timeouts?

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/mo...
File chrome/test/base/mof_data_parser_test_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/mo...
chrome/test/base/mof_data_parser_test_win.cc:12: class MofDataParserTest :
public ::testing::Test {
nit: Could you just rename the file to say unittest instead of test?

[erikwright (departed), 2012-03-09 18:25:50]

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:88: // Set the value for the whole system
and ask everyone to refresh.
I wouldn't mind seeing the functionality of modifying the system environment
turned into a separate scoped object (ScopedSystemEnvironmentVariable?).

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:221: } else {
You're ignoring the result of EnableProviders. Mightn't the caller want to know
about a failure there? Should you StopLogging if that happens?

if (!SUCCEEDED(hr)) {
  ...
  return false;
}

if (!EnableProviders()) {
  StopLogging();
  return false;
}

return true;

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I think the _win suffix most commonly applies to the windows version of a class
that is differently defined on multiple platforms, whereas a 'win' subdirectory
is more commonly used to separate windows-only classes from generic code.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.h:34: class FileLogger {
I think the name might be a little vague. Is it not likely to be confused with
other logging tools? Maybe ChromeFileLogger? ChromeETWFileLogger?

[willchan no longer on Chromium, 2012-03-09 18:37:51]

base/ LGTM, although I don't fully understand the ETW fix. Please make sure
another one of you Windows folks reviews that if no one has already. Sending my
LGTM now so I don't block you.

[erikwright (departed), 2012-03-09 18:50:33]

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
File chrome/test/base/log_file_reader_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.cc:154: if (parser.ReadDWORD(&stack_depth)
&&
Presumably this maps to some writing code somewhere else. Can you add a
reference?

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.cc:176: if (parser.ReadString(&name) &&
Ditto.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/mo...
File chrome/test/base/mof_data_parser_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/mo...
chrome/test/base/mof_data_parser_win.h:59: bool ReadString(base::StringPiece*
value);
I don't understand the logic of stripping \n\0 if there is a \n, but leaving the
\0 if there is not. It strikes me as easier to use if it always strips the \0.

[grt (UTC plus 2), 2012-03-09 20:38:57]

PTAL y'all.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:88: // Set the value for the whole system
and ask everyone to refresh.
On 2012/03/09 18:25:50, erikwright wrote:
> I wouldn't mind seeing the functionality of modifying the system environment
> turned into a separate scoped object (ScopedSystemEnvironmentVariable?).

Good idea.  Done.  I also followed a previous suggestion of yours and got rid of
Uninitialize.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:221: } else {
On 2012/03/09 18:25:50, erikwright wrote:
> You're ignoring the result of EnableProviders. Mightn't the caller want to
know
> about a failure there? Should you StopLogging if that happens?

This was intentional.  If the session can't be established at all, then it makes
sense to return false from here ('cause there'd be no logging in that case).  On
the other hand, it becomes ambiguous as to what the "right thing" is when one of
four providers can't be enabled.  I've chosen to log the fact that the provider
couldn't be enabled, but not consider that sufficiently severe to make
StartLogging fail.  I've just changed it so that StartLogging returns false if
none of the providers could be enabled.  WDYT?

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
File chrome/test/base/file_logger_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/03/09 18:25:50, erikwright wrote:
> I think the _win suffix most commonly applies to the windows version of a
class
> that is differently defined on multiple platforms, whereas a 'win'
subdirectory
> is more commonly used to separate windows-only classes from generic code.

All files are now in chrome/test/logging_win, and the public stuff is now all in
the logging_win namespace.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/fi...
chrome/test/base/file_logger_win.h:34: class FileLogger {
On 2012/03/09 18:25:50, erikwright wrote:
> I think the name might be a little vague. Is it not likely to be confused with
> other logging tools? Maybe ChromeFileLogger? ChromeETWFileLogger?

What do you think of LogFileWriter, which is consistent with log_file_reader and
log_file_printer?  It's now in the logging_win namespace, so it's unlikely to
collide with other modules.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
File chrome/test/base/log_file_reader_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.cc:154: if (parser.ReadDWORD(&stack_depth)
&&
On 2012/03/09 18:50:33, erikwright wrote:
> Presumably this maps to some writing code somewhere else. Can you add a
> reference?

Done.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/lo...
chrome/test/base/log_file_reader_win.cc:176: if (parser.ReadString(&name) &&
On 2012/03/09 18:50:33, erikwright wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/mo...
File chrome/test/base/mof_data_parser_win.h (right):

https://chromiumcodereview.appspot.com/9584017/diff/26019/chrome/test/base/mo...
chrome/test/base/mof_data_parser_win.h:59: bool ReadString(base::StringPiece*
value);
On 2012/03/09 18:50:33, erikwright wrote:
> I don't understand the logic of stripping \n\0 if there is a \n, but leaving
the
> \0 if there is not. It strikes me as easier to use if it always strips the \0.

Sorry, the comment is confusing.  The encoded string must always be
null-terminated.  The StringPiece returned never includes a terminating null. 
It does what you expect it to do.  Is this more clear:

  // Sets |value| such that it points to the string in the data at the current
  // position.  A trailing newline, if present, is not included in the returned
  // piece.  The returned piece is not null-terminated.

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/fi...
File chrome/test/base/file_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/fi...
chrome/test/base/file_logger_win.cc:63: const unsigned int
kBroadcastTimeoutMilliseconds = 2 * 1000;
On 2012/03/09 18:12:12, Paweł Hajdan Jr. wrote:
> Why not base/test/test_timeouts?

The closest timeout that's already defined there (action_timeout) is 10 seconds,
which would be a really long time for this.  It's common for this timeout to be
reached on a regular basis, since all it takes is for a window to not respond to
the WM_SETTINGCHANGE message.  Adding a new member to TestTimeouts for this
seems overkill.

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/mo...
File chrome/test/base/mof_data_parser_test_win.cc (right):

https://chromiumcodereview.appspot.com/9584017/diff/29003/chrome/test/base/mo...
chrome/test/base/mof_data_parser_test_win.cc:12: class MofDataParserTest :
public ::testing::Test {
On 2012/03/09 18:12:12, Paweł Hajdan Jr. wrote:
> nit: Could you just rename the file to say unittest instead of test?

Done.

[robertshield, 2012-03-11 05:06:43]

lgtm

[commit-bot: I haz the power, 2012-03-12 15:50:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/9584017/31004

[commit-bot: I haz the power, 2012-03-12 16:14:55]

Try job failure for 9584017-31004 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-12 17:40:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/9584017/34002

[commit-bot: I haz the power, 2012-03-12 21:42:13]

Change committed as 126240