Port CrashReportUploadThread to Windows

Port CrashReportUploadThread to Windows

Just a simple port now that we have a common Thread class.
Compiled but not yet in use on Windows.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/86419cf7881b37fb9fd21b216102de9479d855fe

[Mark Mentovai, 2015-08-18 21:38:12]

Cool! LGTM

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
File handler/crash_report_upload_thread.cc (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:140: class HelperThread : public Thread {
final (useful as a hint since this isn’t used outside of this translation unit,
but the compiler doesn’t know that.)

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:142: HelperThread(CrashReportUploadThread*
self) : self_(self) {}
explicit

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:151: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
File handler/crash_report_upload_thread.h (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.h:28: class HelperThread;
Since this is just in namespace crashpad, you should probably name it
CrashReportUploadHelperThread so that it’s a little more obvious. Bonus points
for putting it in crashpad::internal.

https://codereview.chromium.org/1295363002/diff/40001/handler/handler.gyp
File handler/handler.gyp (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/handler.gyp#new...
handler/handler.gyp:79: '../compat/compat.gyp:crashpad_compat',
snapshot too

[scottmg, 2015-08-18 22:21:41]

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
File handler/crash_report_upload_thread.cc (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:140: class HelperThread : public Thread {
On 2015/08/18 21:38:11, Mark Mentovai - out til August wrote:
> final (useful as a hint since this isn’t used outside of this translation
unit,
> but the compiler doesn’t know that.)

Done.

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:142: HelperThread(CrashReportUploadThread*
self) : self_(self) {}
On 2015/08/18 21:38:12, Mark Mentovai - out til August wrote:
> explicit

Done.

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.cc:151: };
On 2015/08/18 21:38:11, Mark Mentovai - out til August wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
File handler/crash_report_upload_thread.h (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/crash_report_up...
handler/crash_report_upload_thread.h:28: class HelperThread;
On 2015/08/18 21:38:12, Mark Mentovai - out til August wrote:
> Since this is just in namespace crashpad, you should probably name it
> CrashReportUploadHelperThread so that it’s a little more obvious. Bonus points
> for putting it in crashpad::internal.

Done.

https://codereview.chromium.org/1295363002/diff/40001/handler/handler.gyp
File handler/handler.gyp (right):

https://codereview.chromium.org/1295363002/diff/40001/handler/handler.gyp#new...
handler/handler.gyp:79: '../compat/compat.gyp:crashpad_compat',
On 2015/08/18 21:38:12, Mark Mentovai - out til August wrote:
> snapshot too

Done.

[scottmg, 2015-08-18 22:34:16]

Committed patchset #4 (id:60001) manually as
86419cf7881b37fb9fd21b216102de9479d855fe (presubmit successful).

[mihnea_arkaos.net, 2015-08-19 13:02:48]

On Wednesday, August 19, 2015 at 1:34:16 AM UTC+3, scottmg@chromium.org via
codereview.chromium.org wrote:
> Committed patchset #4 (id:60001) manually as
> 86419cf7881b37fb9fd21b216102de9479d855fe (presubmit successful).
> 
> https://codereview.chromium.org/1295363002/

Hi Scott,

Quick feedback from the trenches: the upload thread is not started.
One liner fix for anyone else syncing to this commit:

---
 handler/crash_report_upload_thread.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/handler/crash_report_upload_thread.cc
b/handler/crash_report_upload_thread.cc
index f5acb07..afc7726 100644
--- a/handler/crash_report_upload_thread.cc
+++ b/handler/crash_report_upload_thread.cc
@@ -182,6 +182,7 @@ void CrashReportUploadThread::Start() {
 
   running_ = true;
   thread_.reset(new internal::CrashReportUploadHelperThread(this));
+  thread_->Start();
 }


Cheers,
Mihnea


 

[scottmg, 2015-08-19 16:33:48]

On 2015/08/19 13:02:48, mihnea_arkaos.net wrote:
> On Wednesday, August 19, 2015 at 1:34:16 AM UTC+3, mailto:scottmg@chromium.org
via
> http://codereview.chromium.org wrote:
> > Committed patchset #4 (id:60001) manually as
> > 86419cf7881b37fb9fd21b216102de9479d855fe (presubmit successful).
> > 
> > https://codereview.chromium.org/1295363002/
> 
> Hi Scott,
> 
> Quick feedback from the trenches: the upload thread is not started.
> One liner fix for anyone else syncing to this commit:
> 
> ---
>  handler/crash_report_upload_thread.cc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/handler/crash_report_upload_thread.cc
> b/handler/crash_report_upload_thread.cc
> index f5acb07..afc7726 100644
> --- a/handler/crash_report_upload_thread.cc
> +++ b/handler/crash_report_upload_thread.cc
> @@ -182,6 +182,7 @@ void CrashReportUploadThread::Start() {
>  
>    running_ = true;
>    thread_.reset(new internal::CrashReportUploadHelperThread(this));
> +  thread_->Start();
>  }
> 
> 
> Cheers,
> Mihnea
> 
> 
>  

Thanks Mihnea, sorry about that. I uploaded the patch here
https://codereview.chromium.org/1286383006/.