Moved exception_handler_test to the more aptly named exception_handler_death_...

Moved exception_handler_test to the more aptly named exception_handler_death_test. It doesn't test anything else than death and exit.
Created the exception_handler_test that test the generation of dump and the dumps themselves.
Moved all dump analysis code from minidump to its right class DumpAnalysis. The class is used by both minidump_test and exception_handler_test. The tests are way simpler that way (ie. no handling of HANDLE).
minidump_test now uses the minidump_generator class instead of using Win32. It works well and pass all tests.
exception_handler now passes both the exception and assertion infos to the client to generate the dump. If one is NULL it's going to be handled correctly.
crash_generation_client can now RequestDump with both exception and assertion info.
minidump_generator returns both the mini and full dump string pointers, and output both (or either) depending on which was generated.
All original interfaces and method signature are still there, but call the new functions if possible.


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

[hansl_g, 2010-05-11 20:47:48]

Please review this. I think I wrote all necessary information to describe this
change, but it's pretty big.

Thanks

[Sigurður Ásgeirsson, 2010-05-11 21:33:41]

Awesome, I have but a couple of nits.

http://codereview.chromium.org/1994015/diff/1/2
File src/client/windows/crash_generation/crash_generation_client.cc (right):

http://codereview.chromium.org/1994015/diff/1/2#newcode292
src/client/windows/crash_generation/crash_generation_client.cc:292: bool
CrashGenerationClient::RequestDump(EXCEPTION_POINTERS* ex_info) {
nice!

http://codereview.chromium.org/1994015/diff/1/3
File src/client/windows/crash_generation/crash_generation_client.h (right):

http://codereview.chromium.org/1994015/diff/1/3#newcode30
src/client/windows/crash_generation/crash_generation_client.h:30: #ifndef
SRC_CLIENT_WINDOWS_CRASH_GENERATION_CRASH_GENERATION_CLIENT_H_
SRC is not usually prefixed?

http://codereview.chromium.org/1994015/diff/1/7
File src/client/windows/unittests/client_tests.gyp (right):

http://codereview.chromium.org/1994015/diff/1/7#newcode48
src/client/windows/unittests/client_tests.gyp:48:
'../crash_generation/crash_generation.gyp:*',
For executable, dll and lib targets, you shouldn't do wildcard deps, because
that breaks the principle of least surprise.
Only "none" type targets should IMHO ever have wildcard deps.

http://codereview.chromium.org/1994015/diff/1/8
File src/client/windows/unittests/dump_analysis.cc (right):

http://codereview.chromium.org/1994015/diff/1/8#newcode113
src/client/windows/unittests/dump_analysis.cc:113: //   void* stream = NULL;
dangling code, or maybe could be a GestStreamImpl invocation now?

http://codereview.chromium.org/1994015/diff/1/9
File src/client/windows/unittests/dump_analysis.h (right):

http://codereview.chromium.org/1994015/diff/1/9#newcode90
src/client/windows/unittests/dump_analysis.h:90: void* dump_file_handle_;
HANDLE dump_file_handle_;  - even though HANDLE is (sometimes) a void*.

http://codereview.chromium.org/1994015/diff/1/11
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/1/11#newcode39
src/client/windows/unittests/exception_handler_test.cc:39: #include
"./dump_analysis.h"
./?

http://codereview.chromium.org/1994015/diff/1/11#newcode54
src/client/windows/unittests/exception_handler_test.cc:54: static std::wstring
dump_file;
make these  static members of ExceptionHandlerTest?

http://codereview.chromium.org/1994015/diff/1/11#newcode112
src/client/windows/unittests/exception_handler_test.cc:112: void
clientDumpCallback(void *dump_context,
make this a static class method in ExceptionHandler test?

http://codereview.chromium.org/1994015/diff/1/11#newcode121
src/client/windows/unittests/exception_handler_test.cc:121: using
google_breakpad::ExceptionHandler;
unused using decl?

http://codereview.chromium.org/1994015/diff/1/11#newcode141
src/client/windows/unittests/exception_handler_test.cc:141: using
google_breakpad::ExceptionHandler;
ditto?

http://codereview.chromium.org/1994015/diff/1/11#newcode168
src/client/windows/unittests/exception_handler_test.cc:168:
full.HasStream(ThreadListStream));
I don't follow the || logic - both dumps should have a thread list, module list,
etc?
You'd want &&, or simply assert on the dumps individually?

[hansl_g, 2010-05-12 16:35:07]

There you go. Happy review!

http://codereview.chromium.org/1994015/diff/1/3
File src/client/windows/crash_generation/crash_generation_client.h (right):

http://codereview.chromium.org/1994015/diff/1/3#newcode30
src/client/windows/crash_generation/crash_generation_client.h:30: #ifndef
SRC_CLIENT_WINDOWS_CRASH_GENERATION_CRASH_GENERATION_CLIENT_H_
On 2010/05/11 21:33:41, Ruðrugis wrote:
> SRC is not usually prefixed?

Done. Although this comes from a lint comment I applied without much thoughts.

http://codereview.chromium.org/1994015/diff/1/7
File src/client/windows/unittests/client_tests.gyp (right):

http://codereview.chromium.org/1994015/diff/1/7#newcode48
src/client/windows/unittests/client_tests.gyp:48:
'../crash_generation/crash_generation.gyp:*',
On 2010/05/11 21:33:41, Ruðrugis wrote:
> For executable, dll and lib targets, you shouldn't do wildcard deps, because
> that breaks the principle of least surprise.
> Only "none" type targets should IMHO ever have wildcard deps.

Done.

http://codereview.chromium.org/1994015/diff/1/8
File src/client/windows/unittests/dump_analysis.cc (right):

http://codereview.chromium.org/1994015/diff/1/8#newcode113
src/client/windows/unittests/dump_analysis.cc:113: //   void* stream = NULL;
On 2010/05/11 21:33:41, Ruðrugis wrote:
> dangling code, or maybe could be a GestStreamImpl invocation now?

Done. This was dangling code that has been replaced by the original version, but
kept for further testing. It has been reinstated.

http://codereview.chromium.org/1994015/diff/1/9
File src/client/windows/unittests/dump_analysis.h (right):

http://codereview.chromium.org/1994015/diff/1/9#newcode90
src/client/windows/unittests/dump_analysis.h:90: void* dump_file_handle_;
On 2010/05/11 21:33:41, Ruðrugis wrote:
> HANDLE dump_file_handle_;  - even though HANDLE is (sometimes) a void*.

Done.

http://codereview.chromium.org/1994015/diff/1/11
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/1/11#newcode39
src/client/windows/unittests/exception_handler_test.cc:39: #include
"./dump_analysis.h"
On 2010/05/11 21:33:41, Ruðrugis wrote:
> ./?

Lint complained that local includes didn't use a . at the start. Should I use
"../unittests/dump_analysis.h" instead, leave it like that or ignore lint
warnings?

http://codereview.chromium.org/1994015/diff/1/11#newcode54
src/client/windows/unittests/exception_handler_test.cc:54: static std::wstring
dump_file;
On 2010/05/11 21:33:41, Ruðrugis wrote:
> make these  static members of ExceptionHandlerTest?
> 

Done.

http://codereview.chromium.org/1994015/diff/1/11#newcode112
src/client/windows/unittests/exception_handler_test.cc:112: void
clientDumpCallback(void *dump_context,
On 2010/05/11 21:33:41, Ruðrugis wrote:
> make this a static class method in ExceptionHandler test?

Done.

http://codereview.chromium.org/1994015/diff/1/11#newcode121
src/client/windows/unittests/exception_handler_test.cc:121: using
google_breakpad::ExceptionHandler;
On 2010/05/11 21:33:41, Ruðrugis wrote:
> unused using decl?

Done. Wasn't used or needed.

http://codereview.chromium.org/1994015/diff/1/11#newcode141
src/client/windows/unittests/exception_handler_test.cc:141: using
google_breakpad::ExceptionHandler;
On 2010/05/11 21:33:41, Ruðrugis wrote:
> ditto?

Done.

http://codereview.chromium.org/1994015/diff/1/11#newcode168
src/client/windows/unittests/exception_handler_test.cc:168:
full.HasStream(ThreadListStream));
On 2010/05/11 21:33:41, Ruðrugis wrote:
> I don't follow the || logic - both dumps should have a thread list, module
list,
> etc?
> You'd want &&, or simply assert on the dumps individually?

Done. I've also made this change for minidump_test.cc

[Sigurður Ásgeirsson, 2010-05-12 17:09:12]

Sweet, still a couple of minor nits...

http://codereview.chromium.org/1994015/diff/1/11
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/1/11#newcode39
src/client/windows/unittests/exception_handler_test.cc:39: #include
"./dump_analysis.h"
On 2010/05/12 16:35:07, hansl wrote:
> On 2010/05/11 21:33:41, Ruðrugis wrote:
> > ./?
> 
> Lint complained that local includes didn't use a . at the start. Should I use
> "../unittests/dump_analysis.h" instead, leave it like that or ignore lint
> warnings?

GRRRR - one way to go is to suffix. //  NOLINT
The relative includes don't fit with Chrome coding style very well, where you
alway prefix the file path, but I guess we leave them as-is as this appears to
be the local style.

http://codereview.chromium.org/1994015/diff/14001/5002
File src/client/windows/crash_generation/crash_generation_client.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5002#newcode293
src/client/windows/crash_generation/crash_generation_client.cc:293: return
this->RequestDump(ex_info, NULL);
actually, now that I look at this, we don't this-> prefix self-invocations :).

http://codereview.chromium.org/1994015/diff/14001/5011
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5011#newcode52
src/client/windows/unittests/exception_handler_test.cc:52: static std::wstring
full_dump_file;
These are still not class members, did you forget to upload?

http://codereview.chromium.org/1994015/diff/14001/5011#newcode72
src/client/windows/unittests/exception_handler_test.cc:72: static void
ExceptionHandlerTest::clientDumpCallback(
Remove the class name, and the callback should be UpperCaseCamels

[Sigurður Ásgeirsson, 2010-05-12 17:09:12]

Sweet, still a couple of minor nits...

http://codereview.chromium.org/1994015/diff/14001/5002
File src/client/windows/crash_generation/crash_generation_client.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5002#newcode293
src/client/windows/crash_generation/crash_generation_client.cc:293: return
this->RequestDump(ex_info, NULL);
actually, now that I look at this, we don't this-> prefix self-invocations :).

http://codereview.chromium.org/1994015/diff/14001/5011
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5011#newcode52
src/client/windows/unittests/exception_handler_test.cc:52: static std::wstring
full_dump_file;
These are still not class members, did you forget to upload?

http://codereview.chromium.org/1994015/diff/14001/5011#newcode72
src/client/windows/unittests/exception_handler_test.cc:72: static void
ExceptionHandlerTest::clientDumpCallback(
Remove the class name, and the callback should be UpperCaseCamels

[hansl_g, 2010-05-12 17:16:08]

All done. I've removed the "./" path and put a NOLINT. Looks better that way.

There is the class prefix (exception_handler_test.cc:52) that I don't get why it
compiled, but it's one of VC++ mysteries I guess :P

Here goes.

http://codereview.chromium.org/1994015/diff/14001/5002
File src/client/windows/crash_generation/crash_generation_client.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5002#newcode293
src/client/windows/crash_generation/crash_generation_client.cc:293: return
this->RequestDump(ex_info, NULL);
On 2010/05/12 17:09:12, Ruðrugis wrote:
> actually, now that I look at this, we don't this-> prefix self-invocations :).

Done.

http://codereview.chromium.org/1994015/diff/14001/5011
File src/client/windows/unittests/exception_handler_test.cc (right):

http://codereview.chromium.org/1994015/diff/14001/5011#newcode52
src/client/windows/unittests/exception_handler_test.cc:52: static std::wstring
full_dump_file;
On 2010/05/12 17:09:12, Ruðrugis wrote:
> These are still not class members, did you forget to upload?

Done.

http://codereview.chromium.org/1994015/diff/14001/5011#newcode72
src/client/windows/unittests/exception_handler_test.cc:72: static void
ExceptionHandlerTest::clientDumpCallback(
On 2010/05/12 17:09:12, Ruðrugis wrote:
> Remove the class name, and the callback should be UpperCaseCamels 

Done.

[Sigurður Ásgeirsson, 2010-05-12 17:27:20]

lgtm with two minor nits, thanks!

http://codereview.chromium.org/1994015/diff/12002/28007
File src/client/windows/unittests/dump_analysis.cc (right):

http://codereview.chromium.org/1994015/diff/12002/28007#newcode34
src/client/windows/unittests/dump_analysis.cc:34: #include "dump_analysis.h"  //
 NOLINT
nit: only one space after //

http://codereview.chromium.org/1994015/diff/12002/28011
File src/client/windows/unittests/minidump_test.cc (right):

http://codereview.chromium.org/1994015/diff/12002/28011#newcode35
src/client/windows/unittests/minidump_test.cc:35: #include "dump_analysis.h"  //
 NOLINT
nit: only one space after //