Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2835913002: Added crashpad support for Windows (Closed)

Created:
3 years, 8 months ago by dvallet
Modified:
3 years, 7 months ago
Reviewers:
Eric Seckler, Sami
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Added crashpad support for Windows This mimics behavior with Mac and Linux, with --enable-crash-reporter --crash-dumps-dir in headless_shell and chrome --headless Note that browsertests cannot be enabled as they are since in Windows the crash handler is forked from the Headless Shell. BUG=686608 Review-Url: https://codereview.chromium.org/2835913002 Cr-Commit-Position: refs/heads/master@{#472017} Committed: https://chromium.googlesource.com/chromium/src/+/266c2738a4fb94018caba538e308c60a44cb9646

Patch Set 1 #

Total comments: 2

Patch Set 2 : Avoid adding crashpad dependencies in chrome.dll and chrome_child.dll #

Patch Set 3 : Remove unnecessary dependency #

Patch Set 4 : fix for windows component build #

Patch Set 5 : use CHROME_MULTIPLE_DLL instead #

Total comments: 2

Patch Set 6 : Added comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -9 lines) Patch
M headless/BUILD.gn View 1 2 3 2 chunks +7 lines, -6 lines 0 comments Download
M headless/app/headless_shell.cc View 1 2 3 2 chunks +11 lines, -0 lines 0 comments Download
M headless/lib/headless_content_main_delegate.cc View 1 2 3 4 5 2 chunks +8 lines, -3 lines 0 comments Download

Messages

Total messages: 32 (21 generated)
Eric Seckler
lgtm https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc#newcode525 headless/app/headless_shell.cc:525: RunChildProcessIfNeeded(argc, argv); I'd have expected a call to ...
3 years, 8 months ago (2017-04-24 09:06:15 UTC) #4
Sami
lgtm https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc#newcode525 headless/app/headless_shell.cc:525: RunChildProcessIfNeeded(argc, argv); On 2017/04/24 09:06:14, Eric Seckler wrote: ...
3 years, 8 months ago (2017-04-24 19:04:28 UTC) #7
Eric Seckler
On 2017/04/24 19:04:28, Sami wrote: > lgtm > > https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc > File headless/app/headless_shell.cc (right): > ...
3 years, 8 months ago (2017-04-25 07:27:23 UTC) #8
dvallet
PTAL. I had to modify this so that dependencies are not added to chrome.dll and ...
3 years, 7 months ago (2017-05-15 07:19:49 UTC) #17
Eric Seckler
lgtm https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_content_main_delegate.cc File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_content_main_delegate.cc#newcode183 headless/lib/headless_content_main_delegate.cc:183: #elif defined(OS_WIN) && !defined(CHROME_MULTIPLE_DLL) nit: add a comment ...
3 years, 7 months ago (2017-05-15 08:47:03 UTC) #20
Sami
lgtm -- good point about adding a comment.
3 years, 7 months ago (2017-05-15 16:58:11 UTC) #21
dvallet
Thanks for the review! https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_content_main_delegate.cc File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_content_main_delegate.cc#newcode183 headless/lib/headless_content_main_delegate.cc:183: #elif defined(OS_WIN) && !defined(CHROME_MULTIPLE_DLL) On ...
3 years, 7 months ago (2017-05-16 01:22:59 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2835913002/100001
3 years, 7 months ago (2017-05-16 01:24:23 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/178897) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 7 months ago (2017-05-16 01:47:51 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2835913002/100001
3 years, 7 months ago (2017-05-16 02:55:52 UTC) #29
commit-bot: I haz the power
3 years, 7 months ago (2017-05-16 05:03:37 UTC) #32
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/266c2738a4fb94018caba538e308...

Powered by Google App Engine
This is Rietveld 408576698