Do not log RecordTotalCount metric on first try in OOBE

The RecordTotalCount metric is incorrectly logged as 0 during OOBE
since the networks have not had a chance to merge.

This CL
1. Adds a pref to avoid the incorrect log of 0 networks during
OOBE.
2. Adds a Teardown in the unittests to temporarily fix flaky tests.

Bug: 1135854
Change-Id: I03328caae2753b0331a9c5ed43d7b79de64dac44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519327Reviewed-by: Jon Mann <jonmann@chromium.org>
Commit-Queue: Claude van der Merwe <cvandermerwe@google.com>
Cr-Commit-Position: refs/heads/master@{#825827}

chrome/browser/sync/wifi_configuration_sync_service_factory.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/sync/model_type_store_service_factory.h"
 #include "chrome/common/channel_info.h"
 #include "chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h"
+#include "chromeos/components/sync_wifi/wifi_configuration_bridge.h"
 #include "chromeos/components/sync_wifi/wifi_configuration_sync_service.h"
 #include "chromeos/network/network_handler.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -67,4 +68,5 @@ void WifiConfigurationSyncServiceFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   chromeos::sync_wifi::PendingNetworkConfigurationTrackerImpl::
       RegisterProfilePrefs(registry);
+  chromeos::sync_wifi::WifiConfigurationBridge::RegisterPrefs(registry);
 }

chromeos/components/sync_wifi/wifi_configuration_bridge.cc

@@ -24,6 +24,8 @@
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_metadata_store.h"
 #include "components/device_event_log/device_event_log.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/model/metadata_batch.h"
 #include "components/sync/model/metadata_change_list.h"
@@ -60,6 +62,7 @@ WifiConfigurationBridge::WifiConfigurationBridge(
     NetworkConfigurationHandler* network_configuration_handler,
     SyncedNetworkMetricsLogger* metrics_recorder,
     TimerFactory* timer_factory,
+    PrefService* pref_service,
     std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
     syncer::OnceModelTypeStoreFactory create_store_callback)
     : ModelTypeSyncBridge(std::move(change_processor)),
@@ -68,6 +71,7 @@ WifiConfigurationBridge::WifiConfigurationBridge(
       network_configuration_handler_(network_configuration_handler),
       metrics_recorder_(metrics_recorder),
       timer_factory_(timer_factory),
+      pref_service_(pref_service),
       network_metadata_store_(nullptr) {
   std::move(create_store_callback)
       .Run(syncer::WIFI_CONFIGURATIONS,
@@ -82,6 +86,11 @@ WifiConfigurationBridge::~WifiConfigurationBridge() {
   OnShuttingDown();
 }
 
+// static
+void WifiConfigurationBridge::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(kIsFirstRun, true);
+}
+
 void WifiConfigurationBridge::OnShuttingDown() {
   if (network_metadata_store_) {
     network_metadata_store_->RemoveObserver(this);
@@ -302,11 +311,22 @@ void WifiConfigurationBridge::OnReadAllData(
     }
     entries_[record.id] = std::move(data);
   }
-
-  metrics_recorder_->RecordTotalCount(entries_.size());
   store_->ReadAllMetadata(
       base::BindOnce(&WifiConfigurationBridge::OnReadAllMetadata,
                      weak_ptr_factory_.GetWeakPtr()));
+
+  int entries_size = entries_.size();
+  // Do not log the total network count during OOBE. It returns 0 even if there
+  // are networks synced since MergeSyncData has not executed yet.
+  if (pref_service_->GetBoolean(kIsFirstRun)) {
+    pref_service_->SetBoolean(kIsFirstRun, false);
+    // This is only meant to filter out 0's that are logged during OOBE. If the
+    // entries_size is greater than zero it should be logged.
+    if (entries_size == 0) {
+      return;
+    }
+  }
+  metrics_recorder_->RecordTotalCount(entries_size);
 }
 
 void WifiConfigurationBridge::OnReadAllMetadata(

chromeos/components/sync_wifi/wifi_configuration_bridge.h

@@ -22,6 +22,9 @@
 #include "components/sync/model/model_type_store.h"
 #include "components/sync/model/model_type_sync_bridge.h"
 
+class PrefRegistrySimple;
+class PrefService;
+
 namespace syncer {
 class ModelTypeChangeProcessor;
 }  // namespace syncer
@@ -33,6 +36,8 @@ class NetworkMetadataStore;
 
 namespace sync_wifi {
 
+const char kIsFirstRun[] = "sync_wifi.is_first_run";
+
 class LocalNetworkCollector;
 class SyncedNetworkMetricsLogger;
 class SyncedNetworkUpdater;
@@ -50,10 +55,13 @@ class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge,
       NetworkConfigurationHandler* network_configuration_handler,
       SyncedNetworkMetricsLogger* metrics_recorder,
       TimerFactory* timer_factory,
+      PrefService* pref_service,
       std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
       syncer::OnceModelTypeStoreFactory create_store_callback);
   ~WifiConfigurationBridge() override;
 
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
   // syncer::ModelTypeSyncBridge:
   std::unique_ptr<syncer::MetadataChangeList> CreateMetadataChangeList()
       override;
@@ -141,6 +149,7 @@ class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge,
   NetworkConfigurationHandler* network_configuration_handler_;
   SyncedNetworkMetricsLogger* metrics_recorder_;
   TimerFactory* timer_factory_;
+  PrefService* pref_service_;
   base::WeakPtr<NetworkMetadataStore> network_metadata_store_;
 
   base::WeakPtrFactory<WifiConfigurationBridge> weak_ptr_factory_{this};

chromeos/components/sync_wifi/wifi_configuration_bridge_unittest.cc

@@ -61,6 +61,7 @@ const char kSsidWoof[] = "woof";
 const char kSsidHonk[] = "honk";
 const char kSyncPsk[] = "sync_psk";
 const char kLocalPsk[] = "local_psk";
+const char kIsFirstRun[] = "sync_wifi.is_first_run";
 
 syncer::EntityData GenerateWifiEntityData(
     const sync_pb::WifiConfigurationSpecifics& data) {
@@ -157,6 +158,9 @@ class WifiConfigurationBridgeTest : public testing::Test {
     device_prefs_ = std::make_unique<TestingPrefServiceSimple>();
     NetworkMetadataStore::RegisterPrefs(user_prefs_->registry());
     NetworkMetadataStore::RegisterPrefs(device_prefs_->registry());
+
+    user_prefs_->registry()->RegisterBooleanPref(kIsFirstRun, true);
+
     network_metadata_store_ = std::make_unique<NetworkMetadataStore>(
         /*network_configuration_handler=*/nullptr,
         /*network_connection_handler=*/nullptr,
@@ -164,13 +168,23 @@ class WifiConfigurationBridgeTest : public testing::Test {
         user_prefs_.get(), device_prefs_.get(),
         /*is_enterprise_enrolled=*/false);
 
+    base::HistogramTester histogram_tester;
     bridge_ = std::make_unique<WifiConfigurationBridge>(
         synced_network_updater(), local_network_collector(),
         /*network_configuration_handler=*/nullptr, metrics_logger_.get(),
-        timer_factory_.get(), mock_processor_.CreateForwardingProcessor(),
+        timer_factory_.get(), user_prefs_.get(),
+        mock_processor_.CreateForwardingProcessor(),
         syncer::ModelTypeStoreTestUtil::MoveStoreToFactory(std::move(store_)));
     bridge_->SetNetworkMetadataStore(network_metadata_store_->GetWeakPtr());
     base::RunLoop().RunUntilIdle();
+
+    // Assert that an incorrect metric was not logged.
+    histogram_tester.ExpectTotalCount(kTotalCountHistogram, 0);
+  }
+
+  void TearDown() override {
+    // TODO(cvandermerwe) Put the shutdown logic into network_test_helper.
+    NetworkHandler::Shutdown();
   }
 
   void DisableBridge() {

chromeos/components/sync_wifi/wifi_configuration_sync_service.cc

@@ -46,7 +46,7 @@ WifiConfigurationSyncService::WifiConfigurationSyncService(
   bridge_ = std::make_unique<sync_wifi::WifiConfigurationBridge>(
       updater_.get(), collector_.get(),
       network_handler->network_configuration_handler(), metrics_logger_.get(),
-      timer_factory_.get(),
+      timer_factory_.get(), pref_service,
       std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
           syncer::WIFI_CONFIGURATIONS,
           base::BindRepeating(&syncer::ReportUnrecoverableError, channel)),