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

components/history/core/browser/thumbnail_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/thumbnail_database.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 238 matching lines @@
 
   uint64 rand = base::RandGenerator(1000000);
   if (error == SQLITE_CORRUPT) {
     // Once the database is known to be corrupt, it will generate a
     // stream of errors until someone fixes it, so give one chance.
     // Set first in case of errors in generating the report.
     reported = true;
 
     // 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
     // later (potentially different) errors in a stream of errors to
     // be reported.
     //
     // TODO(shess): Would it be worthwile to audit for which cases
     // want once-only handling?  Sqlite.Error.Thumbnail shows
     // CORRUPT and READONLY as almost 95% of all reports on these
     // 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);
     }
   }
 }
 
 // NOTE(shess): Schema modifications must consider initial creation in
 // |InitImpl()|, recovery in |RecoverDatabaseOrRaze()|, and history pruning in
 // |RetainDataForPageUrls()|.
 bool InitTables(sql::Connection* db) {
   const char kIconMappingSql[] =
       "CREATE TABLE IF NOT EXISTS icon_mapping"
@@ skipping 1088 matching lines @@
   meta_table_.SetVersionNumber(8);
   meta_table_.SetCompatibleVersionNumber(std::min(8, kCompatibleVersionNumber));
   return true;
 }
 
 bool ThumbnailDatabase::IsFaviconDBStructureIncorrect() {
   return !db_.IsSQLValid("SELECT id, url, icon_type FROM favicons");
 }
 
 }  // namespace history