Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
913273002:
win: Implementation of CrashReportDatabase for Windows (for C++ Windows readability review) (Closed)
Descriptionwin: Implementation of CrashReportDatabase for Windows (for C++ Windows readability review)
Original CL (review, misc support code changes) at https://codereview.chromium.org/867363003/.
Crashpad is a component of Chrome used for capturing crashes in the field and uploading them to crash/ for analysis: https://code.google.com/p/crashpad/.
BUG=crashpad:1
, b/19354950
R=mark@chromium.org, pkasting@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/bd77b3034fb9d759968d30abc111bc491362eb6c
Patch Set 1 #
Total comments: 183
Patch Set 2 : address feedback #Patch Set 3 : . #
Total comments: 8
Patch Set 4 : more changes #Patch Set 5 : more fixes #
Total comments: 6
Patch Set 6 : feedback, add test for report being removed #
Total comments: 8
Patch Set 7 : check unlink return, add comment to MFRR ctor #
Total comments: 4
Patch Set 8 : fix test when completing report moves it #Patch Set 9 : better unlink checks #
Messages
Total messages: 38 (11 generated)
Message was sent while issue was closed.
Patchset #1 (id:1) has been deleted
scottmg@chromium.org changed reviewers: + pkasting@chromium.org
Thanks for participating in the readability process! The change seems largely free of Google Style Guide violations. Most of the comments below are either suggestions, non-style comments, or things not covered by the Google style guide but part of Chromium style or common usage. I don't foresee needing to do many rounds of feedback here. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:46: ASSERT_NE(handle, nullptr); Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:58: ASSERT_TRUE(db_.get()); I believe .get() is unnecessary here (multiple places) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:65: CrashReportDatabase* db() const { return db_.get(); } Const functions should not return pointers to non-const objects; make the returned pointer const or make the function non-const. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:69: CrashReportDatabase::NewReport* new_report; Nit: Should this be initialized? You don't do it here but you do atop UploadReport() below. I would be consistent. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:70: EXPECT_EQ(CrashReportDatabase::kNoError, Change to ASSERT_EQ or add an ASSERT_TRUE(new_report) after this line since you unconditionally deref |new_report| below. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:89: EXPECT_TRUE(report); Again, this should be ASSERT_TRUE, or the EXPECT_EQ above should be ASSERT_EQ and this should be removed. Whatever you do, be consistent with CreateCrashReport(). Similar comments apply in the ErrorWritingCrashReport test below. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:119: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:129: EXPECT_TRUE(db.get()); This should be ASSERT_TRUE since you deref |db| unconditionally below. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:131: std::vector<const CrashReportDatabase::Report> reports; This is out-of-scope for the current CL, but consider adding a typedef for this type (e.g. CrashReportDatabase::Reports) and then making use of it in this file and in the CrashReportDatabase subclasses. While this would perhaps enhance readability and maintainability on its own, it has an optional side nicety. In a number of places where you currently do something like: std::vector<const CrashReportDatabase::Report> reports; ...db()->GetPendingReports(&reports); EXPECT_EQ(4, reports.size()); ... reports.clear(); db()->GetPendingReports(&reports); EXPECT_EQ(4, reports.size()); This would give you the option of making each block a local scope, a la: { CrashReportDatabase::Reports reports; ...db()->GetPendingReports(&reports); EXPECT_EQ(4, reports.size()); } ...without the verbosity of the ugly vector<> type each time. This might help callers to see which pieces of which tests were self-contained and which ones fetched report vectors to be used by the rest of the test code. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:133: EXPECT_TRUE(reports.empty()); Either change this to ASSERT_TRUE or add a "reports.clear()" call just after, since if the vector is non-empty here, GetCompletedReports() below will DCHECK(). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:190: EXPECT_EQ("", report.id); Nit: "" -> std::string() (multiple places) You could also use EXPECT_TRUE(report.id.empty()) or similar in many of these places if you wish. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:222: Nit: I would remove this blank line https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:342: const CrashReportDatabase::Report completed_report_1 = completed[0]; You don't seem to use this. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:38: enum class ReportState : int { Seems like this should be "int32_t" rather than "int" based on MetadataFileReportRecord below. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:55: bool CreateOrEnsureDirectoryExists(const base::FilePath& path) { Nit: "EnsureDirectoryExists" or "CreateDirectoryIfNecessary" seem like less-confusing names. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:58: } else if (GetLastError() == ERROR_ALREADY_EXISTS) { Don't use "else" after "return"; see http://dev.chromium.org/developers/coding-style#Code_Formatting . Consider reversing this conditional so you can unindent most of the rest of the function: // Try to create the directory. if (CreateDirectory(path.value().c_str(), nullptr)) return true; if (GetLastError() == ERROR_ALREADY_EXISTS) { PLOG(ERROR) << "Failed to create directory " << path.value(); return; } // Check that the result is actually a directory. ... https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:61: PLOG(ERROR) << "GetFileAttributes"; Unless you have a way of collecting and analyzing the log data, Chromium code should generally try to avoid logging statements, at least ones that aren't debug-only; they bloat the binary and users don't normally see them anyway. It's also potentially inconsistent that in most error cases you use LOG + return (which will basically be a silent no-op in release) but for things like checked_cast, the program will crash if the cast fails. I'm not certain, but it's possible more of these should be DCHECK ("never happens") or CHECK ("never happens, and if it were to happen, it'd be a security hole, so force a crash") versus LOG + return ("can happen in exceptional circumstances; no-op"). Nit: Consider making your error statements more like complete sentences ("Failed to create temporary directory '<dir>'" or similar), to minimize any possible confusion when someone sees one of these messages. (Several places across different files) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:82: //! initial write. We don't store metadata in dump's file attributes, and Nit: "and" -> "so we"? (Unsure) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:89: //! \brief Manages the metadata for the set of reports, handling serialization Consider pulling this Metadata class and the associated helpers out to another file. It's a little easier to read .cc files when they're basically one class per file, and CrashReportDatabaseWin is arguably the focus of this file, given the filename. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:111: std::vector<const CrashReportDatabase::Report>* reports); This function can be const. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:123: const ReportDisk** report_disk); This function can be const (but see comment below). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:137: const T& mutator); Taking a mutator and passing in lambdas is very clever, but I can't help thinking this would all be even clearer as just a pair of functions, FindReport() and FindReportAnyState(), which take a UUID (and, in the former case, a ReportState) and return the ReportDisk* as an outparam. (If you want FindReportAnyState() to be a const function with a const outparam you could do so, although I might rename as "FindConstReportAnyState" if so.) This would match the construction of VerifyReport, and allow callers to avoid the slight verbosity of the lambda syntax. It would also arguably be a little easier to understand, as (in std:: algorithm terms) it's closer to a "find_if(), then do stuff" paradigm than some sort of strange version of transform(). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, Static methods go below the constructor; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... . https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:142: friend class CrashReportDatabaseWin; Is there really much benefit in making the constructor private and then friending CrashReportDatabaseWin? Since this is a utility class used only by that class, it seems like you should just make the constructor public and assume the creator of this object knows how to use it. This also avoids the need to discuss AcquireMetadata() in the class comments: the class effectively "doesn't care" who uses it. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:152: //! (that is, the dump file has not been removed), that the report is in Nit: that the -> and that the https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:162: bool dirty_; //! \brief Is a Write() required on destruction? Nit: Clearer that you're not uncertain yourself: "True when a Write() is required on destruction." https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:168: Metadata::Metadata(FileHandle handle, const base::FilePath& report_dir) Define functions (including class members) in the same order they were declared; see http://dev.chromium.org/developers/coding-style#Code_Formatting . https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:181: // The format of the metadata file is a MetadataFileHeader, followed by a This is a good comment but it seems like it should be somewhere other than the middle of the file. Perhaps it belongs in the Metadata class header comment, or in a file comment atop the file? Likewise, the struct definitions themselves are somewhat oddly placed in the midst of the Metadata member definitions. Maybe the whole block should move above Metadata? I'm not sure. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:185: #pragma pack(push, 1) I don't think this should actually do anything given the struct layouts below? https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:208: const uint32_t kMetadataFileVersion = 1; Nit: Seems like these should perhaps be up with kMetadataFileName and the other constants; outside the #pragma pack section regardless. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:212: // Reads from the current file position to EOF and returns as uint8_t[]. Nit: Since this doesn't actually return a uint8_t[], perhaps "returns as a string of bytes". https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:220: DCHECK_GE(end, read_from); Nit: If there's 0 space left in the file, you can bail early here; this avoids any potential problems if e.g. creating a string or calling LoggingReadFile with zero length make things unhappy: if (read_from == -1 || end == -1 || original == -1 || read_from == end) return std::string(); DCHECK_EQ(read_from, original); DCHECK_GT(end, read_from); ... https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:225: return buffer; Nit: Optional shorter method: return LoggingReadFile(file, &buffer[0], data_length) ? buffer : std::string(); https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:228: uint32_t AddStringToTable(std::string* string_table, const std::string& str) { Please add a comment above this function indicating what it returns. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:235: uint32_t AddStringToTable(std::string* string_table, const std::wstring& str) { Nit: I'm not 100% certain but I believe we're now trying to use string16 in place of wstring even in Windows-specific code (since one is just an alias for the other). (Several places across multiple files) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:277: DCHECK_EQ(result, 0); Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is (expected, actual) to match the gtest macro order. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:301: auto records_size = base::CheckedNumeric<uint32_t>(header.num_records) * I'm not sure "auto" is best here, mainly because it's not necessarily obvious that CheckedNumeric * size_t == CheckedNumeric, or what the type of the result is, and to figure out one has to peer into the template muck in CheckedNumeric's definitions. I would probably err on the side of declaring this as a CheckedNumeric<uint32_t>. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:309: new MetadataFileReportRecord[header.num_records]); Can this be done with a vector<MetadataFileReportRecord> instead? You can pass the size to the constructor and then pass &records[0] to LoggingReadFile(). This allows you to use range-based for below as well. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:321: for (uint32_t i = 0; i < header.num_records; ++i) { Assuming you changed to a vector above, prefer to use range-based for for brevity and being minimally error-prone: for (const auto& record : records) { ... https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:322: ReportDisk r; Perhaps ReportDisk should define a constructor that takes a const MetadataFileReportRecord& and copies over the values, with appropriate conversion/casting. You would then simply do something like: if (record->file_path_index >= string_table.size() || ...) { ... } reports_.push_back(ReportDisk(record)); This would be easier to read here and would minimize the amount of code external to ReportDisk that has to understand its fields. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:323: const MetadataFileReportRecord* record = &records[i]; Nit: Why not a const&? https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:327: reports_.clear(); Avoid this clear() call by using a temporary of the same type as |reports_| and then doing a reports_.swap() call before exit, which is constant-time. This ensures no one who modifies the code will introduce another early exit path that fails to clear |reports_|. (Use reserve() on the temporary to ensure that the loop never needs to allocate more memory.) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:371: new MetadataFileReportRecord[num_records]); Again, if you can use a vector here, do so; this allows you to reserve() the appropriate size and then use push_back() in the loop below, which in turn allows switching the loop to use range-based for. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:375: MetadataFileReportRecord& record = records[i]; Nit: While not entirely banned, non-const refs are very uncommon in Chromium code. In general, prefer pointers to non-const refs for maximum clarity. If you follow the vector suggestion above, this will turn into a MetadataFileReportRecord temp instead. In that case, I suggest defining a constructor for that object that takes a const ReportDisk&, for the same reasons (and to the same effect) described previously above. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:380: << report_dir_.value().c_str(); Don't use c_str() with <<; it's unnecessary. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:384: AddStringToTable(&string_table, path.BaseName().value().c_str()); Remove this c_str() call, which will simply add an extra string temporary generation to the process. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:395: num_records * sizeof(MetadataFileReportRecord))) { Nit: If you use a vector as suggested above, change this to use records.size() instead of |num_records|. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); Nit: DCHECK_EQ https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:420: reports->push_back(report); Nit: Why not just: if ((report.state == desired_state) && (VerifyReport(report, desired_state) == CrashReportDatabase::kNoError)) reports->push_back(report); (If only we had C++11 library functions available, I'd suggest further simplifying to a single call to std::copy_if() with a lambda.) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:428: for (size_t i = 0; i < reports_.size(); ++i) { Nit: Now that we have C++11 lambdas, use std::find_if() to tell the reader your goal is to find a single record matching a particular predicate: auto report_iter = std::find_if( reports_.begin(), reports_.end(), [uuid](const ReportDisk& report) { return report.uuid = uuid; }); if (report_iter == reports_.end()) return CrashReportDatabase::kReportNotFound; ... Same comment applies in MutateSingleReport() below. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:434: return CrashReportDatabase::kNoError; Nit: Simpler: if (os == CrashReportDatabase::kNoError) *out_report = &reports_[i]; return os; Again, same comment applies to MutateSingleReport() below, though there this doesn't actually save lines, just a tiny bit of verbiage. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:465: return CrashReportDatabase::kNoError; Nit: Shorter: return (fileattr & FILE_ATTRIBUTE_DIRECTORY) ? CrashReportDatabase::kFileSystemError : CrashReportDatabase::kNoError; https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:473: return VerifyReportAnyState(report_disk); Nit: Shorter: return (report_disk.state == desired_state) ? VerifyReportAnyState(report_disk) : CrashReportDatabase::kBusyError; https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:484: OperationStatus PrepareNewCrashReport(NewReport** report) override; This is out-of-scope for the current CL, but consider changing the parent class declaration to return this by scoped_ptr<NewReport>* instead of NewReport**, to force the caller to take ownership, avoiding a potential leak. Same comment on GetReportForUploading(). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:521: if (!CreateOrEnsureDirectoryExists(base_dir_.Append(kReportsDirectory))) Nit: These conditionals (and their comments) could be combined. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:531: NewReport** out_report) { Argument names should match between function declaration and definition. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:532: scoped_ptr<NewReportDisk> report(new NewReportDisk()); Declare this below, as close to the first use as possible. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables . https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:534: ::UUID system_uuid; Oh man, are ::UUID and UUID different types? How confusing :( https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:535: if (UuidCreate(&system_uuid) != RPC_S_OK) { Be consistent about {} on one-line conditional bodies or not; the rest of the file omits them. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:566: ReportDisk report_disk; Nit: Consider adding a constructor for this that will allow you to pass the values below as arguments, or even better, a constructor that takes a const NewReportDisk&. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:572: *uuid = report_disk.uuid; Nit: If you use new_report->uuid here, and follow the ReportDisk(const NewReportDisk&) constructor suggestion above, you can construct the ReportDisk() object directly in the AddNewRecord() call above. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:587: PLOG(ERROR) << "DeleteFile " << scoped_report->path.value().c_str(); Don't use c_str() here. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:605: return kNoError; Nit: Simpler: if (os == kNoError) *report = *report_disk; return os; https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:613: return metadata->FindReports(ReportState::kPending, reports); Nit: Shorter: return metadata ? metadata->FindReports(ReportState::kPending, reports) : kDatabaseError; https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:621: return metadata->FindReports(ReportState::kCompleted, reports); Nit: Shorter: return metadata ? metadata->FindReports(ReportState::kCompleted, reports) : kDatabaseError; https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:661: [successful, id](ReportDisk* report_disk) { This lambda is skirting the border of "too long" -- another reason why changing MutateSingleReport() to a find function and then doing the remaining mutation code after it returns might be better. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:694: database_win.reset(); Nit: If you do this: if (!database_win->Initialize()) return scoped_ptr<CrashReportDatabaseWin>(); ...Then if you also follow the comment below, you can shorten this further by combining with ?: as in some suggestions above. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:696: return scoped_ptr<CrashReportDatabase>(database_win.release()); Simply: return database_win.Pass(); Pass() can be used to auto-upcast, there's no need to do it explicitly. This should save a temp as well. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_dir.h File util/test/scoped_temp_dir.h (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir.h:39: //! \brief Move the temporary directory to a new temporary location. Please use descriptive ("Moves") rather than imperative ("Move") comments; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... . https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { Nit: Consider avoiding using a with-side-effect statement as a loop test. For example: for (dirent* entry = readdir(dir); entry; entry = readdir(dir)) { This is only one line, avoids testing the result of side effect statements, and also places |entry| into a narrower scope (see previous comment). https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:57: CHECK(false) << "Couldn't find temp dir name"; Nit: "Couldn't create a unique temp dir name" might be slightly clearer. (2 places) https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:84: ASSERT_EQ(GetLastError(), ERROR_FILE_NOT_FOUND); Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE (2 places) https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:92: (find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0; Nit: Shorter: if (find_data.dwFileAttributes & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT)) { https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:100: if (!FindNextFile(search_handle, &find_data)) { Nit: This can be used as a loop condition, and the EXPECT moved below the loop. That in turn would allow you to "continue" in the case of "." and ".." like you did in the Linux code, unindenting the rest of the loop one level: do { if (...) continue; ... } while (FindNextFile(search_handle, &find_data)); EXPECT_EQ(ERROR_NO_MORE_FILES, GetLastError());
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Thanks for your thorough review! Unfortunately, because things moved around a bunch, the difference between patch set 1 and 2 looks a bit noisier that it really is. Hopefully I didn't miss anything. One high-level thing that I should have pointed out in the original description in case it wasn't obvious is that I didn't write all of crash_report_database_test.cc. CrashReportDatabase was previously implemented for Mac with tests; I wrote only the Windows implementation and added some test code in support of that (you can see the diff in the original CL link above). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:46: ASSERT_NE(handle, nullptr); On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE So yoda ugly! Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:58: ASSERT_TRUE(db_.get()); On 2015/02/13 00:32:38, Peter Kasting wrote: > I believe .get() is unnecessary here (multiple places) Done. (2 locations) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:65: CrashReportDatabase* db() const { return db_.get(); } On 2015/02/13 00:32:38, Peter Kasting wrote: > Const functions should not return pointers to non-const objects; make the > returned pointer const or make the function non-const. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:69: CrashReportDatabase::NewReport* new_report; On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Should this be initialized? You don't do it here but you do atop > UploadReport() below. I would be consistent. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:70: EXPECT_EQ(CrashReportDatabase::kNoError, On 2015/02/13 00:32:38, Peter Kasting wrote: > Change to ASSERT_EQ or add an ASSERT_TRUE(new_report) after this line since you > unconditionally deref |new_report| below. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:89: EXPECT_TRUE(report); On 2015/02/13 00:32:38, Peter Kasting wrote: > Again, this should be ASSERT_TRUE, or the EXPECT_EQ above should be ASSERT_EQ > and this should be removed. Whatever you do, be consistent with > CreateCrashReport(). > > Similar comments apply in the ErrorWritingCrashReport test below. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:119: }; On 2015/02/13 00:32:38, Peter Kasting wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:129: EXPECT_TRUE(db.get()); On 2015/02/13 00:32:38, Peter Kasting wrote: > This should be ASSERT_TRUE since you deref |db| unconditionally below. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:131: std::vector<const CrashReportDatabase::Report> reports; On 2015/02/13 00:32:38, Peter Kasting wrote: > This is out-of-scope for the current CL, but consider adding a typedef for this > type (e.g. CrashReportDatabase::Reports) and then making use of it in this file > and in the CrashReportDatabase subclasses. > > While this would perhaps enhance readability and maintainability on its own, it > has an optional side nicety. In a number of places where you currently do > something like: > > std::vector<const CrashReportDatabase::Report> reports; > ...db()->GetPendingReports(&reports); > EXPECT_EQ(4, reports.size()); > > ... > > reports.clear(); > db()->GetPendingReports(&reports); > EXPECT_EQ(4, reports.size()); > > This would give you the option of making each block a local scope, a la: > > { > CrashReportDatabase::Reports reports; > ...db()->GetPendingReports(&reports); > EXPECT_EQ(4, reports.size()); > } > > ...without the verbosity of the ugly vector<> type each time. This might help > callers to see which pieces of which tests were self-contained and which ones > fetched report vectors to be used by the rest of the test code. That seems reasonable. I sort of dislike hiding vector behind a typedef that makes it less clear what you're working with. I will defer that to a future change in any case, and Mark/Robert's preference. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:133: EXPECT_TRUE(reports.empty()); On 2015/02/13 00:32:38, Peter Kasting wrote: > Either change this to ASSERT_TRUE or add a "reports.clear()" call just after, > since if the vector is non-empty here, GetCompletedReports() below will > DCHECK(). They're slightly unrelated checks, so I went with clear(). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:190: EXPECT_EQ("", report.id); On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: "" -> std::string() (multiple places) > > You could also use EXPECT_TRUE(report.id.empty()) or similar in many of these > places if you wish. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:222: On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: I would remove this blank line I hate to dissent for something so minor, but because it's used in subsequent blocks that are separated by blanks, I feel not attaching it to line 223 is better. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:342: const CrashReportDatabase::Report completed_report_1 = completed[0]; On 2015/02/13 00:32:38, Peter Kasting wrote: > You don't seem to use this. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:38: enum class ReportState : int { On 2015/02/13 00:32:39, Peter Kasting wrote: > Seems like this should be "int32_t" rather than "int" based on > MetadataFileReportRecord below. This one isn't used directly for serialization, so removed the explicit type. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:55: bool CreateOrEnsureDirectoryExists(const base::FilePath& path) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: "EnsureDirectoryExists" or "CreateDirectoryIfNecessary" seem like > less-confusing names. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:58: } else if (GetLastError() == ERROR_ALREADY_EXISTS) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Don't use "else" after "return"; see > http://dev.chromium.org/developers/coding-style#Code_Formatting . > > Consider reversing this conditional so you can unindent most of the rest of the > function: > > // Try to create the directory. > if (CreateDirectory(path.value().c_str(), nullptr)) > return true; > if (GetLastError() == ERROR_ALREADY_EXISTS) { > PLOG(ERROR) << "Failed to create directory " << path.value(); > return; > } > > // Check that the result is actually a directory. > ... Done. (!= ERROR_ALREADY_EXISTS above) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:61: PLOG(ERROR) << "GetFileAttributes"; On 2015/02/13 00:32:39, Peter Kasting wrote: > Unless you have a way of collecting and analyzing the log data, Chromium code > should generally try to avoid logging statements, at least ones that aren't > debug-only; they bloat the binary and users don't normally see them anyway. > It's also potentially inconsistent that in most error cases you use LOG + return > (which will basically be a silent no-op in release) but for things like > checked_cast, the program will crash if the cast fails. I'm not certain, but > it's possible more of these should be DCHECK ("never happens") or CHECK ("never > happens, and if it were to happen, it'd be a security hole, so force a crash") > versus LOG + return ("can happen in exceptional circumstances; no-op"). > > Nit: Consider making your error statements more like complete sentences ("Failed > to create temporary directory '<dir>'" or similar), to minimize any possible > confusion when someone sees one of these messages. > > (Several places across different files) Noted the general preference to avoid bloat in logging in Chromium. We had a discussion about this in the original review: https://codereview.chromium.org/867363003/#msg42 https://codereview.chromium.org/867363003/#msg45 The preference was to have more extensive logging in this component, perhaps partially because it's related to crashing, so problems within it might require more heroic measures from users to track down problems (like gathering logs). There was also a vague plan to redirect the LOGs to something we could gather and upload in the future. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:82: //! initial write. We don't store metadata in dump's file attributes, and On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: "and" -> "so we"? (Unsure) Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:89: //! \brief Manages the metadata for the set of reports, handling serialization On 2015/02/13 00:32:40, Peter Kasting wrote: > Consider pulling this Metadata class and the associated helpers out to another > file. It's a little easier to read .cc files when they're basically one class > per file, and CrashReportDatabaseWin is arguably the focus of this file, given > the filename. I would prefer not to. I agree it would be a bit easier to read, but the two classes only make sense together, Metadata being an internal detail of how CrashReportDatabaseWin is implemented. If Metadata were in a separate file, I think it would be slightly less clear to the user of the code, who might think that it was available for standalone use. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:111: std::vector<const CrashReportDatabase::Report>* reports); On 2015/02/13 00:32:38, Peter Kasting wrote: > This function can be const. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:123: const ReportDisk** report_disk); On 2015/02/13 00:32:39, Peter Kasting wrote: > This function can be const (but see comment below). Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:137: const T& mutator); On 2015/02/13 00:32:39, Peter Kasting wrote: > Taking a mutator and passing in lambdas is very clever, but I can't help > thinking this would all be even clearer as just a pair of functions, > FindReport() and FindReportAnyState(), which take a UUID (and, in the former > case, a ReportState) and return the ReportDisk* as an outparam. (If you want > FindReportAnyState() to be a const function with a const outparam you could do > so, although I might rename as "FindConstReportAnyState" if so.) Aha! That was how I initially wrote it. (Around ps#13 to ps#14 in the original CL). The drawback of returning non-const ReportDisk* was that the Finds couldn't be generic because then Metadata had to either assume that the metadata was always dirtied (causing unnecessary Write()s in e.g. LookUpCrashReport()) or had to delegate the responsibility to call Write() when the ReportDisk was changed to the caller, which seemed error prone (and more verbose for callers in any case). If you think the lambdas are too cute, we could simply swap MutateSingleReport for FindSingleReportAndMarkDirty. I think that matches what you intended, so I did that. > > This would match the construction of VerifyReport, and allow callers to avoid > the slight verbosity of the lambda syntax. It would also arguably be a little > easier to understand, as (in std:: algorithm terms) it's closer to a "find_if(), > then do stuff" paradigm than some sort of strange version of transform(). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:140: static scoped_ptr<Metadata> Create(const base::FilePath& metadata_file, On 2015/02/13 00:32:38, Peter Kasting wrote: > Static methods go below the constructor; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... > . Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:142: friend class CrashReportDatabaseWin; On 2015/02/13 00:32:40, Peter Kasting wrote: > Is there really much benefit in making the constructor private and then > friending CrashReportDatabaseWin? Since this is a utility class used only by > that class, it seems like you should just make the constructor public and assume > the creator of this object knows how to use it. > > This also avoids the need to discuss AcquireMetadata() in the class comments: > the class effectively "doesn't care" who uses it. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:152: //! (that is, the dump file has not been removed), that the report is in On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: that the -> and that the Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:162: bool dirty_; //! \brief Is a Write() required on destruction? On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Clearer that you're not uncertain yourself: "True when a Write() is > required on destruction." Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:168: Metadata::Metadata(FileHandle handle, const base::FilePath& report_dir) On 2015/02/13 00:32:39, Peter Kasting wrote: > Define functions (including class members) in the same order they were declared; > see http://dev.chromium.org/developers/coding-style#Code_Formatting . Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:181: // The format of the metadata file is a MetadataFileHeader, followed by a On 2015/02/13 00:32:40, Peter Kasting wrote: > This is a good comment but it seems like it should be somewhere other than the > middle of the file. Perhaps it belongs in the Metadata class header comment, or > in a file comment atop the file? Likewise, the struct definitions themselves > are somewhat oddly placed in the midst of the Metadata member definitions. > Maybe the whole block should move above Metadata? I'm not sure. The idea was that there's a small subsection here, from the block comment that describes the format at a high level, the structures the define it, followed by Read() and Write() which are the only methods that use MetadataFile*. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:185: #pragma pack(push, 1) On 2015/02/13 00:32:39, Peter Kasting wrote: > I don't think this should actually do anything given the struct layouts below? Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:208: const uint32_t kMetadataFileVersion = 1; On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: Seems like these should perhaps be up with kMetadataFileName and the other > constants; outside the #pragma pack section regardless. I would prefer to keep them next to MetadataFileHeader. Everything is moved to the top now due to the converter/constructor functions. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:212: // Reads from the current file position to EOF and returns as uint8_t[]. On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: Since this doesn't actually return a uint8_t[], perhaps "returns as a > string of bytes". Oops, done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:220: DCHECK_GE(end, read_from); On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: If there's 0 space left in the file, you can bail early here; this avoids > any potential problems if e.g. creating a string or calling LoggingReadFile with > zero length make things unhappy: > > if (read_from == -1 || end == -1 || original == -1 || read_from == end) > return std::string(); > DCHECK_EQ(read_from, original); > DCHECK_GT(end, read_from); > ... Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:225: return buffer; On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Optional shorter method: > > return LoggingReadFile(file, &buffer[0], data_length) ? > buffer : std::string(); Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:228: uint32_t AddStringToTable(std::string* string_table, const std::string& str) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Please add a comment above this function indicating what it returns. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:235: uint32_t AddStringToTable(std::string* string_table, const std::wstring& str) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: I'm not 100% certain but I believe we're now trying to use string16 in > place of wstring even in Windows-specific code (since one is just an alias for > the other). (Several places across multiple files) Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:277: DCHECK_EQ(result, 0); On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is (expected, > actual) to match the gtest macro order. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:301: auto records_size = base::CheckedNumeric<uint32_t>(header.num_records) * On 2015/02/13 00:32:40, Peter Kasting wrote: > I'm not sure "auto" is best here, mainly because it's not necessarily obvious > that CheckedNumeric * size_t == CheckedNumeric, or what the type of the result > is, and to figure out one has to peer into the template muck in CheckedNumeric's > definitions. I would probably err on the side of declaring this as a > CheckedNumeric<uint32_t>. That was by request. :) But done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:309: new MetadataFileReportRecord[header.num_records]); On 2015/02/13 00:32:38, Peter Kasting wrote: > Can this be done with a vector<MetadataFileReportRecord> instead? You can pass > the size to the constructor and then pass &records[0] to LoggingReadFile(). > This allows you to use range-based for below as well. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:321: for (uint32_t i = 0; i < header.num_records; ++i) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Assuming you changed to a vector above, prefer to use range-based for for > brevity and being minimally error-prone: > > for (const auto& record : records) { > ... Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:322: ReportDisk r; On 2015/02/13 00:32:40, Peter Kasting wrote: > Perhaps ReportDisk should define a constructor that takes a const > MetadataFileReportRecord& and copies over the values, with appropriate > conversion/casting. > > You would then simply do something like: > > if (record->file_path_index >= string_table.size() || ...) { > ... > } > reports_.push_back(ReportDisk(record)); > > This would be easier to read here and would minimize the amount of code external > to ReportDisk that has to understand its fields. It needs the string table and report_dir FilePath to convert indices to pointers. It feels odd to pass those to a constructor; how about a static converter: reports.push_back(ReportDisk::FromMetataFileReportRecord( record, report_dir_, string_table)); https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:323: const MetadataFileReportRecord* record = &records[i]; On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Why not a const&? Done (via const auto&). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:327: reports_.clear(); On 2015/02/13 00:32:40, Peter Kasting wrote: > Avoid this clear() call by using a temporary of the same type as |reports_| and > then doing a reports_.swap() call before exit, which is constant-time. This > ensures no one who modifies the code will introduce another early exit path that > fails to clear |reports_|. (Use reserve() on the temporary to ensure that the > loop never needs to allocate more memory.) Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:371: new MetadataFileReportRecord[num_records]); On 2015/02/13 00:32:40, Peter Kasting wrote: > Again, if you can use a vector here, do so; this allows you to reserve() the > appropriate size and then use push_back() in the loop below, which in turn > allows switching the loop to use range-based for. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:375: MetadataFileReportRecord& record = records[i]; On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: While not entirely banned, non-const refs are very uncommon in Chromium > code. In general, prefer pointers to non-const refs for maximum clarity. > > If you follow the vector suggestion above, this will turn into a > MetadataFileReportRecord temp instead. In that case, I suggest defining a > constructor for that object that takes a const ReportDisk&, for the same reasons > (and to the same effect) described previously above. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:380: << report_dir_.value().c_str(); On 2015/02/13 00:32:39, Peter Kasting wrote: > Don't use c_str() with <<; it's unnecessary. There's no operator<< for wstring/string16 in <ostream>. Perhaps something in Chromium (illegally) adds one to std::? ... Huh. Yup: https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=882 Crashpad is currently building against a very stripped down version of base, and doesn't have this. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:384: AddStringToTable(&string_table, path.BaseName().value().c_str()); On 2015/02/13 00:32:39, Peter Kasting wrote: > Remove this c_str() call, which will simply add an extra string temporary > generation to the process. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:395: num_records * sizeof(MetadataFileReportRecord))) { On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: If you use a vector as suggested above, change this to use records.size() > instead of |num_records|. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: DCHECK_EQ That doesn't compile because ReportState is "enum class" (not enum) and so doesn't match an operator<<. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:420: reports->push_back(report); On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Why not just: > > if ((report.state == desired_state) && > (VerifyReport(report, desired_state) == CrashReportDatabase::kNoError)) > reports->push_back(report); > > (If only we had C++11 library functions available, I'd suggest further > simplifying to a single call to std::copy_if() with a lambda.) Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:428: for (size_t i = 0; i < reports_.size(); ++i) { On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Now that we have C++11 lambdas, use std::find_if() to tell the reader your > goal is to find a single record matching a particular predicate: > > auto report_iter = std::find_if( > reports_.begin(), reports_.end(), > [uuid](const ReportDisk& report) { return report.uuid = uuid; }); > if (report_iter == reports_.end()) > return CrashReportDatabase::kReportNotFound; > ... > > Same comment applies in MutateSingleReport() below. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:434: return CrashReportDatabase::kNoError; On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: Simpler: > > if (os == CrashReportDatabase::kNoError) > *out_report = &reports_[i]; > return os; > > Again, same comment applies to MutateSingleReport() below, though there this > doesn't actually save lines, just a tiny bit of verbiage. To me that feels more complicated. Previously it reads as "check condition, early out, check condition, early out. When everything is OK, prepare the data to be returned and return success". Maybe it's just me. Done, anyway. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:465: return CrashReportDatabase::kNoError; On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Shorter: > > return (fileattr & FILE_ATTRIBUTE_DIRECTORY) ? > CrashReportDatabase::kFileSystemError : CrashReportDatabase::kNoError; Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:473: return VerifyReportAnyState(report_disk); On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Shorter: > > return (report_disk.state == desired_state) ? > VerifyReportAnyState(report_disk) : CrashReportDatabase::kBusyError; Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:484: OperationStatus PrepareNewCrashReport(NewReport** report) override; On 2015/02/13 00:32:40, Peter Kasting wrote: > This is out-of-scope for the current CL, but consider changing the parent class > declaration to return this by scoped_ptr<NewReport>* instead of NewReport**, to > force the caller to take ownership, avoiding a potential leak. > > Same comment on GetReportForUploading(). That seems like a good idea. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:521: if (!CreateOrEnsureDirectoryExists(base_dir_.Append(kReportsDirectory))) On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: These conditionals (and their comments) could be combined. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:531: NewReport** out_report) { On 2015/02/13 00:32:40, Peter Kasting wrote: > Argument names should match between function declaration and definition. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:532: scoped_ptr<NewReportDisk> report(new NewReportDisk()); On 2015/02/13 00:32:39, Peter Kasting wrote: > Declare this below, as close to the first use as possible. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > . Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:534: ::UUID system_uuid; On 2015/02/13 00:32:39, Peter Kasting wrote: > Oh man, are ::UUID and UUID different types? How confusing :( Yeah, partially due to initial implementation being on Mac where the names to not match. We could consider renaming Crashpad's. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:535: if (UuidCreate(&system_uuid) != RPC_S_OK) { On 2015/02/13 00:32:40, Peter Kasting wrote: > Be consistent about {} on one-line conditional bodies or not; the rest of the > file omits them. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:566: ReportDisk report_disk; On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Consider adding a constructor for this that will allow you to pass the > values below as arguments, or even better, a constructor that takes a const > NewReportDisk&. I did this as a ReportDisk constructor, but then undid it. It's doing more than conversion from NewReportDisk->ReportDisk, it's also getting the current (system) time, and setting the state to kPending (which it isn't clear why that constructor should set to that specific value). When it only sets the uuid and file_path, it seems more cluttery to have it out-of-line than just in the one place where we do this. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:572: *uuid = report_disk.uuid; On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: If you use new_report->uuid here, and follow the ReportDisk(const > NewReportDisk&) constructor suggestion above, you can construct the ReportDisk() > object directly in the AddNewRecord() call above. Acknowledged. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:587: PLOG(ERROR) << "DeleteFile " << scoped_report->path.value().c_str(); On 2015/02/13 00:32:40, Peter Kasting wrote: > Don't use c_str() here. See above. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:605: return kNoError; On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Simpler: > > if (os == kNoError) > *report = *report_disk; > return os; Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:613: return metadata->FindReports(ReportState::kPending, reports); On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Shorter: > > return metadata ? > metadata->FindReports(ReportState::kPending, reports) : kDatabaseError; Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:621: return metadata->FindReports(ReportState::kCompleted, reports); On 2015/02/13 00:32:38, Peter Kasting wrote: > Nit: Shorter: > > return metadata ? > metadata->FindReports(ReportState::kCompleted, reports) : kDatabaseError; Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:661: [successful, id](ReportDisk* report_disk) { On 2015/02/13 00:32:39, Peter Kasting wrote: > This lambda is skirting the border of "too long" -- another reason why changing > MutateSingleReport() to a find function and then doing the remaining mutation > code after it returns might be better. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:694: database_win.reset(); On 2015/02/13 00:32:39, Peter Kasting wrote: > Nit: If you do this: > > if (!database_win->Initialize()) > return scoped_ptr<CrashReportDatabaseWin>(); > > ...Then if you also follow the comment below, you can shorten this further by > combining with ?: as in some suggestions above. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:696: return scoped_ptr<CrashReportDatabase>(database_win.release()); On 2015/02/13 00:32:40, Peter Kasting wrote: > Simply: > > return database_win.Pass(); > > Pass() can be used to auto-upcast, there's no need to do it explicitly. This > should save a temp as well. Done. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_dir.h File util/test/scoped_temp_dir.h (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir.h:39: //! \brief Move the temporary directory to a new temporary location. On 2015/02/13 00:32:40, Peter Kasting wrote: > Please use descriptive ("Moves") rather than imperative ("Move") comments; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > . Done. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Consider avoiding using a with-side-effect statement as a loop test. For > example: > > for (dirent* entry = readdir(dir); entry; entry = readdir(dir)) { > > This is only one line, avoids testing the result of side effect statements, and > also places |entry| into a narrower scope (see previous comment). I defer to Robert/Mark if they prefer that in this _posix file. (I could go either way -- I don't like the duplication of readdir() in the for-loop version. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:57: CHECK(false) << "Couldn't find temp dir name"; On 2015/02/13 00:32:41, Peter Kasting wrote: > Nit: "Couldn't create a unique temp dir name" might be slightly clearer. (2 > places) Done. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:84: ASSERT_EQ(GetLastError(), ERROR_FILE_NOT_FOUND); On 2015/02/13 00:32:41, Peter Kasting wrote: > Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE (2 places) Done. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:92: (find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0; On 2015/02/13 00:32:40, Peter Kasting wrote: > Nit: Shorter: > > if (find_data.dwFileAttributes & > (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT)) { Done. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_win.cc:100: if (!FindNextFile(search_handle, &find_data)) { On 2015/02/13 00:32:41, Peter Kasting wrote: > Nit: This can be used as a loop condition, and the EXPECT moved below the loop. > That in turn would allow you to "continue" in the case of "." and ".." like you > did in the Linux code, unindenting the rest of the loop one level: > > do { > if (...) > continue; > ... > } while (FindNextFile(search_handle, &find_data)); > EXPECT_EQ(ERROR_NO_MORE_FILES, GetLastError()); Done.
mark@chromium.org changed reviewers: + mark@chromium.org
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:46: ASSERT_NE(handle, nullptr); Peter Kasting wrote: > Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE This is only true for gtest’s _EQ macros. None of the other ones have this property, including _NE. Reference: https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison Note that a failed _EQ assertion prints x_test.cc:123: Failure Value of: x Actual: 0 Expected: 1 Which is: 1 [ FAILED ] and a failed _NE prints x_test.cc:123: Failure Expected: (0) != (0), actual: 0 vs 0 [ FAILED ] without any reference to which side is “expected” and which side is “actual”. I probably wouldn’t have said anything at all, but I note that this function is now totally unused, and we can remove it altogether.
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:277: DCHECK_EQ(result, 0); Peter Kasting wrote: > Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is (expected, > actual) to match the gtest macro order. Crashpad-wide, all non-gtest _EQ macros are structured “naturally”, (actual, expected).
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:46: ASSERT_NE(handle, nullptr); On 2015/02/13 14:08:37, Mark Mentovai wrote: > Peter Kasting wrote: > > Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE > > This is only true for gtest’s _EQ macros. None of the other ones have this > property, including _NE. > > Reference: https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison > > Note that a failed _EQ assertion prints > > x_test.cc:123: Failure > Value of: x > Actual: 0 > Expected: 1 > Which is: 1 > [ FAILED ] > > and a failed _NE prints > > x_test.cc:123: Failure > Expected: (0) != (0), actual: 0 vs 0 > [ FAILED ] > > without any reference to which side is “expected” and which side is “actual”. > > I probably wouldn’t have said anything at all, but I note that this function is > now totally unused, and we can remove it altogether. Thanks, removed. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:277: DCHECK_EQ(result, 0); On 2015/02/13 14:12:46, Mark Mentovai wrote: > Peter Kasting wrote: > > Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is > (expected, > > actual) to match the gtest macro order. > > Crashpad-wide, all non-gtest _EQ macros are structured “naturally”, (actual, > expected). Rolled back.
(Have not re-reviewed, just replying) https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:131: std::vector<const CrashReportDatabase::Report> reports; On 2015/02/13 06:34:01, scottmg wrote: > On 2015/02/13 00:32:38, Peter Kasting wrote: > > This is out-of-scope for the current CL, but consider adding a typedef for > this > > type (e.g. CrashReportDatabase::Reports) and then making use of it in this > file > > and in the CrashReportDatabase subclasses. > > > > While this would perhaps enhance readability and maintainability on its own, > it > > has an optional side nicety. In a number of places where you currently do > > something like: > > > > std::vector<const CrashReportDatabase::Report> reports; > > ...db()->GetPendingReports(&reports); > > EXPECT_EQ(4, reports.size()); > > > > ... > > > > reports.clear(); > > db()->GetPendingReports(&reports); > > EXPECT_EQ(4, reports.size()); > > > > This would give you the option of making each block a local scope, a la: > > > > { > > CrashReportDatabase::Reports reports; > > ...db()->GetPendingReports(&reports); > > EXPECT_EQ(4, reports.size()); > > } > > > > ...without the verbosity of the ugly vector<> type each time. This might help > > callers to see which pieces of which tests were self-contained and which ones > > fetched report vectors to be used by the rest of the test code. > > That seems reasonable. I sort of dislike hiding vector behind a typedef that > makes it less clear what you're working with. I will defer that to a future > change in any case, and Mark/Robert's preference. It's a minor thing. It wouldn't normally be an issue if functions returned this type now that we have "auto", but since the ones in question here still need to take variables as outparams, it's still a factor. One reason I like "Reports" over "vector<Report>" is that, largely, callers shouldn't really care what precise type of container is being used. If for some reason it becomes appropriate to use a list instead, it's easier to do that with typedefs in place than without. However, if a reader needs to understand that the underlying container is really a vector, then obviously this makes that more opaque. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:222: On 2015/02/13 06:34:01, scottmg wrote: > On 2015/02/13 00:32:38, Peter Kasting wrote: > > Nit: I would remove this blank line > > I hate to dissent for something so minor, but because it's used in subsequent > blocks that are separated by blanks, I feel not attaching it to line 223 is > better. In that case, I wonder why you don't have a blank line after the declaration of |reports| above? It seems like being consistent might be better. I think removing the blank might be more consistent with "declare as close to first use as possible", but that's a very weak argument, so I'm OK with either route. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:61: PLOG(ERROR) << "GetFileAttributes"; On 2015/02/13 06:34:02, scottmg wrote: > On 2015/02/13 00:32:39, Peter Kasting wrote: > > Unless you have a way of collecting and analyzing the log data, Chromium code > > should generally try to avoid logging statements, at least ones that aren't > > debug-only; they bloat the binary and users don't normally see them anyway. > > It's also potentially inconsistent that in most error cases you use LOG + > return > > (which will basically be a silent no-op in release) but for things like > > checked_cast, the program will crash if the cast fails. I'm not certain, but > > it's possible more of these should be DCHECK ("never happens") or CHECK > ("never > > happens, and if it were to happen, it'd be a security hole, so force a crash") > > versus LOG + return ("can happen in exceptional circumstances; no-op"). > > > > Nit: Consider making your error statements more like complete sentences > ("Failed > > to create temporary directory '<dir>'" or similar), to minimize any possible > > confusion when someone sees one of these messages. > > > > (Several places across different files) > > Noted the general preference to avoid bloat in logging in Chromium. > > We had a discussion about this in the original review: > > https://codereview.chromium.org/867363003/#msg42 > https://codereview.chromium.org/867363003/#msg45 > > The preference was to have more extensive logging in this component, perhaps > partially because it's related to crashing, so problems within it might require > more heroic measures from users to track down problems (like gathering logs). > There was also a vague plan to redirect the LOGs to something we could gather > and upload in the future. Ack. I wonder if that decision should be captured in a comment somewhere, but I'm not sure where it would go. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:89: //! \brief Manages the metadata for the set of reports, handling serialization On 2015/02/13 06:34:04, scottmg wrote: > On 2015/02/13 00:32:40, Peter Kasting wrote: > > Consider pulling this Metadata class and the associated helpers out to another > > file. It's a little easier to read .cc files when they're basically one class > > per file, and CrashReportDatabaseWin is arguably the focus of this file, given > > the filename. > > I would prefer not to. I agree it would be a bit easier to read, but the two > classes only make sense together, Metadata being an internal detail of how > CrashReportDatabaseWin is implemented. If Metadata were in a separate file, I > think it would be slightly less clear to the user of the code, who might think > that it was available for standalone use. In principle, why couldn't it be available for standalone use? If there was a second, separate legitimate use for it it seems like that would be appropriate. Your argument suggests to me that it's important to prevent anyone else from ever accessing this class, and I'm not sure why that would be. At the very minimum, when defining multiple classes in the same file, I suggest using divider comments like those in e.g. chrome/browser/ui/omnibox/omnibox_edit_model.cc. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:137: const T& mutator); On 2015/02/13 06:34:04, scottmg wrote: > On 2015/02/13 00:32:39, Peter Kasting wrote: > > Taking a mutator and passing in lambdas is very clever, but I can't help > > thinking this would all be even clearer as just a pair of functions, > > FindReport() and FindReportAnyState(), which take a UUID (and, in the former > > case, a ReportState) and return the ReportDisk* as an outparam. (If you want > > FindReportAnyState() to be a const function with a const outparam you could do > > so, although I might rename as "FindConstReportAnyState" if so.) > > Aha! That was how I initially wrote it. (Around ps#13 to ps#14 in the original > CL). The drawback of returning non-const ReportDisk* was that the Finds couldn't > be generic because then Metadata had to either assume that the metadata was > always dirtied (causing unnecessary Write()s in e.g. LookUpCrashReport()) or had > to delegate the responsibility to call Write() when the ReportDisk was changed > to the caller, which seemed error prone (and more verbose for callers in any > case). > > If you think the lambdas are too cute, we could simply swap MutateSingleReport > for FindSingleReportAndMarkDirty. I think that matches what you intended, so I > did that. Yeah, I think that's fine. It's OK with me for these helpers to be non-generic/not-completely-parallel in structure since their callers are fairly specialized. We can always do something better if we need to add more callsites later. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:181: // The format of the metadata file is a MetadataFileHeader, followed by a On 2015/02/13 06:34:05, scottmg wrote: > On 2015/02/13 00:32:40, Peter Kasting wrote: > > This is a good comment but it seems like it should be somewhere other than the > > middle of the file. Perhaps it belongs in the Metadata class header comment, > or > > in a file comment atop the file? Likewise, the struct definitions themselves > > are somewhat oddly placed in the midst of the Metadata member definitions. > > Maybe the whole block should move above Metadata? I'm not sure. > > The idea was that there's a small subsection here, from the block comment that > describes the format at a high level, the structures the define it, followed by > Read() and Write() which are the only methods that use MetadataFile*. Hmm. I think that connection is not terribly apparent to a reader. I would probably prefer to move these somewhere else to keep all the Metadata:: method definitions together, even at the cost of making the consumer of these objects be further away from their definitions. If you follow my divider comment suggestion above, these would probably still be fairly easy to scan up and find. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:277: DCHECK_EQ(result, 0); On 2015/02/13 14:12:46, Mark Mentovai wrote: > Peter Kasting wrote: > > Nit: While not required, typical Chromium style for [D]CHECK_EQ/NE is > (expected, > > actual) to match the gtest macro order. > > Crashpad-wide, all non-gtest _EQ macros are structured “naturally”, (actual, > expected). Good to know. That's what should be done here too, then. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:322: ReportDisk r; On 2015/02/13 06:34:03, scottmg wrote: > On 2015/02/13 00:32:40, Peter Kasting wrote: > > Perhaps ReportDisk should define a constructor that takes a const > > MetadataFileReportRecord& and copies over the values, with appropriate > > conversion/casting. > > > > You would then simply do something like: > > > > if (record->file_path_index >= string_table.size() || ...) { > > ... > > } > > reports_.push_back(ReportDisk(record)); > > > > This would be easier to read here and would minimize the amount of code > external > > to ReportDisk that has to understand its fields. > > It needs the string table and report_dir FilePath to convert indices to > pointers. It feels odd to pass those to a constructor; how about a static > converter: > > reports.push_back(ReportDisk::FromMetataFileReportRecord( > record, report_dir_, string_table)); I'm OK with either. I don't feel any oddness about passing a constructor the arguments it needs to do its work, even if some of those aren't stored on the object itself -- we do this a lot across the codebase -- and keeping something as a constructor is slightly better in terms of preserving encapsulation of the class members, but there's not such a big difference between the two methods that I would demand one over the other. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:380: << report_dir_.value().c_str(); On 2015/02/13 06:34:03, scottmg wrote: > On 2015/02/13 00:32:39, Peter Kasting wrote: > > Don't use c_str() with <<; it's unnecessary. > > There's no operator<< for wstring/string16 in <ostream>. Perhaps something in > Chromium (illegally) adds one to std::? > > ... > > Huh. Yup: > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=882 > > Crashpad is currently building against a very stripped down version of base, and > doesn't have this. Hmm. The overload for string16 (in string16.h) is not in namespace std. I wonder if we could eliminate the wstring overload in base and do the string16 overload in string16.h regardless of the size of wchar_t. This would break anyone who currently tries to log a wstring directly and doesn't include string16.h, I think, which I doubt would be very many people. It seems like that's beyond the scope of this change, but possibly worth doing. Would you be willing to look into it? https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 06:34:05, scottmg wrote: > On 2015/02/13 00:32:40, Peter Kasting wrote: > > Nit: DCHECK_EQ > > That doesn't compile because ReportState is "enum class" (not enum) and so > doesn't match an operator<<. Since you removed the explicit type on the enum class in the latest patch, it seems like it might be better to just make this a non-class enum (which would allow this to work), unless I'm missing something on what the "class" part buys your code (I confess I haven't used enum class much yet). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:434: return CrashReportDatabase::kNoError; On 2015/02/13 06:34:02, scottmg wrote: > On 2015/02/13 00:32:39, Peter Kasting wrote: > > Nit: Simpler: > > > > if (os == CrashReportDatabase::kNoError) > > *out_report = &reports_[i]; > > return os; > > > > Again, same comment applies to MutateSingleReport() below, though there this > > doesn't actually save lines, just a tiny bit of verbiage. > > To me that feels more complicated. Previously it reads as "check condition, > early out, check condition, early out. When everything is OK, prepare the data > to be returned and return success". > > Maybe it's just me. Done, anyway. That's a legit reading. I read the intent of the code as "return whether the report successfully verified; populate |out_report| if the function succeeds", which is what led to my suggestion. Both are plausible. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:566: ReportDisk report_disk; On 2015/02/13 06:34:03, scottmg wrote: > On 2015/02/13 00:32:38, Peter Kasting wrote: > > Nit: Consider adding a constructor for this that will allow you to pass the > > values below as arguments, or even better, a constructor that takes a const > > NewReportDisk&. > > I did this as a ReportDisk constructor, but then undid it. It's doing more than > conversion from NewReportDisk->ReportDisk, it's also getting the current > (system) time, and setting the state to kPending (which it isn't clear why that > constructor should set to that specific value). When it only sets the uuid and > file_path, it seems more cluttery to have it out-of-line than just in the one > place where we do this. What about just something like: ReportDisk::ReportDisk(UUID, FilePath, time_t, ReportState); This way the caller would still be the one passing in time(nullptr) and ReportState::kPending. https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { On 2015/02/13 06:34:05, scottmg wrote: > On 2015/02/13 00:32:40, Peter Kasting wrote: > > Nit: Consider avoiding using a with-side-effect statement as a loop test. For > > example: > > > > for (dirent* entry = readdir(dir); entry; entry = readdir(dir)) { > > > > This is only one line, avoids testing the result of side effect statements, > and > > also places |entry| into a narrower scope (see previous comment). > > I defer to Robert/Mark if they prefer that in this _posix file. (I could go > either way -- I don't like the duplication of readdir() in the for-loop version. Part of the history here is that a few months back I enabled an MSVC warning that bans "while (a = b)", and I fixed by using a for() loop in those cases. It's possible to avoid the warning by adding an extra set of parens as you've done here but there is at least some level of precedent for doing it the other way.
https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { scottmg wrote: > I defer to Robert/Mark if they prefer that in this _posix file. (I could go > either way -- I don't like the duplication of readdir() in the for-loop version. I missed this appeal the first time I read through the comments. I don’t like the duplication either. This is a well-understood warning and the extra parentheses to tell the compiler that you mean what you say are also well-understood.
https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { On 2015/02/13 22:36:15, Mark Mentovai wrote: > scottmg wrote: > > I defer to Robert/Mark if they prefer that in this _posix file. (I could go > > either way -- I don't like the duplication of readdir() in the for-loop > version. > > I missed this appeal the first time I read through the comments. I don’t like > the duplication either. This is a well-understood warning and the extra > parentheses to tell the compiler that you mean what you say are also > well-understood. OK. I defer to you guys. I think the primary benefit of the for () construct is moving the scope of |entry| to be limited to the loop body, but if that doesn't outweigh this, that's OK.
https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... File util/test/scoped_temp_dir_posix.cc (right): https://codereview.chromium.org/913273002/diff/20001/util/test/scoped_temp_di... util/test/scoped_temp_dir_posix.cc:48: while ((entry = readdir(dir))) { Peter Kasting wrote: > On 2015/02/13 22:36:15, Mark Mentovai wrote: > > scottmg wrote: > > > I defer to Robert/Mark if they prefer that in this _posix file. (I could go > > > either way -- I don't like the duplication of readdir() in the for-loop > > version. > > > > I missed this appeal the first time I read through the comments. I don’t like > > the duplication either. This is a well-understood warning and the extra > > parentheses to tell the compiler that you mean what you say are also > > well-understood. > > OK. I defer to you guys. I think the primary benefit of the for () construct > is moving the scope of |entry| to be limited to the loop body, but if that > doesn't outweigh this, that's OK. I do prefer the for for that reason when it can be done without duplication, but the duplication is worse than the wider-than-necessary scoping in my opinion.
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:131: std::vector<const CrashReportDatabase::Report> reports; On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:01, scottmg wrote: > > On 2015/02/13 00:32:38, Peter Kasting wrote: > > > This is out-of-scope for the current CL, but consider adding a typedef for > > this > > > type (e.g. CrashReportDatabase::Reports) and then making use of it in this > > file > > > and in the CrashReportDatabase subclasses. > > > > > > While this would perhaps enhance readability and maintainability on its own, > > it > > > has an optional side nicety. In a number of places where you currently do > > > something like: > > > > > > std::vector<const CrashReportDatabase::Report> reports; > > > ...db()->GetPendingReports(&reports); > > > EXPECT_EQ(4, reports.size()); > > > > > > ... > > > > > > reports.clear(); > > > db()->GetPendingReports(&reports); > > > EXPECT_EQ(4, reports.size()); > > > > > > This would give you the option of making each block a local scope, a la: > > > > > > { > > > CrashReportDatabase::Reports reports; > > > ...db()->GetPendingReports(&reports); > > > EXPECT_EQ(4, reports.size()); > > > } > > > > > > ...without the verbosity of the ugly vector<> type each time. This might > help > > > callers to see which pieces of which tests were self-contained and which > ones > > > fetched report vectors to be used by the rest of the test code. > > > > That seems reasonable. I sort of dislike hiding vector behind a typedef that > > makes it less clear what you're working with. I will defer that to a future > > change in any case, and Mark/Robert's preference. > > It's a minor thing. It wouldn't normally be an issue if functions returned this > type now that we have "auto", but since the ones in question here still need to > take variables as outparams, it's still a factor. > > One reason I like "Reports" over "vector<Report>" is that, largely, callers > shouldn't really care what precise type of container is being used. If for some > reason it becomes appropriate to use a list instead, it's easier to do that with > typedefs in place than without. However, if a reader needs to understand that > the underlying container is really a vector, then obviously this makes that more > opaque. Acknowledged. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_test.cc:222: On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:01, scottmg wrote: > > On 2015/02/13 00:32:38, Peter Kasting wrote: > > > Nit: I would remove this blank line > > > > I hate to dissent for something so minor, but because it's used in subsequent > > blocks that are separated by blanks, I feel not attaching it to line 223 is > > better. > > In that case, I wonder why you don't have a blank line after the declaration of > |reports| above? It seems like being consistent might be better. > > I think removing the blank might be more consistent with "declare as close to > first use as possible", but that's a very weak argument, so I'm OK with either > route. Fair enough. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:89: //! \brief Manages the metadata for the set of reports, handling serialization On 2015/02/13 22:30:33, Peter Kasting wrote: > On 2015/02/13 06:34:04, scottmg wrote: > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > Consider pulling this Metadata class and the associated helpers out to > another > > > file. It's a little easier to read .cc files when they're basically one > class > > > per file, and CrashReportDatabaseWin is arguably the focus of this file, > given > > > the filename. > > > > I would prefer not to. I agree it would be a bit easier to read, but the two > > classes only make sense together, Metadata being an internal detail of how > > CrashReportDatabaseWin is implemented. If Metadata were in a separate file, I > > think it would be slightly less clear to the user of the code, who might think > > that it was available for standalone use. > > In principle, why couldn't it be available for standalone use? If there was a > second, separate legitimate use for it it seems like that would be appropriate. > Your argument suggests to me that it's important to prevent anyone else from > ever accessing this class, and I'm not sure why that would be. > > At the very minimum, when defining multiple classes in the same file, I suggest > using divider comments like those in e.g. > chrome/browser/ui/omnibox/omnibox_edit_model.cc. It's not important to prevent it, it just wasn't intended for separate use. Someone opening up this directory wondering how to access a crash report database should go here. I've added some dividers to perhaps help make it more clear. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:137: const T& mutator); On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:04, scottmg wrote: > > On 2015/02/13 00:32:39, Peter Kasting wrote: > > > Taking a mutator and passing in lambdas is very clever, but I can't help > > > thinking this would all be even clearer as just a pair of functions, > > > FindReport() and FindReportAnyState(), which take a UUID (and, in the former > > > case, a ReportState) and return the ReportDisk* as an outparam. (If you > want > > > FindReportAnyState() to be a const function with a const outparam you could > do > > > so, although I might rename as "FindConstReportAnyState" if so.) > > > > Aha! That was how I initially wrote it. (Around ps#13 to ps#14 in the original > > CL). The drawback of returning non-const ReportDisk* was that the Finds > couldn't > > be generic because then Metadata had to either assume that the metadata was > > always dirtied (causing unnecessary Write()s in e.g. LookUpCrashReport()) or > had > > to delegate the responsibility to call Write() when the ReportDisk was changed > > to the caller, which seemed error prone (and more verbose for callers in any > > case). > > > > If you think the lambdas are too cute, we could simply swap MutateSingleReport > > for FindSingleReportAndMarkDirty. I think that matches what you intended, so I > > did that. > > Yeah, I think that's fine. It's OK with me for these helpers to be > non-generic/not-completely-parallel in structure since their callers are fairly > specialized. We can always do something better if we need to add more callsites > later. Acknowledged. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:181: // The format of the metadata file is a MetadataFileHeader, followed by a On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:05, scottmg wrote: > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > This is a good comment but it seems like it should be somewhere other than > the > > > middle of the file. Perhaps it belongs in the Metadata class header > comment, > > or > > > in a file comment atop the file? Likewise, the struct definitions > themselves > > > are somewhat oddly placed in the midst of the Metadata member definitions. > > > Maybe the whole block should move above Metadata? I'm not sure. > > > > The idea was that there's a small subsection here, from the block comment that > > describes the format at a high level, the structures the define it, followed > by > > Read() and Write() which are the only methods that use MetadataFile*. > > Hmm. I think that connection is not terribly apparent to a reader. I would > probably prefer to move these somewhere else to keep all the Metadata:: method > definitions together, even at the cost of making the consumer of these objects > be further away from their definitions. If you follow my divider comment > suggestion above, these would probably still be fairly easy to scan up and find. Done. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:322: ReportDisk r; On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:03, scottmg wrote: > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > Perhaps ReportDisk should define a constructor that takes a const > > > MetadataFileReportRecord& and copies over the values, with appropriate > > > conversion/casting. > > > > > > You would then simply do something like: > > > > > > if (record->file_path_index >= string_table.size() || ...) { > > > ... > > > } > > > reports_.push_back(ReportDisk(record)); > > > > > > This would be easier to read here and would minimize the amount of code > > external > > > to ReportDisk that has to understand its fields. > > > > It needs the string table and report_dir FilePath to convert indices to > > pointers. It feels odd to pass those to a constructor; how about a static > > converter: > > > > reports.push_back(ReportDisk::FromMetataFileReportRecord( > > record, report_dir_, string_table)); > > I'm OK with either. I don't feel any oddness about passing a constructor the > arguments it needs to do its work, even if some of those aren't stored on the > object itself -- we do this a lot across the codebase -- and keeping something > as a constructor is slightly better in terms of preserving encapsulation of the > class members, but there's not such a big difference between the two methods > that I would demand one over the other. OK, switched to constructor. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:380: << report_dir_.value().c_str(); On 2015/02/13 22:30:33, Peter Kasting wrote: > On 2015/02/13 06:34:03, scottmg wrote: > > On 2015/02/13 00:32:39, Peter Kasting wrote: > > > Don't use c_str() with <<; it's unnecessary. > > > > There's no operator<< for wstring/string16 in <ostream>. Perhaps something in > > Chromium (illegally) adds one to std::? > > > > ... > > > > Huh. Yup: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=882 > > > > Crashpad is currently building against a very stripped down version of base, > and > > doesn't have this. > > Hmm. The overload for string16 (in string16.h) is not in namespace std. I > wonder if we could eliminate the wstring overload in base and do the string16 > overload in string16.h regardless of the size of wchar_t. This would break > anyone who currently tries to log a wstring directly and doesn't include > string16.h, I think, which I doubt would be very many people. > > It seems like that's beyond the scope of this change, but possibly worth doing. > Would you be willing to look into it? Yeah, I could take a look. In this case though, I think it wouldn't help without further work on changing wstring to string16. e.g. FilePath::StringType is wstring which I think (?) won't match the operator<<(string16). https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:05, scottmg wrote: > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > Nit: DCHECK_EQ > > > > That doesn't compile because ReportState is "enum class" (not enum) and so > > doesn't match an operator<<. > > Since you removed the explicit type on the enum class in the latest patch, it > seems like it might be better to just make this a non-class enum (which would > allow this to work), unless I'm missing something on what the "class" part buys > your code (I confess I haven't used enum class much yet). The primary benefit here is that the enumeration values are scoped. That is, it's named "ReportState::kPending", not "kPending", so the names need not have long(er) uniquifying names. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:434: return CrashReportDatabase::kNoError; On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:02, scottmg wrote: > > On 2015/02/13 00:32:39, Peter Kasting wrote: > > > Nit: Simpler: > > > > > > if (os == CrashReportDatabase::kNoError) > > > *out_report = &reports_[i]; > > > return os; > > > > > > Again, same comment applies to MutateSingleReport() below, though there this > > > doesn't actually save lines, just a tiny bit of verbiage. > > > > To me that feels more complicated. Previously it reads as "check condition, > > early out, check condition, early out. When everything is OK, prepare the data > > to be returned and return success". > > > > Maybe it's just me. Done, anyway. > > That's a legit reading. I read the intent of the code as "return whether the > report successfully verified; populate |out_report| if the function succeeds", > which is what led to my suggestion. Both are plausible. Acknowledged.
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:380: << report_dir_.value().c_str(); On 2015/02/13 23:37:16, scottmg wrote: > On 2015/02/13 22:30:33, Peter Kasting wrote: > > On 2015/02/13 06:34:03, scottmg wrote: > > > On 2015/02/13 00:32:39, Peter Kasting wrote: > > > > Don't use c_str() with <<; it's unnecessary. > > > > > > There's no operator<< for wstring/string16 in <ostream>. Perhaps something > in > > > Chromium (illegally) adds one to std::? > > > > > > ... > > > > > > Huh. Yup: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=882 > > > > > > Crashpad is currently building against a very stripped down version of base, > > and > > > doesn't have this. > > > > Hmm. The overload for string16 (in string16.h) is not in namespace std. I > > wonder if we could eliminate the wstring overload in base and do the string16 > > overload in string16.h regardless of the size of wchar_t. This would break > > anyone who currently tries to log a wstring directly and doesn't include > > string16.h, I think, which I doubt would be very many people. > > > > It seems like that's beyond the scope of this change, but possibly worth > doing. > > Would you be willing to look into it? > > Yeah, I could take a look. In this case though, I think it wouldn't help without > further work on changing wstring to string16. e.g. FilePath::StringType is > wstring which I think (?) won't match the operator<<(string16). Yeah, we should fix FilePath to use string16 in place of wstring. I'm pretty sure wstring should just be excised from the codebase entirely. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 23:37:16, scottmg wrote: > On 2015/02/13 22:30:32, Peter Kasting wrote: > > On 2015/02/13 06:34:05, scottmg wrote: > > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > > Nit: DCHECK_EQ > > > > > > That doesn't compile because ReportState is "enum class" (not enum) and so > > > doesn't match an operator<<. > > > > Since you removed the explicit type on the enum class in the latest patch, it > > seems like it might be better to just make this a non-class enum (which would > > allow this to work), unless I'm missing something on what the "class" part > buys > > your code (I confess I haven't used enum class much yet). > > The primary benefit here is that the enumeration values are scoped. That is, > it's named "ReportState::kPending", not "kPending", so the names need not have > long(er) uniquifying names. But the enum is file-scoped anyway, so we're unlikely to introduce conflicting names, aren't we? https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:566: ReportDisk report_disk; On 2015/02/13 22:30:32, Peter Kasting wrote: > On 2015/02/13 06:34:03, scottmg wrote: > > On 2015/02/13 00:32:38, Peter Kasting wrote: > > > Nit: Consider adding a constructor for this that will allow you to pass the > > > values below as arguments, or even better, a constructor that takes a const > > > NewReportDisk&. > > > > I did this as a ReportDisk constructor, but then undid it. It's doing more > than > > conversion from NewReportDisk->ReportDisk, it's also getting the current > > (system) time, and setting the state to kPending (which it isn't clear why > that > > constructor should set to that specific value). When it only sets the uuid and > > file_path, it seems more cluttery to have it out-of-line than just in the one > > place where we do this. > > What about just something like: > > ReportDisk::ReportDisk(UUID, FilePath, time_t, ReportState); > > This way the caller would still be the one passing in time(nullptr) and > ReportState::kPending. (I think this is the only comment you didn't reply to.)
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 23:45:10, Peter Kasting wrote: > On 2015/02/13 23:37:16, scottmg wrote: > > On 2015/02/13 22:30:32, Peter Kasting wrote: > > > On 2015/02/13 06:34:05, scottmg wrote: > > > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > > > Nit: DCHECK_EQ > > > > > > > > That doesn't compile because ReportState is "enum class" (not enum) and so > > > > doesn't match an operator<<. > > > > > > Since you removed the explicit type on the enum class in the latest patch, > it > > > seems like it might be better to just make this a non-class enum (which > would > > > allow this to work), unless I'm missing something on what the "class" part > > buys > > > your code (I confess I haven't used enum class much yet). > > > > The primary benefit here is that the enumeration values are scoped. That is, > > it's named "ReportState::kPending", not "kPending", so the names need not have > > long(er) uniquifying names. > > But the enum is file-scoped anyway, so we're unlikely to introduce conflicting > names, aren't we? I do like the naming advantage of enum class, but there are others too. An enum class type is much more a proper type than its predecessor enum. enum class forbids undesirable conversions. I think you'd actually like them a lot.
https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:407: DCHECK(new_report_disk.state == ReportState::kPending); On 2015/02/13 23:45:10, Peter Kasting wrote: > On 2015/02/13 23:37:16, scottmg wrote: > > On 2015/02/13 22:30:32, Peter Kasting wrote: > > > On 2015/02/13 06:34:05, scottmg wrote: > > > > On 2015/02/13 00:32:40, Peter Kasting wrote: > > > > > Nit: DCHECK_EQ > > > > > > > > That doesn't compile because ReportState is "enum class" (not enum) and so > > > > doesn't match an operator<<. > > > > > > Since you removed the explicit type on the enum class in the latest patch, > it > > > seems like it might be better to just make this a non-class enum (which > would > > > allow this to work), unless I'm missing something on what the "class" part > > buys > > > your code (I confess I haven't used enum class much yet). > > > > The primary benefit here is that the enumeration values are scoped. That is, > > it's named "ReportState::kPending", not "kPending", so the names need not have > > long(er) uniquifying names. > > But the enum is file-scoped anyway, so we're unlikely to introduce conflicting > names, aren't we? That's true. I think it's also preferable for them to have a type (other than int) so that there's some guard against ending up with a report in state 5. They're still convertible to ints, but only when explicitly casted. The only reason against it seems to be DCHECK_EQ? That feels like a bug in logging.h more than anything. https://codereview.chromium.org/913273002/diff/20001/client/crash_report_data... client/crash_report_database_win.cc:566: ReportDisk report_disk; On 2015/02/13 23:45:10, Peter Kasting wrote: > On 2015/02/13 22:30:32, Peter Kasting wrote: > > On 2015/02/13 06:34:03, scottmg wrote: > > > On 2015/02/13 00:32:38, Peter Kasting wrote: > > > > Nit: Consider adding a constructor for this that will allow you to pass > the > > > > values below as arguments, or even better, a constructor that takes a > const > > > > NewReportDisk&. > > > > > > I did this as a ReportDisk constructor, but then undid it. It's doing more > > than > > > conversion from NewReportDisk->ReportDisk, it's also getting the current > > > (system) time, and setting the state to kPending (which it isn't clear why > > that > > > constructor should set to that specific value). When it only sets the uuid > and > > > file_path, it seems more cluttery to have it out-of-line than just in the > one > > > place where we do this. > > > > What about just something like: > > > > ReportDisk::ReportDisk(UUID, FilePath, time_t, ReportState); > > > > This way the caller would still be the one passing in time(nullptr) and > > ReportState::kPending. > > (I think this is the only comment you didn't reply to.) Sorry, missed clicking done. Done as suggested.
LGTM. Once this is landed, I will mark the readability request bug as fixed, which should grant readability. If you don't see me do this in short order, or don't receive readability within a few days, ping me again and I'll investigate. https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_test.cc:210: UploadReport(reports[1].uuid, false, ""); Nit: Some more "" -> std::string() (multiple places) https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:73: UUID uuid; // UUID is a 16 byte, standard layout structure. Nit: Consider lining up most or all of these EOL comments (I'd probably align to the current |file_path_index| comment, and linebreak between |last_upload_attempt_time| and its comment). This is not required, but is suggested at http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... . https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:114: memset(&r, 0, sizeof(r)); Nit: This is legal, but the following would be more common: MetadataFileReportRecord r{0}; (I realize the "0" here just means "set the first field to zero", and the rest of the zero-initialization is done automatically, but that's sort of a convention.) You could probably even omit the initialization, since it only affects the contents of |padding|. https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:701: return CrashReportDatabase::kNoError; Nit: Shorter, follows the same pattern as earlier: if (os == CrashReportDatabase::kNoError) report_disk->state = ReportState::kCompleted; return os; You're welcome to change the two functions above as well, though there it's the same number of lines, it would merely be a consistency win.
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
Thanks. Mark, please let me know if you have any concerns before landing. https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_test.cc:210: UploadReport(reports[1].uuid, false, ""); On 2015/02/14 00:24:23, Peter Kasting wrote: > Nit: Some more "" -> std::string() (multiple places) Done. https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:73: UUID uuid; // UUID is a 16 byte, standard layout structure. On 2015/02/14 00:24:24, Peter Kasting wrote: > Nit: Consider lining up most or all of these EOL comments (I'd probably align to > the current |file_path_index| comment, and linebreak between > |last_upload_attempt_time| and its comment). This is not required, but is > suggested at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... > . Done. https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:114: memset(&r, 0, sizeof(r)); On 2015/02/14 00:24:23, Peter Kasting wrote: > Nit: This is legal, but the following would be more common: > > MetadataFileReportRecord r{0}; > > (I realize the "0" here just means "set the first field to zero", and the rest > of the zero-initialization is done automatically, but that's sort of a > convention.) > > You could probably even omit the initialization, since it only affects the > contents of |padding|. In the most recent ps (the constructor version), I memset only |padding|. While it's strictly unnecessary, because the structure (including padding bytes) will be accessed by LoggingWriteFile I think ought to be initialized. (More practically, valgrind or similar would probably whine.) https://codereview.chromium.org/913273002/diff/100001/client/crash_report_dat... client/crash_report_database_win.cc:701: return CrashReportDatabase::kNoError; On 2015/02/14 00:24:23, Peter Kasting wrote: > Nit: Shorter, follows the same pattern as earlier: > > if (os == CrashReportDatabase::kNoError) > report_disk->state = ReportState::kCompleted; > return os; > > You're welcome to change the two functions above as well, though there it's the > same number of lines, it would merely be a consistency win. Done.
Still LGTM https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:97: int64_t last_upload_attempt_time; // Holds a time_t. Up to you whether you like this better: ... int64_t creation_time; // Holds a time_t. int64_t last_upload_attempt_time; // Holds a time_t. ... https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:137: uuid = report.uuid; Nit: Prefer to use the constructor initializer list where possible over initializing the variables in the body of the constructor. This is a slight efficiency win and also may allow you to make the members const if you wish. (3 constructors) https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:151: const std::string& string_table) : Report() { Kinda suprised that clang-format (I assume) put ": Report()" at EOL here, but not just below in the next constructor. Be consistent; personally I think the on-new-line way that's done below is clearer.
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:97: int64_t last_upload_attempt_time; // Holds a time_t. On 2015/02/14 00:50:22, Peter Kasting wrote: > Up to you whether you like this better: > > ... > int64_t creation_time; // Holds a time_t. > int64_t last_upload_attempt_time; > // Holds a time_t. > ... Sure. https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:137: uuid = report.uuid; On 2015/02/14 00:50:22, Peter Kasting wrote: > Nit: Prefer to use the constructor initializer list where possible over > initializing the variables in the body of the constructor. This is a slight > efficiency win and also may allow you to make the members const if you wish. (3 > constructors) Done. https://codereview.chromium.org/913273002/diff/140001/client/crash_report_dat... client/crash_report_database_win.cc:151: const std::string& string_table) : Report() { On 2015/02/14 00:50:22, Peter Kasting wrote: > Kinda suprised that clang-format (I assume) put ": Report()" at EOL here, but > not just below in the next constructor. Be consistent; personally I think the > on-new-line way that's done below is clearer. Apparently I didn't hit my format button there for some reason. Done.
https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:92: UUID uuid; // UUID is a 16 byte, standard layout structure. I liked the comment style better before. I don’t really like trying to maintain alignment of the comments in their own column. You wind up with things having to move when the maximum length of the left column changes, or you wind up with the ugly thing that happened on lines 97 and 98. I much prefer to just let each in-line comment stand alone, so that as fields are changed, blame history on the whole structure doesn’t become difficult. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:104: MetadataFileReportRecord() {} Initialization? https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:111: enum class ReportState { Conceptually, this should precede MetadataFileReportRecord. That struct uses ReportState, not by name, but by semantics, so organizing it that way will help human readers. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:124: ReportState state; Normally, constructors precede data members. The same applies to MetadataFileReportRecord. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:138: : uuid(report.uuid), Start with Report() to make it explicit. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:154: uuid = record.uuid; Why not use initializer list syntax? The same applies to the next constructor overload. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:256: bool dirty_; //! \brief True when a Write() is required on destruction. `true` https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:308: DCHECK(new_report_disk.state == ReportState::kPending); DCHECK_EQ didn’t work? https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:397: std::vector<MetadataFileReportRecord> records(header.num_records); Was this change requested? Same on line 448. The scoped_ptr<> had the nice property of not initializing the memory with zeroes first. That’s pointless for a block of memory that’s going to be entirely overwritten with new data. https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... client/crash_report_database_win.cc:639: return kNoError; This was correct before, but now it doesn’t look right. If this change is what was desired, it looks like you now need to change this line to “return os”. https://codereview.chromium.org/913273002/diff/160001/util/test/scoped_temp_d... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/913273002/diff/160001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:92: (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT)) I didn’t catch this before, but is FILE_ATTRIBUTE_REPARSE_POINT correct? Can’t regular files have that bit set too?
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
(I stupidly fat-fingered and deleted the patchset with your most recent comments, so (re-)replying inline. Very sorry.) On 2015/02/17 17:48:48, Mark Mentovai wrote: > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > File client/crash_report_database_win.cc (right): > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:92: UUID uuid; // UUID is a > 16 byte, standard layout structure. > I liked the comment style better before. > > I don’t really like trying to maintain alignment of the comments in their own > column. You wind up with things having to move when the maximum length of the > left column changes, or you wind up with the ugly thing that happened on lines > 97 and 98. I much prefer to just let each in-line comment stand alone, so that > as fields are changed, blame history on the whole structure doesn’t become > difficult. I agree fwiw. I don't feel that strongly about it. My rationale is that they comments have more to do with what they're commenting than each other, so it doesn't make sense to align them with each other. The history argument is a good one, so I've de-aligned these. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:104: MetadataFileReportRecord() {} > Initialization? This is only used when immediately followed by a read, so it didn't seem necessary to initialize all members. If you think it's safer, I can do that here though. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:111: enum class ReportState { > Conceptually, this should precede MetadataFileReportRecord. That struct uses > ReportState, not by name, but by semantics, so organizing it that way will help > human readers. Done. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:124: ReportState state; > Normally, constructors precede data members. The same applies to > MetadataFileReportRecord. Done. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:138: : uuid(report.uuid), > Start with Report() to make it explicit. This one doesn't derive from anything. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:154: uuid = record.uuid; > Why not use initializer list syntax? The same applies to the next constructor > overload. Do you mean add a constructor to Report that takes most of these arguments? I can't initialize elements of Report in the derived class's initializer list. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:256: bool dirty_; //! \brief True when a > Write() is required on destruction. > `true` Done. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:308: DCHECK(new_report_disk.state == > ReportState::kPending); > DCHECK_EQ didn’t work? No, there's no operator<<(ReportState) to output the expected/actual values, and enum class doesn't decay to int. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:397: std::vector<MetadataFileReportRecord> > records(header.num_records); > Was this change requested? Same on line 448. Yes. I think primarily to be able to benefit from the range-based for loop. > > The scoped_ptr<> had the nice property of not initializing the memory with > zeroes first. That’s pointless for a block of memory that’s going to be entirely > overwritten with new data. I think (?) in both cases (allocating a vector, or new[]) it's going to call the default constructor on all elements. In this case, only the UUID constructor does work. Previously, there were no constructors so the default constructor would have been the same as the non-initializing constructor that's there now. It's a bit subtle to have that there though. I do agree the scoped_ptr<[]> seems to match the intent a bit more closely. That is, it's a "bunch of bytes" coming from disk and we're only using the struct to unpack them. It's not actually a vector that's being manipulated in any vector-like ways. OTOH, there doesn't seem to be much drawback at line 448 as it's only reserve() followed by push_backs, so the vector seems more clear. > > https://codereview.chromium.org/913273002/diff/160001/client/crash_report_dat... > client/crash_report_database_win.cc:639: return kNoError; > This was correct before, but now it doesn’t look right. > > If this change is what was desired, it looks like you now need to change this > line to “return os”. Shoot, good catch, thanks. I added a test that would have caught this. > > https://codereview.chromium.org/913273002/diff/160001/util/test/scoped_temp_d... > File util/test/scoped_temp_dir_win.cc (right): > > https://codereview.chromium.org/913273002/diff/160001/util/test/scoped_temp_d... > util/test/scoped_temp_dir_win.cc:92: (FILE_ATTRIBUTE_DIRECTORY | > FILE_ATTRIBUTE_REPARSE_POINT)) > I didn’t catch this before, but is FILE_ATTRIBUTE_REPARSE_POINT correct? Can’t > regular files have that bit set too? You're right. It's not clear what we actually want to do in that case. We probably don't want to follow the link elsewhere, but we also have to do something or the RemoveDirectory will fail. So, I just ASSERT that it doesn't come up since it never will in practice for this code.
LGTM https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_test.cc:461: _wunlink(report_path.value().c_str()); Check the return value, both branches. https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_win.cc:101: MetadataFileReportRecord() {} Regarding initialization: Leave a comment here explaining why there’s no initialization. https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_win.cc:137: : uuid(report.uuid), Regarding no superclass: you’re right. I was thrown off by the ordering, where you have (MFRR declaration, RD declaration, MFRR implementation, RD implementation). They all started to run together in my mind. That’s also why I suggested the initializer list for ReportDisk. It’s OK to leave this all as-is, but maybe this suggests that putting all of MFRR together and all of RD together would be better for readability. I don’t feel strongly about it if you like it structured as-is. https://codereview.chromium.org/913273002/diff/200001/util/test/scoped_temp_d... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/913273002/diff/200001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:91: ASSERT_FALSE(find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT); It’s OK if this was set on a directory, so if you want to move this ASSERT into the else branch, I’d be fine with that too.
New patchsets have been uploaded after l-g-t-m from mark@chromium.org
https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_test.cc:461: _wunlink(report_path.value().c_str()); On 2015/02/17 19:40:40, Mark Mentovai wrote: > Check the return value, both branches. Done. https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... File client/crash_report_database_win.cc (right): https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_win.cc:101: MetadataFileReportRecord() {} On 2015/02/17 19:40:40, Mark Mentovai wrote: > Regarding initialization: Leave a comment here explaining why there’s no > initialization. Done. https://codereview.chromium.org/913273002/diff/200001/client/crash_report_dat... client/crash_report_database_win.cc:137: : uuid(report.uuid), On 2015/02/17 19:40:40, Mark Mentovai wrote: > Regarding no superclass: you’re right. I was thrown off by the ordering, where > you have (MFRR declaration, RD declaration, MFRR implementation, RD > implementation). They all started to run together in my mind. That’s also why I > suggested the initializer list for ReportDisk. It’s OK to leave this all as-is, > but maybe this suggests that putting all of MFRR together and all of RD together > would be better for readability. I don’t feel strongly about it if you like it > structured as-is. The implementation of MFRR has to go below the definition of ReportDisk because this constructor uses ReportDisk. But you're right, I too have gotten my "reports" mixed up with my "records" a few times. https://codereview.chromium.org/913273002/diff/200001/util/test/scoped_temp_d... File util/test/scoped_temp_dir_win.cc (right): https://codereview.chromium.org/913273002/diff/200001/util/test/scoped_temp_d... util/test/scoped_temp_dir_win.cc:91: ASSERT_FALSE(find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT); On 2015/02/17 19:40:40, Mark Mentovai wrote: > It’s OK if this was set on a directory, so if you want to move this ASSERT into > the else branch, I’d be fine with that too. The directory enumeration code in chromium notes that it doesn't follow reparse directories because they can form a cycle/hang (or stack overflow in this case). So I think I'll just leave it as the more conservative version.
Still LGTM https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... client/crash_report_database_test.cc:461: EXPECT_EQ(_wunlink(report_path.value().c_str()), 0); (expected, actual) for gtest. Ugh. https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... client/crash_report_database_test.cc:463: EXPECT_EQ(unlink(report_path.value().c_str()), 0); #include "util/test/errors.h" and << ErrnoMessage("unlink"); on this one.
New patchsets have been uploaded after l-g-t-m from mark@chromium.org
https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... File client/crash_report_database_test.cc (right): https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... client/crash_report_database_test.cc:461: EXPECT_EQ(_wunlink(report_path.value().c_str()), 0); On 2015/02/17 19:56:23, Mark Mentovai wrote: > (expected, actual) for gtest. Ugh. Sigh, keep forgetting. Done. https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... client/crash_report_database_test.cc:463: EXPECT_EQ(unlink(report_path.value().c_str()), 0); On 2015/02/17 19:56:23, Mark Mentovai wrote: > #include "util/test/errors.h" and << ErrnoMessage("unlink"); on this one. Done.
LGTM
Message was sent while issue was closed.
Committed patchset #9 (id:260001) manually as bd77b3034fb9d759968d30abc111bc491362eb6c (presubmit successful).
Message was sent while issue was closed.
On 2015/02/17 20:00:39, scottmg wrote: > https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... > File client/crash_report_database_test.cc (right): > > https://codereview.chromium.org/913273002/diff/220001/client/crash_report_dat... > client/crash_report_database_test.cc:461: > EXPECT_EQ(_wunlink(report_path.value().c_str()), 0); > On 2015/02/17 19:56:23, Mark Mentovai wrote: > > (expected, actual) for gtest. Ugh. > > Sigh, keep forgetting. Done. (That's why most Chromium code simply does (expected, actual) on all _EQ or _NE macros, inside or outside of gtest -- it's much easier to be in the habit of doing it everywhere than to try and remember that a few cases are exceptional.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
