This is the separate change mentioned in comments of https://codereview.chromium.org/246553003/.

It emits NCN.NetworkOperatorMCCMNC_NewMetricsLog on new metric log creation with the network operator MCC_MNC code 

BUG=355604

Review URL: https://codereview.chromium.org/253203002

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

chrome/browser/chrome_browser_metrics_service_observer.cc

+// Copyright 2014 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/chrome_browser_metrics_service_observer.h"
+
+#include "chrome/browser/metrics/metrics_service.h"
+#include "net/base/network_change_notifier.h"
+
+ChromeBrowserMetricsServiceObserver::ChromeBrowserMetricsServiceObserver() {
+  MetricsServiceHelper::AddMetricsServiceObserver(this);
+}
+
+ChromeBrowserMetricsServiceObserver::~ChromeBrowserMetricsServiceObserver() {
+  MetricsServiceHelper::RemoveMetricsServiceObserver(this);
+}
+
+void ChromeBrowserMetricsServiceObserver::OnDidCreateMetricsLog() {
+  net::NetworkChangeNotifier::LogOperatorCodeHistogram(
+      net::NetworkChangeNotifier::GetConnectionType());
+}

chrome/browser/chrome_browser_metrics_service_observer.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_CHROME_BROWSER_METRICS_SERVICE_OBSERVER_H_
+#define CHROME_BROWSER_CHROME_BROWSER_METRICS_SERVICE_OBSERVER_H_
+
+#include "chrome/browser/metrics/metrics_service_observer.h"
+
+// ChromeBrowserMetricsServiceObserver receives notifications when the metrics
+// service creates a new metrics log.
+class ChromeBrowserMetricsServiceObserver : public MetricsServiceObserver {
+ public:
+  ChromeBrowserMetricsServiceObserver();
+  virtual ~ChromeBrowserMetricsServiceObserver();
+
+  // MetricsServiceObserver:
+  virtual void OnDidCreateMetricsLog() OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMetricsServiceObserver);
+};
+
+#endif  // CHROME_BROWSER_CHROME_BROWSER_METRICS_SERVICE_OBSERVER_H_

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main.h"
+#include "chrome/browser/chrome_browser_metrics_service_observer.h"
 #include "chrome/browser/pref_service_flags_storage.h"
 #include "chrome/browser/shell_integration.h"
 #include "content/public/browser/browser_thread.h"
@@ -154,6 +155,12 @@ void ChromeBrowserMainExtraPartsMetrics::PostBrowserStart() {
       FROM_HERE,
       base::Bind(&RecordStartupMetricsOnBlockingPool),
       base::TimeDelta::FromSeconds(kStartupMetricsGatheringDelaySeconds));
+
+  // Create the metrics log observer.
+  // We only need this for Android for now.
+#if defined(ANDROID)
+  metrics_service_observer_.reset(new ChromeBrowserMetricsServiceObserver());
+#endif
 }
 
 namespace chrome {

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h

@@ -7,9 +7,11 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
 
 class ChromeBrowserMainParts;
+class ChromeBrowserMetricsServiceObserver;
 
 namespace chrome {
 void AddMetricsExtraParts(ChromeBrowserMainParts* main_parts);
@@ -26,6 +28,9 @@ class ChromeBrowserMainExtraPartsMetrics : public ChromeBrowserMainExtraParts {
   virtual void PostBrowserStart() OVERRIDE;
 
  private:
+  // Observe and log histograms on new metric logs.
+  scoped_ptr<ChromeBrowserMetricsServiceObserver> metrics_service_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsMetrics);
 };
 

chrome/browser/metrics/metrics_service.cc

@@ -1040,6 +1040,7 @@ void MetricsService::ReceivedProfilerData(
     initial_metrics_log_.reset(
         new MetricsLog(state_manager_->client_id(), session_id_,
                        MetricsLog::ONGOING_LOG));
+    NotifyOnDidCreateMetricsLog();
   }
 
   initial_metrics_log_->RecordProfilerData(process_data, process_type);
@@ -1073,6 +1074,22 @@ void MetricsService::GetUptimes(PrefService* pref,
   }
 }
 
+void MetricsService::AddObserver(MetricsServiceObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  observers_.AddObserver(observer);
+}
+
+void MetricsService::RemoveObserver(MetricsServiceObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  observers_.RemoveObserver(observer);
+}
+
+void MetricsService::NotifyOnDidCreateMetricsLog() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  FOR_EACH_OBSERVER(
+      MetricsServiceObserver, observers_, OnDidCreateMetricsLog());
+}
+
 //------------------------------------------------------------------------------
 // State save methods
 
@@ -1108,6 +1125,7 @@ void MetricsService::OpenNewLog() {
   log_manager_.BeginLoggingWithLog(
       new MetricsLog(state_manager_->client_id(), session_id_,
                      MetricsLog::ONGOING_LOG));
+  NotifyOnDidCreateMetricsLog();
   if (state_ == INITIALIZED) {
     // We only need to schedule that run once.
     state_ = INIT_TASK_SCHEDULED;
@@ -1405,6 +1423,10 @@ void MetricsService::PrepareInitialStabilityLog() {
   scoped_ptr<MetricsLog> initial_stability_log(
       new MetricsLog(state_manager_->client_id(), session_id_,
                      MetricsLog::INITIAL_STABILITY_LOG));
+
+  // Do not call NotifyOnDidCreateMetricsLog here because the stability
+  // log describes stats from the _previous_ session.
+
   if (!initial_stability_log->LoadSavedEnvironmentFromPrefs())
     return;
   initial_stability_log->RecordStabilityMetrics(base::TimeDelta(),
@@ -1934,3 +1956,17 @@ bool MetricsServiceHelper::IsCrashReportingEnabled() {
   return false;
 #endif
 }
+
+void MetricsServiceHelper::AddMetricsServiceObserver(
+    MetricsServiceObserver* observer) {
+  MetricsService* metrics_service = g_browser_process->metrics_service();
+  if (metrics_service)
+    metrics_service->AddObserver(observer);
+}
+
+void MetricsServiceHelper::RemoveMetricsServiceObserver(
+    MetricsServiceObserver* observer) {
+  MetricsService* metrics_service = g_browser_process->metrics_service();
+  if (metrics_service)
+    metrics_service->RemoveObserver(observer);
+}

chrome/browser/metrics/metrics_service.h

@@ -18,9 +18,12 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/user_metrics.h"
+#include "base/observer_list.h"
 #include "base/process/kill.h"
+#include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/metrics/metrics_log.h"
+#include "chrome/browser/metrics/metrics_service_observer.h"
 #include "chrome/browser/metrics/tracking_synchronizer_observer.h"
 #include "chrome/common/metrics/metrics_service_base.h"
 #include "chrome/installer/util/google_update_settings.h"
@@ -36,6 +39,7 @@
 #include "chrome/browser/chromeos/external_metrics.h"
 #endif
 
+class ChromeBrowserMetricsServiceObserver;
 class MetricsReportingScheduler;
 class PrefService;
 class PrefRegistrySimple;
@@ -364,6 +368,11 @@ class MetricsService
   // Set up client ID, session ID, etc.
   void InitializeMetricsState();
 
+  // Registers/unregisters |observer| to receive MetricsLog notifications.
+  void AddObserver(MetricsServiceObserver* observer);
+  void RemoveObserver(MetricsServiceObserver* observer);
+  void NotifyOnDidCreateMetricsLog();
+
   // Schedule the next save of LocalState information.  This is called
   // automatically by the task that performs each save to schedule the next one.
   void ScheduleNextStateSave();
@@ -585,7 +594,15 @@ class MetricsService
   // Field trial groups that map to Chrome configuration states.
   SyntheticTrialGroups synthetic_trial_groups_;
 
+  ObserverList<MetricsServiceObserver> observers_;
+
+  // Confirms single-threaded access to |observers_| in debug builds.
+  base::ThreadChecker thread_checker_;
+
+  friend class MetricsServiceHelper;
+
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess);
+  FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, MetricsServiceObserver);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest,
                            PermutedEntropyCacheClearedWhenLowEntropyReset);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, RegisterSyntheticTrial);
@@ -593,12 +610,13 @@ class MetricsService
   DISALLOW_COPY_AND_ASSIGN(MetricsService);
 };
 
-// This class limits and documents access to the IsMetricsReportingEnabled() and
-// IsCrashReportingEnabled() methods. Since these methods are private, each user
-// has to be explicitly declared as a 'friend' below.
+// This class limits and documents access to metrics service helper methods.
+// Since these methods are private, each user has to be explicitly declared
+// as a 'friend' below.
 class MetricsServiceHelper {
  private:
   friend bool prerender::IsOmniboxEnabled(Profile* profile);
+  friend class ::ChromeBrowserMetricsServiceObserver;
   friend class ChromeRenderMessageFilter;
   friend class ::CrashesDOMHandler;
   friend class extensions::ExtensionDownloader;
@@ -620,6 +638,11 @@ class MetricsServiceHelper {
   // IsMetricsReportingEnabled for desktop Chrome.
   static bool IsCrashReportingEnabled();
 
+  // Registers/unregisters |observer| to receive MetricsLog notifications
+  // from metrics service.
+  static void AddMetricsServiceObserver(MetricsServiceObserver* observer);
+  static void RemoveMetricsServiceObserver(MetricsServiceObserver* observer);
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(MetricsServiceHelper);
 };
 

chrome/browser/metrics/metrics_service_observer.cc

+// Copyright 2014 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/chrome_browser_metrics_service_observer.h"
+
+MetricsServiceObserver::MetricsServiceObserver() {
+}
+
+MetricsServiceObserver::~MetricsServiceObserver() {
+}

chrome/browser/metrics/metrics_service_observer.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_METRICS_METRICS_SERVICE_OBSERVER_H_
+#define CHROME_BROWSER_METRICS_METRICS_SERVICE_OBSERVER_H_
+
+#include "base/macros.h"
+
+// MetricsServiceObserver receives notifications from MetricsService.
+// An observer must be added, removed, and notified on the same thread.
+class MetricsServiceObserver {
+ public:
+  // Called when a new MetricsLog is created.
+  virtual void OnDidCreateMetricsLog() = 0;
+
+ protected:
+  MetricsServiceObserver();
+  virtual ~MetricsServiceObserver();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MetricsServiceObserver);
+};
+
+#endif  // CHROME_BROWSER_METRICS_METRICS_SERVICE_OBSERVER_H_

chrome/browser/metrics/metrics_service_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/command_line.h"
 #include "base/threading/platform_thread.h"
+#include "chrome/browser/metrics/metrics_service_observer.h"
 #include "chrome/browser/metrics/metrics_state_manager.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
@@ -145,6 +146,22 @@ class MetricsServiceTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(MetricsServiceTest);
 };
 
+class TestMetricsServiceObserver : public MetricsServiceObserver {
+ public:
+  TestMetricsServiceObserver(): observed_(0) {}
+  virtual ~TestMetricsServiceObserver() {}
+
+  virtual void OnDidCreateMetricsLog() OVERRIDE {
+    ++observed_;
+  }
+  int observed() const { return observed_; }
+
+ private:
+  int observed_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMetricsServiceObserver);
+};
+
 }  // namespace
 
 TEST_F(MetricsServiceTest, IsPluginProcess) {
@@ -313,3 +330,34 @@ TEST_F(MetricsServiceTest, CrashReportingEnabled) {
   EXPECT_FALSE(MetricsServiceHelper::IsCrashReportingEnabled());
 #endif  // defined(GOOGLE_CHROME_BUILD)
 }
+
+TEST_F(MetricsServiceTest, MetricsServiceObserver) {
+  MetricsService service(GetMetricsStateManager());
+  TestMetricsServiceObserver observer1;
+  TestMetricsServiceObserver observer2;
+
+  service.AddObserver(&observer1);
+  EXPECT_EQ(0, observer1.observed());
+  EXPECT_EQ(0, observer2.observed());
+
+  service.OpenNewLog();
+  EXPECT_EQ(1, observer1.observed());
+  EXPECT_EQ(0, observer2.observed());
+  service.log_manager_.FinishCurrentLog();
+
+  service.AddObserver(&observer2);
+
+  service.OpenNewLog();
+  EXPECT_EQ(2, observer1.observed());
+  EXPECT_EQ(1, observer2.observed());
+  service.log_manager_.FinishCurrentLog();
+
+  service.RemoveObserver(&observer1);
+
+  service.OpenNewLog();
+  EXPECT_EQ(2, observer1.observed());
+  EXPECT_EQ(2, observer2.observed());
+  service.log_manager_.FinishCurrentLog();
+
+  service.RemoveObserver(&observer2);
+}

chrome/chrome_browser.gypi

@@ -402,6 +402,8 @@
         'browser/chrome_browser_main_posix.h',
         'browser/chrome_browser_main_win.cc',
         'browser/chrome_browser_main_win.h',
+        'browser/chrome_browser_metrics_service_observer.cc',
+        'browser/chrome_browser_metrics_service_observer.h',
         'browser/chrome_content_browser_client.cc',
         'browser/chrome_content_browser_client.h',
         'browser/chrome_elf_init_win.cc',
@@ -1196,6 +1198,8 @@
         'browser/metrics/metrics_reporting_scheduler.cc',
         'browser/metrics/metrics_reporting_scheduler.h',
         'browser/metrics/metrics_service_android.cc',
+        'browser/metrics/metrics_service_observer.cc',
+        'browser/metrics/metrics_service_observer.h',
         'browser/metrics/metrics_service.cc',
         'browser/metrics/metrics_service.h',
         'browser/metrics/metrics_services_manager.cc',

net/base/network_change_notifier.cc

@@ -235,22 +235,7 @@ class HistogramWatcher
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OfflineChange", state_duration);
     }
 
-#if defined(OS_ANDROID)
-    // On a connection type change to 2/3/4G, log the network operator MCC/MNC.
-    // Log zero in other cases.
-    unsigned mcc_mnc = 0;
-    if (type == NetworkChangeNotifier::CONNECTION_2G ||
-        type == NetworkChangeNotifier::CONNECTION_3G ||
-        type == NetworkChangeNotifier::CONNECTION_4G) {
-      // Log zero if not perfectly converted.
-      if (!base::StringToUint(
-          net::android::GetTelephonyNetworkOperator(), &mcc_mnc)) {
-        mcc_mnc = 0;
-      }
-    }
-    UMA_HISTOGRAM_SPARSE_SLOWLY(
-        "NCN.NetworkOperatorMCCMNC_ConnectionChange", mcc_mnc);
-#endif
+    NetworkChangeNotifier::LogOperatorCodeHistogram(type);
 
     UMA_HISTOGRAM_MEDIUM_TIMES(
         "NCN.IPAddressChangeToConnectionTypeChange",
@@ -594,6 +579,25 @@ void NetworkChangeNotifier::ShutdownHistogramWatcher() {
   g_network_change_notifier->histogram_watcher_.reset();
 }
 
+// static
+void NetworkChangeNotifier::LogOperatorCodeHistogram(ConnectionType type) {
+#if defined(OS_ANDROID)
+  // On a connection type change to 2/3/4G, log the network operator MCC/MNC.
+  // Log zero in other cases.
+  unsigned mcc_mnc = 0;
+  if (type == NetworkChangeNotifier::CONNECTION_2G ||
+      type == NetworkChangeNotifier::CONNECTION_3G ||
+      type == NetworkChangeNotifier::CONNECTION_4G) {
+    // Log zero if not perfectly converted.
+    if (!base::StringToUint(
+        net::android::GetTelephonyNetworkOperator(), &mcc_mnc)) {
+      mcc_mnc = 0;
+    }
+  }
+  UMA_HISTOGRAM_SPARSE_SLOWLY("NCN.NetworkOperatorMCCMNC", mcc_mnc);
+#endif
+}
+
 #if defined(OS_LINUX)
 // static
 const internal::AddressTrackerLinux*

net/base/network_change_notifier.h

@@ -232,6 +232,9 @@ class NET_EXPORT NetworkChangeNotifier {
   // should be called from the network thread to avoid race conditions.
   static void ShutdownHistogramWatcher();
 
+  // Log the |NCN.NetworkOperatorMCCMNC| histogram.
+  static void LogOperatorCodeHistogram(ConnectionType type);
+
   // Allows a second NetworkChangeNotifier to be created for unit testing, so
   // the test suite can create a MockNetworkChangeNotifier, but platform
   // specific NetworkChangeNotifiers can also be created for testing.  To use,

tools/metrics/histograms/histograms.xml

@@ -10772,15 +10772,15 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <summary>The scheme of the URL for each main-frame navigation.</summary>
 </histogram>
 
-<histogram name="NCN.NetworkOperatorMCCMNC_ConnectionChange">
+<histogram name="NCN.NetworkOperatorMCCMNC">
   <owner>bolian@chromium.org</owner>
   <owner>bengr@google.com</owner>
   <owner>marq@google.com</owner>
   <summary>
     The MCC (mobile country code) and MNC (mobile network code) of the network
-    operator when the network connection is changed to a mobile network. Value
-    zero means the connection is changed to a non-mobile network or the operator
-    code is unknown.
+    operator when a new metrics log is created or when the network connection is
+    changed. A value of zero means a non-mobile network or the operator code is
+    unknown.
   </summary>
 </histogram>