Add support for dispatch contexts on BindingSet

mojo/public/cpp/bindings/binding_set.h

Index: mojo/public/cpp/bindings/binding_set.h
diff --git a/mojo/public/cpp/bindings/binding_set.h b/mojo/public/cpp/bindings/binding_set.h
index b1baca6a808dc3e6e637bbd408e7f424dfae828d..6add930800c96ee89f417349b079138ec4714e3d 100644
--- a/mojo/public/cpp/bindings/binding_set.h
+++ b/mojo/public/cpp/bindings/binding_set.h
@@ -5,107 +5,138 @@
 #ifndef MOJO_PUBLIC_CPP_BINDINGS_BINDING_SET_H_
 #define MOJO_PUBLIC_CPP_BINDINGS_BINDING_SET_H_
 
-#include <algorithm>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/interface_ptr.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/bindings/message.h"
 
 namespace mojo {
 
+template <typename BindingType>
+struct BindingSetTraits;
+
+template <typename Interface>
+struct BindingSetTraits<Binding<Interface>> {
+  using ProxyType = InterfacePtr<Interface>;
+  using RequestType = InterfaceRequest<Interface>;
+
+  static RequestType GetProxy(ProxyType* proxy) {
+    return mojo::GetProxy(proxy);
+  }
+};
+
 // Use this class to manage a set of bindings, which are automatically destroyed
 // and removed from the set when the pipe they are bound to is disconnected.
-template <typename Interface>
+template <typename Interface, typename BindingType = Binding<Interface>>
 class BindingSet {
  public:
+  using Traits = BindingSetTraits<BindingType>;
+  using ProxyType = typename Traits::ProxyType;
+  using RequestType = typename Traits::RequestType;
+
   BindingSet() {}
-  ~BindingSet() { CloseAllBindings(); }
 
   void set_connection_error_handler(const base::Closure& error_handler) {
     error_handler_ = error_handler;
   }
 
-  void AddBinding(Interface* impl, InterfaceRequest<Interface> request) {
-    auto binding = new Element(impl, std::move(request));
-    binding->set_connection_error_handler(
-        base::Bind(&BindingSet::OnConnectionError, base::Unretained(this)));
-    bindings_.push_back(binding->GetWeakPtr());
+  void AddBinding(Interface* impl,
+                  RequestType request,
+                  void* context = nullptr) {
+    std::unique_ptr<Entry> entry =
+        base::MakeUnique<Entry>(impl, std::move(request), this, context);
+    bindings_.insert(std::make_pair(entry.get(), std::move(entry)));
   }
 
-  // Returns an InterfacePtr bound to one end of a pipe whose other end is
-  // bound to |this|.
-  InterfacePtr<Interface> CreateInterfacePtrAndBind(Interface* impl) {
-    InterfacePtr<Interface> interface_ptr;
-    AddBinding(impl, GetProxy(&interface_ptr));
-    return interface_ptr;
+  // Returns a proxy bound to one end of a pipe whose other end is bound to
+  // |this|.
+  ProxyType CreateInterfacePtrAndBind(Interface* impl) {
+    ProxyType proxy;
+    AddBinding(impl, Traits::GetProxy(&proxy));

[yzshen1, 2016/08/26 16:30:48]

Is it intentional that we don't provide a similar method for
AssociatedBindingSet?

[Ken Rockot(use gerrit already), 2016/08/26 17:01:20]

It's not possible to implement since we wouldn't know what group to associate
with. In practice, all bindings in an AssociatedBindingSet will likely belong to
the same group, but that isn't something we know for sure. There's no link to a
master interface.

We could add a version of CreateInterfacePtrAndBind which takes an
AssociatedGroup and calls a different GetProxy signature, but I'm not sure I
like CreateInterfacePtrAndBind anyway. WDYT?

[yzshen1, 2016/08/26 18:11:28]

Right. I don't think it is that important to support this method for associated
bindings.

+    return proxy;
   }
 
-  void CloseAllBindings() {
-    for (const auto& it : bindings_) {
-      if (it) {
-        it->Close();
-        delete it.get();
-      }
-    }
-    bindings_.clear();
-  }
+  void CloseAllBindings() {}

[yzshen1, 2016/08/26 16:30:48]

Why this is a non-op?

[Ken Rockot(use gerrit already), 2016/08/26 17:01:20]

Oops :>  Fixed

 
   bool empty() const { return bindings_.empty(); }
 
+  // Implementations may call this when processing a dispatched message. During
+  // the extent of a message dispatch (or connection error handler invocation),
+  // this will return the context associated with the binding which received
+  // the message (or error.) Use AddBinding() to associated a context with a
+  // specific binding.
+  void* dispatch_context() const { return dispatch_context_; }
+
  private:
-  class Element {
+  friend class Entry;
+
+  class Entry {
    public:
-    Element(Interface* impl, InterfaceRequest<Interface> request)
-        : binding_(impl, std::move(request)), weak_ptr_factory_(this) {
-      binding_.set_connection_error_handler(
-          base::Bind(&Element::OnConnectionError, base::Unretained(this)));
+    Entry(Interface* impl,
+          RequestType request,
+          BindingSet* binding_set,
+          void* context)
+        : binding_(impl, std::move(request)),
+          binding_set_(binding_set),
+          context_(context) {
+      binding_.AddFilter(base::MakeUnique<DispatchFilter>(this));

[yzshen1, 2016/08/26 16:30:48]

Do we expect that setting context is a common operation for BindingSet? If not,
maybe we could consider a mode of BindingSet which skips adding such filter.
This filter is one more layer of method call (which won't be inlined) for each
message.

What do you think?

[Ken Rockot(use gerrit already), 2016/08/26 17:01:20]

Seems reasonable. I've added a mojo::BindingSetDispatchMode enum and a
BindingSet constructor which takes takes it. Default constructor does
WITHOUT_CONTEXT.

+      binding_.set_connection_error_handler(base::Bind(
+          &Entry::OnConnectionError, base::Unretained(this)));
     }
 
-    ~Element() {}
+   private:
+    class DispatchFilter : public MessageReceiver {
+     public:
+      explicit DispatchFilter(Entry* entry) : entry_(entry) {}
+      ~DispatchFilter() override {}
+
+     private:
+      // MessageReceiver:
+      bool Accept(Message* message) override {
+        entry_->WillDispatch();
+        return true;
+      }
+
+      Entry* entry_;
 
-    void set_connection_error_handler(const base::Closure& error_handler) {
-      error_handler_ = error_handler;
-    }
+      DISALLOW_COPY_AND_ASSIGN(DispatchFilter);
+    };
 
-    base::WeakPtr<Element> GetWeakPtr() {
-      return weak_ptr_factory_.GetWeakPtr();
+    void WillDispatch() {
+      binding_set_->set_dispatch_context(context_);
     }
 
-    void Close() { binding_.Close(); }
-
     void OnConnectionError() {
-      base::Closure error_handler = error_handler_;
-      delete this;
-      if (!error_handler.is_null())
-        error_handler.Run();
+      WillDispatch();
+      binding_set_->OnConnectionError(this);
     }
 
-   private:
-    Binding<Interface> binding_;
-    base::Closure error_handler_;
-    base::WeakPtrFactory<Element> weak_ptr_factory_;
+    BindingType binding_;
+    BindingSet* const binding_set_;
+    void* const context_;
 
-    DISALLOW_COPY_AND_ASSIGN(Element);
+    DISALLOW_COPY_AND_ASSIGN(Entry);
   };
 
-  void OnConnectionError() {
-    // Clear any deleted bindings.
-    bindings_.erase(std::remove_if(bindings_.begin(), bindings_.end(),
-                                   [](const base::WeakPtr<Element>& p) {
-                                     return p.get() == nullptr;
-                                   }),
-                    bindings_.end());
+  void set_dispatch_context(void* context) { dispatch_context_ = context; }
+
+  void OnConnectionError(Entry* entry) {
+    auto it = bindings_.find(entry);
+    DCHECK(it != bindings_.end());
+    bindings_.erase(it);
 
     if (!error_handler_.is_null())
       error_handler_.Run();
   }
 
   base::Closure error_handler_;
-  std::vector<base::WeakPtr<Element>> bindings_;
+  std::map<Entry*, std::unique_ptr<Entry>> bindings_;
+  void* dispatch_context_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(BindingSet);
 };