Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422643002:
Windows install_static refactor. (Closed)
|
Created:
4 years, 2 months ago by grt (UTC plus 2) Modified:
3 years, 3 months ago Reviewers:
Sigurður Ásgeirsson, bradnelson, penny, Nico, robertshield, Derek Schuff, Paweł Hajdan Jr., ananta CC:
chromium-reviews, pennymac+watch_chromium.org, caitkp+watch_chromium.org, gab Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWindows 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
Messages
Total messages: 135 (88 generated)
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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 ========== to ========== 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 ==========
grt@chromium.org changed reviewers: + ananta@chromium.org
grt@chromium.org changed reviewers: + robertshield@chromium.org
grt@chromium.org changed reviewers: + siggi@chromium.org
This is some house cleaning in preparation for introducing side-by-side dev and beta. There will be follow-on changes in chrome/installer/util, chrome/installer/setup, and other places. My intent is for install_static::InstallDetails to be the one process-wide (including setup.exe) container for "how is Chrome installed". I may eventually replace BrowserDistribution with something more light-weight, but it may live for a little while. In the end, it is my plan that enabling sxs dev and beta will mean adding some new constants in google_chrome_install_modes.cc's kInstallModes[] and then trickle changes in the product for things like picking the correct icons here and there. Review assignments! ananta: please look at chrome/install_static. robertshield: chrome_elf and chrome/install_static if you have the patience for it. siggi: please look at the way I'm sharing a POD-struct allocated in chrome_elf with other modules in the process; specifically the static methods on InstallDetails and their uses in chrome_elf_main, chrome_main, chrome_exe_main_win, and chrome_watcher_main. Thanks!
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for my tardiness - looks reasonable to me, though I'd sprinkle in std::is_pod asserts on all things :). https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); I'm not sure I understand why this can't profitably be a POD pointer. As-is, it seems we're passing InstallDetails::Payload* from module to module, cast to intptr_t? Note that if you so desire, you can forward-declare the Payload publicly, then define the class in the .cc to achieve pimpl-like machinations without exposing the Payload to anyone. Another way to show/hide would be to declare Payload with all-private members, and use friendship to grant access to it. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:110: }; maybe you want a static_assert(std::is_pod<Payload>::value, ...) somewhere? https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:122: // class is initialized early on in process startup. Its underlying data (its in a single module - like chrome_elf?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_x6...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Tardy? Hardly! Thanks for the feedback. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/14 21:09:12, Sigurður Ásgeirsson wrote: > I'm not sure I understand why this can't profitably be a POD pointer. As-is, it > seems we're passing InstallDetails::Payload* from module to module, cast to > intptr_t? Yes. I figure that Payload* is an implementation detail that callers of this don't need to be concerned with. You don't like? > Note that if you so desire, you can forward-declare the Payload publicly, then > define the class in the .cc to achieve pimpl-like machinations without exposing > the Payload to anyone. Another way to show/hide would be to declare Payload with > all-private members, and use friendship to grant access to it. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:110: }; On 2016/10/14 21:09:13, Sigurður Ásgeirsson wrote: > maybe you want a static_assert(std::is_pod<Payload>::value, ...) somewhere? Cool, thanks! https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:122: // class is initialized early on in process startup. Its underlying data (its On 2016/10/14 21:09:13, Sigurður Ásgeirsson wrote: > in a single module - like chrome_elf? Comment tweaked a bit. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've read though and I think I understand what's going on :-) This is a big thing to refactor, serious props for tackling it, I greatly like the direction. Musings ahead of more detailed feedback on implementation below. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. Perhaps mention why the no dependencies bit is important ("making it suitable for use in other dependency-free libraries"). Is this dependency-free nature enforced by tests or otherwise? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:33: ## Operational Details Some of these details may be more appropriate as code comments, the chances of the README and implementation diverging are a bit larger than code and nearby comments. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:35: Chrome calls `InitializeModuleProductDetails` early in process startup to naming bikeshedding: perhaps "InitializeProductDetailsForModule" or ""InitializeProductDetailsInModule" or maybe even ""InitializeProductDetails". "ModuleProduct" is a bit confusing https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_constants.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_constants.h:23: // A POD-struct defining constants for a brand's install mode. A brand may have Please elaborate somewhat extensively here on what is meant by "install mode". This struct seems to contain most of the state one has to keep in their head today to understand what kind of Chrome install is present. Documenting the snot out of this (with examples), will serve posterity for years to come. It might be helpful to describe a few common examples of installs today (Stable, Dev/Beta, Canary, User / System) and describe what their InstallConstants would look like. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_constants.h:51: }; does is_msi() belong here for msi managed installs? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.cc:19: // This module's instance. Comment here that this is intentionally leaked at shutdown. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:80: static void SetForProcess(std::unique_ptr<PrimaryInstallDetails> details); It seems like this must always be called, exactly once, for the PrimatyInstallDetails instance that gets created. Why not have PrimaryInstallDetails set the g_module_details for the current module on creation? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.cc:68: wchar_t value[MAX_PATH]; = {} (though it's not really needed here, but to guard against future refactorings) https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.h:22: // Functions from here on down are exposed for the sake of testing. What's the current thinking. re calling things XXXForTesting when they're exposed for testing?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > Perhaps mention why the no dependencies bit is important ("making it suitable > for use in other dependency-free libraries"). Done > Is this dependency-free nature enforced by tests or otherwise? Not in this library. +pennymac to confirm whether or not chrome_elf has a test for dependencies. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:33: ## Operational Details On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > Some of these details may be more appropriate as code comments, the chances of > the README and implementation diverging are a bit larger than code and nearby > comments. Good point. I've moved most of what I'd typed here into other places. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:35: Chrome calls `InitializeModuleProductDetails` early in process startup to On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > naming bikeshedding: perhaps "InitializeProductDetailsForModule" or > ""InitializeProductDetailsInModule" or maybe even ""InitializeProductDetails". > > "ModuleProduct" is a bit confusing Done. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_constants.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_constants.h:23: // A POD-struct defining constants for a brand's install mode. A brand may have On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > Please elaborate somewhat extensively here on what is meant by "install mode". Comment added to top of file. My hope is that the reader will also look toward README.md. > This struct seems to contain most of the state one has to keep in their head > today to understand what kind of Chrome install is present. Documenting the snot > out of this (with examples), will serve posterity for years to come. Point taken, and docs added. > It might be helpful to describe a few common examples of installs today (Stable, > Dev/Beta, Canary, User / System) and describe what their InstallConstants would > look like. I will direct the reader to the definitions rather than restate them here. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_constants.h:51: }; On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > does is_msi() belong here for msi managed installs? No, since that varies based on install-time factors rather than brand-specific build-time factors. That would be a candidate for InstallDetails, although I am thinking I'll keep that in a setup-specific location since I hope that the product itself will never need to know whether it originates from the .msi or not. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.cc:19: // This module's instance. On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > Comment here that this is intentionally leaked at shutdown. Done. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:80: static void SetForProcess(std::unique_ptr<PrimaryInstallDetails> details); On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > It seems like this must always be called, exactly once, for the > PrimatyInstallDetails instance that gets created. Why not have > PrimaryInstallDetails set the g_module_details for the current module on > creation? So that unittests can play with PIDs without impacting global state, and so that tests can explicitly set the global state as needed. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.cc:68: wchar_t value[MAX_PATH]; On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > = {} (though it's not really needed here, but to guard against future > refactorings) added *value = L'\0'; on the next line so that it's a properly terminated string. Do you think it's necessary to zero out the whole thing? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.h:22: // Functions from here on down are exposed for the sake of testing. On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > What's the current thinking. re calling things XXXForTesting when they're > exposed for testing? I thought it'd be fugly to put that on each of these. There's no inherent reason why they're not suitable for general consumption, there just aren't other consumers at the moment. Do you think I should remove the comment? I could also put them into an "internal" namespace. I don't really care for that too much.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I had a few unsent drafts stuck in rietveld somehow. These are from an earlier patch set but look like they still apply. https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:318: // crash_reporter::CrashReporterClient. Consider sticking these TODOs on CrashReporterClient? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:36: // This mode's index into the brand's array of install modes. This will match This is the first use of the word "mode" in this file. Could you define install mode here, or above in the class comment? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:87: static void ImportFromModule(const wchar_t* module_name, bikeshedding: How about |InitializeFromModule| since this method initializes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > Perhaps mention why the no dependencies bit is important ("making it suitable > > for use in other dependency-free libraries"). > > Done > > > Is this dependency-free nature enforced by tests or otherwise? > > Not in this library. +pennymac to confirm whether or not chrome_elf has a test > for dependencies. Elf has a now-release-only-because-gn-source-sets (:-() test here: https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.cc:68: wchar_t value[MAX_PATH]; On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > = {} (though it's not really needed here, but to guard against future > > refactorings) > > added > *value = L'\0'; > on the next line so that it's a properly terminated string. Do you think it's > necessary to zero out the whole thing? Probably not? GetEnvironmentVariable is documented as null-terminating the string that gets returned, so this is probably fine. I'm really just being paranoid, on account of them being out to get me. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.h:22: // Functions from here on down are exposed for the sake of testing. On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > What's the current thinking. re calling things XXXForTesting when they're > > exposed for testing? > > I thought it'd be fugly to put that on each of these. There's no inherent reason > why they're not suitable for general consumption, there just aren't other > consumers at the moment. Do you think I should remove the comment? I could also > put them into an "internal" namespace. I don't really care for that too much. If they're suitable for external consumption, I'd drop the comment on line 22. Folk aren't guaranteed to notice it and update it if they do start calling any of the below. Stuffing stuff in a namespace / renaming the methods is only needed if we really don't want them called by non-testing code imo. https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... File chrome/install_static/google_chrome_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... chrome/install_static/google_chrome_install_modes.h:17: STABLE_INDEX, Do you plan to include Beta and Dev in here in the future? If so, maybe set the numeric values of these indices to 0 and 3 respectively leaving room for the other two?
https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/16 09:15:25, grt (UTC plus 2) wrote: > On 2016/10/14 21:09:12, Sigurður Ásgeirsson wrote: > > I'm not sure I understand why this can't profitably be a POD pointer. As-is, > it > > seems we're passing InstallDetails::Payload* from module to module, cast to > > intptr_t? > > Yes. I figure that Payload* is an implementation detail that callers of this > don't need to be concerned with. You don't like? I generally lean towards compiler-enforced typing wherever practical, so not overly fond of integral types for passing pointers. Also, I wonder whether there's room and reason to add signature or version or size or some other identifier to Payload. I figure there's the usual incentives for diverting the flow of data here - would be nice to at least detect skew?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Keep 'em coming! https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/24 15:04:06, Sigurður Ásgeirsson wrote: > On 2016/10/16 09:15:25, grt (UTC plus 2) wrote: > > On 2016/10/14 21:09:12, Sigurður Ásgeirsson wrote: > > > I'm not sure I understand why this can't profitably be a POD pointer. As-is, > > it > > > seems we're passing InstallDetails::Payload* from module to module, cast to > > > intptr_t? > > > > Yes. I figure that Payload* is an implementation detail that callers of this > > don't need to be concerned with. You don't like? > > I generally lean towards compiler-enforced typing wherever practical, so not > overly fond of integral types for passing pointers. Okay. I've made Payload public but not done any other rigmarole. > Also, I wonder whether there's room and reason to add signature or version or > size or some other identifier to Payload. I figure there's the usual incentives > for diverting the flow of data here - would be nice to at least detect skew? One thing I can imagine, which should be fairly simple, is to take a dependency on //components/version_info:generate_version_info, put a ptr to a string holding PRODUCT_VERSION into the payload in PrimaryInstallDetails, and check for a match in InitializeForModule. How does that sound to you? What action should we take on mismatch? CHECK? DumpWithoutCrash? https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:318: // crash_reporter::CrashReporterClient. On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > Consider sticking these TODOs on CrashReporterClient? I was trying to keep the size of the CL in check by doing it here. I plan to address this TODO in a subsequent change as I ripple InstallDetails through the product and installer. Are you okay with keeping it here for now? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > Perhaps mention why the no dependencies bit is important ("making it > suitable > > > for use in other dependency-free libraries"). > > > > Done > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > Not in this library. +pennymac to confirm whether or not chrome_elf has a test > > for dependencies. > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... Thanks. I see I forgot to do the actual + thing for pennymac. :-) Do you think that test is sufficient? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:36: // This mode's index into the brand's array of install modes. This will match On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > This is the first use of the word "mode" in this file. Could you define install > mode here, or above in the class comment? Pointer to install_modes.h added to class comment. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:87: static void ImportFromModule(const wchar_t* module_name, On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > bikeshedding: How about |InitializeFromModule| since this method initializes? What do you think of InitializeForModule for harmony with SetForProcess? https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.cc:68: wchar_t value[MAX_PATH]; On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > = {} (though it's not really needed here, but to guard against future > > > refactorings) > > > > added > > *value = L'\0'; > > on the next line so that it's a properly terminated string. Do you think it's > > necessary to zero out the whole thing? > > Probably not? GetEnvironmentVariable is documented as null-terminating the > string that gets returned, so this is probably fine. I'm really just being > paranoid, on account of them being out to get me. This code uses the length provided by GEV rather than trusting that it's properly terminated, so I think it's safe. Let me know if it keeps you up and night and I'll change it. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/product_install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/product_install_details.h:22: // Functions from here on down are exposed for the sake of testing. On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > What's the current thinking. re calling things XXXForTesting when they're > > > exposed for testing? > > > > I thought it'd be fugly to put that on each of these. There's no inherent > reason > > why they're not suitable for general consumption, there just aren't other > > consumers at the moment. Do you think I should remove the comment? I could > also > > put them into an "internal" namespace. I don't really care for that too much. > > If they're suitable for external consumption, I'd drop the comment on line 22. Done. > Folk aren't guaranteed to notice it and update it if they do start calling any > of the below. Stuffing stuff in a namespace / renaming the methods is only > needed if we really don't want them called by non-testing code imo. Ack. https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... File chrome/install_static/google_chrome_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... chrome/install_static/google_chrome_install_modes.h:17: STABLE_INDEX, On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > Do you plan to include Beta and Dev in here in the future? Yessir! > If so, maybe set the > numeric values of these indices to 0 and 3 respectively leaving room for the > other two? The numeric values don't have any meaning beyond a single run at the moment, so I don't see a need to reserve values 1 and 2. If I find a reason to send the index via UMA or something, I will do as you propose.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with one or two optional suggestions, defer to Siggi for the cross module wrangling https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:318: // crash_reporter::CrashReporterClient. On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > > Consider sticking these TODOs on CrashReporterClient? > > I was trying to keep the size of the CL in check by doing it here. I plan to > address this TODO in a subsequent change as I ripple InstallDetails through the > product and installer. Are you okay with keeping it here for now? Oh, sure, that's fine too since the winds of change are blowing and all that. If you postpone subsequent changes for reasons, consider moving the TODOs at some point in the future. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > > Perhaps mention why the no dependencies bit is important ("making it > > suitable > > > > for use in other dependency-free libraries"). > > > > > > Done > > > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > > > Not in this library. +pennymac to confirm whether or not chrome_elf has a > test > > > for dependencies. > > > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > > > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... > > Thanks. I see I forgot to do the actual + thing for pennymac. :-) > > Do you think that test is sufficient? The test is imho the barest minimum to prevent regressions. I wasn't able to think of anything better at the time. It's not robust against dynamic loading or anything like that. Worth checking with Penny directly if there have been any changes in thinking since then, but for a module that wishes to stay dependency free, having a similar test is a good idea. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:87: static void ImportFromModule(const wchar_t* module_name, On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > > bikeshedding: How about |InitializeFromModule| since this method initializes? > > What do you think of InitializeForModule for harmony with SetForProcess? Harmony is cool (https://www.youtube.com/watch?v=LRP8d7hhpoQ) but if I was bikeshedding I'd say that InitializeForModule("awesome.dll", "GetTheThing") reads like it is doing something to awesome.dll whereas InitializeFromModule("awesome.dll", "GetTheThing") reads like it is pulling something from awesome.dll this is just bikeshedding though. One last thought since I'm here (still in the optional nit category): what do you think about dropping |export_name| and making the export name fixed by implementation and declared as a constant in install_details? This would drop the need to declare a constant in chrome_elf_constants.h|cc. This too might be six-o'-one. Anyhow, feel free to ignore any of the above if you like InitializeForModule better. https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... File chrome/install_static/google_chrome_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/180001/chrome/install_static/... chrome/install_static/google_chrome_install_modes.h:17: STABLE_INDEX, On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > Do you plan to include Beta and Dev in here in the future? > > Yessir! > > > If so, maybe set the > > numeric values of these indices to 0 and 3 respectively leaving room for the > > other two? > > The numeric values don't have any meaning beyond a single run at the moment, so > I don't see a need to reserve values 1 and 2. If I find a reason to send the > index via UMA or something, I will do as you propose. Okey dokey. https://codereview.chromium.org/2422643002/diff/240001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2422643002/diff/240001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:32: GetInstallDetailsPayload() { maybe add a comment that this export name must match kChromeElfInstallDetailsPayloadFunctionName
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
grt@chromium.org changed reviewers: + pennymac@chromium.org
pennymac: Do you have any tips on how to ensure that install_static doesn't accidentally pick up new runtime dependencies? I'm going to try to make a simple test now that builds a DLL with the library and then checks its imports. I'd love to hear any alternative/better ideas you may have. Thanks. Siggi: PTAL Ananta: Do you have time for a looksie? https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:318: // crash_reporter::CrashReporterClient. On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > > > Consider sticking these TODOs on CrashReporterClient? > > > > I was trying to keep the size of the CL in check by doing it here. I plan to > > address this TODO in a subsequent change as I ripple InstallDetails through > the > > product and installer. Are you okay with keeping it here for now? > > Oh, sure, that's fine too since the winds of change are blowing and all that. > > If you postpone subsequent changes for reasons, consider moving the TODOs at > some point in the future. Will do. I plan for the next CL to be the one that brings InstallDetails into setup.exe. Once that's there, I can clean up CrashReporterClient with ease. It shouldn't be too long. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > > > Perhaps mention why the no dependencies bit is important ("making it > > > suitable > > > > > for use in other dependency-free libraries"). > > > > > > > > Done > > > > > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > > > > > Not in this library. +pennymac to confirm whether or not chrome_elf has a > > test > > > > for dependencies. > > > > > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > > > > > > > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... > > > > Thanks. I see I forgot to do the actual + thing for pennymac. :-) > > > > Do you think that test is sufficient? > > The test is imho the barest minimum to prevent regressions. I wasn't able to > think of anything better at the time. It's not robust against dynamic loading or > anything like that. > > Worth checking with Penny directly if there have been any changes in thinking > since then, but for a module that wishes to stay dependency free, having a > similar test is a good idea. I'll run off and try to make something simple for this library while checking with pennymac. Thanks for the advice. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/install_details.h:87: static void ImportFromModule(const wchar_t* module_name, On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > On 2016/10/24 13:56:21, robertshield_slow_reviews wrote: > > > bikeshedding: How about |InitializeFromModule| since this method > initializes? > > > > What do you think of InitializeForModule for harmony with SetForProcess? > > Harmony is cool (https://www.youtube.com/watch?v=LRP8d7hhpoQ) but if I was > bikeshedding I'd say that > > InitializeForModule("awesome.dll", "GetTheThing") reads like it is doing > something to awesome.dll > > whereas > > InitializeFromModule("awesome.dll", "GetTheThing") reads like it is pulling > something from awesome.dll > > this is just bikeshedding though. > > One last thought since I'm here (still in the optional nit category): what do > you think about dropping |export_name| and making the export name fixed by > implementation and declared as a constant in install_details? This would drop > the need to declare a constant in chrome_elf_constants.h|cc. This too might be > six-o'-one. I like it. > Anyhow, feel free to ignore any of the above if you like InitializeForModule > better. InitializeFromPrimaryModule! https://codereview.chromium.org/2422643002/diff/240001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2422643002/diff/240001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:32: GetInstallDetailsPayload() { On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > maybe add a comment that this export name must match > kChromeElfInstallDetailsPayloadFunctionName Added comment about this being in support of InstallDetails::InitializeFromPrimaryModule.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 10:59:44, grt (UTC plus 2) wrote: > On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > > > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > > > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > > > > Perhaps mention why the no dependencies bit is important ("making it > > > > suitable > > > > > > for use in other dependency-free libraries"). > > > > > > > > > > Done > > > > > > > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > > > > > > > Not in this library. +pennymac to confirm whether or not chrome_elf has > a > > > test > > > > > for dependencies. > > > > > > > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > > > > > > > > > > > > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... > > > > > > Thanks. I see I forgot to do the actual + thing for pennymac. :-) > > > > > > Do you think that test is sufficient? > > > > The test is imho the barest minimum to prevent regressions. I wasn't able to > > think of anything better at the time. It's not robust against dynamic loading > or > > anything like that. > > > > Worth checking with Penny directly if there have been any changes in thinking > > since then, but for a module that wishes to stay dependency free, having a > > similar test is a good idea. > > I'll run off and try to make something simple for this library while checking > with pennymac. Thanks for the advice. Hmm. My something simple was to make a dummy dll and have a test that would check its imports. This doesn't pan out unless the dummy dll exercises every function of the install_static library. Since chrome_elf is the thing that really needs to not have excess dependencies, it seems to me that that's really the right place to evaluate them. What do you two (robertshield and pennymac) think?
https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 14:20:33, grt (UTC plus 2) wrote: > On 2016/10/28 10:59:44, grt (UTC plus 2) wrote: > > On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > > > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > > > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > > > > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > > > > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > > > > > Perhaps mention why the no dependencies bit is important ("making it > > > > > suitable > > > > > > > for use in other dependency-free libraries"). > > > > > > > > > > > > Done > > > > > > > > > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > > > > > > > > > Not in this library. +pennymac to confirm whether or not chrome_elf > has > > a > > > > test > > > > > > for dependencies. > > > > > > > > > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > > > > > > > > > > > > > > > > > > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... > > > > > > > > Thanks. I see I forgot to do the actual + thing for pennymac. :-) > > > > > > > > Do you think that test is sufficient? > > > > > > The test is imho the barest minimum to prevent regressions. I wasn't able to > > > think of anything better at the time. It's not robust against dynamic > loading > > or > > > anything like that. > > > > > > Worth checking with Penny directly if there have been any changes in > thinking > > > since then, but for a module that wishes to stay dependency free, having a > > > similar test is a good idea. > > > > I'll run off and try to make something simple for this library while checking > > with pennymac. Thanks for the advice. > > Hmm. My something simple was to make a dummy dll and have a test that would > check its imports. This doesn't pan out unless the dummy dll exercises every > function of the install_static library. Since chrome_elf is the thing that > really needs to not have excess dependencies, it seems to me that that's really > the right place to evaluate them. What do you two (robertshield and pennymac) > think? Ah right. s/every function/every exported function/ I guess, but as you say if chrome_elf depends on install_static and is the only thing that has the no-dependencies requirement, then the existing chrome_elf test should suffice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
watcher and module-passing details lgtm, modulo a couple of nits. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > On 2016/10/24 15:04:06, Sigurður Ásgeirsson wrote: > > On 2016/10/16 09:15:25, grt (UTC plus 2) wrote: > > > On 2016/10/14 21:09:12, Sigurður Ásgeirsson wrote: > > > > I'm not sure I understand why this can't profitably be a POD pointer. > As-is, > > > it > > > > seems we're passing InstallDetails::Payload* from module to module, cast > to > > > > intptr_t? > > > > > > Yes. I figure that Payload* is an implementation detail that callers of this > > > don't need to be concerned with. You don't like? > > > > I generally lean towards compiler-enforced typing wherever practical, so not > > overly fond of integral types for passing pointers. > > Okay. I've made Payload public but not done any other rigmarole. > > > Also, I wonder whether there's room and reason to add signature or version or > > size or some other identifier to Payload. I figure there's the usual > incentives > > for diverting the flow of data here - would be nice to at least detect skew? > > One thing I can imagine, which should be fairly simple, is to take a dependency > on //components/version_info:generate_version_info, put a ptr to a string > holding PRODUCT_VERSION into the payload in PrimaryInstallDetails, and check for > a match in InitializeForModule. How does that sound to you? What action should > we take on mismatch? CHECK? DumpWithoutCrash? SGTM - I'm not sure what action is best, DumpWithoutCrash sounds reasonable, also maybe schedule a Ward report? :) https://codereview.chromium.org/2422643002/diff/280001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:192: chrome::kChromeElfDllName); ah - nice! https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/google_chrome_install_modes.cc (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/google_chrome_install_modes.cc:38: L"{4ea16ac7-fd5a-47c3-875b-dbf4a2008c20}", ubenit: this GUID is lower case, the other one is upper. Makes my poor OCD eyes bleed :). https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_constants.h (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_constants.h:32: struct InstallConstants { Because an array of this structure is passed across modules, I would be tempted to start this structure with a size_t size = sizeof(InstallContants); field, and to assert on a common understanding of the size on the far side. I have a feeling this'll end up making diagnosis of weird problems easier in due course. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_details.cc (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.cc:70: // Intentionally leaked at shutdown. here is probably a good bottleneck to assert on a common understanding of module-traversing PODs. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.h:30: struct Payload { this structure also goes across modules and may also evolve separately from InstallConstants, so maybe another size field here for belt-and-suspenders? https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.h:89: // path for the binaries if |binaries| and Chrome is multi-install. Otherwise, as an outside reader, I don't quite understand these comments. Are we talking registry paths in all cases here, or are we talking file paths, or both?
Thanks. PTAL. https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/60001/chrome/install_static/i... chrome/install_static/install_details.h:84: static intptr_t GetPayload(); On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > On 2016/10/24 15:04:06, Sigurður Ásgeirsson wrote: > > > On 2016/10/16 09:15:25, grt (UTC plus 2) wrote: > > > > On 2016/10/14 21:09:12, Sigurður Ásgeirsson wrote: > > > > > I'm not sure I understand why this can't profitably be a POD pointer. > > As-is, > > > > it > > > > > seems we're passing InstallDetails::Payload* from module to module, cast > > to > > > > > intptr_t? > > > > > > > > Yes. I figure that Payload* is an implementation detail that callers of > this > > > > don't need to be concerned with. You don't like? > > > > > > I generally lean towards compiler-enforced typing wherever practical, so not > > > overly fond of integral types for passing pointers. > > > > Okay. I've made Payload public but not done any other rigmarole. > > > > > Also, I wonder whether there's room and reason to add signature or version > or > > > size or some other identifier to Payload. I figure there's the usual > > incentives > > > for diverting the flow of data here - would be nice to at least detect skew? > > > > One thing I can imagine, which should be fairly simple, is to take a > dependency > > on //components/version_info:generate_version_info, put a ptr to a string > > holding PRODUCT_VERSION into the payload in PrimaryInstallDetails, and check > for > > a match in InitializeForModule. How does that sound to you? What action should > > we take on mismatch? CHECK? DumpWithoutCrash? > > SGTM - I'm not sure what action is best, DumpWithoutCrash sounds reasonable, > also maybe schedule a Ward report? :) Went with DWC from ChromeMain. We can do something more sophisticated in the future if we find it is ever hit. https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... File chrome/install_static/README.md (right): https://codereview.chromium.org/2422643002/diff/140001/chrome/install_static/... chrome/install_static/README.md:6: kernel32.dll and version.dll. On 2016/10/28 14:34:21, robertshield_slow_reviews wrote: > On 2016/10/28 14:20:33, grt (UTC plus 2) wrote: > > On 2016/10/28 10:59:44, grt (UTC plus 2) wrote: > > > On 2016/10/28 06:56:52, robertshield_slow_reviews wrote: > > > > On 2016/10/24 19:36:00, grt (UTC plus 2) wrote: > > > > > On 2016/10/24 14:59:26, robertshield_slow_reviews wrote: > > > > > > On 2016/10/24 11:17:51, grt (UTC plus 2) wrote: > > > > > > > On 2016/10/17 05:27:23, robertshield_slow_reviews wrote: > > > > > > > > Perhaps mention why the no dependencies bit is important ("making > it > > > > > > suitable > > > > > > > > for use in other dependency-free libraries"). > > > > > > > > > > > > > > Done > > > > > > > > > > > > > > > Is this dependency-free nature enforced by tests or otherwise? > > > > > > > > > > > > > > Not in this library. +pennymac to confirm whether or not chrome_elf > > has > > > a > > > > > test > > > > > > > for dependencies. > > > > > > > > > > > > Elf has a now-release-only-because-gn-source-sets (:-() test here: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codesearch.chromium.org/chromium/src/chrome_elf/elf_imports_unittest.... > > > > > > > > > > Thanks. I see I forgot to do the actual + thing for pennymac. :-) > > > > > > > > > > Do you think that test is sufficient? > > > > > > > > The test is imho the barest minimum to prevent regressions. I wasn't able > to > > > > think of anything better at the time. It's not robust against dynamic > > loading > > > or > > > > anything like that. > > > > > > > > Worth checking with Penny directly if there have been any changes in > > thinking > > > > since then, but for a module that wishes to stay dependency free, having a > > > > similar test is a good idea. > > > > > > I'll run off and try to make something simple for this library while > checking > > > with pennymac. Thanks for the advice. > > > > Hmm. My something simple was to make a dummy dll and have a test that would > > check its imports. This doesn't pan out unless the dummy dll exercises every > > function of the install_static library. Since chrome_elf is the thing that > > really needs to not have excess dependencies, it seems to me that that's > really > > the right place to evaluate them. What do you two (robertshield and pennymac) > > think? > > Ah right. > > s/every function/every exported function/ I guess, but as you say if chrome_elf > depends on install_static and is the only thing that has the no-dependencies > requirement, then the existing chrome_elf test should suffice. Acknowledged. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/google_chrome_install_modes.cc (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/google_chrome_install_modes.cc:38: L"{4ea16ac7-fd5a-47c3-875b-dbf4a2008c20}", On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > ubenit: this GUID is lower case, the other one is upper. Makes my poor OCD eyes > bleed :). This is the way it's always been (values copied from google_chrome_distribution.cc and google_chrome_sxs_distribution.cc). I'm inclined to leave them as-is despite the bleeding. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_constants.h (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_constants.h:32: struct InstallConstants { On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > Because an array of this structure is passed across modules, I would be tempted > to start this structure with a > > size_t size = sizeof(InstallContants); > > field, and to assert on a common understanding of the size on the far side. I > have a feeling this'll end up making diagnosis of weird problems easier in due > course. Done. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_details.cc (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.cc:70: // Intentionally leaked at shutdown. On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > here is probably a good bottleneck to assert on a common understanding of > module-traversing PODs. Yes. Sadly, it's not possible to trigger a dump here due to dependency issues, so I've put that in chrome_main.cc after DumpWithoutCrashing has been set up. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... File chrome/install_static/install_details.h (right): https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.h:30: struct Payload { On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > this structure also goes across modules and may also evolve separately from > InstallConstants, so maybe another size field here for belt-and-suspenders? Done. https://codereview.chromium.org/2422643002/diff/280001/chrome/install_static/... chrome/install_static/install_details.h:89: // path for the binaries if |binaries| and Chrome is multi-install. Otherwise, On 2016/10/28 15:44:16, Sigurður Ásgeirsson wrote: > as an outside reader, I don't quite understand these comments. Are we talking > registry paths in all cases here, or are we talking file paths, or both? Registry only. I've added a little more color to the comments, but don't want to go into too much detail here. Do you think much is needed? I could remove these and instead have private helpers in install_util.cc.
grt@chromium.org changed reviewers: + bradnelson@chromium.org, thakis@chromium.org
grt@chromium.org changed reviewers: + phajdan.jr@chromium.org, phajdan@google.com
OWNERS reviews, please: bradnelson: - components/nacl/broker/BUILD.gn - chrome/nacl/* thakis: - new dependency in chrome/install_static on components/version_info/version_info_values.h - new test-only dependency in chrome/install_static on base phajdan.jr: - chrome/test/base/chrome_test_launcher.cc
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chrome/test LGTM
my files lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+CC gab as an FYI, no need for your eyes on the review.
still lgtm
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
grt@chromium.org changed reviewers: + dschuff@chromium.org - phajdan@google.com
dschuff: could you do an OWNERS review of the NACL bits? Thanks.
On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: > dschuff: could you do an OWNERS review of the NACL bits? Thanks. @bradn: IIUC nacl_exe_win64.cc has the entry point for both the broker process *and* the actual NaCl loader process, right? If so, I want to make sure this is actually necessary to inject into the process (especially for the NaCl loader/helper process, as it could have security implications). @grt: Is this just being added to all the processes, or is there some specific reason that it's needed by the NaCl loader?
On 2016/11/02 16:12:14, Derek Schuff wrote: > On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: > > dschuff: could you do an OWNERS review of the NACL bits? Thanks. > > @bradn: IIUC nacl_exe_win64.cc has the entry point for both the broker process > *and* the actual NaCl loader process, right? If so, I want to make sure this is > actually necessary to inject into the process (especially for the NaCl > loader/helper process, as it could have security implications). > @grt: Is this just being added to all the processes, or is there some specific > reason that it's needed by the NaCl loader? It's needed for any process that connects to crashpad or uses chrome/installer/util. Thanks for taking a look.
On 2016/11/02 16:15:26, grt (UTC plus 1) wrote: > On 2016/11/02 16:12:14, Derek Schuff wrote: > > On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: > > > dschuff: could you do an OWNERS review of the NACL bits? Thanks. > > > > @bradn: IIUC nacl_exe_win64.cc has the entry point for both the broker process > > *and* the actual NaCl loader process, right? If so, I want to make sure this > is > > actually necessary to inject into the process (especially for the NaCl > > loader/helper process, as it could have security implications). > > @grt: Is this just being added to all the processes, or is there some specific > > reason that it's needed by the NaCl loader? > > It's needed for any process that connects to crashpad or uses > chrome/installer/util. Thanks for taking a look. Hi Derek and Brad. Is there any more info I can share with you to help you with this review? If so, please let me know how I can be of service. Otherwise, if this looks good to you, please feel at liberty to check the CQ box, as a NaCL OWNER's lgtm is the last one I'm waiting for. Thanks!
On 2016/11/04 21:32:32, grt (UTC plus 1) wrote: > On 2016/11/02 16:15:26, grt (UTC plus 1) wrote: > > On 2016/11/02 16:12:14, Derek Schuff wrote: > > > On 2016/11/02 09:12:29, grt (UTC plus 1) wrote: > > > > dschuff: could you do an OWNERS review of the NACL bits? Thanks. > > > > > > @bradn: IIUC nacl_exe_win64.cc has the entry point for both the broker > process > > > *and* the actual NaCl loader process, right? If so, I want to make sure this > > is > > > actually necessary to inject into the process (especially for the NaCl > > > loader/helper process, as it could have security implications). > > > @grt: Is this just being added to all the processes, or is there some > specific > > > reason that it's needed by the NaCl loader? > > > > It's needed for any process that connects to crashpad or uses > > chrome/installer/util. Thanks for taking a look. > > Hi Derek and Brad. Is there any more info I can share with you to help you with > this review? If so, please let me know how I can be of service. Otherwise, if > this looks good to you, please feel at liberty to check the CQ box, as a NaCL > OWNER's lgtm is the last one I'm waiting for. Thanks! ping
Sorry was out of the office last week. LGTM
The CQ bit was checked by dschuff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, siggi@chromium.org, thakis@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2422643002/#ps320001 (title: "fix chromium channel name -- it should be empty rather than unknown")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/08 19:00:47, Derek Schuff wrote: > Sorry was out of the office last week. LGTM Groovy. Thanks.
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/92bc27b8b101d2b300a7b3afd79265b3cfba1152 Cr-Commit-Position: refs/heads/master@{#430728}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:320001) has been created in https://codereview.chromium.org/2491463002/ by grt@chromium.org. The reason for reverting is: Causes a crash at uninstall; see https://crbug.com/663703..
Message was sent while issue was closed.
sync to position 430922
Message was sent while issue was closed.
fixed uninstall crash and a bad constant for reland
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ==========
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Robert. The diff between patch set 16 and 17 contains two fixes to this. Would you please take a look? Feel free to hit the CQ bit if it looks good to you. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sync to position 431863
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertshield@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org, thakis@chromium.org, dschuff@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2422643002/#ps380001 (title: "sync to position 431863")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_x6...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#430728} ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/e29f8ab494ddbb5c85407d544204c5168e5028d8 Cr-Commit-Position: refs/heads/master@{#432270}
Message was sent while issue was closed.
Zombie review comment: https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, What's the point of this constant? Maybe there could be a comment explaining this? (Background: I'm looking at turning on -Wunreachable-code, and I'm getting things like ../../chrome/install_static/install_details_unittest.cc(27,28): error: code will never be executed [-Werror,-Wunreachable-code] constants.app_guid = L"testguid"; ^~~~~~~~~~~ and I'm having trouble making sense of the warning. It's obviously correct, but I'm not sure what to to about it since I don't understand what this constant here is for.)
Message was sent while issue was closed.
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/11 01:12:24, Nico wrote: > What's the point of this constant? It controls whether or not Omaha integration is included in the build. A buildflag would have been a better choice. I think it would be nice to switch from this to a buildflag at some point. > Maybe there could be a comment explaining this? The comment is in install_modes.h: // - kUseGoogleUpdateIntegration: true or false indicating whether or not the // brand uses Chrome's integration with Google Update. Google Chrome does, // while Chromium does not.
Message was sent while issue was closed.
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/11 12:31:37, grt (UTC plus 2) wrote: > On 2017/09/11 01:12:24, Nico wrote: > > What's the point of this constant? > > It controls whether or not Omaha integration is included in the build. A > buildflag would have been a better choice. I think it would be nice to switch > from this to a buildflag at some point. > > > Maybe there could be a comment explaining this? > > The comment is in install_modes.h: > > // - kUseGoogleUpdateIntegration: true or false indicating whether or not the > // brand uses Chrome's integration with Google Update. Google Chrome does, > // while Chromium does not. Thanks for the explanation. I still don't quite understand it: How do we set this bool to true? Does the release builder carry a local patch to this file?
Message was sent while issue was closed.
https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... File chrome/install_static/chromium_install_modes.h (right): https://codereview.chromium.org/2422643002/diff/380001/chrome/install_static/... chrome/install_static/chromium_install_modes.h:13: kUseGoogleUpdateIntegration = false, On 2017/09/14 21:53:20, Nico wrote: > On 2017/09/11 12:31:37, grt (UTC plus 2) wrote: > > On 2017/09/11 01:12:24, Nico wrote: > > > What's the point of this constant? > > > > It controls whether or not Omaha integration is included in the build. A > > buildflag would have been a better choice. I think it would be nice to switch > > from this to a buildflag at some point. > > > > > Maybe there could be a comment explaining this? > > > > The comment is in install_modes.h: > > > > // - kUseGoogleUpdateIntegration: true or false indicating whether or not the > > // brand uses Chrome's integration with Google Update. Google Chrome does, > > // while Chromium does not. > > Thanks for the explanation. I still don't quite understand it: How do we set > this bool to true? Does the release builder carry a local patch to this file? No. The value is unconditionally false for Chromium builds. This file (chromium_install_modes.h) is a brand-specific file for Chromium. The corresponding file for Google Chrome (google_chrome_install_modes.h) sets it to true for Google Chrome-branded builds.
Message was sent while issue was closed.
Aha! And then we conditionally include one of those two headers based on build type? On Sep 15, 2017 7:25 AM, <grt@chromium.org> wrote: > > 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: > > On 2017/09/11 12:31:37, grt (UTC plus 2) wrote: > > > On 2017/09/11 01:12:24, Nico wrote: > > > > What's the point of this constant? > > > > > > It controls whether or not Omaha integration is included in the > build. A > > > buildflag would have been a better choice. I think it would be nice > to switch > > > from this to a buildflag at some point. > > > > > > > Maybe there could be a comment explaining this? > > > > > > The comment is in install_modes.h: > > > > > > // - kUseGoogleUpdateIntegration: true or false indicating whether > or not the > > > // brand uses Chrome's integration with Google Update. Google > Chrome does, > > > // while Chromium does not. > > > > Thanks for the explanation. I still don't quite understand it: How do > we set > > this bool to true? Does the release builder carry a local patch to > this file? > > No. The value is unconditionally false for Chromium builds. This file > (chromium_install_modes.h) is a brand-specific file for Chromium. The > corresponding file for Google Chrome (google_chrome_install_modes.h) > sets it to true for Google Chrome-branded builds. > > https://codereview.chromium.org/2422643002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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... |
