[webkit-reviews] review denied: [Bug 122319] WinLauncher needs crash report : [Attachment 213336] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Alex Christensen
<alex.christensen at flexsim.com>'s request for review:
Bug 122319: WinLauncher needs crash report
https://bugs.webkit.org/show_bug.cgi?id=122319

Attachment 213336: Patch
https://bugs.webkit.org/attachment.cgi?id=213336&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213336&action=review


This looks great -- but I have a few questions/suggestions before we can land
this.

> Tools/WinLauncher/WinLauncher.cpp:180
> +#ifdef _DEBUG

I don't understand this change. Why don't we want to see why the load failed in
Release mode? If you don't you will be puzzled by the browser window not
displaying any content when you enter a new URL.

> Tools/WinLauncher/WinLauncher.cpp:416
> +    HANDLE miniDumpFile = ::CreateFile(L"CrashReport.dmp", GENERIC_WRITE, 0,
0, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);

You should create this crash report in some definite user-writable location. 
See the documentation of SHGetFolderPath
(http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181(v=vs.85).aspx
) -- I think you probably want to put these files in a sub-folder of
CSIDL_LOCAL_APPDATA.  Then input THAT string into this ::CreateFile function.

> Tools/WinLauncher/WinLauncher.cpp:433
> +	   processCrashReport();

So will this be dealt with in a future bug? Or is it left for WinLauncher
"users" to flesh out in their own releases.  If it's the latter, I think you
should add a comment about this in the "WinLauncherReplace.h" header.