arc: Provide a per-service getter for ArcServiceManager

components/arc/arc_service_manager.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 COMPONENTS_ARC_ARC_SERVICE_MANAGER_H_
 #define COMPONENTS_ARC_ARC_SERVICE_MANAGER_H_
 
 #include <memory>
+#include <string>
+#include <type_traits>
+#include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
 #include "base/task_runner.h"
 #include "base/threading/thread_checker.h"
 #include "components/arc/intent_helper/activity_icon_loader.h"
 #include "components/arc/intent_helper/local_activity_resolver.h"
 
 namespace arc {
 
 class ArcBridgeService;
 class ArcIntentHelperObserver;
 class ArcService;
 
+namespace internal {
+
+// If an ArcService is declared with a Name, e.g.:
+//
+// class MyArcService : public ArcService {
+//  public:
+//   static const char Name[];

[Yusuke Sato, 2017/01/10 07:11:11]

The variable name 'Name' doesn't seem to follow the coding style (it looks like
a type while it's actually not.)  Also, since ARC service classes tend to
implement multiple interfaces from other namespaces like ash, adding a prefix to
the special member seems to be a safer choice.

Maybe 'static const char kArcServiceName[];' or something like that?

[Luis Héctor Chávez, 2017/01/10 18:41:55]

Done.

+//   ...
+// };
+//
+// it can then be retrieved from ArcServiceManager in a type-safe way using
+// Get<T>(). This struct allows the use of std::enable_if<...> to choose the
+// correct implementation of the AddService() functions to enable this
+// functionality.
+template <typename T>

[hidehiko, 2017/01/10 08:03:12]

Optional: How about minimizing template magic? E.g.;

// GetName(arc_service) returns its corresponding kArcServiceName if exists, or
empty string.

// If T::kArcServiceName exists, this is used.
template<typename T>
auto GetName(T* unused) -> decltype(T::kArcServiceName, std::string()) {
  return T::kArcServiceName;
}

// Otherwise, this is used.
std::string GetName(...) {
  return std::string();
}

So, in AddService will be;

template<typename T>
void AddService(std::unique_ptr<T> service) {
  std::string name = GetName(service.get());
  AddServiceInternal(name, std::move(service));
}

template<typename T>
T* GetService() {
  // Let compiler report an error for GetService<T>
  // if T does not have kArcServiceName.
  return GetServiceInternal(T::kArcServiceName);
}

in .cc;

void AddServiceInternal(const std::string& name,
                        std::unqiue_ptr<ArcService> service) {
  if (name.empty()) {
    services_.push_back(std::move(service));
  } else {
    named_services_.emplace(name, std::move(service));
  }
}

void GetServiceInternal(const std::string& name) {
  ...
}

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Simplified the first function a bit, but done.

+struct NamedServiceTraits {
+ private:
+  typedef char No[1];

[Yusuke Sato, 2017/01/10 07:11:11]

nit: do we have a reason to skip 0? No can be [0] and Yes can be [1].

[hidehiko, 2017/01/10 08:03:12]

FYI: 0 length is illegal, IIUC.
http://stackoverflow.com/questions/6180012/array-with-size-0

Anyway, let's use std::true_type and std::false_type instead.

[Luis Héctor Chávez, 2017/01/10 18:41:56]

This is a commonly-used Method Detector idiom:
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector, but the
alternative seems to work, so I'll use it.

[hidehiko, 2017/01/11 05:17:45]

Yes, I know, but it was developed in C++03 era, and for this specific case, I
thought we can redux the code a bit. Thank you for cleaning up!

+  typedef char Yes[2];
+
+  template <typename U>
+  static Yes& test(decltype(U::Name) *);

[Yusuke Sato, 2017/01/10 07:11:11]

nit: no space before *, maybe?

[hidehiko, 2017/01/10 08:03:12]

Optional: Even in this way, trailing return type can be used.

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Acknowledged.

+
+  template <typename U>
+  static No& test(...);
+
+ public:
+  enum : bool { HasName = sizeof(test<T>(nullptr)) == sizeof(Yes) };

[hidehiko, 2017/01/10 08:03:12]

With std::{true,false}_type,

constexpr bool kValue = decltype(test<T>(nullptr))::value;

looks simpler?

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Acknowledged.

+};
+
+}  // namespace internal
+
 // Manages creation and destruction of services that communicate with the ARC
 // instance via the ArcBridgeService.
 class ArcServiceManager {
  public:
   class Observer {
    public:
     // Called when ArcServiceManager is being shut down. Observer
     // implementation should clean up ARC related stuff here. One of the
     // typical use cases is calling ArcServiceManager::RemoveObserver().
     virtual void OnArcShutdown() = 0;
 
     // Called when intent filters are added or removed.
     virtual void OnIntentFiltersUpdated() = 0;
 
    protected:
     virtual ~Observer() = default;
   };
 
   explicit ArcServiceManager(
       scoped_refptr<base::TaskRunner> blocking_task_runner);
   ~ArcServiceManager();
 
   // |arc_bridge_service| can only be accessed on the thread that this
   // class was created on.
   ArcBridgeService* arc_bridge_service();
 
   // Adds a service to the managed services list.
-  void AddService(std::unique_ptr<ArcService> service);
+  template <typename T,
+            typename std::enable_if<!internal::NamedServiceTraits<T>::HasName,
+                                    int>::type = 0>
+  void AddService(std::unique_ptr<T> service) {
+    AddServiceInternal(std::move(service));
+  }
+
+  // Adds a named service to the managed services list.
+  template <typename T,
+            typename std::enable_if<internal::NamedServiceTraits<T>::HasName,
+                                    int>::type = 0>
+  void AddService(std::unique_ptr<T> service) {
+    AddNamedServiceInternal(std::string(T::Name), std::move(service));

[Yusuke Sato, 2017/01/10 07:11:11]

nit: the cast (to string) seems unnecessary

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Done.

+  }
+
+  // Gets the named service from the managed services list.

[Yusuke Sato, 2017/01/10 07:11:11]

For developers who don't want to read the template magic, it's probably better
to explicitly note something like 'You can call GetService<YourArcService>()
only when YourArcService has a static member variable called kArcServiceName[].'

[hidehiko, 2017/01/10 08:03:12]

Also, could you explicitly comment that, this will report a compile error if
YourArcService::kArcServiceName is not declared?

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Done.

+  template <typename T>
+  T* GetService() {

[Yusuke Sato, 2017/01/10 07:11:11]

I think GetService<T>() is easy to test. How about adding some unit tests?

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Done.

+    return static_cast<T*>(GetNamedServiceInternal(T::Name));
+  }
 
   // Gets the global instance of the ARC Service Manager. This can only be
   // called on the thread that this class was created on.
   static ArcServiceManager* Get();
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
   // Called to shut down all ARC services.
   void Shutdown();
@@ skipping 11 matching lines @@
   }
 
   // Returns the IntentHelperObserver instance owned by ArcServiceManager.
   ArcIntentHelperObserver* intent_helper_observer() {
     return intent_helper_observer_.get();
   }
 
  private:
   class IntentHelperObserverImpl;  // implemented in arc_service_manager.cc.
 
+  // Helper methods for AddService and GetService.
+  void AddServiceInternal(std::unique_ptr<ArcService> service);
+  void AddNamedServiceInternal(const std::string& name,
+                               std::unique_ptr<ArcService> service);
+  ArcService* GetNamedServiceInternal(const std::string& name);
+
   base::ThreadChecker thread_checker_;
   scoped_refptr<base::TaskRunner> blocking_task_runner_;
 
   // An object for observing the ArcIntentHelper instance in |services_|.
   std::unique_ptr<ArcIntentHelperObserver> intent_helper_observer_;
 
   std::unique_ptr<ArcBridgeService> arc_bridge_service_;
   std::vector<std::unique_ptr<ArcService>> services_;
+  std::unordered_map<std::string, std::unique_ptr<ArcService>> named_services_;

[hidehiko, 2017/01/10 08:03:12]

Optional: how about mergine services_ and named_services_?

// Unnamed service's name is "" (empty string). All non-empty key must be unique
per service.
unordered_multimap<std::string, std::unique_ptr<ArcService>> services_;

I think, the cost should be ignoreable. WDYT?

[Luis Héctor Chávez, 2017/01/10 18:41:56]

Done.

   scoped_refptr<ActivityIconLoader> icon_loader_;
   scoped_refptr<LocalActivityResolver> activity_resolver_;
 
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcServiceManager);
 };
 
 }  // namespace arc
 
 #endif  // COMPONENTS_ARC_ARC_SERVICE_MANAGER_H_