[Metrics] Re-try failed protocol buffer uploads as well as failed XML uploads.

BUG=109818
TEST=none


Review URL: https://chromiumcodereview.appspot.com/10546044

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@141570 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 111 insertions, 10 deletions

diff --git a/chrome/common/metrics/metrics_log_manager.cc b/chrome/common/metrics/metrics_log_manager.cc
index 2e1261f..6a6907a 100644
--- a/chrome/common/metrics/metrics_log_manager.cc
+++ b/chrome/common/metrics/metrics_log_manager.cc
@@ -93,11 +93,17 @@ void MetricsLogManager::StageNextLogForUpload() {
 }
 
 bool MetricsLogManager::has_staged_log() const {
-  return !staged_log_text().empty();
+  return has_staged_log_proto() || has_staged_log_xml();
 }
 
 bool MetricsLogManager::has_staged_log_proto() const {
-  return has_staged_log() && staged_log_text().proto != kDiscardedLog;
+  return !staged_log_text().proto.empty() &&
+      staged_log_text().proto != kDiscardedLog;
+}
+
+bool MetricsLogManager::has_staged_log_xml() const {
+  return !staged_log_text().xml.empty() &&
+      staged_log_text().xml != kDiscardedLog;
 }
 
 void MetricsLogManager::DiscardStagedLog() {
@@ -108,6 +114,18 @@ void MetricsLogManager::DiscardStagedLog() {
 
 void MetricsLogManager::DiscardStagedLogProto() {
   staged_log_text_.proto = kDiscardedLog;
+
+  // If we're discarding the last piece of the log, reset the staged log state.
+  if (!has_staged_log())
+    DiscardStagedLog();
+}
+
+void MetricsLogManager::DiscardStagedLogXml() {
+  staged_log_text_.xml = kDiscardedLog;
+
+  // If we're discarding the last piece of the log, reset the staged log state.
+  if (!has_staged_log())
+    DiscardStagedLog();
 }
 
 void MetricsLogManager::DiscardCurrentLog() {
diff --git a/chrome/common/metrics/metrics_log_manager.h b/chrome/common/metrics/metrics_log_manager.h
index d276194..8f1ee39 100644
--- a/chrome/common/metrics/metrics_log_manager.h
+++ b/chrome/common/metrics/metrics_log_manager.h
@@ -71,9 +71,14 @@ class MetricsLogManager {
 
   // Returns true if there is a protobuf log that needs to be uploaded.
   // In the case that an XML upload needs to be re-issued due to a previous
-  // failure, has_staged_log() will return true while this returns false.
+  // failure, has_staged_log() can return true while this returns false.
   bool has_staged_log_proto() const;
 
+  // Returns true if there is an xml log that needs to be uploaded.
+  // In the case that a protobuf upload needs to be re-issued due to a previous
+  // failure, has_staged_log() can return true while this returns false.
+  bool has_staged_log_xml() const;
+
   // The text of the staged log, in compressed XML or protobuf format. Empty if
   // there is no staged log, or if compression of the staged log failed.
   const SerializedLog& staged_log_text() const {
@@ -88,6 +93,11 @@ class MetricsLogManager {
   // due to XML upload failures.
   void DiscardStagedLogProto();
 
+  // Discards the XML data in the staged log.
+  // This is useful to prevent needlessly re-issuing successful XML uploads
+  // due to protobuf upload failures.
+  void DiscardStagedLogXml();
+
   // Closes and discards |current_log|.
   void DiscardCurrentLog();
 
diff --git a/chrome/common/metrics/metrics_log_manager_unittest.cc b/chrome/common/metrics/metrics_log_manager_unittest.cc
index 8a154ce..dc58454 100644
--- a/chrome/common/metrics/metrics_log_manager_unittest.cc
+++ b/chrome/common/metrics/metrics_log_manager_unittest.cc
@@ -69,7 +69,8 @@ TEST(MetricsLogManagerTest, StandardFlow) {
   EXPECT_TRUE(log_manager.has_staged_log());
   EXPECT_FALSE(log_manager.staged_log_text().empty());
 
-  log_manager.DiscardStagedLog();
+  log_manager.DiscardStagedLogXml();
+  log_manager.DiscardStagedLogProto();
   EXPECT_EQ(second_log, log_manager.current_log());
   EXPECT_FALSE(log_manager.has_staged_log());
   EXPECT_FALSE(log_manager.has_unsent_logs());
@@ -112,7 +113,8 @@ TEST(MetricsLogManagerTest, InterjectedLog) {
 
   EXPECT_FALSE(log_manager.has_staged_log());
   log_manager.StageNextLogForUpload();
-  log_manager.DiscardStagedLog();
+  log_manager.DiscardStagedLogXml();
+  log_manager.DiscardStagedLogProto();
   EXPECT_FALSE(log_manager.has_unsent_logs());
 }
 
@@ -128,7 +130,8 @@ TEST(MetricsLogManagerTest, InterjectedLogPreservesType) {
   log_manager.FinishCurrentLog();
   log_manager.ResumePausedLog();
   log_manager.StageNextLogForUpload();
-  log_manager.DiscardStagedLog();
+  log_manager.DiscardStagedLogXml();
+  log_manager.DiscardStagedLogProto();
 
   // Verify that the remaining log (which is the original ongoing log) still
   // has the right type.
@@ -194,7 +197,8 @@ TEST(MetricsLogManagerTest, StoreAndLoad) {
     EXPECT_TRUE(log_manager.has_unsent_logs());
 
     log_manager.StageNextLogForUpload();
-    log_manager.DiscardStagedLog();
+    log_manager.DiscardStagedLogXml();
+    log_manager.DiscardStagedLogProto();
     // The initial log should be sent first; update the persisted storage to
     // verify.
     log_manager.PersistUnsentLogs();
@@ -203,12 +207,14 @@ TEST(MetricsLogManagerTest, StoreAndLoad) {
 
     // Handle the first ongoing log.
     log_manager.StageNextLogForUpload();
-    log_manager.DiscardStagedLog();
+    log_manager.DiscardStagedLogXml();
+    log_manager.DiscardStagedLogProto();
     EXPECT_TRUE(log_manager.has_unsent_logs());
 
     // Handle the last log.
     log_manager.StageNextLogForUpload();
-    log_manager.DiscardStagedLog();
+    log_manager.DiscardStagedLogXml();
+    log_manager.DiscardStagedLogProto();
     EXPECT_FALSE(log_manager.has_unsent_logs());
 
     // Nothing should have changed "on disk" since PersistUnsentLogs hasn't been
@@ -310,7 +316,8 @@ TEST(MetricsLogManagerTest, ProvisionalStoreNoop) {
     log_manager.StageNextLogForUpload();
     log_manager.StoreStagedLogAsUnsent(MetricsLogManager::PROVISIONAL_STORE);
     log_manager.StageNextLogForUpload();
-    log_manager.DiscardStagedLog();
+    log_manager.DiscardStagedLogXml();
+    log_manager.DiscardStagedLogProto();
     log_manager.BeginLoggingWithLog(log2, MetricsLogManager::ONGOING_LOG);
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
@@ -345,3 +352,69 @@ TEST(MetricsLogManagerTest, ProvisionalStoreNoop) {
     EXPECT_EQ(1U, serializer->TypeCount(MetricsLogManager::ONGOING_LOG));
   }
 }
+
+// Test that discarding just the XML log, then the protobuf log, works.
+TEST(MetricsLogManagerTest, DiscardXmlLogFirst) {
+  MetricsLogManager log_manager;
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+
+  MetricsLogBase* initial_log = new MetricsLogBase("id", 0, "version");
+  log_manager.BeginLoggingWithLog(initial_log, MetricsLogManager::INITIAL_LOG);
+  log_manager.FinishCurrentLog();
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+
+  log_manager.StageNextLogForUpload();
+  EXPECT_TRUE(log_manager.has_staged_log());
+  EXPECT_TRUE(log_manager.has_staged_log_xml());
+  EXPECT_TRUE(log_manager.has_staged_log_proto());
+  EXPECT_FALSE(log_manager.staged_log_text().empty());
+
+  log_manager.DiscardStagedLogXml();
+  EXPECT_TRUE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_TRUE(log_manager.has_staged_log_proto());
+  EXPECT_FALSE(log_manager.staged_log_text().empty());
+
+  log_manager.DiscardStagedLogProto();
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+  EXPECT_TRUE(log_manager.staged_log_text().empty());
+}
+
+// Test that discarding just the protobuf log, then the XML log, works.
+TEST(MetricsLogManagerTest, DiscardProtoLogFirst) {
+  MetricsLogManager log_manager;
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+
+  MetricsLogBase* initial_log = new MetricsLogBase("id", 0, "version");
+  log_manager.BeginLoggingWithLog(initial_log, MetricsLogManager::INITIAL_LOG);
+  log_manager.FinishCurrentLog();
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+
+  log_manager.StageNextLogForUpload();
+  EXPECT_TRUE(log_manager.has_staged_log());
+  EXPECT_TRUE(log_manager.has_staged_log_xml());
+  EXPECT_TRUE(log_manager.has_staged_log_proto());
+  EXPECT_FALSE(log_manager.staged_log_text().empty());
+
+  log_manager.DiscardStagedLogProto();
+  EXPECT_TRUE(log_manager.has_staged_log());
+  EXPECT_TRUE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+  EXPECT_FALSE(log_manager.staged_log_text().empty());
+
+  log_manager.DiscardStagedLogXml();
+  EXPECT_FALSE(log_manager.has_staged_log());
+  EXPECT_FALSE(log_manager.has_staged_log_xml());
+  EXPECT_FALSE(log_manager.has_staged_log_proto());
+  EXPECT_TRUE(log_manager.staged_log_text().empty());
+}