chrome/installer/setup/installer_crash_reporting.h - Issue 1475643004: Add crash keys for the installer covering simple InstallerState fields.

Side by Side Diff: chrome/installer/setup/installer_crash_reporting.h

Issue 1475643004: Add crash keys for the installer covering simple InstallerState fields. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Fix review comments Created 5 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « chrome/installer/setup/installer_crash_reporter_client.cc ('k') | chrome/installer/setup/installer_crash_reporting.cc » ('j') | chrome/installer/setup/installer_crash_reporting.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2015 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CHROME_INSTALLER_SETUP_INSTALLER_CRASH_REPORTING_H_

	6 #define CHROME_INSTALLER_SETUP_INSTALLER_CRASH_REPORTING_H_

	7

	8 #include <vector>

	9

	10 #include "base/debug/crash_logging.h"

	11

	12 namespace installer {

	13

	14 class InstallerState;

	15

	16 // Set up the crash reporting system for the installer.
	grt (UTC plus 2) 2015/11/26 21:37:18 nit "Sets up..." as per https://google.github.io/s nit "Sets up..." as per https://google.github.io/styleguide/cppguide.html#Function_Comments. here and elsewhere. Joe Mason 2015/11/26 22:28:54 Done. Show quoted text 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.
	17 void ConfigureCrashReporting(const InstallerState& installer_state);

	18

	19 // Add the installer's default crash keys to \|keys\|.
	grt (UTC plus 2) 2015/11/26 21:37:18 What does "default" mean? If this is to support ot 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()? Joe Mason 2015/11/26 22:28:54 Done. Show quoted text 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.
	20 void GetCrashKeys(std::vector<base::debug::CrashKey>* keys);

	21

	22 // Set all crash keys that are available after initializing \|installer_state\|
	grt (UTC plus 2) 2015/11/26 21:37:18 I think you're describing an implementation detail 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. Joe Mason 2015/11/26 22:28:54 Since these keys don't vary, there's no need to ca Show quoted text 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.
	23 // and do not vary during execution.

	24 void SetInitialCrashKeys(const InstallerState& installer_state);

	25

	26 } // namespace installer

	27

	28 #endif // CHROME_INSTALLER_SETUP_INSTALLER_CRASH_REPORTING_H_

OLD	NEW