Migrate startup URLs pref.

chrome/browser/prefs/pref_metrics_service.cc

 // Copyright 2013 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 "chrome/browser/prefs/pref_metrics_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/metrics/histogram.h"
@@ skipping 144 matching lines @@
         if (start_url.is_valid()) {
           UMA_HISTOGRAM_ENUMERATION(
               "Settings.StartupPageEngineTypes",
               TemplateURLPrepopulateData::GetEngineType(start_url),
               SEARCH_ENGINE_MAX);
         }
       }
     }
   }
 
+  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",
                               startup_tabs.size(), 1, 50, 20);
   for (size_t i = 0; i < startup_tabs.size(); ++i) {
     GURL start_url(startup_tabs.at(i).url);
     if (start_url.is_valid()) {
       UMA_HISTOGRAM_ENUMERATION(
           "Settings.PinnedTabEngineTypes",
           TemplateURLPrepopulateData::GetEngineType(start_url),
@@ skipping 15 matching lines @@
       &PrefMetricsService::LogBooleanPrefChange, base::Unretained(this));
 
   AddPrefObserver(prefs::kShowHomeButton, "ShowHomeButton", booleanHandler);
   AddPrefObserver(prefs::kHomePageIsNewTabPage, "HomePageIsNewTabPage",
                   booleanHandler);
 
   AddPrefObserver(prefs::kRestoreOnStartup, "StartupPageLoadSettings",
                   base::Bind(&PrefMetricsService::LogIntegerPrefChange,
                              base::Unretained(this),
                              kSessionStartupPrefValueMax));
+
+  AddPrefObserver(prefs::kURLsToRestoreOnStartupOld, "StartupURLsResetTime",
+                  base::Bind(&PrefMetricsService::LogStartupURLsPrefChange,
+                             base::Unretained(this)));
 }
 
 void PrefMetricsService::AddPrefObserver(
     const std::string& path,
     const std::string& histogram_name_prefix,
     const LogHistogramValueCallback& callback) {
   synced_pref_change_registrar_->Add(path.c_str(),
       base::Bind(&PrefMetricsService::OnPrefChanged,
                  base::Unretained(this),
                  histogram_name_prefix, callback));
@@ skipping 31 matching lines @@
     return;
   base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
       histogram_name,
       1,
       boundary_value,
       boundary_value + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   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) {
   device_id_ = device_id;
   // On Aura, this seems to be called twice.
   if (!checked_tracked_prefs_)
     CheckTrackedPreferences();
 }
 
 // To detect changes to Preferences that happen outside of Chrome, we hash
@@ skipping 179 matching lines @@
 }
 
 bool PrefMetricsService::Factory::ServiceIsNULLWhileTesting() const {
   return false;
 }
 
 content::BrowserContext* PrefMetricsService::Factory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextRedirectedInIncognito(context);
 }