Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1475643004:
Add crash keys for the installer covering simple InstallerState fields. (Closed)
|
Created:
5 years ago by Joe Mason Modified:
5 years ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd crash keys for the installer covering simple InstallerState fields.
BUG=558552
Committed: https://crrev.com/dd966dbaf7b2669d5b27fda1a0d65d3e6688525d
Cr-Commit-Position: refs/heads/master@{#362036}
Patch Set 1 #Patch Set 2 : Add state_key #Patch Set 3 : Add previous version, plus some comments #
Total comments: 13
Patch Set 4 : Refactor, remove current-version #Patch Set 5 : Move command-line switch refactor to another patch for easier review #
Total comments: 27
Patch Set 6 : Fix review comments #
Total comments: 12
Patch Set 7 : More review comments[H #Patch Set 8 : Missed include in last patch #
Total comments: 3
Patch Set 9 : Fix nits #Patch Set 10 : Convert state_key from wstring to string16 #Patch Set 11 : Tweak GN build #
Total comments: 1
Patch Set 12 : Move include file to fix last nit #
Dependent Patchsets: Messages
Total messages: 41 (14 generated)
joenotcharles@chromium.org changed reviewers: + grt@chromium.org
This is all the simple stuff now. Will start new reviews for the more complex (ap value, full or diff update)
Looks pretty good. Some suggestions below. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_constants.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_constants.cc:43: const char kPackageType[] = "package-type"; since this is keyed off the --multi-install command line switch, i think "multi-install" with values of ("false", "true") will be easier to understand on the backend. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_constants.cc:44: const char kPrivilegeLevel[] = "priv-level"; likewise, i think "system-level" with ("false", "true") is better here. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1337: void SetStartingCrashKeys(const InstallerState& installer_state, wdyt of moving this and the impl of InstallerCrashReporterClient::RegisterCrashKeys() out into an installer_crash_keys.{cc,h} pair? this file can expose helper functions called at various points of installation to set/clear crash keys as appropriate. this mirrors chrome/common/crash_keys.cc a bit, so it will also help keep the installer's handling of crash keys more conceptually similar to Chrome's. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1340: scoped_ptr<base::Version> current_version( CheckPreInstallConditions mutates |installer_state|, so GetCurrentVersion should not be pulled from it until after that func is called in InstallProducts. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1347: switch (installer_state.state_type()) { i think this is cleaner like so: const char* DistributionTypeToString(BrowserDistribution::Type type) { switch (type) { case BrowserDistribution::CHROME_BROWSER: return "chrome browser"; case BrowserDistribution::CHROME_FRAME: return "chrome frame"; case BrowserDistribution::CHROME_BINARIES: return "chrome binaries"; case BrowserDistribution::NUM_TYPES: break; } NOTREACHED(); return ""; } void SetStartingCrashKeys(...) { base::debug::SetCrashKeyValue( installer::crash_keys::kDistributionType, DistributionTypeToString(installer_state.state_type())); } https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1362: assert(false); use NOTREACHED() (to crash debug builds) or CHECK(FALSE) (to crash debug and release builds) rather than assert. CHECK(FALSE) should generally be used sparingly for things that represent security vulns where continuing execution would be bad. in this case, the invalid value should never happen in practice, so NOTREACHED() is the right choice.
https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1337: void SetStartingCrashKeys(const InstallerState& installer_state, On 2015/11/25 16:44:09, grt wrote: > wdyt of moving this and the impl of > InstallerCrashReporterClient::RegisterCrashKeys() out into an > installer_crash_keys.{cc,h} pair? this file can expose helper functions called > at various points of installation to set/clear crash keys as appropriate. this > mirrors chrome/common/crash_keys.cc a bit, so it will also help keep the > installer's handling of crash keys more conceptually similar to Chrome's. Good idea. It will help with the more invasive keys like AP value. I'll do it in a followup when I implement that. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1347: switch (installer_state.state_type()) { On 2015/11/25 16:44:09, grt wrote: > i think this is cleaner like so: > > const char* DistributionTypeToString(BrowserDistribution::Type type) { > switch (type) { > case BrowserDistribution::CHROME_BROWSER: > return "chrome browser"; > case BrowserDistribution::CHROME_FRAME: > return "chrome frame"; > case BrowserDistribution::CHROME_BINARIES: > return "chrome binaries"; > case BrowserDistribution::NUM_TYPES: > break; > } > NOTREACHED(); > return ""; > } What do you think of making this a BrowserDistribution method?
https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_constants.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_constants.cc:43: const char kPackageType[] = "package-type"; On 2015/11/25 16:44:09, grt wrote: > since this is keyed off the --multi-install command line switch, i think > "multi-install" with values of ("false", "true") will be easier to understand on > the backend. Shouldn't we plan for expansion such as adding further package types? (I'm following the example of the comment saying InstallerState::is_multi_install is deprecated, so trying to map to the enum values instead of a bool.)
https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_constants.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_constants.cc:43: const char kPackageType[] = "package-type"; On 2015/11/25 17:20:48, joenotcharles wrote: > On 2015/11/25 16:44:09, grt wrote: > > since this is keyed off the --multi-install command line switch, i think > > "multi-install" with values of ("false", "true") will be easier to understand > on > > the backend. > > Shouldn't we plan for expansion such as adding further package types? (I'm > following the example of the comment saying InstallerState::is_multi_install is > deprecated, so trying to map to the enum values instead of a bool.) Yeah, about that... :-) We're working on removing the concept of package types and multi-install. There will never be more than single and multi, and multi will go away leaving only single. When that happens we'll delete this crash key altogether. https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1347: switch (installer_state.state_type()) { On 2015/11/25 17:16:34, joenotcharles wrote: > On 2015/11/25 16:44:09, grt wrote: > > i think this is cleaner like so: > > > > const char* DistributionTypeToString(BrowserDistribution::Type type) { > > switch (type) { > > case BrowserDistribution::CHROME_BROWSER: > > return "chrome browser"; > > case BrowserDistribution::CHROME_FRAME: > > return "chrome frame"; > > case BrowserDistribution::CHROME_BINARIES: > > return "chrome binaries"; > > case BrowserDistribution::NUM_TYPES: > > break; > > } > > NOTREACHED(); > > return ""; > > } > > What do you think of making this a BrowserDistribution method? I think of this mapping being specific to the crash keys, so I think it belongs with the crash key impl.
https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1340: scoped_ptr<base::Version> current_version( On 2015/11/25 16:44:09, grt wrote: > CheckPreInstallConditions mutates |installer_state|, so GetCurrentVersion should > not be pulled from it until after that func is called in InstallProducts. installer_state is also mutated by HandleNonInstallCmdLineOptions and UninstallMultiChromeFrameIfPresent. Do you think it's worth it to set the current version key at startup and then update it with each mutation?
https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... chrome/installer/setup/setup_main.cc:1340: scoped_ptr<base::Version> current_version( On 2015/11/25 20:12:52, joenotcharles wrote: > On 2015/11/25 16:44:09, grt wrote: > > CheckPreInstallConditions mutates |installer_state|, so GetCurrentVersion > should > > not be pulled from it until after that func is called in InstallProducts. > > installer_state is also mutated by HandleNonInstallCmdLineOptions and > UninstallMultiChromeFrameIfPresent. Do you think it's worth it to set the > current version key at startup and then update it with each mutation? I think it would be fine to set this crash key within install.cc's InstallNewVersion when GetCurrentVersion is ordinarily called. I don't think it's necessary to repeatedly update this particular key.
Description was changed from ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 ========== to ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 ==========
joenotcharles@chromium.org changed reviewers: + rsesek@chromium.org
joenotcharles@chromium.org changed reviewers: - rsesek@chromium.org
On 2015/11/25 16:44:09, grt wrote: > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_constants.cc:43: const char kPackageType[] = > "package-type"; > since this is keyed off the --multi-install command line switch, i think > "multi-install" with values of ("false", "true") will be easier to understand on > the backend. Done. > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_constants.cc:44: const char kPrivilegeLevel[] = > "priv-level"; > likewise, i think "system-level" with ("false", "true") is better here. Done. > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1337: void SetStartingCrashKeys(const > InstallerState& installer_state, > wdyt of moving this and the impl of > InstallerCrashReporterClient::RegisterCrashKeys() out into an > installer_crash_keys.{cc,h} pair? this file can expose helper functions called > at various points of installation to set/clear crash keys as appropriate. this > mirrors chrome/common/crash_keys.cc a bit, so it will also help keep the > installer's handling of crash keys more conceptually similar to Chrome's. Done. > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1340: scoped_ptr<base::Version> > current_version( > CheckPreInstallConditions mutates |installer_state|, so GetCurrentVersion should > not be pulled from it until after that func is called in InstallProducts. I removed current version from this patch because I'm still not sold on where to call it from. Will add it back in a followup. > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1347: switch (installer_state.state_type()) > { > i think this is cleaner like so: > > const char* DistributionTypeToString(BrowserDistribution::Type type) { Done. > https://codereview.chromium.org/1475643004/diff/40001/chrome/installer/setup/... > chrome/installer/setup/setup_main.cc:1362: assert(false); > use NOTREACHED() (to crash debug builds) or CHECK(FALSE) (to crash debug and > release builds) rather than assert. CHECK(FALSE) should generally be used > sparingly for things that represent security vulns where continuing execution > would be bad. in this case, the invalid value should never happen in practice, > so NOTREACHED() is the right choice. Done.
https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:110: void ConfigureCrashReporting(const InstallerState& installer_state) { this does a lot more than just operate on/for crash keys, so this file isn't obviously the natural place for it. wdyt of pulling lines 125-132 out of this function and into InitCrashKeys() in this file? the rest of this function can stay where it was in setup_main.cc with a call to installer::InitCrashKeys() at the right place. could this new InitCrashKeys also do all of the work in SetStartingCrashKeys? https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:143: void RegisterCrashKeys(std::vector<base::debug::CrashKey>& keys) { out params must be passed by non-const pointer rather than non-const ref (https://google.github.io/styleguide/cppguide.html#Reference_Arguments). https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:157: keys.insert(keys.end(), std::begin(kFixedKeys), std::end(kFixedKeys)); go c++11! https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:161: base::debug::SetCrashKeyValue(kDistributionType, DistributionTypeToString( consider "using base::debug::SetCrashKeyValue;" at the top of this function if that will reduce wrapping and make the function more readable below. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:165: base::debug::SetCrashKeyValue(kIsMultiInstall, PackageTypeToString( nit: the following is easier to read: installer_state.is_multi_install() ? "true" : "false" https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:167: base::debug::SetCrashKeyValue(kIsSystemLevel, SystemLevelToString( installer_state.system_install() ? "true" : "false" https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:171: if (!state_key.empty()) { formatting nit: if (!state_key.empty()) base::debug::SetCrashKeyValue(kStateKey, base::UTF16ToUTF8(state_key)); https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.h (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:7: #include <vector> https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:14: void ConfigureCrashReporting(const InstallerState& installer_state); please add a doc comment for each of these (https://google.github.io/styleguide/cppguide.html#Function_Comments) https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:16: void RegisterCrashKeys(std::vector<base::debug::CrashKey>& keys); GetCrashKeys is more accurate since this doesn't actually register them. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:18: void SetStartingCrashKeys(const InstallerState& installer_state); nit: i prefer SetInitialCrashKeys https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_reporter_client.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_reporter_client.cc:130: return base::debug::InitCrashKeys(&keys.at(0), keys.size(), &keys.at(0) -> keys.data()
https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:110: void ConfigureCrashReporting(const InstallerState& installer_state) { On 2015/11/26 19:24:45, grt wrote: > this does a lot more than just operate on/for crash keys, so this file isn't > obviously the natural place for it. wdyt of pulling lines 125-132 out of this > function and into InitCrashKeys() in this file? the rest of this function can > stay where it was in setup_main.cc with a call to installer::InitCrashKeys() at > the right place. What about just renaming installer_crash_keys.* to installer_crash_reporting.*? I think everything in this function is closely related enough to be worth keeping together. > could this new InitCrashKeys also do all of the work in SetStartingCrashKeys? I prefer to keep SetStartingCrashKeys separate to give the freedom to change the parameter list or move the invocation around as more crash keys are added. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:165: base::debug::SetCrashKeyValue(kIsMultiInstall, PackageTypeToString( On 2015/11/26 19:24:45, grt wrote: > nit: the following is easier to read: > installer_state.is_multi_install() ? "true" : "false" I agree, but I'm following the comment in InstallerState saying to deprecate is_multi_install in favour of package_type.
https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:110: void ConfigureCrashReporting(const InstallerState& installer_state) { On 2015/11/26 19:47:02, joenotcharles wrote: > On 2015/11/26 19:24:45, grt wrote: > > this does a lot more than just operate on/for crash keys, so this file isn't > > obviously the natural place for it. wdyt of pulling lines 125-132 out of this > > function and into InitCrashKeys() in this file? the rest of this function can > > stay where it was in setup_main.cc with a call to installer::InitCrashKeys() > at > > the right place. > > What about just renaming installer_crash_keys.* to installer_crash_reporting.*? > I think everything in this function is closely related enough to be worth > keeping together. Ack. > > could this new InitCrashKeys also do all of the work in SetStartingCrashKeys? > > I prefer to keep SetStartingCrashKeys separate to give the freedom to change the > parameter list or move the invocation around as more crash keys are added. Ack. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:165: base::debug::SetCrashKeyValue(kIsMultiInstall, PackageTypeToString( On 2015/11/26 19:47:02, joenotcharles wrote: > On 2015/11/26 19:24:45, grt wrote: > > nit: the following is easier to read: > > installer_state.is_multi_install() ? "true" : "false" > > I agree, but I'm following the comment in InstallerState saying to deprecate > is_multi_install in favour of package_type. As the author of that comment, I authorize you to completely disregard it (and even remove it). :-)
On 2015/11/26 19:50:35, grt wrote: > As the author of that comment, I authorize you to completely disregard it (and > even remove it). :-) Done.
https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:143: void RegisterCrashKeys(std::vector<base::debug::CrashKey>& keys) { On 2015/11/26 19:24:45, grt wrote: > out params must be passed by non-const pointer rather than non-const ref > (https://google.github.io/styleguide/cppguide.html#Reference_Arguments). Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:161: base::debug::SetCrashKeyValue(kDistributionType, DistributionTypeToString( On 2015/11/26 19:24:45, grt wrote: > consider "using base::debug::SetCrashKeyValue;" at the top of this function if > that will reduce wrapping and make the function more readable below. Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:165: base::debug::SetCrashKeyValue(kIsMultiInstall, PackageTypeToString( On 2015/11/26 19:24:45, grt wrote: > nit: the following is easier to read: > installer_state.is_multi_install() ? "true" : "false" Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:167: base::debug::SetCrashKeyValue(kIsSystemLevel, SystemLevelToString( On 2015/11/26 19:24:45, grt wrote: > installer_state.system_install() ? "true" : "false" Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:171: if (!state_key.empty()) { On 2015/11/26 19:24:45, grt wrote: > formatting nit: > if (!state_key.empty()) > base::debug::SetCrashKeyValue(kStateKey, base::UTF16ToUTF8(state_key)); Done. (You're talking about the extra line break, right? I'd prefer to keep the braces. The style guide says, "In general, curly braces are not required for single-line statements, but they are allowed if you like them," and I'm reveling in no longer having to follow the WebKit style guide that bans them absolutely.) https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.h (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:7: On 2015/11/26 19:24:45, grt wrote: > #include <vector> Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:14: void ConfigureCrashReporting(const InstallerState& installer_state); On 2015/11/26 19:24:45, grt wrote: > please add a doc comment for each of these > (https://google.github.io/styleguide/cppguide.html#Function_Comments) Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:16: void RegisterCrashKeys(std::vector<base::debug::CrashKey>& keys); On 2015/11/26 19:24:45, grt wrote: > GetCrashKeys is more accurate since this doesn't actually register them. Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.h:18: void SetStartingCrashKeys(const InstallerState& installer_state); On 2015/11/26 19:24:45, grt wrote: > nit: i prefer SetInitialCrashKeys Done. https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_reporter_client.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_reporter_client.cc:130: return base::debug::InitCrashKeys(&keys.at(0), keys.size(), On 2015/11/26 19:24:45, grt wrote: > &keys.at(0) -> keys.data() Done.
https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... File chrome/installer/setup/installer_crash_keys.cc (right): https://codereview.chromium.org/1475643004/diff/80001/chrome/installer/setup/... chrome/installer/setup/installer_crash_keys.cc:171: if (!state_key.empty()) { On 2015/11/26 20:49:05, joenotcharles wrote: > On 2015/11/26 19:24:45, grt wrote: > > formatting nit: > > if (!state_key.empty()) > > base::debug::SetCrashKeyValue(kStateKey, base::UTF16ToUTF8(state_key)); > > Done. > > (You're talking about the extra line break, right? I'd prefer to keep the > braces. > > The style guide says, "In general, curly braces are not required for single-line > statements, but they are allowed if you like them," and I'm reveling in no > longer having to follow the WebKit style guide that bans them absolutely.) By and large, Chromium does not use them for single-line conditionals like this, and the installer in particular definitely does not (though I'm sure you'll find exceptions if you look). The relevant style rule is "Be consistent with existing code" (https://google.github.io/styleguide/cppguide.html#Goals), so please remove them here. You can try using them elsewhere in Chromium, but it will be at the whim of the reviewer. :-) https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:32: DCHECK(breakpad::CrashKeysWin::keeper()); #include "base/logging.h" https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:127: DCHECK(keys); checking arguments like this should be the first thing in the function https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:128: if (!keys) don't try to handle DCHECK failures (https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...). it's better to crash hard on the deref of |keys| than to silently proceed. in the former case, we get a crash report telling us that we've messed up somewhere. in the latter case, we never learn that the code has regressed in some way. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.h (right): https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:16: // Set up the crash reporting system for the installer. nit "Sets up..." as per https://google.github.io/styleguide/cppguide.html#Function_Comments. here and elsewhere. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:19: // Add the installer's default crash keys to |keys|. What does "default" mean? If this is to support other functions that would add other keys to the collection, how do you expect this flexibility to be used? As it is, I don't think the separation between this function and InstallerCrashReporterClient::RegisterCrashKeys is needed. How about changing the latter to: size_t InstallerCrashReporterClient::RegisterCrashKeys() { return installer::RegisterCrashKeys(); } and turning this into size_t RegisterCrashKeys()? https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:22: // Set all crash keys that are available after initializing |installer_state| I think you're describing an implementation detail here (not varying during execution). The salient fact for the caller is that this sets crash keys for which data is available during process startup. The fact that it takes an InstallerState implies that the caller needs to make one of those first, so it doesn't need to be documented.
https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:32: DCHECK(breakpad::CrashKeysWin::keeper()); On 2015/11/26 21:37:18, grt wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:127: DCHECK(keys); On 2015/11/26 21:37:17, grt wrote: > checking arguments like this should be the first thing in the function Done. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:128: if (!keys) On 2015/11/26 21:37:18, grt wrote: > don't try to handle DCHECK failures > (https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...). > it's better to crash hard on the deref of |keys| than to silently proceed. in > the former case, we get a crash report telling us that we've messed up > somewhere. in the latter case, we never learn that the code has regressed in > some way. Done. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.h (right): https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:16: // Set up the crash reporting system for the installer. On 2015/11/26 21:37:18, grt wrote: > nit "Sets up..." as per > https://google.github.io/styleguide/cppguide.html#Function_Comments. here and > elsewhere. Done. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:19: // Add the installer's default crash keys to |keys|. On 2015/11/26 21:37:18, grt wrote: > What does "default" mean? If this is to support other functions that would add > other keys to the collection, how do you expect this flexibility to be used? As > it is, I don't think the separation between this function and > InstallerCrashReporterClient::RegisterCrashKeys is needed. How about changing > the latter to: > > size_t InstallerCrashReporterClient::RegisterCrashKeys() { > return installer::RegisterCrashKeys(); > } > > and turning this into size_t RegisterCrashKeys()? Done. https://codereview.chromium.org/1475643004/diff/100001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:22: // Set all crash keys that are available after initializing |installer_state| On 2015/11/26 21:37:18, grt wrote: > I think you're describing an implementation detail here (not varying during > execution). The salient fact for the caller is that this sets crash keys for > which data is available during process startup. The fact that it takes an > InstallerState implies that the caller needs to make one of those first, so it > doesn't need to be documented. Since these keys don't vary, there's no need to call the function again, unlike the upcoming SetAPValueCrashKey. That's a usage detail. Updated the doc string to be more clear.
nice! lgtm with final nits. https://codereview.chromium.org/1475643004/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1475643004/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:142: const std::wstring state_key = state.state_key(); #include <string> https://codereview.chromium.org/1475643004/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.h (right): https://codereview.chromium.org/1475643004/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:8: #include <vector> unused. :-) https://codereview.chromium.org/1475643004/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.h:10: #include "base/debug/crash_logging.h" move to .cc file
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/1475643004/#ps160001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475643004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475643004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/1475643004/#ps180001 (title: "Convert state_key from wstring to string16")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475643004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475643004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
Can you give patches 9 to 11 a quick look before I submit?
lgtm w/ a nit. thanks! https://codereview.chromium.org/1475643004/diff/200001/chrome/installer/util/... File chrome/installer/util/installer_state.cc (right): https://codereview.chromium.org/1475643004/diff/200001/chrome/installer/util/... chrome/installer/util/installer_state.cc:17: #include "base/strings/string16.h" move this to the .h since it's needed by consumers of the api
Move include file to fix last nit
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/1475643004/#ps220001 (title: "Move include file to fix last nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475643004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475643004/220001
Message was sent while issue was closed.
Description was changed from ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 ========== to ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 ========== to ========== Add crash keys for the installer covering simple InstallerState fields. BUG=558552 Committed: https://crrev.com/dd966dbaf7b2669d5b27fda1a0d65d3e6688525d Cr-Commit-Position: refs/heads/master@{#362036} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/dd966dbaf7b2669d5b27fda1a0d65d3e6688525d Cr-Commit-Position: refs/heads/master@{#362036} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
