Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(30)

Issue 2183263003: [chrome_elf] Big ELF cleanup. Part 1. (Closed)

Created:
3 years, 8 months ago by penny
Modified:
3 years, 7 months ago
Reviewers:
robertshield
CC:
chromium-reviews, 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

[chrome_elf] Big cleanup and removing dependencies that recently crept in. PART 1. - Moving all crash related APIs into one place (and out of chrome_elf_main). - Started to clean up external dependencies - starting with hook_utils. - Removed dependency on base/win/iat_patch_function. BUG=631771 Committed: https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58 Committed: https://crrev.com/5446d89cd2b5e01f28c0b479eef46e2d9fd9385a Cr-Original-Commit-Position: refs/heads/master@{#411982} Cr-Commit-Position: refs/heads/master@{#414898}

Patch Set 1 #

Patch Set 2 : Updating gyp to match gn. #

Patch Set 3 : quick fix. #

Patch Set 4 : Adjusted g_crash_reports. #

Total comments: 23

Patch Set 5 : Code review fixes, part 1. #

Total comments: 16

Patch Set 6 : Code review fixes, part 2. Also, git cl format. #

Patch Set 7 : Fix up tests for no chrash handling. #

Total comments: 2

Patch Set 8 : Code review fixes, part 3. #

Patch Set 9 : Remove debug asserts that hang tests. #

Patch Set 10 : Update with latest trunk. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+822 lines, -334 lines) Patch
M chrome_elf/BUILD.gn View 1 2 3 4 5 6 7 8 8 chunks +50 lines, -43 lines 0 comments Download
M chrome_elf/blacklist.gypi View 1 3 chunks +1 line, -3 lines 0 comments Download
M chrome_elf/blacklist/blacklist.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M chrome_elf/blacklist/blacklist_interceptions.cc View 2 chunks +2 lines, -2 lines 0 comments Download
D chrome_elf/blacklist/crashpad_helper.h View 1 chunk +0 lines, -15 lines 0 comments Download
D chrome_elf/blacklist/crashpad_helper.cc View 1 chunk +0 lines, -13 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 3 chunks +31 lines, -7 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -78 lines 0 comments Download
M chrome_elf/chrome_elf_security.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_security.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_util_unittest.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
A chrome_elf/crash/crash_helper.h View 1 2 3 4 5 6 7 8 1 chunk +32 lines, -0 lines 0 comments Download
A chrome_elf/crash/crash_helper.cc View 1 2 3 4 5 6 7 8 9 1 chunk +149 lines, -0 lines 0 comments Download
M chrome_elf/elf_imports_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
A chrome_elf/hook_util/hook_util.h View 1 2 3 4 5 6 1 chunk +66 lines, -0 lines 0 comments Download
A chrome_elf/hook_util/hook_util.cc View 1 2 3 4 5 6 7 8 1 chunk +322 lines, -0 lines 0 comments Download
A chrome_elf/hook_util/test/hook_util_test.cc View 1 2 3 4 5 6 7 1 chunk +103 lines, -0 lines 0 comments Download
A chrome_elf/hook_util/test/hook_util_test_dll.h View 1 2 3 4 5 6 7 1 chunk +14 lines, -0 lines 0 comments Download
A chrome_elf/hook_util/test/hook_util_test_dll.cc View 1 2 3 4 5 6 7 1 chunk +22 lines, -0 lines 0 comments Download
D chrome_elf/hook_util/thunk_getter.h View 1 chunk +0 lines, -16 lines 0 comments Download
D chrome_elf/hook_util/thunk_getter.cc View 1 chunk +0 lines, -152 lines 0 comments Download

Messages

Total messages: 61 (44 generated)
penny
Hello Robert! Here's the first in a number of CLs I'll land to clean up ...
3 years, 8 months ago (2016-07-29 23:28:53 UTC) #8
penny
On 2016/07/29 23:28:53, penny wrote: > Hello Robert! > > Here's the first in a ...
3 years, 8 months ago (2016-08-01 17:52:26 UTC) #9
robertshield
cool stuff, comments and thoughts inline https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_helper.cc File chrome_elf/crash/crash_helper.cc (right): https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_helper.cc#newcode29 chrome_elf/crash/crash_helper.cc:29: std::wstring file_name_string = ...
3 years, 8 months ago (2016-08-02 02:38:41 UTC) #10
penny
Thanks Robert. Fixes are in! https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_helper.cc File chrome_elf/crash/crash_helper.cc (right): https://codereview.chromium.org/2183263003/diff/60001/chrome_elf/crash/crash_helper.cc#newcode29 chrome_elf/crash/crash_helper.cc:29: std::wstring file_name_string = file_path; ...
3 years, 7 months ago (2016-08-02 21:07:44 UTC) #11
robertshield
Looks really cool (this is fun code to review :-)). One or two thoughts about ...
3 years, 7 months ago (2016-08-03 04:52:46 UTC) #21
penny
I'm glad you're enjoying it Robert - there's more goodness to come. ;D Here's the ...
3 years, 7 months ago (2016-08-04 18:55:35 UTC) #24
robertshield
Code LGTM, small request for an extra couple of lines in the hook test if ...
3 years, 7 months ago (2016-08-10 20:14:48 UTC) #28
penny
Robert, I think I finally managed to get all the tests running clean. :) Feel ...
3 years, 7 months ago (2016-08-13 19:22:15 UTC) #34
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/2183263003/300001
3 years, 7 months ago (2016-08-15 16:05:56 UTC) #41
commit-bot: I haz the power
Committed patchset #9 (id:300001)
3 years, 7 months ago (2016-08-15 18:04:20 UTC) #43
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/d2767f8b41a8f7f70bddbd0783a8ef27a9370e58 Cr-Commit-Position: refs/heads/master@{#411982}
3 years, 7 months ago (2016-08-15 18:11:16 UTC) #45
Avi (use Gerrit)
A revert of this CL (patchset #9 id:300001) has been created in https://codereview.chromium.org/2242323002/ by avi@chromium.org. ...
3 years, 7 months ago (2016-08-15 20:07:08 UTC) #46
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/2183263003/320001
3 years, 7 months ago (2016-08-26 23:25:15 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/270070)
3 years, 7 months ago (2016-08-27 00:33:43 UTC) #53
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/2183263003/360001
3 years, 7 months ago (2016-08-27 03:46:09 UTC) #57
commit-bot: I haz the power
Committed patchset #10 (id:360001)
3 years, 7 months ago (2016-08-27 10:45:34 UTC) #59
commit-bot: I haz the power
3 years, 7 months ago (2016-08-27 10:47:33 UTC) #61
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/5446d89cd2b5e01f28c0b479eef46e2d9fd9385a
Cr-Commit-Position: refs/heads/master@{#414898}

Powered by Google App Engine
This is Rietveld 408576698