Replace base::CallbackList with base::ObserverList in CastConfigDelegate

ash/cast_config_delegate.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 ASH_CAST_CONFIG_DELEGATE_H_
 #define ASH_CAST_CONFIG_DELEGATE_H_
 
 #include <string>
 #include <vector>
 
 #include "ash/ash_export.h"
 #include "base/callback.h"

[achuithb, 2016/01/13 09:21:25]

Do you need this?

[jdufault, 2016/01/13 19:43:26]

Done.

-#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "url/gurl.h"
 
 namespace ash {
 
 // This delegate allows the UI code in ash, e.g. |TrayCastDetailedView|,
 // to access the cast extension.
 class CastConfigDelegate {
@@ skipping 40 matching lines @@
   };
 
   struct ASH_EXPORT ReceiverAndActivity {
     ReceiverAndActivity();
     ~ReceiverAndActivity();
 
     Receiver receiver;
     Activity activity;
   };
 
-  // The key is the receiver id.
   using ReceiversAndActivities = std::vector<ReceiverAndActivity>;
-  using ReceiversAndActivitesCallback =
-      base::Callback<void(const ReceiversAndActivities&)>;
-  using DeviceUpdateSubscription = scoped_ptr<
-      base::CallbackList<void(const ReceiversAndActivities&)>::Subscription>;
+
+  class ASH_EXPORT Observer {
+   public:
+    virtual ~Observer() {}

[achuithb, 2016/01/13 09:21:25]

I think this can be protected?

[jdufault, 2016/01/13 19:43:26]

Needs to be public so the extension API can call OnDevicesUpdated.

[stevenjb, 2016/01/13 20:02:18]

OnDevicesUpdated should be public, but the destructor should be protected.

[achuithb, 2016/01/13 21:08:33]

Yup, sorry I wasn't clear.

+
+    // Invoked whenever there is new receiver or activity information available.
+    virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0;
+  };

[achuithb, 2016/01/13 09:21:25]

DISALLOW_COPY_AND_ASSIGN perhaps?

[jdufault, 2016/01/13 19:43:26]

Doing so also requires defining a default constructor, which means that the
class is mostly boilerplate. I just looked at a couple of other observers and
they do not have DISALLOW_COPY_AND_ASSIGN.

The other observers have a protected dtor, so I've adjusted that.

[stevenjb, 2016/01/13 20:02:18]

It shouldn't. This is the preferred pattern:

class ASH_EXPORT Observer {
  public:
   virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0;

  protected:
   virtual ~Observer() {}

  private:
   DISALLOW_ASSIGN(NetworkConfigurationObserver);
};

[achuithb, 2016/01/13 21:08:33]

+1. Thanks for clarifying, Steven

[jdufault, 2016/01/13 22:02:23]

Done.

 
   virtual ~CastConfigDelegate() {}
 
   // Returns true if cast extension is installed.
   // TODO(jdufault): Remove this function once the CastConfigDelegateChromeos is
   // gone. See crbug.com/551132.
   virtual bool HasCastExtension() const = 0;
 
-  // Adds a listener that will get invoked whenever the receivers or their
-  // associated activites have changed.
-  virtual DeviceUpdateSubscription RegisterDeviceUpdateObserver(
-      const ReceiversAndActivitesCallback& callback) = 0;
+  // Add or remove an observer.

[achuithb, 2016/01/13 09:21:25]

I feel like these administrative functions should be the last functions in this
class since they are the least interesting for readers of this file.

[jdufault, 2016/01/13 19:43:26]

Done.

+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
 
   // Request fresh data from the backend. When the data is available, all
   // registered observers will get called.
   virtual void RequestDeviceRefresh() = 0;
 
   // Cast to a receiver specified by |receiver_id|.
   virtual void CastToReceiver(const std::string& receiver_id) = 0;
 
   // Stop an ongoing cast (this should be a user initiated stop). |activity_id|
   // is the identifier of the activity/route that should be stopped.
   virtual void StopCasting(const std::string& activity_id) = 0;
 
   // Does the device have a settings page?
   // TODO(jdufault): Remove this function once the CastConfigDelegateChromeos is
   // gone. See crbug.com/551132.
   virtual bool HasOptions() const = 0;
 
   // Opens Options page for cast.
   // TODO(jdufault): Remove this function once the CastConfigDelegateChromeos is
   // gone. See crbug.com/551132.
   virtual void LaunchCastOptions() = 0;
 
  private:
   DISALLOW_ASSIGN(CastConfigDelegate);
 };
 
 }  // namespace ash
 
 #endif  // ASH_CAST_CONFIG_DELEGATE_H_