[Sync] Record FieldTrial user events.

FieldTrials should be recorded through sync when an event logger client
registers a dependency, their dependent event is recorded, and the
trial is finalized. All trials are recorded whenever the FieldTrial
event is emitted, which should be on a change or period, but no more.
Feature [and params] exist but may never actually change.

Because FieldTrials (and other imminent types) are not tied to
navigations, reworked the ShouldRecordEvent() check to be more
permissive, but also enforce presence (or lack) of navigation id.

Bug: 719038
Change-Id: Ib62a18949dde41d194935347768b3fef0d2623d6
Reviewed-on: https://chromium-review.googlesource.com/764408
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521052}

chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc

@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <stdint.h>
+
 #include "base/macros.h"
-#include "base/time/time.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
@@ -13,9 +14,8 @@
 #include "chrome/browser/sync/user_event_service_factory.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync/user_events/user_event_service.h"
+#include "components/variations/variations_associated_data.h"
 
-using base::Time;
-using base::TimeDelta;
 using fake_server::FakeServer;
 using sync_pb::UserEventSpecifics;
 using sync_pb::SyncEntity;
@@ -23,10 +23,10 @@ using sync_pb::CommitResponse;
 
 namespace {
 
-UserEventSpecifics CreateEvent(int minutes_ago) {
+UserEventSpecifics CreateEvent(int microseconds) {
   UserEventSpecifics specifics;
-  specifics.set_event_time_usec(
-      (Time::Now() - TimeDelta::FromMinutes(minutes_ago)).ToInternalValue());
+  specifics.set_event_time_usec(microseconds);
+  specifics.set_navigation_id(microseconds);
   specifics.mutable_test_event();
   return specifics;
 }
@@ -76,24 +76,22 @@ class UserEventEqualityChecker : public SingleClientStatusChangeChecker {
       return false;
     }
 
-    // Because we have a multimap, we cannot just check counts and equality of
-    // items, but we need to make sure 2 of A and 1 of B is not the same as 1 of
-    // A and 2 of B. So to make this easy, copy the multimap and remove items.
-    std::multimap<int64_t, UserEventSpecifics> copied_expected_(
-        expected_specifics_.begin(), expected_specifics_.end());
+    // Number of events on server matches expected, exit condition is satisfied.
+    // Let's verify that content matches as well. It is safe to modify
+    // |expected_specifics_|.
     for (const SyncEntity& entity : entities) {
       UserEventSpecifics server_specifics = entity.specifics().user_event();
-      auto iter = copied_expected_.find(server_specifics.event_time_usec());
+      auto iter = expected_specifics_.find(server_specifics.event_time_usec());
       // We don't expect to encounter id matching events with different values,
       // this isn't going to recover so fail the test case now.
-      EXPECT_TRUE(copied_expected_.end() != iter);
+      EXPECT_TRUE(expected_specifics_.end() != iter);
       // TODO(skym): This may need to change if we start updating navigation_id
       // based on what sessions data is committed, and end up committing the
       // same event multiple times.
       EXPECT_EQ(iter->second.navigation_id(), server_specifics.navigation_id());
       EXPECT_EQ(iter->second.event_case(), server_specifics.event_case());
 
-      copied_expected_.erase(iter);
+      expected_specifics_.erase(iter);
     }
 
     return true;
@@ -108,6 +106,57 @@ class UserEventEqualityChecker : public SingleClientStatusChangeChecker {
   std::multimap<int64_t, UserEventSpecifics> expected_specifics_;
 };
 
+// A more simplistic version of UserEventEqualityChecker that only checks the
+// case of the events. This is helpful if you do not know (or control) some of
+// the fields of the events that are created.
+class UserEventCaseChecker : public SingleClientStatusChangeChecker {
+ public:
+  UserEventCaseChecker(
+      browser_sync::ProfileSyncService* service,
+      FakeServer* fake_server,
+      std::multiset<UserEventSpecifics::EventCase> expected_cases)
+      : SingleClientStatusChangeChecker(service),
+        fake_server_(fake_server),
+        expected_cases_(expected_cases) {}
+
+  bool IsExitConditionSatisfied() override {
+    std::vector<SyncEntity> entities =
+        fake_server_->GetSyncEntitiesByModelType(syncer::USER_EVENTS);
+
+    // |entities.size()| is only going to grow, if |entities.size()| ever
+    // becomes bigger then all hope is lost of passing, stop now.
+    EXPECT_GE(expected_cases_.size(), entities.size());
+
+    if (expected_cases_.size() > entities.size()) {
+      return false;
+    }
+
+    // Number of events on server matches expected, exit condition is satisfied.
+    // Let's verify that content matches as well. It is safe to modify
+    // |expected_specifics_|.
+    for (const SyncEntity& entity : entities) {
+      UserEventSpecifics::EventCase actual =
+          entity.specifics().user_event().event_case();
+      auto iter = expected_cases_.find(actual);
+      if (iter != expected_cases_.end()) {
+        expected_cases_.erase(iter);
+      } else {
+        ADD_FAILURE() << actual;
+      }
+    }
+
+    return true;
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting server side USER_EVENTS to match expected.";
+  }
+
+ private:
+  FakeServer* fake_server_;
+  std::multiset<UserEventSpecifics::EventCase> expected_cases_;
+};
+
 class SingleClientUserEventsSyncTest : public SyncTest {
  public:
   SingleClientUserEventsSyncTest() : SyncTest(SINGLE_CLIENT) {
@@ -225,7 +274,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoSessions) {
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, Encryption) {
   const UserEventSpecifics specifics1 = CreateEvent(1);
   const UserEventSpecifics specifics2 = CreateEvent(2);
-  const GURL url("http://www.one.com/");
 
   ASSERT_TRUE(SetupSync());
   syncer::UserEventService* event_service =
@@ -241,10 +289,25 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, Encryption) {
   // something else through the system that we can wait on before checking.
   // Tab/SESSIONS data was picked fairly arbitrarily, note that we expect 2
   // entries, one for the window/header and one for the tab.
-  sessions_helper::OpenTab(0, url);
+  sessions_helper::OpenTab(0, GURL("http://www.one.com/"));
   ServerCountMatchStatusChecker(syncer::SESSIONS, 2);
   UserEventEqualityChecker(GetSyncService(0), GetFakeServer(), {specifics1})
       .Wait();
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, FieldTrial) {
+  const std::string trial_name = "TrialName";
+  const std::string group_name = "GroupName";
+
+  ASSERT_TRUE(SetupSync());
+  variations::AssociateGoogleVariationID(variations::CHROME_SYNC_SERVICE,
+                                         trial_name, group_name, 123);
+  base::FieldTrialList::CreateFieldTrial(trial_name, group_name);
+  base::FieldTrialList::FindFullName(trial_name);
+
+  UserEventCaseChecker(GetSyncService(0), GetFakeServer(),
+                       {UserEventSpecifics::kFieldTrialEvent})
+      .Wait();
+}
+
 }  // namespace

components/sync/BUILD.gn

@@ -570,6 +570,8 @@ static_library("sync") {
     "user_events/fake_user_event_service.h",
     "user_events/no_op_user_event_service.cc",
     "user_events/no_op_user_event_service.h",
+    "user_events/trial_recorder.cc",
+    "user_events/trial_recorder.h",
     "user_events/user_event_service.cc",
     "user_events/user_event_service.h",
     "user_events/user_event_service_impl.cc",
@@ -600,6 +602,7 @@ static_library("sync") {
     "//components/signin/core/browser",
     "//components/strings",
     "//components/sync/engine_impl/attachments/proto",
+    "//components/variations",
     "//components/version_info",
     "//crypto",
     "//google_apis",
@@ -946,6 +949,7 @@ source_set("unit_tests") {
     "syncable/syncable_enum_conversions_unittest.cc",
     "syncable/syncable_id_unittest.cc",
     "syncable/syncable_unittest.cc",
+    "user_events/trial_recorder_unittest.cc",
     "user_events/user_event_service_impl_unittest.cc",
     "user_events/user_event_sync_bridge_unittest.cc",
   ]
@@ -976,6 +980,7 @@ source_set("unit_tests") {
     "//components/sync/engine_impl/attachments/proto",
     "//components/sync_preferences",
     "//components/sync_preferences:test_support",
+    "//components/variations",
     "//components/version_info",
     "//components/version_info:version_string",
     "//google_apis",

components/sync/driver/sync_driver_switches.cc

@@ -41,6 +41,10 @@ const base::Feature kSyncClearDataOnPassphraseEncryption{
 const base::Feature kSyncUserEvents{"SyncUserEvents",
                                     base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Gates emission of FieldTrial events.
+const base::Feature kSyncUserFieldTrialEvents{"SyncUserFieldTrialEvents",
+                                              base::FEATURE_ENABLED_BY_DEFAULT};
+
 // Gates registration for user language detection events.
 const base::Feature kSyncUserLanguageDetectionEvents{
     "SyncUserLanguageDetectionEvents", base::FEATURE_DISABLED_BY_DEFAULT};

components/sync/driver/sync_driver_switches.h

@@ -21,6 +21,7 @@ extern const char kSyncShortNudgeDelayForTest[];
 
 extern const base::Feature kSyncClearDataOnPassphraseEncryption;
 extern const base::Feature kSyncUserEvents;
+extern const base::Feature kSyncUserFieldTrialEvents;
 extern const base::Feature kSyncUserLanguageDetectionEvents;
 extern const base::Feature kSyncUserTranslationEvents;
 extern const base::Feature kSyncUSSAutocomplete;

components/sync/protocol/proto_visitors.h

@@ -475,14 +475,8 @@ VISIT_PROTO_FIELDS(const sync_pb::FaviconTrackingSpecifics& proto) {
   VISIT(is_bookmarked);
 }
 
-VISIT_PROTO_FIELDS(
-    const sync_pb::UserEventSpecifics::FieldTrial::FieldTrialPair& proto) {
-  VISIT(name_id);
-  VISIT(group_id);
-}
-
 VISIT_PROTO_FIELDS(const sync_pb::UserEventSpecifics::FieldTrial& proto) {
-  VISIT_REP(field_trial_pairs);
+  VISIT_REP(variation_ids);
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::GcmChannelFlags& proto) {

components/sync/protocol/user_event_specifics.proto

@@ -31,13 +31,7 @@ message UserEventSpecifics {
 
   // Reports field trial membership for the subset of trials that have been
   // registered as important to other event types.
-  message FieldTrial {
-    message FieldTrialPair {
-      optional fixed32 name_id = 1;
-      optional fixed32 group_id = 2;
-    }
-    repeated FieldTrialPair field_trial_pairs = 1;
-  }
+  message FieldTrial { repeated fixed32 variation_ids = 1; }
 
   // Language detection output.
   message LanguageDetection {

components/sync/user_events/DEPS

@@ -4,4 +4,5 @@ include_rules = [
   "+components/sync/driver",
   "+components/sync/model",
   "+components/sync/protocol",
+  "+components/variations",
 ]

components/sync/user_events/fake_user_event_service.cc

@@ -27,20 +27,9 @@ base::WeakPtr<ModelTypeSyncBridge> FakeUserEventService::GetSyncBridge() {
   return base::WeakPtr<ModelTypeSyncBridge>();
 }
 
-void FakeUserEventService::RegisterDependentFieldTrial(
-    const std::string& trial_name,
-    UserEventSpecifics::EventCase event_case) {
-  registered_dependent_field_trials_[trial_name].insert(event_case);
-}
-
 const std::vector<UserEventSpecifics>&
 FakeUserEventService::GetRecordedUserEvents() const {
   return recorded_user_events_;
 }
 
-const std::map<std::string, std::set<sync_pb::UserEventSpecifics::EventCase>>&
-FakeUserEventService::GetRegisteredDependentFieldTrials() const {
-  return registered_dependent_field_trials_;
-}
-
 }  // namespace syncer

components/sync/user_events/fake_user_event_service.h

@@ -5,9 +5,7 @@
 #ifndef COMPONENTS_SYNC_USER_EVENTS_FAKE_USER_EVENT_SERVICE_H_
 #define COMPONENTS_SYNC_USER_EVENTS_FAKE_USER_EVENT_SERVICE_H_
 
-#include <map>
 #include <memory>
-#include <set>
 #include <string>
 #include <vector>
 
@@ -31,19 +29,12 @@ class FakeUserEventService : public UserEventService {
   void RecordUserEvent(
       std::unique_ptr<sync_pb::UserEventSpecifics> specifics) override;
   void RecordUserEvent(const sync_pb::UserEventSpecifics& specifics) override;
-  void RegisterDependentFieldTrial(
-      const std::string& trial_name,
-      sync_pb::UserEventSpecifics::EventCase event_case) override;
   base::WeakPtr<ModelTypeSyncBridge> GetSyncBridge() override;
 
   const std::vector<sync_pb::UserEventSpecifics>& GetRecordedUserEvents() const;
-  const std::map<std::string, std::set<sync_pb::UserEventSpecifics::EventCase>>&
-  GetRegisteredDependentFieldTrials() const;
 
  private:
   std::vector<sync_pb::UserEventSpecifics> recorded_user_events_;
-  std::map<std::string, std::set<sync_pb::UserEventSpecifics::EventCase>>
-      registered_dependent_field_trials_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeUserEventService);
 };

components/sync/user_events/no_op_user_event_service.cc

@@ -24,8 +24,4 @@ base::WeakPtr<ModelTypeSyncBridge> NoOpUserEventService::GetSyncBridge() {
   return base::WeakPtr<ModelTypeSyncBridge>();
 }
 
-void NoOpUserEventService::RegisterDependentFieldTrial(
-    const std::string& trial_name,
-    UserEventSpecifics::EventCase event_case) {}
-
 }  // namespace syncer

components/sync/user_events/no_op_user_event_service.h

@@ -26,9 +26,6 @@ class NoOpUserEventService : public UserEventService {
   void RecordUserEvent(
       std::unique_ptr<sync_pb::UserEventSpecifics> specifics) override;
   void RecordUserEvent(const sync_pb::UserEventSpecifics& specifics) override;
-  void RegisterDependentFieldTrial(
-      const std::string& trial_name,
-      sync_pb::UserEventSpecifics::EventCase event_case) override;
   base::WeakPtr<ModelTypeSyncBridge> GetSyncBridge() override;
 
  private:

components/sync/user_events/trial_recorder.cc

+// Copyright 2017 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 "components/sync/user_events/trial_recorder.h"
+
+#include <memory>
+#include <set>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/feature_list.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/stl_util.h"
+#include "base/time/time.h"
+#include "components/sync/driver/sync_driver_switches.h"
+#include "components/variations/active_field_trials.h"
+
+using sync_pb::UserEventSpecifics;
+
+namespace syncer {
+
+namespace {
+
+// A FieldTrial is recorded periodically, using this delay. Although upon chance
+// an event is immediately recorded and we reset to using this delay for the
+// next time.
+base::TimeDelta GetFieldTrialDelay() {
+  return base::TimeDelta::FromSeconds(base::GetFieldTrialParamByFeatureAsInt(
+      switches::kSyncUserFieldTrialEvents, "field_trial_delay_seconds",
+      base::TimeDelta::FromDays(1).InSeconds()));
+}
+
+}  // namespace
+
+TrialRecorder::TrialRecorder(UserEventService* user_event_service)
+    : user_event_service_(user_event_service),
+      variations_(variations::CHROME_SYNC_SERVICE,
+                  base::BindRepeating(&TrialRecorder::OnNewVariationId,
+                                      base::Unretained(this))) {
+  DCHECK(user_event_service_);
+  RecordFieldTrials();
+}
+
+TrialRecorder::~TrialRecorder() {}
+
+void TrialRecorder::RecordFieldTrials() {
+  if (!base::FeatureList::IsEnabled(switches::kSyncUserFieldTrialEvents)) {
+    return;
+  }
+
+  std::set<variations::VariationID> ids = variations_.GetIds();
+  if (!ids.empty()) {
+    auto specifics = std::make_unique<UserEventSpecifics>();
+    specifics->set_event_time_usec(
+        (base::Time::Now() - base::Time()).InMicroseconds());
+
+    for (variations::VariationID id : ids) {
+      DCHECK_NE(variations::EMPTY_ID, id);
+      specifics->mutable_field_trial_event()->add_variation_ids(id);
+    }
+    user_event_service_->RecordUserEvent(std::move(specifics));
+  }
+
+  field_trial_timer_.Start(
+      FROM_HERE, GetFieldTrialDelay(),
+      base::BindRepeating(&TrialRecorder::RecordFieldTrials,
+                          base::Unretained(this)));
+}
+
+void TrialRecorder::OnNewVariationId(variations::VariationID id) {
+  RecordFieldTrials();
+}
+
+}  // namespace syncer

components/sync/user_events/trial_recorder.h

+// Copyright 2017 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 COMPONENTS_SYNC_USER_EVENTS_TRIAL_RECORDER_H_
+#define COMPONENTS_SYNC_USER_EVENTS_TRIAL_RECORDER_H_
+
+#include "base/macros.h"
+#include "base/timer/timer.h"
+#include "components/sync/protocol/user_event_specifics.pb.h"
+#include "components/sync/user_events/user_event_service.h"
+#include "components/variations/variations_associated_data.h"
+#include "components/variations/variations_id_collection.h"
+
+namespace syncer {
+
+// Watches finalization of trails and records FieldTrial events through its
+// UserEventService on construction, on change, and every so often.
+class TrialRecorder {
+ public:
+  explicit TrialRecorder(UserEventService* user_event_service);
+  ~TrialRecorder();
+
+ private:
+  // Construct and record a field trial event if applicable.
+  void RecordFieldTrials();
+
+  // Simply drops the |id| param and calls RecordFieldTrials().
+  void OnNewVariationId(variations::VariationID id);
+
+  // Non-owning pointer to interface of how events are actually recorded.
+  UserEventService* user_event_service_;
+
+  // Tracks all the variation ids that we we care about.
+  variations::VariationsIdCollection variations_;
+
+  // Timer used to record a field trial event every given interval.
+  base::OneShotTimer field_trial_timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(TrialRecorder);
+};
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_USER_EVENTS_TRIAL_RECORDER_H_

components/sync/user_events/trial_recorder_unittest.cc

+// Copyright 2017 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 "components/sync/user_events/trial_recorder.h"
+
+#include <memory>
+#include <set>
+#include <string>
+
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
+#include "base/test/scoped_feature_list.h"
+#include "components/sync/driver/sync_driver_switches.h"
+#include "components/sync/protocol/sync.pb.h"
+#include "components/sync/user_events/fake_user_event_service.h"
+#include "components/variations/variations_associated_data.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using sync_pb::UserEventSpecifics;
+
+namespace syncer {
+
+namespace {
+
+const char kTrial1[] = "TrialNameOne";
+const char kTrial2[] = "TrialNameTwo";
+const char kGroup[] = "GroupName";
+const variations::VariationID kVariation1 = 111;
+const variations::VariationID kVariation2 = 222;
+
+void VerifyEvent(std::set<variations::VariationID> expected_variations,
+                 const UserEventSpecifics& actual) {
+  ASSERT_EQ(
+      expected_variations.size(),
+      static_cast<size_t>(actual.field_trial_event().variation_ids_size()));
+  for (int actaul_variation : actual.field_trial_event().variation_ids()) {
+    auto iter = expected_variations.find(actaul_variation);
+    if (iter == expected_variations.end()) {
+      FAIL() << actaul_variation;
+    } else {
+      // Remove to make sure the event doesn't contain duplicates.
+      expected_variations.erase(iter);
+    }
+  }
+}
+
+void SetupAndFinalizeTrial(const std::string& trial_name,
+                           variations::VariationID id) {
+  variations::AssociateGoogleVariationID(variations::CHROME_SYNC_SERVICE,
+                                         trial_name, kGroup, id);
+  base::FieldTrialList::CreateFieldTrial(trial_name, kGroup);
+  base::FieldTrialList::FindFullName(trial_name);
+  base::RunLoop().RunUntilIdle();
+}
+
+class TrialRecorderTest : public testing::Test {
+ public:
+  TrialRecorderTest() : field_trial_list_(nullptr) {}
+
+  ~TrialRecorderTest() override { variations::testing::ClearAllVariationIDs(); }
+
+  FakeUserEventService* service() { return &service_; }
+  TrialRecorder* recorder() { return recorder_.get(); }
+
+  void VerifyLastEvent(std::set<variations::VariationID> expected_variations) {
+    ASSERT_LE(1u, service()->GetRecordedUserEvents().size());
+    VerifyEvent(expected_variations,
+                *service()->GetRecordedUserEvents().rbegin());
+  }
+
+  void InitRecorder() {
+    recorder_ = std::make_unique<TrialRecorder>(&service_);
+  }
+
+ private:
+  base::MessageLoop message_loop_;
+  base::FieldTrialList field_trial_list_;
+  FakeUserEventService service_;
+  std::unique_ptr<TrialRecorder> recorder_;
+};
+
+TEST_F(TrialRecorderTest, FinalizedBeforeInit) {
+  SetupAndFinalizeTrial(kTrial1, kVariation1);
+  SetupAndFinalizeTrial(kTrial2, kVariation2);
+  // Should check on initialization to see if there are any trails to record.
+  InitRecorder();
+  VerifyLastEvent({kVariation1, kVariation2});
+}
+
+TEST_F(TrialRecorderTest, FinalizedAfterInit) {
+  InitRecorder();
+  SetupAndFinalizeTrial(kTrial1, kVariation1);
+  SetupAndFinalizeTrial(kTrial2, kVariation2);
+  VerifyLastEvent({kVariation1, kVariation2});
+}
+
+TEST_F(TrialRecorderTest, FinalizedMix) {
+  SetupAndFinalizeTrial(kTrial1, kVariation1);
+  InitRecorder();
+  VerifyLastEvent({kVariation1});
+
+  SetupAndFinalizeTrial(kTrial2, kVariation2);
+  VerifyLastEvent({kVariation1, kVariation2});
+}
+
+TEST_F(TrialRecorderTest, WrongVariationKey) {
+  InitRecorder();
+  variations::AssociateGoogleVariationID(variations::GOOGLE_WEB_PROPERTIES,
+                                         kTrial1, kGroup, kVariation1);
+  base::FieldTrialList::CreateFieldTrial(kTrial1, kGroup);
+  base::FieldTrialList::FindFullName(kTrial1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0u, service()->GetRecordedUserEvents().size());
+}
+
+TEST_F(TrialRecorderTest, NoVariation) {
+  InitRecorder();
+  base::FieldTrialList::CreateFieldTrial(kTrial1, kGroup);
+  base::FieldTrialList::FindFullName(kTrial1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0u, service()->GetRecordedUserEvents().size());
+}
+
+TEST_F(TrialRecorderTest, FieldTrialFeatureDisabled) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      switches::kSyncUserFieldTrialEvents);
+  InitRecorder();
+  SetupAndFinalizeTrial(kTrial1, kVariation1);
+
+  EXPECT_EQ(0u, service()->GetRecordedUserEvents().size());
+}
+
+TEST_F(TrialRecorderTest, FieldTrialTimer) {
+  SetupAndFinalizeTrial(kTrial2, kVariation2);
+
+  {
+    // Start with 0 delay, which should mean we post immediately to record.
+    // Need to be not call any methods that might invoke RunUntilIdle() while we
+    // have a delay of 0, like SetupAndFinalizeTrial().
+    base::test::ScopedFeatureList scoped_feature_list;
+    scoped_feature_list.InitAndEnableFeatureWithParameters(
+        switches::kSyncUserFieldTrialEvents,
+        {{"field_trial_delay_seconds", "0"}});
+    InitRecorder();
+    EXPECT_EQ(1u, service()->GetRecordedUserEvents().size());
+  }
+
+  // Now that |scoped_feature_list| is gone, we should reset to default,
+  // otherwise our RunUntilIdle() would infinitively loop with a delay of 0.
+  base::RunLoop().RunUntilIdle();
+  // Should have picked up exactly one more event.
+  EXPECT_EQ(2u, service()->GetRecordedUserEvents().size());
+}
+
+}  // namespace
+
+}  // namespace syncer

components/sync/user_events/user_event_service.h

@@ -30,12 +30,6 @@ class UserEventService : public KeyedService {
   virtual void RecordUserEvent(
       const sync_pb::UserEventSpecifics& specifics) = 0;
 
-  // Register that knowledge about a given field trial is important when
-  // interpreting specified user event type, and should be recorded if assigned.
-  virtual void RegisterDependentFieldTrial(
-      const std::string& trial_name,
-      sync_pb::UserEventSpecifics::EventCase event_case) = 0;
-
   // Returns the underlying Sync integration point.
   virtual base::WeakPtr<ModelTypeSyncBridge> GetSyncBridge() = 0;
 

components/sync/user_events/user_event_service_impl.cc

@@ -6,27 +6,67 @@
 
 #include <utility>
 
-#include "base/bind.h"
 #include "base/feature_list.h"
 #include "base/rand_util.h"
+#include "base/stl_util.h"
+#include "base/time/time.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
-#include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/user_events/user_event_sync_bridge.h"
 
 using sync_pb::UserEventSpecifics;
 
 namespace syncer {
 
+namespace {
+
+enum NavigationPresence {
+  kMustHave,
+  kCannotHave,
+  kEitherOkay,
+};
+
+NavigationPresence GetNavigationPresence(
+    UserEventSpecifics::EventCase event_case) {
+  switch (event_case) {
+    case UserEventSpecifics::kTestEvent:
+      return kEitherOkay;
+    case UserEventSpecifics::kFieldTrialEvent:
+      return kCannotHave;
+    case UserEventSpecifics::kLanguageDetectionEvent:
+      return kMustHave;
+    case UserEventSpecifics::kTranslationEvent:
+      return kMustHave;
+    case UserEventSpecifics::kUserConsent:
+      return kCannotHave;
+    case UserEventSpecifics::kGaiaPasswordReuseEvent:
+      return kMustHave;
+    case UserEventSpecifics::EVENT_NOT_SET:
+      break;
+  }
+  NOTREACHED();
+  return kEitherOkay;
+}
+
+bool NavigationPresenceValid(UserEventSpecifics::EventCase event_case,
+                             bool has_navigation_id) {
+  NavigationPresence presence = GetNavigationPresence(event_case);
+  return presence == kEitherOkay ||
+         (presence == kMustHave && has_navigation_id) ||
+         (presence == kCannotHave && !has_navigation_id);
+}
+
+}  // namespace
+
 UserEventServiceImpl::UserEventServiceImpl(
     SyncService* sync_service,
     std::unique_ptr<UserEventSyncBridge> bridge)
     : sync_service_(sync_service),
       bridge_(std::move(bridge)),
-      session_id_(base::RandUint64()) {
+      session_id_(base::RandUint64()),
+      trial_recorder_(this) {
   DCHECK(bridge_);
   DCHECK(sync_service_);
-  // TODO(skym): Subscribe to events about field trial membership changing.
 }
 
 UserEventServiceImpl::~UserEventServiceImpl() {}
@@ -58,20 +98,29 @@ bool UserEventServiceImpl::MightRecordEvents(bool off_the_record,
          base::FeatureList::IsEnabled(switches::kSyncUserEvents);
 }
 
-bool UserEventServiceImpl::ShouldRecordEvent(
-    const UserEventSpecifics& specifics) {
-  // We only record events if the user is syncing history (as indicated by
-  // GetPreferredDataTypes()) and has not enabled a custom passphrase (as
-  // indicated by IsUsingSecondaryPassphrase()).
+bool UserEventServiceImpl::CanRecordHistory() {
+  // Before the engine is initialized, we cannot trust the other fields.
   return sync_service_->IsEngineInitialized() &&
          !sync_service_->IsUsingSecondaryPassphrase() &&
          sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
 }
 
-void UserEventServiceImpl::RegisterDependentFieldTrial(
-    const std::string& trial_name,
-    UserEventSpecifics::EventCase event_case) {
-  // TODO(skym): Implementation.
+bool UserEventServiceImpl::ShouldRecordEvent(
+    const UserEventSpecifics& specifics) {
+  if (specifics.event_case() == UserEventSpecifics::EVENT_NOT_SET) {
+    return false;
+  }
+
+  if (!NavigationPresenceValid(specifics.event_case(),
+                               specifics.has_navigation_id())) {
+    return false;
+  }
+
+  if (specifics.has_navigation_id() && !CanRecordHistory()) {
+    return false;
+  }
+
+  return true;
 }
 
 }  // namespace syncer

components/sync/user_events/user_event_service_impl.h

@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
+#include "components/sync/user_events/trial_recorder.h"
 #include "components/sync/user_events/user_event_service.h"
 
 namespace syncer {
@@ -33,15 +34,15 @@ class UserEventServiceImpl : public UserEventService {
   void RecordUserEvent(
       std::unique_ptr<sync_pb::UserEventSpecifics> specifics) override;
   void RecordUserEvent(const sync_pb::UserEventSpecifics& specifics) override;
-  void RegisterDependentFieldTrial(
-      const std::string& trial_name,
-      sync_pb::UserEventSpecifics::EventCase event_case) override;
   base::WeakPtr<ModelTypeSyncBridge> GetSyncBridge() override;
 
   // Checks known (and immutable) conditions that should not change at runtime.
   static bool MightRecordEvents(bool off_the_record, SyncService* sync_service);
 
  private:
+  // Whether allowed to record events that link to navigation data.
+  bool CanRecordHistory();
+
   // Checks dynamic or event specific conditions.
   bool ShouldRecordEvent(const sync_pb::UserEventSpecifics& specifics);
 
@@ -54,6 +55,9 @@ class UserEventServiceImpl : public UserEventService {
   // which events came from the same session.
   uint64_t session_id_;
 
+  // Tracks and records field trails when appropriate.
+  TrialRecorder trial_recorder_;
+
   DISALLOW_COPY_AND_ASSIGN(UserEventServiceImpl);
 };
 

components/sync/user_events/user_event_service_impl_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/sync/user_events/user_event_service_impl.h"
 
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/fake_sync_service.h"
@@ -13,14 +14,45 @@
 #include "components/sync/model/recording_model_type_change_processor.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/user_events/user_event_sync_bridge.h"
+#include "components/variations/variations_associated_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::test::ScopedFeatureList;
 using sync_pb::UserEventSpecifics;
 
 namespace syncer {
 
 namespace {
 
+std::unique_ptr<UserEventSpecifics> Event() {
+  return std::make_unique<UserEventSpecifics>();
+}
+
+std::unique_ptr<UserEventSpecifics> AsTest(
+    std::unique_ptr<UserEventSpecifics> specifics) {
+  specifics->mutable_test_event();
+  return specifics;
+}
+
+std::unique_ptr<UserEventSpecifics> AsDetection(
+    std::unique_ptr<UserEventSpecifics> specifics) {
+  specifics->mutable_language_detection_event();
+  return specifics;
+}
+
+std::unique_ptr<UserEventSpecifics> AsTrial(
+    std::unique_ptr<UserEventSpecifics> specifics) {
+  specifics->mutable_field_trial_event();
+  return specifics;
+}
+
+std::unique_ptr<UserEventSpecifics> WithNav(
+    std::unique_ptr<UserEventSpecifics> specifics,
+    int64_t navigation_id = 1) {
+  specifics->set_navigation_id(navigation_id);
+  return specifics;
+}
+
 class TestSyncService : public FakeSyncService {
  public:
   TestSyncService(bool is_engine_initialized,
@@ -53,7 +85,8 @@ class TestGlobalIdMapper : public GlobalIdMapper {
 class UserEventServiceImplTest : public testing::Test {
  protected:
   UserEventServiceImplTest()
-      : sync_service_(true, false, {HISTORY_DELETE_DIRECTIVES}) {}
+      : field_trial_list_(nullptr),
+        sync_service_(true, false, {HISTORY_DELETE_DIRECTIVES}) {}
 
   std::unique_ptr<UserEventSyncBridge> MakeBridge() {
     return std::make_unique<UserEventSyncBridge>(
@@ -66,10 +99,11 @@ class UserEventServiceImplTest : public testing::Test {
   const RecordingModelTypeChangeProcessor& processor() { return *processor_; }
 
  private:
+  base::MessageLoop message_loop_;
+  base::FieldTrialList field_trial_list_;
   TestSyncService sync_service_;
   RecordingModelTypeChangeProcessor* processor_;
   TestGlobalIdMapper mapper_;
-  base::MessageLoop message_loop_;
 };
 
 TEST_F(UserEventServiceImplTest, MightRecordEventsFeatureEnabled) {
@@ -83,50 +117,94 @@ TEST_F(UserEventServiceImplTest, MightRecordEventsFeatureEnabled) {
 
 TEST_F(UserEventServiceImplTest, MightRecordEventsFeatureDisabled) {
   // Will not record because the default on feature is overridden.
-  base::test::ScopedFeatureList scoped_feature_list;
+  ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndDisableFeature(switches::kSyncUserEvents);
   EXPECT_FALSE(UserEventServiceImpl::MightRecordEvents(false, sync_service()));
 }
 
-TEST_F(UserEventServiceImplTest, ShouldNotRecordNoHistory) {
+TEST_F(UserEventServiceImplTest, ShouldRecord) {
+  UserEventServiceImpl service(sync_service(), MakeBridge());
+  service.RecordUserEvent(AsTest(Event()));
+  EXPECT_EQ(1u, processor().put_multimap().size());
+}
+
+TEST_F(UserEventServiceImplTest, ShouldRecordNoHistory) {
   TestSyncService no_history_sync_service(true, false, ModelTypeSet());
   UserEventServiceImpl service(&no_history_sync_service, MakeBridge());
-  service.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+
+  // Only record events without navigation ids when history sync is off.
+  service.RecordUserEvent(WithNav(AsTest(Event())));
   EXPECT_EQ(0u, processor().put_multimap().size());
+  service.RecordUserEvent(AsTest(Event()));
+  EXPECT_EQ(1u, processor().put_multimap().size());
 }
 
-TEST_F(UserEventServiceImplTest, ShouldNotRecordPassphrase) {
+TEST_F(UserEventServiceImplTest, ShouldRecordPassphrase) {
   TestSyncService passphrase_sync_service(true, true,
                                           {HISTORY_DELETE_DIRECTIVES});
   UserEventServiceImpl service(&passphrase_sync_service, MakeBridge());
-  service.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+
+  // Only record events without navigation ids when a passphrase is used.
+  service.RecordUserEvent(WithNav(AsTest(Event())));
   EXPECT_EQ(0u, processor().put_multimap().size());
+  service.RecordUserEvent(AsTest(Event()));
+  EXPECT_EQ(1u, processor().put_multimap().size());
 }
 
-TEST_F(UserEventServiceImplTest, ShouldNotRecordEngineOff) {
+TEST_F(UserEventServiceImplTest, ShouldRecordEngineOff) {
   TestSyncService engine_not_initialized_sync_service(
       false, false, {HISTORY_DELETE_DIRECTIVES});
   UserEventServiceImpl service(&engine_not_initialized_sync_service,
                                MakeBridge());
-  service.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+
+  // Only record events without navigation ids when the engine is off.
+  service.RecordUserEvent(WithNav(AsTest(Event())));
   EXPECT_EQ(0u, processor().put_multimap().size());
+  service.RecordUserEvent(AsTest(Event()));
+  EXPECT_EQ(1u, processor().put_multimap().size());
 }
 
-TEST_F(UserEventServiceImplTest, ShouldRecord) {
+TEST_F(UserEventServiceImplTest, ShouldRecordEmpty) {
+  UserEventServiceImpl service(sync_service(), MakeBridge());
+
+  // All untyped events should always be ignored.
+  service.RecordUserEvent(Event());
+  EXPECT_EQ(0u, processor().put_multimap().size());
+  service.RecordUserEvent(WithNav(Event()));
+  EXPECT_EQ(0u, processor().put_multimap().size());
+}
+
+TEST_F(UserEventServiceImplTest, ShouldRecordHasNavigationId) {
   UserEventServiceImpl service(sync_service(), MakeBridge());
-  service.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+
+  // Verify logic for types that might or might not have a navigation id.
+  service.RecordUserEvent(AsTest(Event()));
   EXPECT_EQ(1u, processor().put_multimap().size());
+  service.RecordUserEvent(WithNav(AsTest(Event())));
+  EXPECT_EQ(2u, processor().put_multimap().size());
+
+  // Verify logic for types that must have a navigation id.
+  service.RecordUserEvent(AsDetection(Event()));
+  EXPECT_EQ(2u, processor().put_multimap().size());
+  service.RecordUserEvent(WithNav(AsDetection(Event())));
+  EXPECT_EQ(3u, processor().put_multimap().size());
+
+  // Verify logic for types that cannot have a navigation id.
+  service.RecordUserEvent(AsTrial(Event()));
+  EXPECT_EQ(4u, processor().put_multimap().size());
+  service.RecordUserEvent(WithNav(AsTrial(Event())));
+  EXPECT_EQ(4u, processor().put_multimap().size());
 }
 
 TEST_F(UserEventServiceImplTest, SessionIdIsDifferent) {
   UserEventServiceImpl service1(sync_service(), MakeBridge());
-  service1.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+  service1.RecordUserEvent(AsTest(Event()));
   ASSERT_EQ(1u, processor().put_multimap().size());
   auto put1 = processor().put_multimap().begin();
   int64_t session_id1 = put1->second->specifics.user_event().session_id();
 
   UserEventServiceImpl service2(sync_service(), MakeBridge());
-  service2.RecordUserEvent(std::make_unique<UserEventSpecifics>());
+  service2.RecordUserEvent(AsTest(Event()));
   // The object processor() points to has changed to be |service2|'s processor.
   ASSERT_EQ(1u, processor().put_multimap().size());
   auto put2 = processor().put_multimap().begin();
@@ -135,6 +213,20 @@ TEST_F(UserEventServiceImplTest, SessionIdIsDifferent) {
   EXPECT_NE(session_id1, session_id2);
 }
 
+TEST_F(UserEventServiceImplTest, FieldTrial) {
+  variations::AssociateGoogleVariationID(variations::CHROME_SYNC_SERVICE,
+                                         "trial", "group", 123);
+  base::FieldTrialList::CreateFieldTrial("trial", "group");
+  base::FieldTrialList::FindFullName("trial");
+
+  UserEventServiceImpl service(sync_service(), MakeBridge());
+  ASSERT_EQ(1u, processor().put_multimap().size());
+  const UserEventSpecifics specifics =
+      processor().put_multimap().begin()->second->specifics.user_event();
+  ASSERT_EQ(1, specifics.field_trial_event().variation_ids_size());
+  EXPECT_EQ(123u, specifics.field_trial_event().variation_ids(0));
+}
+
 }  // namespace
 
 }  // namespace syncer

components/variations/BUILD.gn

@@ -50,6 +50,8 @@ static_library("variations") {
     "variations_experiment_util.h",
     "variations_http_header_provider.cc",
     "variations_http_header_provider.h",
+    "variations_id_collection.cc",
+    "variations_id_collection.h",
     "variations_request_scheduler.cc",
     "variations_request_scheduler.h",
     "variations_seed_processor.cc",
@@ -130,6 +132,7 @@ source_set("unit_tests") {
     "synthetic_trial_registry_unittest.cc",
     "variations_associated_data_unittest.cc",
     "variations_http_header_provider_unittest.cc",
+    "variations_id_collection_unittest.cc",
     "variations_request_scheduler_unittest.cc",
     "variations_seed_processor_unittest.cc",
     "variations_seed_simulator_unittest.cc",

components/variations/variations_associated_data.h

@@ -38,7 +38,7 @@
 //
 // VariationID id = GetGoogleVariationID(GOOGLE_WEB_PROPERTIES, "trial",
 //                                       "group1");
-// if (id != variations::kEmptyID) {
+// if (id != variations::EMPTY_ID) {
 //   // use |id|
 // }
 
@@ -97,7 +97,7 @@ void AssociateGoogleVariationIDForceHashes(IDCollectionKey key,
 
 // Retrieve the variations::VariationID associated with a FieldTrial group for
 // collection |key|. The group is denoted by |trial_name| and |group_name|.
-// This will return variations::kEmptyID if there is currently no associated ID
+// This will return variations::EMPTY_ID if there is currently no associated ID
 // for the named group. This API can be nicely combined with
 // FieldTrial::GetActiveFieldTrialGroups() to enumerate the variation IDs for
 // all active FieldTrial groups. Thread safe.

components/variations/variations_id_collection.cc

+// Copyright 2017 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 "components/variations/variations_id_collection.h"
+
+#include "base/bind_helpers.h"
+#include "base/feature_list.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/stl_util.h"
+#include "base/time/time.h"
+#include "components/variations/active_field_trials.h"
+
+namespace variations {
+
+VariationsIdCollection::VariationsIdCollection(
+    IDCollectionKey collection_key,
+    base::RepeatingCallback<void(VariationID)> new_id_callback)
+    : collection_key_(collection_key) {
+  base::FieldTrialList::AddObserver(this);
+
+  base::FieldTrial::ActiveGroups initial_groups;
+  base::FieldTrialList::GetActiveFieldTrialGroups(&initial_groups);
+  for (const base::FieldTrial::ActiveGroup& group : initial_groups) {
+    OnFieldTrialGroupFinalized(group.trial_name, group.group_name);
+  }
+
+  // Delay setting |new_id_callback_| until initialization is over.
+  new_id_callback_ = new_id_callback;
+}
+
+VariationsIdCollection::~VariationsIdCollection() {
+  base::FieldTrialList::RemoveObserver(this);
+}
+
+void VariationsIdCollection::OnFieldTrialGroupFinalized(
+    const std::string& trial_name,
+    const std::string& group_name) {
+  const VariationID id =
+      GetGoogleVariationID(collection_key_, trial_name, group_name);
+  if (id != EMPTY_ID) {
+    bool modified = id_set_.insert(id).second;
+    if (modified && new_id_callback_)
+      new_id_callback_.Run(id);
+  }
+}
+
+const std::set<VariationID>& VariationsIdCollection::GetIds() {
+  return id_set_;
+}
+
+}  // namespace variations

components/variations/variations_id_collection.h

+// Copyright 2017 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 COMPONENTS_VARIATIONS_VARIATIONS_ID_COLLECTION_H_
+#define COMPONENTS_VARIATIONS_VARIATIONS_ID_COLLECTION_H_
+
+#include <set>
+#include <string>
+
+#include "base/macros.h"
+#include "base/metrics/field_trial.h"
+#include "components/variations/variations_associated_data.h"
+
+namespace variations {
+
+// Watches finalization of trials that may have a variation id for the given
+// key. Maintains a list of ids for the given key, and invokes a callback every
+// time a new id is found. Does not invoke the callback for ids for trials that
+// were finalized prior to the construction of a VariationsIdCollection
+// instance. Is not thread safe.
+class VariationsIdCollection : public base::FieldTrialList::Observer {
+ public:
+  // Will not invoke |new_id_callback| for ids that correspond to trials that
+  // were finalized before construction of this object. Callers may want to
+  // call |GetIds()| manually after construction to batch the initial ids.
+  VariationsIdCollection(
+      IDCollectionKey collection_key,
+      base::RepeatingCallback<void(VariationID)> new_id_callback);
+
+  ~VariationsIdCollection() override;
+
+  // base::FieldTrialList::Observer implementation.
+  void OnFieldTrialGroupFinalized(const std::string& trial_name,
+                                  const std::string& group_name) override;
+
+  // Returns a set of all variations ids for trials finalized that are part of
+  // |collection_key_|.
+  const std::set<VariationID>& GetIds();
+
+ private:
+  const IDCollectionKey collection_key_;
+
+  // Will not be set until the end of initialization.
+  base::RepeatingCallback<void(VariationID)> new_id_callback_;
+
+  std::set<VariationID> id_set_;
+
+  DISALLOW_COPY_AND_ASSIGN(VariationsIdCollection);
+};
+
+}  // namespace variations
+
+#endif  // COMPONENTS_VARIATIONS_VARIATIONS_ID_COLLECTION_H_

components/variations/variations_id_collection_unittest.cc

+// Copyright 2017 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 "components/variations/variations_id_collection.h"
+
+#include <memory>
+#include <vector>
+
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
+#include "components/variations/variations_associated_data.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace variations {
+
+namespace {
+
+const char kTrial1[] = "TrialNameOne";
+const char kTrial2[] = "TrialNameTwo";
+const char kTrial3[] = "TrialNameThree";
+const char kGroup[] = "GroupName";
+const VariationID kVariation1 = 111;
+const VariationID kVariation2 = 222;
+const VariationID kVariation3 = 333;
+
+// Awkward helper functions to allow initializer lists inside macros.
+std::set<VariationID> Set(std::set<VariationID> set) {
+  return set;
+}
+std::vector<VariationID> Vec(std::vector<VariationID> vec) {
+  return vec;
+}
+
+void SetupTrial(const std::string& trial_name,
+                IDCollectionKey key,
+                VariationID id) {
+  AssociateGoogleVariationID(key, trial_name, kGroup, id);
+  base::FieldTrialList::CreateFieldTrial(trial_name, kGroup);
+}
+
+void FinalizeTrial(const std::string& trial_name) {
+  EXPECT_EQ(kGroup, base::FieldTrialList::FindFullName(trial_name));
+  base::RunLoop().RunUntilIdle();
+}
+
+void SetupAndFinalizeTrial(const std::string& trial_name,
+                           IDCollectionKey key,
+                           VariationID id) {
+  SetupTrial(trial_name, key, id);
+  FinalizeTrial(trial_name);
+}
+
+}  // namespace
+
+class VariationsIdCollectionTest : public ::testing::Test {
+ public:
+  VariationsIdCollectionTest() : field_trial_list_(nullptr) {}
+
+  ~VariationsIdCollectionTest() override { testing::ClearAllVariationIDs(); }
+
+  void OnNewId(VariationID new_id) { new_ids_.push_back(new_id); }
+
+  void ResetCollection(IDCollectionKey key) {
+    collection_ = std::make_unique<VariationsIdCollection>(
+        key, base::BindRepeating(&VariationsIdCollectionTest::OnNewId,
+                                 base::Unretained(this)));
+  }
+
+  const std::vector<VariationID>& GetNewIds() { return new_ids_; }
+
+  VariationsIdCollection* collection() { return collection_.get(); }
+
+ private:
+  base::MessageLoop message_loop_;
+  base::FieldTrialList field_trial_list_;
+  std::unique_ptr<VariationsIdCollection> collection_;
+  std::vector<VariationID> new_ids_;
+};
+
+TEST_F(VariationsIdCollectionTest, VariousSetupAndFinalization) {
+  SetupAndFinalizeTrial(kTrial1, GOOGLE_WEB_PROPERTIES, kVariation1);
+  SetupTrial(kTrial2, GOOGLE_WEB_PROPERTIES, kVariation2);
+  ResetCollection(GOOGLE_WEB_PROPERTIES);
+
+  EXPECT_EQ(Set({kVariation1}), collection()->GetIds());
+  EXPECT_EQ(Vec({}), GetNewIds());
+
+  FinalizeTrial(kTrial2);
+  SetupAndFinalizeTrial(kTrial3, GOOGLE_WEB_PROPERTIES, kVariation3);
+
+  EXPECT_EQ(Set({kVariation1, kVariation2, kVariation3}),
+            collection()->GetIds());
+  EXPECT_EQ(Vec({kVariation2, kVariation3}), GetNewIds());
+}
+
+TEST_F(VariationsIdCollectionTest, VariousKeys) {
+  SetupAndFinalizeTrial(kTrial1, GOOGLE_WEB_PROPERTIES, kVariation1);
+  SetupAndFinalizeTrial(kTrial2, GOOGLE_WEB_PROPERTIES_SIGNED_IN, kVariation2);
+  SetupAndFinalizeTrial(kTrial3, GOOGLE_WEB_PROPERTIES_TRIGGER, kVariation3);
+  ResetCollection(GOOGLE_WEB_PROPERTIES_SIGNED_IN);
+
+  EXPECT_EQ(Set({kVariation2}), collection()->GetIds());
+  EXPECT_EQ(Vec({}), GetNewIds());
+}
+
+TEST_F(VariationsIdCollectionTest, MultipleFinalization) {
+  ResetCollection(GOOGLE_WEB_PROPERTIES);
+  collection()->OnFieldTrialGroupFinalized(kTrial1, kGroup);
+  EXPECT_EQ(Set({}), collection()->GetIds());
+  EXPECT_EQ(Vec({}), GetNewIds());
+
+  // Even though OnFieldTrialGroupFinalized is called, the VariationID lookup
+  // should still fail and it should be gracefully handled.
+  SetupTrial(kTrial2, GOOGLE_WEB_PROPERTIES, kVariation1);
+  collection()->OnFieldTrialGroupFinalized(kTrial1, kGroup);
+  EXPECT_EQ(Set({}), collection()->GetIds());
+  EXPECT_EQ(Vec({}), GetNewIds());
+
+  FinalizeTrial(kTrial2);
+  EXPECT_EQ(Set({kVariation1}), collection()->GetIds());
+  EXPECT_EQ(Vec({kVariation1}), GetNewIds());
+
+  // This shouldn't create any duplicates.
+  collection()->OnFieldTrialGroupFinalized(kTrial1, kGroup);
+  EXPECT_EQ(Set({kVariation1}), collection()->GetIds());
+  EXPECT_EQ(Vec({kVariation1}), GetNewIds());
+}
+
+}  // namespace variations