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

Issue 11776040: Add Windows implementation for base/debug/crash_logging.h. (Closed)

Created:
7 years, 11 months ago by Robert Sesek
Modified:
7 years, 11 months ago
CC:
chromium-reviews, Scott Hess - ex-Googler, raymes, eroman
Visibility:
Public.

Description

Add Windows implementation for base/debug/crash_logging.h. BUG=77656, 137062 TEST=Official build still reports crash keys. TBR=thakis@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178572

Patch Set 1 #

Patch Set 2 : Ready for review #

Total comments: 3

Patch Set 3 : Range check #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -1 line) Patch
M chrome/app/breakpad_win.cc View 1 2 6 chunks +46 lines, -1 line 3 comments Download
M chrome/chrome_exe.gypi View 1 2 chunks +3 lines, -0 lines 1 comment Download

Messages

Total messages: 18 (0 generated)
Robert Sesek
7 years, 11 months ago (2013-01-16 01:16:25 UTC) #1
cpu_(ooo_6.6-7.5)
note https://code.google.com/p/chromium/issues/detail?id=137062
7 years, 11 months ago (2013-01-16 21:25:27 UTC) #2
cpu_(ooo_6.6-7.5)
first off, any reason to keep the old way?
7 years, 11 months ago (2013-01-16 21:26:09 UTC) #3
Robert Sesek
On 2013/01/16 21:25:27, cpu wrote: > note > https://code.google.com/p/chromium/issues/detail?id=137062 In the BUG= line already :) ...
7 years, 11 months ago (2013-01-16 21:28:05 UTC) #4
Scott Hess - ex-Googler
Just FYI review. https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc File chrome/app/breakpad_win.cc (right): https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc#newcode111 chrome/app/breakpad_win.cc:111: DynamicEntriesMap* g_dynamic_entries = NULL; I went ...
7 years, 11 months ago (2013-01-16 22:45:28 UTC) #5
Robert Sesek
https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc File chrome/app/breakpad_win.cc (right): https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc#newcode690 chrome/app/breakpad_win.cc:690: entry = &(*g_custom_entries)[g_dynamic_keys_offset++]; On 2013/01/16 22:45:28, shess wrote: > ...
7 years, 11 months ago (2013-01-17 19:39:10 UTC) #6
Robert Sesek
cpu: ping
7 years, 11 months ago (2013-01-18 17:01:47 UTC) #7
cpu_(ooo_6.6-7.5)
I swear I made a comment yesterday, it got lost or user error. Anyhow, this ...
7 years, 11 months ago (2013-01-19 02:57:42 UTC) #8
cpu_(ooo_6.6-7.5)
.
7 years, 11 months ago (2013-01-19 02:58:09 UTC) #9
Robert Sesek
*ping*
7 years, 11 months ago (2013-01-23 16:54:01 UTC) #10
Robert Sesek
eroman & raymes: PING
7 years, 11 months ago (2013-01-23 22:57:01 UTC) #11
eroman
How do these changes relate to base::debug::SetCrashKeyValue() https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc File chrome/app/breakpad_win.cc (right): https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc#newcode690 chrome/app/breakpad_win.cc:690: if (it ...
7 years, 11 months ago (2013-01-24 00:47:13 UTC) #12
Robert Sesek
On 2013/01/24 00:47:13, eroman wrote: > How do these changes relate to base::debug::SetCrashKeyValue() This is ...
7 years, 11 months ago (2013-01-24 01:03:45 UTC) #13
eroman
lgtm then
7 years, 11 months ago (2013-01-24 01:26:58 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/11776040/4003
7 years, 11 months ago (2013-01-24 01:32:36 UTC) #15
commit-bot: I haz the power
Presubmit check for 11776040-4003 failed and returned exit status 1. Running presubmit commit checks ...
7 years, 11 months ago (2013-01-24 01:32:41 UTC) #16
Robert Sesek
+thakis for OWNERS
7 years, 11 months ago (2013-01-24 01:33:00 UTC) #17
Nico
7 years, 11 months ago (2013-01-24 18:01:14 UTC) #18
Message was sent while issue was closed.
I should honor the spirit of this review and not respond until I've been pinged
three times.

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:111: DynamicEntriesMap* g_dynamic_entries = NULL;
nit: Why is this not static too?

(I realize it's in an unnamed namespace, but so is the rest.)

https://codereview.chromium.org/11776040/diff/4003/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/chrome_exe.gypi#new...
chrome/chrome_exe.gypi:478: 'common/crash_keys.h',
Should these be in the sources section at the top and then be removed on non-win
platforms? That way they would show up in IDEs on other platforms, and it's what
we usually do I think.

Powered by Google App Engine
This is Rietveld 408576698