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

Issue 2077323004: Remove dependency on base/win/win_util.cc from crashpad. (Closed)

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

Description

Remove dependency on base/win/win_util.cc from crashpad. Fixing the official ASAN builder which fails when chrome_crash_reporter_client_win.cc is compiled with chrome_elf.dll Crashpad is now included as part of chrome_elf and hence it cannot depend on functionality in base or elsewhere which pulls in user32 The ElfImportsTest.ChromeElfSanityCheck is disabled for component builds to fix Dr Memory issues. This reverts commit f546a78ac6eb865d418defe38cf70c1f0e3988c0. BUG=621460 TBR=robertshield Committed: https://crrev.com/e10c4579e9e096bd9af5174315c36ffcb9ab7af3 Cr-Commit-Position: refs/heads/master@{#400839}

Patch Set 1 #

Patch Set 2 : Dont run the ELFImportsTest for component builds #

Total comments: 4

Patch Set 3 : Address review comments and update patch description #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -3 lines) Patch
M chrome_elf/BUILD.gn View 2 chunks +7 lines, -0 lines 0 comments Download
M chrome_elf/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M chrome_elf/elf_imports_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/crash/content/app/crashpad.cc View 2 chunks +50 lines, -2 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 18 (9 generated)
ananta
4 years, 6 months ago (2016-06-20 19:42:29 UTC) #2
scottmg
EnumProcessModules is in kernel32 on 7. The copy of the function avoids some win_util.cc deps ...
4 years, 6 months ago (2016-06-20 19:46:16 UTC) #5
scottmg
https://codereview.chromium.org/2077323004/diff/20001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/2077323004/diff/20001/chrome_elf/chrome_elf.gyp#newcode40 chrome_elf/chrome_elf.gyp:40: '../chrome/app/chrome_crash_reporter_client_win.cc', Add the .h's here too. https://codereview.chromium.org/2077323004/diff/20001/chrome_elf/elf_imports_unittest.cc File chrome_elf/elf_imports_unittest.cc ...
4 years, 6 months ago (2016-06-20 19:47:53 UTC) #6
ananta
https://codereview.chromium.org/2077323004/diff/20001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/2077323004/diff/20001/chrome_elf/chrome_elf.gyp#newcode40 chrome_elf/chrome_elf.gyp:40: '../chrome/app/chrome_crash_reporter_client_win.cc', On 2016/06/20 19:47:52, scottmg (slow 20june) wrote: > ...
4 years, 6 months ago (2016-06-20 19:50:50 UTC) #8
ananta
TBR'ing robertshield as the change to chrome_elf is based on the previously lgtm'ed patch. Will ...
4 years, 6 months ago (2016-06-20 23:39:23 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077323004/40001
4 years, 6 months ago (2016-06-20 23:40:21 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 6 months ago (2016-06-20 23:50:07 UTC) #15
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/e10c4579e9e096bd9af5174315c36ffcb9ab7af3 Cr-Commit-Position: refs/heads/master@{#400839}
4 years, 6 months ago (2016-06-20 23:51:15 UTC) #17
robertshield
4 years, 6 months ago (2016-06-21 02:07:49 UTC) #18
Message was sent while issue was closed.
Sorry for the delay, LGTM

Powered by Google App Engine
This is Rietveld 408576698