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

Issue 904213002: [components/crash] Add crash client method to enable microdumps (Closed)

Created:
5 years, 10 months ago by Primiano Tucci (use gerrit)
Modified:
5 years, 10 months ago
CC:
chromium-reviews, kalyank, Mark Mentovai, sadrul, Torne
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[components/crash] Add crash client method to enable microdumps Background context: - This CL is about breakpad *micro*dumps, not to be confused with minidumps (see crbug.com/410294 for details and design doc). - Breakpad microdumps are, before and after this CL, enabled only for Android. - At present state, the logic that decides whether microdumps should be enabled or not is hardcoded in breakpad_linux.cc. Such logic today enables breakpad only when unwind tables are stripped out. So far so good as it was the only intended use case. New use case which justifies this CL: At this stage of the project, Android WebView is also going to get microdumps. Conversely to what happens in Chrome, however, WebView needs microdumps to be always enabled. The rationale of this choice is: WebView can not take advantage of the regular minidump uploader and microdumps are going to represent the only form of crash reporting. From the crash/ code perspective, this invalidates the assumption that microdumps can just be enabled when NO_UNWIND_TABLES == 1 and requires the introduction of a more customizable logic. Description of the change: This CL is a minimal refactoring required to introduce such logic. It adds a ShouldEnableBreakpadMicrodumps method in CrashReporterClient and moves the NO_UNWIND_TABLES logic around. The overall behavior of "when are microdumps enabled" is unchanged after this CL. BUG=410294, 456494 Committed: https://crrev.com/61b6a6a609434dc42082e5f59278eca17b2d9417 Cr-Commit-Position: refs/heads/master@{#315328}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -7 lines) Patch
M components/crash/app/breakpad_linux.cc View 1 chunk +5 lines, -7 lines 0 comments Download
M components/crash/app/crash_reporter_client.h View 1 chunk +4 lines, -0 lines 0 comments Download
M components/crash/app/crash_reporter_client.cc View 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
Primiano Tucci (use gerrit)
jochen, can I ask you a review on this very small refactoring? This is required ...
5 years, 10 months ago (2015-02-07 14:53:29 UTC) #2
jochen (gone - plz use gerrit)
lgtm
5 years, 10 months ago (2015-02-09 14:20:29 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/904213002/1
5 years, 10 months ago (2015-02-09 16:30:18 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 10 months ago (2015-02-09 17:17:38 UTC) #6
commit-bot: I haz the power
5 years, 10 months ago (2015-02-09 17:18:28 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/61b6a6a609434dc42082e5f59278eca17b2d9417
Cr-Commit-Position: refs/heads/master@{#315328}

Powered by Google App Engine
This is Rietveld 408576698