Migrate startup URLs pref.

chrome/browser/prefs/pref_metrics_service.cc

Index: chrome/browser/prefs/pref_metrics_service.cc
diff --git a/chrome/browser/prefs/pref_metrics_service.cc b/chrome/browser/prefs/pref_metrics_service.cc
index 2ee5e1ae2b49ceb63710c778462df3e242ac14dd..bd268dfaf6054ad6b78be932ba0b3b1d48ee1fca 100644
--- a/chrome/browser/prefs/pref_metrics_service.cc
+++ b/chrome/browser/prefs/pref_metrics_service.cc
@@ -162,6 +162,10 @@ void PrefMetricsService::RecordLaunchPrefs() {
     }
   }
 
+  const ListValue* url_list = prefs_->GetList(
+      prefs::kURLsToRestoreOnStartupOld);
+  LogStartupURLsPrefChange("StartupURLsResetTime", url_list);

[grt (UTC plus 2), 2013/09/27 13:24:45]

to match when Log..() is called by the ChangeRegistrar, use
"Settings.StartupURLsResetTime.PushedToSync". alternatively, call OnPrefChanged
to simulate a true pref change.

[MAD, 2013/09/27 14:54:20]

I don't think I need that since this is not a sync'able pref.

+
 #if !defined(OS_ANDROID)
   StartupTabs startup_tabs = PinnedTabCodec::ReadPinnedTabs(profile_);
   UMA_HISTOGRAM_CUSTOM_COUNTS("Settings.PinnedTabs",
@@ -197,6 +201,10 @@ void PrefMetricsService::RegisterSyncedPrefObservers() {
                   base::Bind(&PrefMetricsService::LogIntegerPrefChange,
                              base::Unretained(this),
                              kSessionStartupPrefValueMax));
+
+  AddPrefObserver(prefs::kURLsToRestoreOnStartupOld, "StartupURLsResetTime",
+                  base::Bind(&PrefMetricsService::LogStartupURLsPrefChange,
+                             base::Unretained(this)));
 }
 
 void PrefMetricsService::AddPrefObserver(
@@ -248,18 +256,27 @@ void PrefMetricsService::LogIntegerPrefChange(int boundary_value,
   histogram->Add(integer_value);
 }
 
-void PrefMetricsService::LogListPrefChange(
-    const LogHistogramValueCallback& item_callback,
+void PrefMetricsService::LogStartupURLsPrefChange(
     const std::string& histogram_name,

[grt (UTC plus 2), 2013/09/27 13:24:45]

why not use this on line 273 (it should be
"Settings.StartupURLsResetTime.PushedToSync" when invoked by the OnPrefChanged)?

[MAD, 2013/09/27 14:54:20]

Because this is not a sync'able pref.

[grt (UTC plus 2), 2013/09/27 15:50:21]

This code is confusing as-is. There are two ways to get into this function: from
PrefMetricsService::OnPrefChanged by way of the SyncedPrefChangeRegistrar, and
from the direct call on line 167. In the former case, histogram_name is
"Settings.StartupURLsResetTime.PushedToSync". In the latter case, histogram_name
is "StartupURLsResetTime". In both cases it is ignored. Please change something
here. I think things are more obvious if the histogram name is consistent with
the other histograms, and if argument values aren't different and ignored.

[MAD, 2013/09/27 17:35:39]

OK, you convinced me, I'll work something up...

     const Value* value) {
   const ListValue* items = NULL;
-  if (!value->GetAsList(&items))
+  if (!value->GetAsList(&items) || !items->GetSize())
     return;
-  for (size_t i = 0; i < items->GetSize(); ++i) {
-    const Value *item_value = NULL;
-    if (items->Get(i, &item_value))
-      item_callback.Run(histogram_name, item_value);
+
+  // Empty migration time delta will identify initial migration.

[grt (UTC plus 2), 2013/09/27 13:24:45]

do you want to log initial migration?

[MAD, 2013/09/27 14:54:20]

It is logged with a value of 0.0.

[grt (UTC plus 2), 2013/09/27 15:50:21]

If I understand the use of histograms here, there are only 50 buckets for a
range from 60 to 604800 seconds, so the 0.0 value may be in the same bucket as a
fairly large reset-delta range. Will we be able to extract meaning from this
first bucket?

[MAD, 2013/09/27 17:35:39]

D'Ho! Of course... I didn't realize that because all my tests needed enough time
to reach the next bucket...

Thanks!

+  base::TimeDelta migration_time;
+  base::Time now(base::Time::Now());
+  if (prefs_->GetUserPrefValue(prefs::kRestoreStartupURLsMigrated)) {
+    migration_time = now - base::Time::FromDoubleT(
+        prefs_->GetDouble(prefs::kRestoreStartupURLsMigrated));
   }
+  HISTOGRAM_CUSTOM_TIMES("Settings.StartupURLsResetTime",
+                         migration_time,
+                         base::TimeDelta::FromMinutes(1),
+                         base::TimeDelta::FromDays(7),
+                         50);
+  prefs_->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue());
+  prefs_->SetDouble(prefs::kRestoreStartupURLsMigrated, now.ToDoubleT());
 }
 
 void PrefMetricsService::GetDeviceIdCallback(const std::string& device_id) {