[sql] Track uploads of diagnostic data to prevent duplication.

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index 35afa12319fc9a021b6c7c3d7dfd30971e35cf5f..e9226b2c7f4cc313028b6ce1bef1e73d8db95424 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -256,14 +256,14 @@ void GenerateDiagnostics(sql::Connection* db,
 
     // Corrupt cases currently dominate, report them very infrequently.
     static const uint64 kCorruptReportsPerMillion = 10000;
-    if (rand < kCorruptReportsPerMillion)
+    if (rand < kCorruptReportsPerMillion && db->RegisterIntentToUpload())

[sky, 2015/10/16 15:07:24]

Naming is hard, but looking at this code it isn't clear why something called
'register intent to upload' would dictate whether an error is reported.
'register intent to upload' reads as a verb, and nothing something that you need
to check the return value from or dictates why it would effect reporting an
error. OTOH if RegisterIntentToUpload was named ShouldReportError, then this
code makes total sense. WDYT?

[Scott Hess - ex-Googler, 2015/10/16 15:27:43]

Previously it was more like ShouldReportError(), but that seemed confusing (to
both of us) that it had a side effect which changed the future return value.  I
changed to this under the theory that if it failed to register intent, then it
shouldn't report.

It is shaped a little like a test-and-set function, but that didn't really help
with naming.  I suppose it doesn't have to be that way, it could have separate
query and set calls, maybe with the contents cached between them.  But the set
would have to happen before the report, otherwise failure to set wouldn't
prevent uploads, so the code would look like:
  if (one_test && db->ShouldReportError()) {
    db->GoingToReportError();
    ReportError(...);
  }

Peter, WDYT?  Back to ShouldReportError(), side effect and all?

Long-term, 

       ReportCorrupt(db, startup_kb);
   } else if (error == SQLITE_READONLY) {
     // SQLITE_READONLY appears similar to SQLITE_CORRUPT - once it
     // is seen, it is almost guaranteed to be seen again.
     reported = true;
 
-    if (rand < kReportsPerMillion)
+    if (rand < kReportsPerMillion && db->RegisterIntentToUpload())
       ReportError(db, extended_error);
   } else {
     // Only set the flag when making a report.  This should allow
@@ -276,7 +276,8 @@ void GenerateDiagnostics(sql::Connection* db,
     // channels, so probably easier to just harvest from the field.
     if (rand < kReportsPerMillion) {
       reported = true;
-      ReportError(db, extended_error);
+      if (db->RegisterIntentToUpload())
+        ReportError(db, extended_error);
     }
   }
 }