Add UMA histograms for data usage tab model

chrome/browser/android/data_usage/data_use_tab_model.cc

Index: chrome/browser/android/data_usage/data_use_tab_model.cc
diff --git a/chrome/browser/android/data_usage/data_use_tab_model.cc b/chrome/browser/android/data_usage/data_use_tab_model.cc
index c4d8e36e39e2716ec47af99ce4fe3453efac5227..1a6e7245e13afa3a4d324b0fa3667a5e24add630 100644
--- a/chrome/browser/android/data_usage/data_use_tab_model.cc
+++ b/chrome/browser/android/data_usage/data_use_tab_model.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/android/data_usage/data_use_tab_model.h"
 
+#include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/data_usage/external_data_use_observer.h"
 #include "chrome/browser/android/data_usage/tab_data_use_entry.h"
@@ -19,6 +20,13 @@ bool IsValidTabID(int32_t tab_id) {
   return tab_id >= 0;
 }
 
+const char kUMAExpiredInactiveTabEntryRemovaltimeSecondsHistogram[] =

[tbansal1, 2015/11/16 17:17:38]

May be remove Seconds from the variable names (if you end up using
UMA_HISTOGRAM_TIMES)?

[Raj, 2015/11/16 20:09:36]

Not sure. Each UMA needs different precision.

+    "DataUse.TabModel.ExpiredInactiveTabEntryRemovaltime";
+const char kUMAExpiredActiveTabEntryRemovaltimeHoursHistogram[] =
+    "DataUse.TabModel.ExpiredActiveTabEntryRemovaltime";
+const char kUMAUnexpiredTabEntryRemovaltimeMinutesHistogram[] =
+    "DataUse.TabModel.UnexpiredTabEntryLifetime";
+
 }  // namespace
 
 namespace chrome {
@@ -40,6 +48,10 @@ DataUseTabModel::~DataUseTabModel() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
+base::TimeTicks DataUseTabModel::Now() const {

[tbansal1, 2015/11/16 17:17:38]

Follow the same order as in .h file. Declare before  GetMaxTabEntriesForTests().

[Raj, 2015/11/16 20:09:36]

Done.

+  return base::TimeTicks::Now();
+}
+
 base::WeakPtr<DataUseTabModel> DataUseTabModel::GetWeakPtr() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return weak_factory_.GetWeakPtr();
@@ -168,10 +180,23 @@ void DataUseTabModel::CompactTabEntries() {
   // Remove expired tab entries.
   for (TabEntryMap::iterator tab_entry_iterator = active_tabs_.begin();
        tab_entry_iterator != active_tabs_.end();) {
-    if (tab_entry_iterator->second.IsExpired())
-      active_tabs_.erase(tab_entry_iterator++);
-    else
-      ++tab_entry_iterator;
+    const auto& tab_entry = tab_entry_iterator->second;
+    if (tab_entry.IsExpired()) {
+      // Track the lifetime of expired tab entry.
+      const base::TimeDelta removal_time =
+          Now() - tab_entry.GetLatestStartOrEndTime();
+      if (!tab_entry.IsTrackingDataUse()) {
+        UMA_HISTOGRAM_COUNTS_1000(

[tbansal1, 2015/11/16 17:17:38]

Why not UMA_HISTOGRAM_TIMES, or its sibling UMA_HISTOGRAM_LONG_TIMES etc.?
Here, and everywhere else in this CL?

[Raj, 2015/11/16 20:09:36]

Tracking expired inactive tab entry removal times in the range 1 second to 16.67
minutes.

UMA_HISTOGRAM_TIMES and its variations track in milliseconds and spend a lot of
buckets in milliseconds.
For tab model millisecond tracking is not needed.
So following examples similar to 'Ash.TouchView.TouchViewInactiveTotal', etc.

[tbansal1, 2015/11/16 21:09:13]

 UMA_HISTOGRAM_CUSTOM_TIMES?

[Raj, 2015/11/17 07:04:03]

Done.

+            kUMAExpiredInactiveTabEntryRemovaltimeSecondsHistogram,
+            base::TimeDelta(removal_time).InSeconds());
+      } else {
+        UMA_HISTOGRAM_CUSTOM_COUNTS(

[tbansal1, 2015/11/16 17:17:38]

Why not UMA_HISTOGRAM_TIMES?

[Raj, 2015/11/16 20:09:36]

Tracking expired active tab entry removal times in the range 1 hour to 5 days.
5 days is the default max removal time - kOpenTabExpirationDurationSeconds. So
this UMA could help tune that parameter via field-trial.

+            kUMAExpiredActiveTabEntryRemovaltimeHoursHistogram,
+            base::TimeDelta(removal_time).InHours(), 1, 120, 50);
+      }
+      active_tabs_.erase(tab_entry_iterator);

[tbansal1, 2015/11/16 17:17:38]

Is this correct? Documentation for erase() says that iterator is invalidated
after element is erased.

I think what you want to do is
if(...isExpired) {
  ....
  tab_entry_iterator = active_tabs_.erase(tab_entry_iterator);
} else {
 ++tab_entry_iterator;
}

[Raj, 2015/11/16 20:09:36]

Done.

+    }
+    ++tab_entry_iterator;
   }
 
   if (active_tabs_.size() <= kMaxTabEntries)
@@ -189,6 +214,11 @@ void DataUseTabModel::CompactTabEntries() {
       }
     }
     DCHECK(oldest_tab_entry_iterator != active_tabs_.end());
+    UMA_HISTOGRAM_COUNTS_1000(

[tbansal1, 2015/11/16 17:17:38]

Why not UMA_HISTOGRAM_TIMES?

[Raj, 2015/11/16 20:09:36]

Tracking unexpired tab entry removal times in the range 1 minute to 16.67 hours.

+        kUMAUnexpiredTabEntryRemovaltimeMinutesHistogram,
+        base::TimeDelta(
+            Now() - oldest_tab_entry_iterator->second.GetLatestStartOrEndTime())
+            .InMinutes());
     active_tabs_.erase(oldest_tab_entry_iterator);
   }
 }