BackgroundSyncManager tracks client registrations

content/browser/background_sync/client_background_sync_registration.h

+// Copyright 2015 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 CONTENT_BROWSER_BACKGROUND_SYNC_CLIENT_BACKGROUND_SYNC_REGISTRATION_H_
+#define CONTENT_BROWSER_BACKGROUND_SYNC_CLIENT_BACKGROUND_SYNC_REGISTRATION_H_
+
+#include "base/callback.h"
+#include "content/browser/background_sync/background_sync_registration.h"
+#include "content/browser/background_sync/background_sync_status.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+class BackgroundSyncManager;
+
+// BackgroundSyncRegistrations that are exposed to clients. They call
+// ReleaseRegistration on deletion and provide access to Unregister.
+// ClientBackgroundSyncRegistration objects must be deleted before the
+// BackgroundSyncManager is deleted.
+class CONTENT_EXPORT ClientBackgroundSyncRegistration
+    : public BackgroundSyncRegistration {

[michaeln, 2015/08/19 01:56:01]

This derivation seems a little odd to me.

(I think) the 'client' class acts as a unique handle to an underlying
registration. There can be many handles to the same underlyaing registration.
Each handle has a unique 'id' which is not at all related to the 'id' of the
underlying registration. It's not a kind of registration, it's a layer of
indirection between a client and the registartion.

I asked this elsewhere too, but why does this class get a copy of the underlying
registrations values instead of referring to the RefCounted instance for the
values?

[jkarlin, 2015/08/19 12:47:06]

That's fair. Perhaps it shouldn't be a sub-class of BackgroundSyncRegistration
and should wrap a BackgroundSyncRegistration instead. What do you think?
To prevent ref-counted objects from escaping the BackgroundSyncManager. This way
the BSM can DCHECK at destruction to verify that all of the registrations are
destroyed.

[jkarlin, 2015/08/19 17:24:22]

Ah, I think perhaps you're getting at something like this instead? This does
seem better:

class ClientBackgroundSyncHandle {
  BackgroundSyncRegistrationOptions* options() { return
background_sync_manager_->GetRegistrationOptionsById(reference_id_); }

  ~ClientBackgroundSyncHandle() {
background_sync_manager_->ReleaseRegistration(reference_id_); }

  Unregister() { background_sync_manager_->Unregister(...); }

private:
  int64 reference_id_;
  BackgroundSyncManager* background_sync_manager_;
};

+ public:
+  using StatusCallback = base::Callback<void(BackgroundSyncStatus)>;
+
+  explicit ClientBackgroundSyncRegistration(
+      BackgroundSyncManager* background_sync_manager);
+  ~ClientBackgroundSyncRegistration() override;
+
+  // Unregisters the background sync registration.  Calls |callback|
+  // with BACKGROUND_SYNC_STATUS_OK if it succeeds.
+  void Unregister(int64 service_worker_id, const StatusCallback& callback);

[michaeln, 2015/08/19 01:56:01]

ah, now i see there unregister has gone

[jkarlin, 2015/08/19 12:47:06]

Acknowledged.

+
+  bool IsValid() const override;
+
+ private:
+  // This object must be deleted before the BackgroundSyncManager
+  // is deleted.

[michaeln, 2015/08/19 01:56:01]

It might be worth using weakptrs here since these objects are given to whomever
is calling the public GetRegistration() and to not assume all caller have
deleted them prior to ~BSM.

[jkarlin, 2015/08/19 12:47:06]

There is a DCHECK to verify that clients call it prior to ~BSM. If they call it
later it's a bug.

+  BackgroundSyncManager* background_sync_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientBackgroundSyncRegistration);
+};
+
+}  // namespace
+
+#endif  // CONTENT_BROWSER_BACKGROUND_SYNC_CLIENT_BACKGROUND_SYNC_REGISTRATION_H_