[BackgroundSyncManager] Tags with different periodicities can overlap

The background sync spec (see https://github.com/slightlyoff/BackgroundSync/pull/74) allows for
registrations to coexist with the same tag so long as they have different periodicities (one-shot vs periodic).

This CL changes Registration::fire_once to Registration::periodicity and indexes registrations by RegistrationKey instead of by tag. RegistrationKey incorporates both the tag and the periodicity.

BUG=474573

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

Cr-Commit-Position: refs/heads/master@{#324691}

4 files changed, 211 insertions, 113 deletions

diff --git a/content/browser/background_sync/background_sync.proto b/content/browser/background_sync/background_sync.proto
index 3717ca1..b5f2e09 100644
--- a/content/browser/background_sync/background_sync.proto
+++ b/content/browser/background_sync/background_sync.proto
@@ -19,10 +19,15 @@ enum SyncPowerState {
   POWER_STATE_AVOID_DRAINING = 1;
 }
 
+enum SyncPeriodicity {
+  SYNC_PERIODIC = 0;
+  SYNC_ONE_SHOT = 1;
+}
+
 message BackgroundSyncRegistrationProto {
   required int64 id = 1;
   required string tag = 2;
-  required bool fire_once = 3;
+  required SyncPeriodicity periodicity = 3;
   required int64 min_period = 4;
   required SyncNetworkState network_state = 5;
   required SyncPowerState power_state = 6;
diff --git a/content/browser/background_sync/background_sync_manager.cc b/content/browser/background_sync/background_sync_manager.cc
index 8c3fa98..4b679bc 100644
--- a/content/browser/background_sync/background_sync_manager.cc
+++ b/content/browser/background_sync/background_sync_manager.cc
@@ -21,11 +21,11 @@ const BackgroundSyncManager::BackgroundSyncRegistration::RegistrationId
         -1;
 
 const BackgroundSyncManager::BackgroundSyncRegistration::RegistrationId
-    BackgroundSyncManager::BackgroundSyncRegistrations::kInitialId = 0;
+    BackgroundSyncManager::BackgroundSyncRegistration::kInitialId = 0;
 
 BackgroundSyncManager::BackgroundSyncRegistrations::
     BackgroundSyncRegistrations()
-    : next_id(kInitialId) {
+    : next_id(BackgroundSyncRegistration::kInitialId) {
 }
 BackgroundSyncManager::BackgroundSyncRegistrations::BackgroundSyncRegistrations(
     BackgroundSyncRegistration::RegistrationId next_id)
@@ -48,6 +48,17 @@ BackgroundSyncManager::~BackgroundSyncManager() {
   service_worker_context_->RemoveObserver(this);
 }
 
+BackgroundSyncManager::RegistrationKey::RegistrationKey(
+    const BackgroundSyncRegistration& registration)
+    : RegistrationKey(registration.tag, registration.periodicity) {
+}
+
+BackgroundSyncManager::RegistrationKey::RegistrationKey(
+    const std::string& tag,
+    SyncPeriodicity periodicity)
+    : value_(periodicity == SYNC_ONE_SHOT ? "o_" + tag : "p_" + tag) {
+}
+
 void BackgroundSyncManager::Register(
     const GURL& origin,
     int64 sw_registration_id,
@@ -74,6 +85,7 @@ void BackgroundSyncManager::Unregister(
     const GURL& origin,
     int64 sw_registration_id,
     const std::string& sync_registration_tag,
+    SyncPeriodicity periodicity,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
     const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -84,9 +96,11 @@ void BackgroundSyncManager::Unregister(
     return;
   }
 
+  RegistrationKey registration_key(sync_registration_tag, periodicity);
+
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::UnregisterImpl, weak_ptr_factory_.GetWeakPtr(),
-      origin, sw_registration_id, sync_registration_tag, sync_registration_id,
+      origin, sw_registration_id, registration_key, sync_registration_id,
       MakeStatusCompletion(callback)));
 }
 
@@ -94,6 +108,7 @@ void BackgroundSyncManager::GetRegistration(
     const GURL& origin,
     int64 sw_registration_id,
     const std::string sync_registration_tag,
+    SyncPeriodicity periodicity,
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -104,10 +119,12 @@ void BackgroundSyncManager::GetRegistration(
     return;
   }
 
+  RegistrationKey registration_key(sync_registration_tag, periodicity);
+
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::GetRegistrationImpl,
       weak_ptr_factory_.GetWeakPtr(), origin, sw_registration_id,
-      sync_registration_tag, MakeStatusAndRegistrationCompletion(callback)));
+      registration_key, MakeStatusAndRegistrationCompletion(callback)));
 }
 
 void BackgroundSyncManager::OnRegistrationDeleted(int64 registration_id,
@@ -193,12 +210,14 @@ void BackgroundSyncManager::InitDidGetDataFromBackend(
           break;
         }
 
+        RegistrationKey registration_key(registration_proto.tag(),
+                                         registration_proto.periodicity());
         BackgroundSyncRegistration* registration =
-            &registrations->tag_to_registration_map[registration_proto.tag()];
+            &registrations->registration_map[registration_key];
 
         registration->id = registration_proto.id();
         registration->tag = registration_proto.tag();
-        registration->fire_once = registration_proto.fire_once();
+        registration->periodicity = registration_proto.periodicity();
         registration->min_period = registration_proto.min_period();
         registration->network_state = registration_proto.network_state();
         registration->power_state = registration_proto.power_state();
@@ -233,7 +252,7 @@ void BackgroundSyncManager::RegisterImpl(
   }
 
   BackgroundSyncRegistration existing_registration;
-  if (LookupRegistration(sw_registration_id, sync_registration.tag,
+  if (LookupRegistration(sw_registration_id, RegistrationKey(sync_registration),
                          &existing_registration)) {
     if (existing_registration.Equals(sync_registration)) {
       base::MessageLoop::current()->PostTask(
@@ -306,7 +325,7 @@ void BackgroundSyncManager::DisableAndClearManagerClearedOne(
 
 bool BackgroundSyncManager::LookupRegistration(
     int64 sw_registration_id,
-    const std::string& sync_registration_tag,
+    const RegistrationKey& registration_key,
     BackgroundSyncRegistration* existing_registration) {
   SWIdToRegistrationsMap::iterator it =
       sw_to_registrations_map_.find(sw_registration_id);
@@ -314,13 +333,13 @@ bool BackgroundSyncManager::LookupRegistration(
     return false;
 
   const BackgroundSyncRegistrations& registrations = it->second;
-  const auto tag_and_registration_iter =
-      registrations.tag_to_registration_map.find(sync_registration_tag);
-  if (tag_and_registration_iter == registrations.tag_to_registration_map.end())
+  const auto key_and_registration_iter =
+      registrations.registration_map.find(registration_key);
+  if (key_and_registration_iter == registrations.registration_map.end())
     return false;
 
   if (existing_registration)
-    *existing_registration = tag_and_registration_iter->second;
+    *existing_registration = key_and_registration_iter->second;
 
   return true;
 }
@@ -335,15 +354,14 @@ void BackgroundSyncManager::StoreRegistrations(
   BackgroundSyncRegistrationsProto registrations_proto;
   registrations_proto.set_next_registration_id(registrations.next_id);
 
-  for (const auto& tag_and_registration :
-       registrations.tag_to_registration_map) {
+  for (const auto& key_and_registration : registrations.registration_map) {
     const BackgroundSyncRegistration& registration =
-        tag_and_registration.second;
+        key_and_registration.second;
     BackgroundSyncRegistrationProto* registration_proto =
         registrations_proto.add_registration();
     registration_proto->set_id(registration.id);
     registration_proto->set_tag(registration.tag);
-    registration_proto->set_fire_once(registration.fire_once);
+    registration_proto->set_periodicity(registration.periodicity);
     registration_proto->set_min_period(registration.min_period);
     registration_proto->set_network_state(registration.network_state);
     registration_proto->set_power_state(registration.power_state);
@@ -385,14 +403,13 @@ void BackgroundSyncManager::RegisterDidStore(
 
 void BackgroundSyncManager::RemoveRegistrationFromMap(
     int64 sw_registration_id,
-    const std::string& sync_registration_tag) {
-  DCHECK(
-      LookupRegistration(sw_registration_id, sync_registration_tag, nullptr));
+    const RegistrationKey& registration_key) {
+  DCHECK(LookupRegistration(sw_registration_id, registration_key, nullptr));
 
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
 
-  registrations->tag_to_registration_map.erase(sync_registration_tag);
+  registrations->registration_map.erase(registration_key);
 }
 
 void BackgroundSyncManager::AddRegistrationToMap(
@@ -403,34 +420,35 @@ void BackgroundSyncManager::AddRegistrationToMap(
 
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
-  registrations->tag_to_registration_map[sync_registration.tag] =
-      sync_registration;
+
+  RegistrationKey registration_key(sync_registration);
+  registrations->registration_map[registration_key] = sync_registration;
 }
 
 void BackgroundSyncManager::StoreDataInBackend(
     int64 sw_registration_id,
     const GURL& origin,
-    const std::string& key,
+    const std::string& backend_key,
     const std::string& data,
     const ServiceWorkerStorage::StatusCallback& callback) {
   service_worker_context_->context()->storage()->StoreUserData(
-      sw_registration_id, origin, key, data, callback);
+      sw_registration_id, origin, backend_key, data, callback);
 }
 
 void BackgroundSyncManager::GetDataFromBackend(
-    const std::string& key,
+    const std::string& backend_key,
     const ServiceWorkerStorage::GetUserDataForAllRegistrationsCallback&
         callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   service_worker_context_->context()->storage()->GetUserDataForAllRegistrations(
-      key, callback);
+      backend_key, callback);
 }
 
 void BackgroundSyncManager::UnregisterImpl(
     const GURL& origin,
     int64 sw_registration_id,
-    const std::string& sync_registration_tag,
+    const RegistrationKey& registration_key,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
     const StatusCallback& callback) {
   if (disabled_) {
@@ -440,7 +458,7 @@ void BackgroundSyncManager::UnregisterImpl(
   }
 
   BackgroundSyncRegistration existing_registration;
-  if (!LookupRegistration(sw_registration_id, sync_registration_tag,
+  if (!LookupRegistration(sw_registration_id, registration_key,
                           &existing_registration) ||
       existing_registration.id != sync_registration_id) {
     base::MessageLoop::current()->PostTask(
@@ -448,7 +466,7 @@ void BackgroundSyncManager::UnregisterImpl(
     return;
   }
 
-  RemoveRegistrationFromMap(sw_registration_id, sync_registration_tag);
+  RemoveRegistrationFromMap(sw_registration_id, registration_key);
 
   StoreRegistrations(
       origin, sw_registration_id,
@@ -482,7 +500,7 @@ void BackgroundSyncManager::UnregisterDidStore(
 void BackgroundSyncManager::GetRegistrationImpl(
     const GURL& origin,
     int64 sw_registration_id,
-    const std::string sync_registration_tag,
+    const RegistrationKey& registration_key,
     const StatusAndRegistrationCallback& callback) {
   if (disabled_) {
     base::MessageLoop::current()->PostTask(
@@ -492,7 +510,7 @@ void BackgroundSyncManager::GetRegistrationImpl(
   }
 
   BackgroundSyncRegistration out_registration;
-  if (!LookupRegistration(sw_registration_id, sync_registration_tag,
+  if (!LookupRegistration(sw_registration_id, registration_key,
                           &out_registration)) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(callback, ERROR_TYPE_NOT_FOUND,
diff --git a/content/browser/background_sync/background_sync_manager.h b/content/browser/background_sync/background_sync_manager.h
index 82c7f43..aee435c 100644
--- a/content/browser/background_sync/background_sync_manager.h
+++ b/content/browser/background_sync/background_sync_manager.h
@@ -52,10 +52,11 @@ class CONTENT_EXPORT BackgroundSyncManager
   struct CONTENT_EXPORT BackgroundSyncRegistration {
     using RegistrationId = int64;
     static const RegistrationId kInvalidRegistrationId;
+    static const RegistrationId kInitialId;
     BackgroundSyncRegistration() {}
 
     bool Equals(const BackgroundSyncRegistration& other) {
-      return this->tag == other.tag && this->fire_once == other.fire_once &&
+      return this->tag == other.tag && this->periodicity == other.periodicity &&
              this->min_period == other.min_period &&
              network_state == other.network_state &&
              power_state == other.power_state;
@@ -63,26 +64,12 @@ class CONTENT_EXPORT BackgroundSyncManager
 
     RegistrationId id = kInvalidRegistrationId;
     std::string tag;
-    bool fire_once = true;
+    SyncPeriodicity periodicity = SYNC_ONE_SHOT;
     int64 min_period = 0;
     SyncNetworkState network_state = NETWORK_STATE_ONLINE;
     SyncPowerState power_state = POWER_STATE_AVOID_DRAINING;
   };
 
-  struct CONTENT_EXPORT BackgroundSyncRegistrations {
-    using TagToRegistrationMap =
-        std::map<std::string, BackgroundSyncRegistration>;
-    static const BackgroundSyncRegistration::RegistrationId kInitialId;
-
-    BackgroundSyncRegistrations();
-    explicit BackgroundSyncRegistrations(
-        BackgroundSyncRegistration::RegistrationId next_id);
-    ~BackgroundSyncRegistrations();
-
-    TagToRegistrationMap tag_to_registration_map;
-    BackgroundSyncRegistration::RegistrationId next_id;
-  };
-
   using StatusCallback = base::Callback<void(ErrorType)>;
   using StatusAndRegistrationCallback =
       base::Callback<void(ErrorType, const BackgroundSyncRegistration&)>;
@@ -92,34 +79,36 @@ class CONTENT_EXPORT BackgroundSyncManager
   ~BackgroundSyncManager() override;
 
   // Stores the given background sync registration and adds it to the scheduling
-  // queue. Overwrites any existing registration with the same tag but
-  // different parameters (other than the id). Calls |callback| with ErrorTypeOK
-  // and the accepted registration on success. The accepted registration will
-  // have a unique id. It may also have altered parameters if the user or UA
-  // chose different parameters than those supplied.
+  // queue. It will overwrite an existing registration with the same tag and
+  // periodicity unless they're identical (save for the id). Calls |callback|
+  // with ErrorTypeOK and the accepted registration on success. The accepted
+  // registration will have a unique id. It may also have altered parameters if
+  // the user or UA chose different parameters than those supplied.
   void Register(const GURL& origin,
                 int64 sw_registration_id,
                 const BackgroundSyncRegistration& sync_registration,
                 const StatusAndRegistrationCallback& callback);
 
-  // Removes the background sync registration with |sync_registration_tag| if
-  // the |sync_registration_id| matches. |sync_registration_id| will not match
-  // if, for instance, a new registration with the same tag has replaced it.
-  // Calls |callback| with ErrorTypeNotFound if no match is found. Calls
-  // |callback| with ErrorTypeOK on success.
+  // Removes the background sync with tag |sync_registration_tag|, periodicity
+  // |periodicity|, and id |sync_registration_id|. Calls |callback| with
+  // ErrorTypeNotFound if no match is found. Calls |callback| with ErrorTypeOK
+  // on success.
   void Unregister(
       const GURL& origin,
       int64 sw_registration_id,
       const std::string& sync_registration_tag,
+      SyncPeriodicity periodicity,
       BackgroundSyncRegistration::RegistrationId sync_registration_id,
       const StatusCallback& callback);
 
   // Finds the background sync registration associated with
-  // |sw_registration_id|. Calls |callback| with ErrorTypeNotFound if it doesn't
-  // exist. Calls |callback| with ErrorTypeOK on success.
+  // |sw_registration_id| with periodicity |periodicity|. Calls
+  // |callback| with ErrorTypeNotFound if it doesn't exist. Calls |callback|
+  // with ErrorTypeOK on success.
   void GetRegistration(const GURL& origin,
                        int64 sw_registration_id,
                        const std::string sync_registration_tag,
+                       SyncPeriodicity periodicity,
                        const StatusAndRegistrationCallback& callback);
 
   // ServiceWorkerContextObserver overrides.
@@ -138,15 +127,43 @@ class CONTENT_EXPORT BackgroundSyncManager
   virtual void StoreDataInBackend(
       int64 sw_registration_id,
       const GURL& origin,
-      const std::string& key,
+      const std::string& backend_key,
       const std::string& data,
       const ServiceWorkerStorage::StatusCallback& callback);
   virtual void GetDataFromBackend(
-      const std::string& key,
+      const std::string& backend_key,
       const ServiceWorkerStorage::GetUserDataForAllRegistrationsCallback&
           callback);
 
  private:
+  class RegistrationKey {
+   public:
+    explicit RegistrationKey(const BackgroundSyncRegistration& registration);
+    RegistrationKey(const std::string& tag, SyncPeriodicity periodicity);
+    RegistrationKey(const RegistrationKey& other) = default;
+    RegistrationKey& operator=(const RegistrationKey& other) = default;
+
+    bool operator<(const RegistrationKey& rhs) const {
+      return value_ < rhs.value_;
+    }
+
+   private:
+    std::string value_;
+  };
+
+  struct BackgroundSyncRegistrations {
+    using RegistrationMap =
+        std::map<RegistrationKey, BackgroundSyncRegistration>;
+
+    BackgroundSyncRegistrations();
+    explicit BackgroundSyncRegistrations(
+        BackgroundSyncRegistration::RegistrationId next_id);
+    ~BackgroundSyncRegistrations();
+
+    RegistrationMap registration_map;
+    BackgroundSyncRegistration::RegistrationId next_id;
+  };
+
   using PermissionStatusCallback = base::Callback<void(bool)>;
   using SWIdToRegistrationsMap = std::map<int64, BackgroundSyncRegistrations>;
 
@@ -166,7 +183,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   // Returns the existing registration in |existing_registration| if it is not
   // null.
   bool LookupRegistration(int64 sw_registration_id,
-                          const std::string& sync_registration_tag,
+                          const RegistrationKey& registration_key,
                           BackgroundSyncRegistration* existing_registration);
 
   // Store all registrations for a given |sw_registration_id|.
@@ -176,7 +193,7 @@ class CONTENT_EXPORT BackgroundSyncManager
 
   // Removes the registration if it is in the map.
   void RemoveRegistrationFromMap(int64 sw_registration_id,
-                                 const std::string& sync_registration_tag);
+                                 const RegistrationKey& registration_key);
 
   void AddRegistrationToMap(
       int64 sw_registration_id,
@@ -202,7 +219,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   void UnregisterImpl(
       const GURL& origin,
       int64 sw_registration_id,
-      const std::string& sync_registration_tag,
+      const RegistrationKey& registration_key,
       BackgroundSyncRegistration::RegistrationId sync_registration_id,
       const StatusCallback& callback);
   void UnregisterDidStore(
@@ -213,7 +230,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   // GetRegistration callbacks
   void GetRegistrationImpl(const GURL& origin,
                            int64 sw_registration_id,
-                           const std::string sync_registration_tag,
+                           const RegistrationKey& registration_key,
                            const StatusAndRegistrationCallback& callback);
 
   // OnRegistrationDeleted callbacks
diff --git a/content/browser/background_sync/background_sync_manager_unittest.cc b/content/browser/background_sync/background_sync_manager_unittest.cc
index 7540697..83b6970 100644
--- a/content/browser/background_sync/background_sync_manager_unittest.cc
+++ b/content/browser/background_sync/background_sync_manager_unittest.cc
@@ -229,7 +229,7 @@ class BackgroundSyncManagerTest : public testing::Test {
     bool was_called = false;
     background_sync_manager_->Unregister(
         GURL(kOrigin), sw_registration_id, sync_registration.tag,
-        sync_registration.id,
+        sync_registration.periodicity, sync_registration.id,
         base::Bind(&BackgroundSyncManagerTest::StatusCallback,
                    base::Unretained(this), &was_called));
     base::RunLoop().RunUntilIdle();
@@ -237,24 +237,29 @@ class BackgroundSyncManagerTest : public testing::Test {
     return callback_error_ == BackgroundSyncManager::ERROR_TYPE_OK;
   }
 
-  bool GetRegistration(const std::string& sync_registration_tag) {
+  bool GetRegistration(const BackgroundSyncManager::BackgroundSyncRegistration&
+                           sync_registration) {
     return GetRegistrationWithServiceWorkerId(sw_registration_id_1_,
-                                              sync_registration_tag);
+                                              sync_registration);
   }
 
   bool GetRegistrationWithServiceWorkerId(
       int64 sw_registration_id,
-      const std::string& sync_registration_tag) {
+      const BackgroundSyncManager::BackgroundSyncRegistration&
+          sync_registration) {
     bool was_called = false;
     background_sync_manager_->GetRegistration(
-        GURL(kOrigin), sw_registration_id, sync_registration_tag,
+        GURL(kOrigin), sw_registration_id, sync_registration.tag,
+        sync_registration.periodicity,
         base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                    base::Unretained(this), &was_called));
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(was_called);
 
-    if (callback_error_ == BackgroundSyncManager::ERROR_TYPE_OK)
-      EXPECT_TRUE(sync_registration_tag == callback_registration_.tag);
+    if (callback_error_ == BackgroundSyncManager::ERROR_TYPE_OK) {
+      EXPECT_STREQ(sync_registration.tag.c_str(),
+                   callback_registration_.tag.c_str());
+    }
 
     return callback_error_ == BackgroundSyncManager::ERROR_TYPE_OK;
   }
@@ -326,13 +331,28 @@ TEST_F(BackgroundSyncManagerTest, RegisterOverwrites) {
   EXPECT_FALSE(callback_registration_.Equals(first_registration));
 }
 
+TEST_F(BackgroundSyncManagerTest, RegisterOverlappingPeriodicAndOneShotTags) {
+  // Registrations with the same tags but different periodicities should not
+  // collide.
+  sync_reg_1_.tag = "";
+  sync_reg_2_.tag = "";
+  sync_reg_1_.periodicity = SYNC_PERIODIC;
+  sync_reg_2_.periodicity = SYNC_ONE_SHOT;
+  EXPECT_TRUE(Register(sync_reg_1_));
+  EXPECT_TRUE(Register(sync_reg_2_));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_EQ(SYNC_PERIODIC, callback_registration_.periodicity);
+  EXPECT_TRUE(GetRegistration(sync_reg_2_));
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.periodicity);
+}
+
 TEST_F(BackgroundSyncManagerTest, RegisterBadBackend) {
   TestBackgroundSyncManager* manager = UseTestBackgroundSyncManager();
   manager->set_corrupt_backend(true);
   EXPECT_FALSE(Register(sync_reg_1_));
   manager->set_corrupt_backend(false);
   EXPECT_FALSE(Register(sync_reg_1_));
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, TwoRegistrations) {
@@ -341,32 +361,32 @@ TEST_F(BackgroundSyncManagerTest, TwoRegistrations) {
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationNonExisting) {
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationExisting) {
   EXPECT_TRUE(Register(sync_reg_1_));
-  EXPECT_TRUE(GetRegistration(sync_reg_1_.tag));
-  EXPECT_FALSE(GetRegistration(sync_reg_2_.tag));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_FALSE(GetRegistration(sync_reg_2_));
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationBadBackend) {
   TestBackgroundSyncManager* manager = UseTestBackgroundSyncManager();
   EXPECT_TRUE(Register(sync_reg_1_));
   manager->set_corrupt_backend(true);
-  EXPECT_TRUE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
   EXPECT_FALSE(Register(sync_reg_2_));
   // Registration should have discovered the bad backend and disabled the
   // BackgroundSyncManager.
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
   manager->set_corrupt_backend(false);
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, Unregister) {
   EXPECT_TRUE(Register(sync_reg_1_));
   EXPECT_TRUE(Unregister(callback_registration_));
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, UnregisterWrongId) {
@@ -390,7 +410,7 @@ TEST_F(BackgroundSyncManagerTest, UnregisterSecond) {
   EXPECT_TRUE(Register(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_2_));
   EXPECT_TRUE(Unregister(callback_registration_));
-  EXPECT_TRUE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_2_));
 }
 
@@ -404,8 +424,8 @@ TEST_F(BackgroundSyncManagerTest, UnregisterBadBackend) {
   // Unregister should have discovered the bad backend and disabled the
   // BackgroundSyncManager.
   manager->set_corrupt_backend(false);
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
-  EXPECT_FALSE(GetRegistration(sync_reg_2_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
+  EXPECT_FALSE(GetRegistration(sync_reg_2_));
 }
 
 TEST_F(BackgroundSyncManagerTest, RegistrationIncreasesId) {
@@ -415,7 +435,7 @@ TEST_F(BackgroundSyncManagerTest, RegistrationIncreasesId) {
   BackgroundSyncManager::BackgroundSyncRegistration::RegistrationId cur_id =
       callback_registration_.id;
 
-  EXPECT_TRUE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_2_));
   EXPECT_LT(cur_id, callback_registration_.id);
   cur_id = callback_registration_.id;
@@ -431,8 +451,8 @@ TEST_F(BackgroundSyncManagerTest, RebootRecovery) {
   background_sync_manager_ =
       BackgroundSyncManager::Create(helper_->context_wrapper());
 
-  EXPECT_TRUE(GetRegistration(sync_reg_1_.tag));
-  EXPECT_FALSE(GetRegistration(sync_reg_2_.tag));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_FALSE(GetRegistration(sync_reg_2_));
 }
 
 TEST_F(BackgroundSyncManagerTest, RebootRecoveryTwoServiceWorkers) {
@@ -442,19 +462,19 @@ TEST_F(BackgroundSyncManagerTest, RebootRecoveryTwoServiceWorkers) {
   background_sync_manager_ =
       BackgroundSyncManager::Create(helper_->context_wrapper());
 
-  EXPECT_TRUE(GetRegistrationWithServiceWorkerId(sw_registration_id_1_,
-                                                 sync_reg_1_.tag));
-  EXPECT_FALSE(GetRegistrationWithServiceWorkerId(sw_registration_id_1_,
-                                                  sync_reg_2_.tag));
-  EXPECT_FALSE(GetRegistrationWithServiceWorkerId(sw_registration_id_2_,
-                                                  sync_reg_1_.tag));
-  EXPECT_TRUE(GetRegistrationWithServiceWorkerId(sw_registration_id_2_,
-                                                 sync_reg_2_.tag));
+  EXPECT_TRUE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_1_, sync_reg_1_));
+  EXPECT_FALSE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_1_, sync_reg_2_));
+  EXPECT_FALSE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_2_, sync_reg_1_));
+  EXPECT_TRUE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_2_, sync_reg_2_));
 
-  EXPECT_TRUE(GetRegistrationWithServiceWorkerId(sw_registration_id_1_,
-                                                 sync_reg_1_.tag));
-  EXPECT_TRUE(GetRegistrationWithServiceWorkerId(sw_registration_id_2_,
-                                                 sync_reg_2_.tag));
+  EXPECT_TRUE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_1_, sync_reg_1_));
+  EXPECT_TRUE(
+      GetRegistrationWithServiceWorkerId(sw_registration_id_2_, sync_reg_2_));
 
   EXPECT_TRUE(RegisterWithServiceWorkerId(sw_registration_id_1_, sync_reg_2_));
   EXPECT_TRUE(RegisterWithServiceWorkerId(sw_registration_id_2_, sync_reg_1_));
@@ -468,7 +488,7 @@ TEST_F(BackgroundSyncManagerTest, InitWithBadBackend) {
   manager->DoInit();
 
   EXPECT_FALSE(Register(sync_reg_1_));
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
@@ -481,7 +501,7 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
   manager->DoInit();
 
   const int64 kExpectedInitialId =
-      BackgroundSyncManager::BackgroundSyncRegistrations::kInitialId;
+      BackgroundSyncManager::BackgroundSyncRegistration::kInitialId;
 
   bool register_called = false;
   bool unregister_called = false;
@@ -491,11 +511,12 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &register_called));
   manager->Unregister(GURL(kOrigin), sw_registration_id_1_, sync_reg_1_.tag,
-                      kExpectedInitialId,
+                      sync_reg_1_.periodicity, kExpectedInitialId,
                       base::Bind(&BackgroundSyncManagerTest::StatusCallback,
                                  base::Unretained(this), &unregister_called));
   manager->GetRegistration(
       GURL(kOrigin), sw_registration_id_1_, sync_reg_1_.tag,
+      sync_reg_1_.periodicity,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &get_registration_called));
 
@@ -533,7 +554,7 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
 TEST_F(BackgroundSyncManagerTest, UnregisterServiceWorker) {
   EXPECT_TRUE(Register(sync_reg_1_));
   UnregisterServiceWorker(sw_registration_id_1_);
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest,
@@ -562,14 +583,14 @@ TEST_F(BackgroundSyncManagerTest,
   EXPECT_EQ(BackgroundSyncManager::ERROR_TYPE_STORAGE, callback_error_);
 
   manager->set_delay_backend(false);
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, DeleteAndStartOverServiceWorkerContext) {
   EXPECT_TRUE(Register(sync_reg_1_));
   helper_->context()->ScheduleDeleteAndStartOver();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterBrowserRestart) {
@@ -584,13 +605,13 @@ TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterBrowserRestart) {
   // The manager is now disabled and not accepting new requests until browser
   // restart or notification that the storage has been wiped.
   manager->set_corrupt_backend(false);
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
   EXPECT_FALSE(Register(sync_reg_2_));
 
   // Simulate restarting the browser by creating a new BackgroundSyncManager.
   background_sync_manager_.reset(
       new TestBackgroundSyncManager(helper_->context_wrapper()));
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_1_));
 }
 
@@ -618,8 +639,8 @@ TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterDeleteAndStartOver) {
   EXPECT_TRUE(called);
 
   EXPECT_TRUE(Register(sync_reg_2_));
-  EXPECT_FALSE(GetRegistration(sync_reg_1_.tag));
-  EXPECT_TRUE(GetRegistration(sync_reg_2_.tag));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
+  EXPECT_TRUE(GetRegistration(sync_reg_2_));
 }
 
 TEST_F(BackgroundSyncManagerTest, RegistrationEqualsId) {
@@ -639,11 +660,12 @@ TEST_F(BackgroundSyncManagerTest, RegistrationEqualsTag) {
   EXPECT_FALSE(reg_1.Equals(reg_2));
 }
 
-TEST_F(BackgroundSyncManagerTest, RegistrationEqualsFireOnce) {
+TEST_F(BackgroundSyncManagerTest, RegistrationEqualsPeriodicity) {
   BackgroundSyncManager::BackgroundSyncRegistration reg_1;
   BackgroundSyncManager::BackgroundSyncRegistration reg_2;
   EXPECT_TRUE(reg_1.Equals(reg_2));
-  reg_2.fire_once = !reg_1.fire_once;
+  reg_1.periodicity = SYNC_PERIODIC;
+  reg_2.periodicity = SYNC_ONE_SHOT;
   EXPECT_FALSE(reg_1.Equals(reg_2));
 }
 
@@ -677,7 +699,8 @@ TEST_F(BackgroundSyncManagerTest, StoreAndRetrievePreservesValues) {
   BackgroundSyncManager::BackgroundSyncRegistration reg_1;
   // Set non-default values for each field.
   reg_1.tag = "foo";
-  reg_1.fire_once = !reg_1.fire_once;
+  EXPECT_NE(SYNC_PERIODIC, reg_1.periodicity);
+  reg_1.periodicity = SYNC_PERIODIC;
   reg_1.min_period += 1;
   EXPECT_NE(NETWORK_STATE_ANY, reg_1.network_state);
   reg_1.network_state = NETWORK_STATE_ANY;
@@ -691,8 +714,43 @@ TEST_F(BackgroundSyncManagerTest, StoreAndRetrievePreservesValues) {
   // disk.
   UseTestBackgroundSyncManager();
 
-  EXPECT_TRUE(GetRegistration(reg_1.tag));
+  EXPECT_TRUE(GetRegistration(reg_1));
   EXPECT_TRUE(reg_1.Equals(callback_registration_));
 }
 
+TEST_F(BackgroundSyncManagerTest, EmptyTagSupported) {
+  sync_reg_1_.tag = "a";
+  EXPECT_TRUE(Register(sync_reg_1_));
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_TRUE(sync_reg_1_.Equals(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
+}
+
+TEST_F(BackgroundSyncManagerTest, OverlappingPeriodicAndOneShotTags) {
+  // Registrations with the same tags but different periodicities should not
+  // collide.
+  sync_reg_1_.tag = "";
+  sync_reg_2_.tag = "";
+  sync_reg_1_.periodicity = SYNC_PERIODIC;
+  sync_reg_2_.periodicity = SYNC_ONE_SHOT;
+
+  EXPECT_TRUE(Register(sync_reg_1_));
+  EXPECT_TRUE(Register(sync_reg_2_));
+
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_EQ(SYNC_PERIODIC, callback_registration_.periodicity);
+  EXPECT_TRUE(GetRegistration(sync_reg_2_));
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.periodicity);
+
+  EXPECT_TRUE(GetRegistration(sync_reg_1_));
+  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_FALSE(GetRegistration(sync_reg_1_));
+  EXPECT_TRUE(GetRegistration(sync_reg_2_));
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.periodicity);
+
+  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_FALSE(GetRegistration(sync_reg_2_));
+}
+
 }  // namespace content