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

Issue 2422643002: Windows install_static refactor. (Closed)

Created:
3 years, 4 months ago by grt (UTC plus 2)
Modified:
2 years, 5 months ago
CC:
chromium-reviews, pennymac+watch_chromium.org, caitkp+watch_chromium.org, gab
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Windows install_static refactor. This change introduces install_static::InstallDetails, a module-global holder of details relating to the current install. It also more clearly defines the concept of a primary install mode and one or more secondary install modes (e.g., "Chrome SxS" for Google Chrome's canary channel). Furthermore, it creates a clear boundary for brand-specific constants so that Chromium and Google Chrome don't bleed into one another. See chrome/install_static/README.md for more information. BUG=373987 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/92bc27b8b101d2b300a7b3afd79265b3cfba1152 Committed: https://crrev.com/e29f8ab494ddbb5c85407d544204c5168e5028d8 Cr-Original-Commit-Position: refs/heads/master@{#430728} Cr-Commit-Position: refs/heads/master@{#432270}

Patch Set 1 #

Patch Set 2 : clang fix #

Patch Set 3 : fix static build #

Total comments: 10

Patch Set 4 : maybe fix browser_tests, and use is_pod #

Patch Set 5 : maybe fix browser_tests better #

Patch Set 6 : maybe fix nacl64 #

Total comments: 39

Patch Set 7 : robertshield comments #

Patch Set 8 : sync to position 427054 #

Total comments: 3

Patch Set 9 : NACL! #

Patch Set 10 : more comments #

Patch Set 11 : plug test leak #

Total comments: 2

Patch Set 12 : more comments #

Patch Set 13 : sync to position 428354 #

Total comments: 11

Patch Set 14 : siggi comments #

Patch Set 15 : fix chromium channel name -- it should be empty rather than unknown #

Patch Set 16 : sync to position 430922 for reland #

Patch Set 17 : fixed uninstall crash and a bad constant for reland #

Patch Set 18 : sync to position 431863 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+1941 lines, -964 lines) Patch
M chrome/app/chrome_crash_reporter_client_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +9 lines, -11 lines 0 comments Download
M chrome/app/chrome_exe_main_win.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/app/chrome_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +12 lines, -1 line 0 comments Download
M chrome/app/main_dll_loader_win.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/chrome_watcher/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M chrome/chrome_watcher/DEPS View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/chrome_watcher/chrome_watcher_main.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -4 lines 0 comments Download
M chrome/chrome_watcher/chrome_watcher_main_api.h View 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/install_static/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +26 lines, -0 lines 0 comments Download
M chrome/install_static/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +10 lines, -1 line 0 comments Download
A chrome/install_static/README.md View 1 2 3 4 5 6 1 chunk +10 lines, -0 lines 0 comments Download
A chrome/install_static/chromium_install_modes.h View 1 chunk +23 lines, -0 lines 4 comments Download
A chrome/install_static/chromium_install_modes.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +41 lines, -0 lines 0 comments Download
A chrome/install_static/google_chrome_install_modes.h View 1 chunk +24 lines, -0 lines 0 comments Download
A chrome/install_static/google_chrome_install_modes.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +51 lines, -0 lines 0 comments Download
A chrome/install_static/install_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +67 lines, -0 lines 0 comments Download
A chrome/install_static/install_details.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +172 lines, -0 lines 0 comments Download
A chrome/install_static/install_details.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +91 lines, -0 lines 0 comments Download
A chrome/install_static/install_details_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +151 lines, -0 lines 0 comments Download
A chrome/install_static/install_modes.h View 1 2 3 4 5 6 1 chunk +82 lines, -0 lines 0 comments Download
A chrome/install_static/install_modes.cc View 1 chunk +51 lines, -0 lines 0 comments Download
A chrome/install_static/install_modes_unittest.cc View 1 chunk +105 lines, -0 lines 0 comments Download
M chrome/install_static/install_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +15 lines, -61 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 14 chunks +173 lines, -345 lines 0 comments Download
M chrome/install_static/install_util_unittest.cc View 3 chunks +211 lines, -103 lines 0 comments Download
A chrome/install_static/product_install_details.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +49 lines, -0 lines 0 comments Download
A chrome/install_static/product_install_details.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +148 lines, -0 lines 0 comments Download
A chrome/install_static/product_install_details_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +354 lines, -0 lines 0 comments Download
M chrome/installer/util/google_chrome_distribution.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +11 lines, -4 lines 0 comments Download
M chrome/nacl/DEPS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/nacl/nacl_exe_win_64.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/test/base/chrome_test_launcher.cc View 1 2 3 4 4 chunks +16 lines, -0 lines 0 comments Download
M chrome_elf/DEPS View 1 chunk +1 line, -1 line 0 comments Download
M chrome_elf/chrome_elf.def View 1 2 3 4 5 6 7 1 chunk +3 lines, -2 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +11 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_util_unittest.cc View 3 chunks +0 lines, -419 lines 0 comments Download
M components/nacl/broker/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 135 (88 generated)
grt (UTC plus 2)
This is some house cleaning in preparation for introducing side-by-side dev and beta. There will ...
3 years, 4 months ago (2016-10-14 13:58:41 UTC) #9
Sigurður Ásgeirsson
Sorry for my tardiness - looks reasonable to me, though I'd sprinkle in std::is_pod asserts ...
3 years, 4 months ago (2016-10-14 21:09:13 UTC) #16
grt (UTC plus 2)
Tardy? Hardly! Thanks for the feedback. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h#newcode84 chrome/install_static/install_details.h:84: static intptr_t GetPayload(); ...
3 years, 4 months ago (2016-10-16 09:15:25 UTC) #25
robertshield
I've read though and I think I understand what's going on :-) This is a ...
3 years, 4 months ago (2016-10-17 05:27:23 UTC) #37
grt (UTC plus 2)
Thanks! https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md#newcode6 chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: ...
3 years, 4 months ago (2016-10-24 11:17:52 UTC) #39
robertshield
I had a few unsent drafts stuck in rietveld somehow. These are from an earlier ...
3 years, 4 months ago (2016-10-24 13:56:21 UTC) #45
robertshield
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md#newcode6 chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/24 11:17:51, grt (UTC plus ...
3 years, 4 months ago (2016-10-24 14:59:26 UTC) #50
Sigurður Ásgeirsson
https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h#newcode84 chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/16 09:15:25, grt (UTC plus ...
3 years, 4 months ago (2016-10-24 15:04:07 UTC) #51
grt (UTC plus 2)
Keep 'em coming! https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h#newcode84 chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/24 15:04:06, ...
3 years, 4 months ago (2016-10-24 19:36:00 UTC) #56
robertshield
LGTM with one or two optional suggestions, defer to Siggi for the cross module wrangling ...
3 years, 4 months ago (2016-10-28 06:56:53 UTC) #59
grt (UTC plus 2)
pennymac: Do you have any tips on how to ensure that install_static doesn't accidentally pick ...
3 years, 4 months ago (2016-10-28 10:59:45 UTC) #65
grt (UTC plus 2)
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md#newcode6 chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 10:59:44, grt (UTC plus ...
3 years, 4 months ago (2016-10-28 14:20:33 UTC) #68
robertshield
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/README.md#newcode6 chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 14:20:33, grt (UTC plus ...
3 years, 4 months ago (2016-10-28 14:34:21 UTC) #69
Sigurður Ásgeirsson
watcher and module-passing details lgtm, modulo a couple of nits. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h#newcode84 ...
3 years, 4 months ago (2016-10-28 15:44:17 UTC) #72
grt (UTC plus 2)
Thanks. PTAL. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/install_details.h#newcode84 chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/28 15:44:16, Sigurður ...
3 years, 3 months ago (2016-10-31 10:44:31 UTC) #73
grt (UTC plus 2)
OWNERS reviews, please: bradnelson: - components/nacl/broker/BUILD.gn - chrome/nacl/* thakis: - new dependency in chrome/install_static on ...
3 years, 3 months ago (2016-10-31 10:58:06 UTC) #76
Paweł Hajdan Jr.
chrome/test LGTM
3 years, 3 months ago (2016-10-31 11:08:11 UTC) #79
Nico
my files lgtm
3 years, 3 months ago (2016-10-31 12:43:59 UTC) #80
grt (UTC plus 2)
+CC gab as an FYI, no need for your eyes on the review.
3 years, 3 months ago (2016-10-31 13:55:20 UTC) #83
Sigurður Ásgeirsson
still lgtm
3 years, 3 months ago (2016-10-31 13:59:21 UTC) #84
grt (UTC plus 2)
dschuff: could you do an OWNERS review of the NACL bits? Thanks.
3 years, 3 months ago (2016-11-02 09:12:29 UTC) #90
Derek Schuff
On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: > dschuff: could you do an OWNERS ...
3 years, 3 months ago (2016-11-02 16:12:14 UTC) #91
grt (UTC plus 2)
On 2016/11/02 16:12:14, Derek Schuff wrote: > On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: ...
3 years, 3 months ago (2016-11-02 16:15:26 UTC) #92
grt (UTC plus 2)
On 2016/11/02 16:15:26, grt (UTC plus 1) wrote: > On 2016/11/02 16:12:14, Derek Schuff wrote: ...
3 years, 3 months ago (2016-11-04 21:32:32 UTC) #93
grt (UTC plus 2)
On 2016/11/04 21:32:32, grt (UTC plus 1) wrote: > On 2016/11/02 16:15:26, grt (UTC plus ...
3 years, 3 months ago (2016-11-08 10:05:08 UTC) #94
Derek Schuff
Sorry was out of the office last week. LGTM
3 years, 3 months ago (2016-11-08 19:00:47 UTC) #95
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/2422643002/320001
3 years, 3 months ago (2016-11-08 19:02:03 UTC) #98
grt (UTC plus 2)
On 2016/11/08 19:00:47, Derek Schuff wrote: > Sorry was out of the office last week. ...
3 years, 3 months ago (2016-11-08 20:55:39 UTC) #99
commit-bot: I haz the power
Committed patchset #15 (id:320001)
3 years, 3 months ago (2016-11-08 21:58:01 UTC) #101
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/92bc27b8b101d2b300a7b3afd79265b3cfba1152 Cr-Commit-Position: refs/heads/master@{#430728}
3 years, 3 months ago (2016-11-08 22:01:32 UTC) #103
grt (UTC plus 2)
A revert of this CL (patchset #15 id:320001) has been created in https://codereview.chromium.org/2491463002/ by grt@chromium.org. ...
3 years, 3 months ago (2016-11-09 12:22:00 UTC) #104
grt (UTC plus 2)
sync to position 430922
3 years, 3 months ago (2016-11-10 09:50:22 UTC) #105
grt (UTC plus 2)
fixed uninstall crash and a bad constant for reland
3 years, 3 months ago (2016-11-10 09:55:15 UTC) #106
grt (UTC plus 2)
Hi Robert. The diff between patch set 16 and 17 contains two fixes to this. ...
3 years, 3 months ago (2016-11-10 10:27:13 UTC) #110
grt (UTC plus 2)
sync to position 431863
3 years, 3 months ago (2016-11-14 12:23:29 UTC) #113
robertshield
lgtm
3 years, 3 months ago (2016-11-15 16:17:16 UTC) #119
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/2422643002/380001
3 years, 3 months ago (2016-11-15 16:17:40 UTC) #121
commit-bot: I haz the power
Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/453)
3 years, 3 months ago (2016-11-15 18:14:18 UTC) #123
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/2422643002/380001
3 years, 3 months ago (2016-11-15 19:43:43 UTC) #125
commit-bot: I haz the power
Committed patchset #18 (id:380001)
3 years, 3 months ago (2016-11-15 22:27:07 UTC) #127
commit-bot: I haz the power
Patchset 18 (id:??) landed as https://crrev.com/e29f8ab494ddbb5c85407d544204c5168e5028d8 Cr-Commit-Position: refs/heads/master@{#432270}
3 years, 3 months ago (2016-11-15 22:32:32 UTC) #129
Nico
Zombie review comment: https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h#newcode13 chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, What's the point ...
2 years, 5 months ago (2017-09-11 01:12:25 UTC) #130
grt (UTC plus 2)
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h#newcode13 chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/11 01:12:24, Nico wrote: > ...
2 years, 5 months ago (2017-09-11 12:31:37 UTC) #131
Nico
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h#newcode13 chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/11 12:31:37, grt (UTC plus ...
2 years, 5 months ago (2017-09-14 21:53:21 UTC) #132
grt (UTC plus 2)
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/chromium_install_modes.h#newcode13 chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/14 21:53:20, Nico wrote: > ...
2 years, 5 months ago (2017-09-15 11:25:31 UTC) #133
Nico
Aha! And then we conditionally include one of those two headers based on build type? ...
2 years, 5 months ago (2017-09-15 12:20:39 UTC) #134
grt (UTC plus 2)
2 years, 5 months ago (2017-09-15 12:50:30 UTC) #135
Message was sent while issue was closed.
On 2017/09/15 12:20:39, Nico wrote:
> Aha! And then we conditionally include one of those two headers based on
> build type?

Precisely!
https://cs.chromium.org/chromium/src/chrome/install_static/BUILD.gn?type=cs&q...

Powered by Google App Engine
This is Rietveld 408576698