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

Issue 2031833002: Remove FilePath usage from the CrashReporterClient interface on Windows. (Closed)

Created:
4 years, 6 months ago by ananta
Modified:
4 years, 6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, sadrul, kalyank
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove FilePath usage from the CrashReporterClient interface on Windows. The CrashReporterClient implementation on Windows which is implemented by the ChromeCrashReporterClient class will eventually move into chrome_elf which loads very early in the chrome process. This requires it to not depend on anything other than kernel32.dll and advapi32.dll. Depending on base::FilePath pulls in FileUtil which on Windows brings with it dependencies on message loop etc which won't work in chrome_elf. Additionally the ChromeCrashReporterClient class currently depends on content for constants, etc. I defined these constants in the install_static library along with TODOs to unify them. I plan to do that after finishing up the work for chrome_elf. The changes in this patch are as below:- 1. Remove FilePath from the CrashReporterClient functions on Windows. 2. Use the constants from install_static instead of those from content and base::Env. 3. Remove usage of base::Version from ChromeCrashReporterClient as that pulls in base logging which is not good for us. 4. Replace DCHECKs with asserts in ChromeCrashReporterClient BUG=604923 Committed: https://crrev.com/e76a92ef5942fe99139ae02e7dc5162dc0b24358 Cr-Commit-Position: refs/heads/master@{#397865}

Patch Set 1 #

Patch Set 2 : Update comment and #define DCHECK assert #

Patch Set 3 : Fix build error #

Total comments: 7

Patch Set 4 : Address review comments #

Patch Set 5 : Fix build error #

Total comments: 14

Patch Set 6 : Address review comments #

Patch Set 7 : Fix build error #

Total comments: 8

Patch Set 8 : Address review comments #

Patch Set 9 : Fix clang redness #

Total comments: 2

Patch Set 10 : Fix clang redness #

Patch Set 11 : Fix clang error #

Patch Set 12 : Attempt 3 at fixing clang #

Patch Set 13 : Remove unused constant to fix clang #

Unified diffs Side-by-side diffs Delta from patch set Stats (+223 lines, -124 lines) Patch
M chrome/app/chrome_crash_reporter_client_win.h View 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/app/chrome_crash_reporter_client_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +33 lines, -59 lines 0 comments Download
M chrome/install_static/install_util.h View 1 2 3 4 5 6 4 chunks +24 lines, -2 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +69 lines, -20 lines 0 comments Download
M chrome/install_static/install_util_unittest.cc View 1 2 3 4 5 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/installer/setup/installer_crash_reporter_client.h View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/installer/setup/installer_crash_reporter_client.cc View 1 2 3 4 5 chunks +11 lines, -6 lines 0 comments Download
M components/crash/content/app/breakpad_win.cc View 2 chunks +1 line, -3 lines 0 comments Download
M components/crash/content/app/crash_keys_win.cc View 3 chunks +3 lines, -4 lines 0 comments Download
M components/crash/content/app/crash_keys_win_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +7 lines, -7 lines 0 comments Download
M components/crash/content/app/crash_reporter_client.h View 4 chunks +9 lines, -3 lines 0 comments Download
M components/crash/content/app/crash_reporter_client.cc View 1 4 chunks +18 lines, -5 lines 0 comments Download
M components/crash/content/app/crashpad_win.cc View 1 2 3 4 5 3 chunks +7 lines, -3 lines 0 comments Download
M content/shell/app/shell_crash_reporter_client.h View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M content/shell/app/shell_crash_reporter_client.cc View 1 2 3 4 5 3 chunks +15 lines, -3 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 34 (10 generated)
ananta
4 years, 6 months ago (2016-06-01 21:39:23 UTC) #2
ananta
+grt for install_util changes.
4 years, 6 months ago (2016-06-01 21:39:46 UTC) #4
scottmg
https://codereview.chromium.org/2031833002/diff/40001/chrome/app/chrome_crash_reporter_client_win.h File chrome/app/chrome_crash_reporter_client_win.h (right): https://codereview.chromium.org/2031833002/diff/40001/chrome/app/chrome_crash_reporter_client_win.h#newcode11 chrome/app/chrome_crash_reporter_client_win.h:11: class ChromeCrashReporterClient : public crash_reporter::CrashReporterClient { You're going to ...
4 years, 6 months ago (2016-06-02 00:01:13 UTC) #5
scottmg
On 2016/06/02 00:01:13, scottmg wrote: > https://codereview.chromium.org/2031833002/diff/40001/chrome/app/chrome_crash_reporter_client_win.h > File chrome/app/chrome_crash_reporter_client_win.h (right): > > https://codereview.chromium.org/2031833002/diff/40001/chrome/app/chrome_crash_reporter_client_win.h#newcode11 > ...
4 years, 6 months ago (2016-06-02 00:01:59 UTC) #6
scottmg
The string16 stuff seems ok to me, I'm not too excited about the copy of ...
4 years, 6 months ago (2016-06-02 00:09:30 UTC) #7
scottmg
Oh, could you confirm that assert() really gets compiled out in Release builds of chrome_elf ...
4 years, 6 months ago (2016-06-02 00:10:06 UTC) #8
ananta
I was thinking about putting these constants in a common place which does not link ...
4 years, 6 months ago (2016-06-02 00:22:57 UTC) #9
ananta
On 2016/06/02 00:10:06, scottmg wrote: > Oh, could you confirm that assert() really gets compiled ...
4 years, 6 months ago (2016-06-02 00:31:21 UTC) #10
grt (UTC plus 2)
https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc#newcode39 chrome/app/chrome_crash_reporter_client_win.cc:39: *crash_dir = install_static::UTF8ToUTF16(alternate_crash_dump_location); GetEnvironmentString does a UTF16ToUTF8 under the ...
4 years, 6 months ago (2016-06-02 12:55:33 UTC) #11
ananta
https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc#newcode39 chrome/app/chrome_crash_reporter_client_win.cc:39: *crash_dir = install_static::UTF8ToUTF16(alternate_crash_dump_location); On 2016/06/02 12:55:33, grt (slow) wrote: ...
4 years, 6 months ago (2016-06-02 20:34:20 UTC) #12
ananta
Added couple tests for TokenizeString16.
4 years, 6 months ago (2016-06-02 20:37:31 UTC) #13
scottmg
lgtm https://codereview.chromium.org/2031833002/diff/120001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/120001/chrome/app/chrome_crash_reporter_client_win.cc#newcode73 chrome/app/chrome_crash_reporter_client_win.cc:73: restart_info, L'|', true); // true = Trim shitespace. ...
4 years, 6 months ago (2016-06-02 22:01:49 UTC) #14
grt (UTC plus 2)
https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc#newcode94 chrome/app/chrome_crash_reporter_client_win.cc:94: bool ChromeCrashReporterClient::GetDeferredUploadsSupported( On 2016/06/02 20:34:20, ananta wrote: > On ...
4 years, 6 months ago (2016-06-02 22:57:44 UTC) #15
scottmg
https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/80001/chrome/app/chrome_crash_reporter_client_win.cc#newcode94 chrome/app/chrome_crash_reporter_client_win.cc:94: bool ChromeCrashReporterClient::GetDeferredUploadsSupported( On 2016/06/02 22:57:44, grt (slow) wrote: > ...
4 years, 6 months ago (2016-06-02 23:01:09 UTC) #16
ananta
https://codereview.chromium.org/2031833002/diff/120001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2031833002/diff/120001/chrome/app/chrome_crash_reporter_client_win.cc#newcode38 chrome/app/chrome_crash_reporter_client_win.cc:38: return !!crash_dir->empty(); On 2016/06/02 22:57:44, grt (slow) wrote: > ...
4 years, 6 months ago (2016-06-02 23:10:30 UTC) #17
grt (UTC plus 2)
lgtm https://codereview.chromium.org/2031833002/diff/160001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2031833002/diff/160001/chrome/install_static/install_util.cc#newcode435 chrome/install_static/install_util.cc:435: std::basic_istringstream<StringType::value_type> buffer(str); clang says you need "typename" here, ...
4 years, 6 months ago (2016-06-03 15:41:41 UTC) #18
ananta
https://codereview.chromium.org/2031833002/diff/160001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2031833002/diff/160001/chrome/install_static/install_util.cc#newcode435 chrome/install_static/install_util.cc:435: std::basic_istringstream<StringType::value_type> buffer(str); On 2016/06/03 15:41:41, grt (slow) wrote: > ...
4 years, 6 months ago (2016-06-03 22:32:07 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031833002/240001
4 years, 6 months ago (2016-06-03 22:33:52 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/195088)
4 years, 6 months ago (2016-06-03 22:42:02 UTC) #24
ananta
+peter for content\shell owners
4 years, 6 months ago (2016-06-03 22:48:26 UTC) #27
Peter Beverloo
On 2016/06/03 22:48:26, ananta wrote: > +peter for content\shell owners lgtm
4 years, 6 months ago (2016-06-03 23:00:47 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031833002/240001
4 years, 6 months ago (2016-06-03 23:27:51 UTC) #30
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 6 months ago (2016-06-04 00:49:31 UTC) #32
commit-bot: I haz the power
4 years, 6 months ago (2016-06-04 00:51:55 UTC) #34
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/e76a92ef5942fe99139ae02e7dc5162dc0b24358
Cr-Commit-Position: refs/heads/master@{#397865}

Powered by Google App Engine
This is Rietveld 408576698