Add a CallbackRegistry class to base/ to manage callbacks

base/callback_handle.h

 // Copyright 2013 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 "base/power_monitor/power_monitor_device_source.h"
+#ifndef BASE_CALLBACK_HANDLE_H_
+#define BASE_CALLBACK_HANDLE_H_
 
 namespace base {
 
-bool PowerMonitorDeviceSource::IsOnBatteryPowerImpl() {
-  NOTIMPLEMENTED();
-  return false;
-}
+class CallbackHandle {

[awong, 2013/09/06 19:03:24]

Are you expecting to forward declare this class?

If not, I don't see a reason to make it top-level and would just nest it inside
the CallbackRegistryBase.

Also, I think Subscription is a better name.

CallbackHandle implies too many things since Callback<> itself is basically a
ref-counted handle to the result of a Bind().

[Cait (Slow), 2013/09/06 19:18:37]

I think it makes sense to have a top-level base-class here. If I have to store
handles/subscriptions from several different registries (esp. if I don't want to
deregister any of them before my destruction), then it would be easier for them
to all be the same base type so I can stick them in a container, whereas if I
have one CallbackRegistry<A>::Handle, one CallbackRegistry<B>::Handle etc. I
have to hold them all separately.

[awong, 2013/09/06 19:31:02]

Gotcha.

I'd still move it back into callback_registry.h and give it a name that implies
it's
  (a) for use with CallbackRegistry
  (b) has something to do with deregistering, unsubscribing something from the
registry.

As a standalone interface with a name like CallbackHandle, it's too generic.

[Cait (Slow), 2013/09/06 20:18:31]

I don't think moving it into callback_registry.h will work (unless we move
callbackRegistryBase there as well, as CallbackRegistryBase needs to know about
CallbackHandle).

My preference would be to keep CallbackHandle as a separate file, but give it a
better name (CallbackRegistrySubscription? or just Subscription if that's too
long)?

+ public:
+  virtual ~CallbackHandle() {}

[erikwright (departed), 2013/09/06 21:01:07]

Add an '= 0' here to force this class to be derived.

IIRC that requires you to define the method out of line. So you would have:

class CallbackHandle {
 public:
  virtual ~CallbackHandle() = 0;
};

CallbackHandle::~CallbackHandle() {}

It would still be correct, IMHO, to define the destructor in the .h file in this
case.

If you don't do this, you need to mark this class DISALLOW_... and define a
default constructor.

[Bernhard Bauer, 2013/09/06 21:08:58]

That won't work. Every class needs to have a destructor, so you can't make it
pure virtual. If you want to prevent someone from instantiating an object of
this class (as opposed to a subclass), make the constructor protected.

[awong, 2013/09/06 23:49:30]

I might be wrong, but I thought the pattern was to make the destructor pure
virtual while *also* providing the definition.

(Pure virtual doesn't imply not concrete definition...)

[erikwright (departed), 2013/09/09 16:09:38]

You can define it but still mark it pure virtual:

erikwright@argon18:~$ cat test.cc
#include <iostream>

class A {
 public:
 virtual ~A() = 0;
};

A::~A() {
  std::cout << "World." << std::endl;
}

class B : public A {
 public:
  virtual ~B() {
    std::cout << "Hello ";
  }
};

int main(int argc, char** argv) {
  B b;
}
erikwright@argon18:~$ gcc /usr/local/google/home/erikwright/test.cc -lstdc++
erikwright@argon18:~$ ./a.out 
Hello World.
erikwright@argon18:~$ 

This approach is, IMHO, more correct than the protected constructor because the
protected constructor still permits the class itself (for example, via a public
static method, or from an instance method invoked via a derived class) to
instantiate an instance of itself.

With the '= 0' above, it's impossible for anyone, including 'A' to instantiate
A.

[Bernhard Bauer, 2013/09/09 16:40:17]

Oh, wow. TIL :)
Well, yes, but usually it's easy to find and look through all the code in A and
verify that it doesn't do that. Of course, somebody could go ahead later and
change it, but if they can already modify class A, they could simply remove the
`= 0` from the destructor as well ;-)

+};
 
 }  // namespace base
+
+#endif  // BASE_CALLBACK_HANDLE_H_