Use WeakBindingSet to manage inspector connections

Use WeakBindingSet to manage inspector connections

The sky inspector service wants to implement a 1:N broadcast interface with whichever
frontends want to connect. This manages the pipes using a WeakBindingSet, which is a set of
bindings owned by the pipe combined with a list of weak pointers to those bindings allowing
sending messages to all connected clients without having to explicitly keep track of
which clients are connected at a particular point in time.

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/f9dd9b84a5945c5d4d296540f5d849fb9505dd0b

[jamesr, 2014-11-12 00:53:11]

jamesr@chromium.org changed reviewers:
+ aa@chromium.org, eseidel@chromium.org

[jamesr, 2014-11-12 00:53:12]

This depends on https://codereview.chromium.org/718473003/

Aaron - I've tried to implement the WeakSet idea with FrontendWeakBinding.  It's
tricky to make this very generic since mojo/public/cpp/ can't depend on base/
for WeakPtr and we don't have a standard library weak pointer type until we can
use C++11 libraries everywhere.  Is this roughly what you had in mind?

[Aaron Boodman, 2014-11-12 02:36:04]

Yup, this is it. Bummer about WeakPtr.

[eseidel, 2014-11-12 02:39:35]

That's definitely better than what we have.  Still some verbiage.

[jamesr, 2014-11-12 20:47:38]

https://codereview.chromium.org/711413005/diff/20001/sky/services/inspector/s...
File sky/services/inspector/server.cc (right):

https://codereview.chromium.org/711413005/diff/20001/sky/services/inspector/s...
sky/services/inspector/server.cc:28: // This provides a binding to an
InspectorFrontend implementation that exposes a
this could be made generic and live somewhere more common that can still depend
on base (maybe mojo/common).  That would make the initial patch bigger but make
it easier to reuse if this pattern is indeed common (and I think it is).  I'll
wait until the prerequisite patch lands and then see about how to approach this.

[jamesr, 2014-11-13 22:49:50]

Eric - PTAL at sky/services/inspector/ and see what you think about the magic
level there.  This is nice in terms of line-count but I'm not entirely sure the
primitive is very usable yet.

[eseidel, 2014-11-14 00:30:16]

As a user, this looks great to me.

https://codereview.chromium.org/711413005/diff/80001/sky/services/inspector/s...
File sky/services/inspector/server.cc (right):

https://codereview.chromium.org/711413005/diff/80001/sky/services/inspector/s...
sky/services/inspector/server.cc:89: frontend_bindings_.CloseAllBindings();
I would just inline this now.

[jamesr, 2014-11-18 23:10:35]

PTAL

[eseidel, 2014-11-18 23:47:50]

lgtm

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
File mojo/common/weak_binding_set.h (right):

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
mojo/common/weak_binding_set.h:33: if (it.get())
Does it not coerce to bool?

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
mojo/common/weak_binding_set.h:53: return p.get() == nullptr;
Seems like a helper functor would be nice here.  &WeakPtr::isNull?

[jamesr, 2014-11-19 00:17:18]

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
File mojo/common/weak_binding_set.h (right):

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
mojo/common/weak_binding_set.h:33: if (it.get())
On 2014/11/18 23:47:49, eseidel wrote:
> Does it not coerce to bool?

WeakPtr is testable, so if (it) should work. will change

https://codereview.chromium.org/711413005/diff/80001/mojo/common/weak_binding...
mojo/common/weak_binding_set.h:53: return p.get() == nullptr;
On 2014/11/18 23:47:50, eseidel wrote:
> Seems like a helper functor would be nice here.  &WeakPtr::isNull?

Yeah - that'd be nice but would require upstream changes.

[jamesr, 2014-11-19 00:20:16]

Committed patchset #5 (id:80001) manually as
f9dd9b84a5945c5d4d296540f5d849fb9505dd0b (presubmit successful).