List all crashes in chrome://crashes, including those not uploaded

chrome/browser/crash_upload_list_crashpad.cc

Index: chrome/browser/crash_upload_list_crashpad.cc
diff --git a/chrome/browser/crash_upload_list_crashpad.cc b/chrome/browser/crash_upload_list_crashpad.cc
index f7d970529627a582c19bbe943723f6f6d6992f87..bbba7ee973fa1378f369c4151c0f0a18d9ab7b48 100644
--- a/chrome/browser/crash_upload_list_crashpad.cc
+++ b/chrome/browser/crash_upload_list_crashpad.cc
@@ -15,29 +15,46 @@
 namespace {
 
 #if defined(OS_WIN)
-typedef void (*GetUploadedReportsPointer)(
-    const crash_reporter::UploadedReport** reports,
+typedef void (*GetCrashReportsPointer)(
+    const crash_reporter::Report** reports,

[grt (UTC plus 2), 2016/06/20 23:26:08]

const betwixt the two stars? The caller of this shouldn't be able to modify the
array, should they?

[scottmg, 2016/06/21 00:06:30]

My const placement cdecl skillz are crud, but doesn't `X* const* x` mean I can't
change the value of x?

reports is the address of a pointer (so should mutable)

*reports is the array we're retrieving. We don't want to change the value of
that pointer from the callee's perspective, but the pointer is copied by value
here (so has to be mutable)

(*reports)[0..N-1] are the reports we're referencing (and those definitely
shouldn't be changed... that's the leading existing const I think, right?)

Hrm. Sorry if I'm lost and sound-like-it here. :)

[grt (UTC plus 2), 2016/06/21 01:32:50]

Ooops, you're right. I was thinking that the other file was working with a
vector of pointers to reports rather than a vector of reports themselves.
Apologies for the noise. This is what I get for trying to do a code review on a
phone while cooking dinner. :-(

     size_t* report_count);
 
-void GetUploadedReportsThunk(
-    std::vector<crash_reporter::UploadedReport>* uploaded_reports) {
-  static GetUploadedReportsPointer get_uploaded_reports = []() {
+void GetReportsThunk(
+    std::vector<crash_reporter::Report>* reports) {
+  static GetCrashReportsPointer get_crash_reports = []() {
     HMODULE exe_module = GetModuleHandle(chrome::kBrowserProcessExecutableName);
-    return reinterpret_cast<GetUploadedReportsPointer>(
-        exe_module ? GetProcAddress(exe_module, "GetUploadedReportsImpl")
+    return reinterpret_cast<GetCrashReportsPointer>(
+        exe_module ? GetProcAddress(exe_module, "GetCrashReportsImpl")
                    : nullptr);
   }();
 
-  if (get_uploaded_reports) {
-    const crash_reporter::UploadedReport* reports;
+  if (get_crash_reports) {

[grt (UTC plus 2), 2016/06/20 23:26:08]

I don't follow this. Apologies if this is a an uninformed question, but does
this run the lambda twice, once here and once below? If yes, why not ensure that
it only happens once? If no, then how is this actually working? Is this
condition merely checking whether or not the lambda has a value, and not
actually run it at all?

[scottmg, 2016/06/21 00:06:29]

Just once; the sneaky part is the () after the closing } in the definition of
the lambda, so the if is checking the value of the pointer returned by running
the lambda right after it's defined.

It might be a bit too clever. I think the idea was that it avoids requerying
repeatedly in the case that the function is not found assuming it was written
instead as:

  static Pointer p = nullptr;
  if (!p) {
    p = GetProcAddress(...)
  }

[grt (UTC plus 2), 2016/06/21 01:32:50]

Ah, very clever indeed. I shouldn't review on phone. All of the scrolling around
makes it hard to see the forest from the trees.

+    const crash_reporter::Report* reports_pointer;
     size_t report_count;
-    get_uploaded_reports(&reports, &report_count);
-    *uploaded_reports = std::vector<crash_reporter::UploadedReport>(
-        reports, reports + report_count);
+    get_crash_reports(&reports_pointer, &report_count);
+    *reports = std::vector<crash_reporter::Report>(
+        reports_pointer, reports_pointer + report_count);
   }
 }
 #endif  // OS_WIN
 
+UploadList::UploadInfo::State ReportUploadStateToUploadInfoState(
+    crash_reporter::ReportUploadState state) {
+  switch (state) {
+    case crash_reporter::ReportUploadState::NotUploaded:
+      return UploadList::UploadInfo::State::NotUploaded;
+
+    case crash_reporter::ReportUploadState::Pending:
+      return UploadList::UploadInfo::State::Pending;
+
+    case crash_reporter::ReportUploadState::Uploaded:
+      return UploadList::UploadInfo::State::Uploaded;
+  }
+
+  NOTREACHED();
+  return UploadList::UploadInfo::State::Uploaded;

[Lei Zhang, 2016/06/20 21:49:30]

Shouldn't our compilers know all the enum values are covered in the switch() and
this is not needed?

[scottmg, 2016/06/20 22:00:50]

No such luck. https://codereview.chromium.org/2070993002/#msg16 has the warning
from cl.exe

[[[ I don't think it's illegal to store a 7 in an enum class that only has 0-2
defined, is it? clang doesn't seem to mind either, so I'm not sure that cl is
really wrong.

[scottmg:~/work/x]$ cat e.cc
enum class Foo { A, B, C };
Foo bar = Foo(44);
[scottmg:~/work/x]$ clang++ -std=c++11 -Wall -Werror -c e.cc
]]]

+}
+
 }  // namespace
 
 CrashUploadListCrashpad::CrashUploadListCrashpad(
@@ -49,22 +66,21 @@ CrashUploadListCrashpad::~CrashUploadListCrashpad() {}
 
 void CrashUploadListCrashpad::LoadUploadList(
     std::vector<UploadList::UploadInfo>* uploads) {
-  std::vector<crash_reporter::UploadedReport> uploaded_reports;
+  std::vector<crash_reporter::Report> reports;
 #if defined(OS_WIN)
   // On Windows, we only link crash client into chrome.exe (not the dlls), and
   // it does the registration. That means the global that holds the crash report
   // database lives in the .exe, so we need to grab a pointer to a helper in the
-  // exe to get our uploaded reports list.
-  GetUploadedReportsThunk(&uploaded_reports);
+  // exe to get our reports list.
+  GetReportsThunk(&reports);
 #else
-  crash_reporter::GetUploadedReports(&uploaded_reports);
+  crash_reporter::GetReports(&reports);
 #endif
 
-  for (const crash_reporter::UploadedReport& uploaded_report :
-       uploaded_reports) {
+  for (const crash_reporter::Report& report : reports) {
     uploads->push_back(
-        UploadInfo(uploaded_report.remote_id,
-                   base::Time::FromTimeT(uploaded_report.creation_time),
-                   uploaded_report.local_id, base::Time()));
+        UploadInfo(report.remote_id, base::Time::FromTimeT(report.upload_time),
+                   report.local_id, base::Time::FromTimeT(report.capture_time),
+                   ReportUploadStateToUploadInfoState(report.state)));
   }
 }