Make it possible to have multiple InspectorBackends

Make it possible to have multiple InspectorBackends

This will make it possible to connect a c++
inspector backend in addition to the JS one.

We lose the feature of running multiple inspector
servers in this patch, but we weren't using that feature
and could easily add it back if we plan to use it.

This patch has a *ton* of boilerplate code due
to crbug.com/431911, hopefully that will be fixed
soon and we can delete all the Impl nonsense.

R=abarth@chromium.org

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

[eseidel, 2014-11-10 19:51:32]

This doesn't actually work, I must have made a typo.

[eseidel, 2014-11-10 21:47:00]

Works now, ptal

[eseidel, 2014-11-11 01:54:21]

Hopefully now ServerImpl won't leak and the pattern is the same between
ServerImpl and InspectorFrontendImpl

[abarth-chromium, 2014-11-11 02:53:57]

lgtm

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

https://codereview.chromium.org/710043004/diff/20001/sky/services/inspector/s...
sky/services/inspector/server.cc:5: #include "base/lazy_instance.h"
I don't think this is needed now.

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/inspector_frontend_impl.cc (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/inspector_frontend_impl.cc:25: delete this;
Should we reference crbug.com/431911 here?

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/inspector_frontend_impl.h (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/inspector_frontend_impl.h:26:
InspectorFrontendImpl(Delegate* delegate);
explicit

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/server.cc (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:5: #include "base/lazy_instance.h"
Is this needed?

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:18: class ServerImpl : public
mojo::InterfaceImpl<InspectorServer> {
Should we move this into server.h and create another file for the application
delegate?

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:26: ServerImpl(Delegate* delegate) :
delegate_(delegate) {
explicit

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:117: {
These lines should be merged in Chromium style.

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:182: // TODO(eseidel): Early-out here if we're
already bound to the right port.
Is this comment still accurate?  The network semantics are different because
we'll close all the outstanding connections currently.

https://codereview.chromium.org/710043004/diff/100001/sky/viewer/services/ins...
File sky/viewer/services/inspector_impl.cc (right):

https://codereview.chromium.org/710043004/diff/100001/sky/viewer/services/ins...
sky/viewer/services/inspector_impl.cc:24: void Ignored() {}
Can you move this to around line 15 and put it in an anonymous namespace?

https://codereview.chromium.org/710043004/diff/100001/sky/viewer/services/ins...
sky/viewer/services/inspector_impl.cc:30: mojo::ServiceProviderPtr
inpector_service_provider;
s/inpector_service_provider/inspector_service_provider/

[Aaron Boodman, 2014-11-11 16:12:49]

aa@chromium.org changed reviewers:
+ aa@chromium.org

[Aaron Boodman, 2014-11-11 16:12:50]

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/i...
File sky/services/inspector/inspector_frontend_impl.cc (right):

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/i...
sky/services/inspector/inspector_frontend_impl.cc:20: client()->OnDisconnect();
The client will find out when this end of the pipe is closed. You don't
typically need a separate message for this.

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

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/s...
sky/services/inspector/server.cc:29: delegate_->SetImpl(this);
If two InspectorServer interfaces are requested, the first one will leak.

In general, I think it should be mojo style to support any number of
connections. Providers shouldn't make assumptions about the number of
connections they will receive.

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/s...
sky/services/inspector/server.cc:44: delete this;
If you are just going to delete yourself on error, then you may as well use
BindToPipe rather than WeakBindToPipe.

https://codereview.chromium.org/710043004/diff/40001/sky/viewer/services/insp...
File sky/viewer/services/inspector_impl.cc (right):

https://codereview.chromium.org/710043004/diff/40001/sky/viewer/services/insp...
sky/viewer/services/inspector_impl.cc:23: void Ignored() {}
We should put a templated version of this in bindings somewhere. Everyone ends
up coding this silly no-op function.

I think it should be possible to do something fancy that deduces the right
signature, so you just do:

inspector->Listen(9898, base::Bind(&mojo::EmptyFunction)).

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/inspector_frontend_impl.cc (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/inspector_frontend_impl.cc:25: delete this;
On 2014/11/11 02:53:57, abarth wrote:
> Should we reference crbug.com/431911 here?

I've been thinking about this.

I know jamesr has something fancier in mind, but here is another take:

Imagine we had base/memory/weak_set.h. It would work sort of like ScopedVector,
except a set, and with WeakPtrs, and when items die, they are removed from the
set (this could be accomplished lazily, as the set is mutated).

Now imagine that we used this to own keep track of InterfaceImpls we created. If
the II dies on its own, then hooray, it's removed from the set. If the parent
object dies first, then we can just iterate through the set and tell all the IIs
to shoot themselves. And of course that could be wrapped up too in some
abstraction.

This doesn't go all the way to solving the problems raised in this CL, but it
seems like a small useful part to me.

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/inspector_frontend_impl.h (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/inspector_frontend_impl.h:17: class InspectorFrontendImpl
: public mojo::InterfaceImpl<InspectorFrontend> {
I find that it makes implementation easier to put these InterfaceImpls in the
same cc file with the class you're forwarding to. For example, you wouldn't need
the delegate interface if you did that.

Anyway that's the convention that has been developing elsewhere.

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/inspector_frontend_impl.h:21: virtual void
Register(InspectorFrontendImpl*) = 0;
You can avoid the Register() function by just having the factory do the
registration on creation. One less boilerplate.

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
File sky/services/inspector/server.cc (right):

https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
sky/services/inspector/server.cc:18: class ServerImpl : public
mojo::InterfaceImpl<InspectorServer> {
On 2014/11/11 02:53:57, abarth wrote:
> Should we move this into server.h and create another file for the application
> delegate?

As I said in other class, I think the opposite.

[eseidel, 2014-11-11 18:15:26]

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/i...
File sky/services/inspector/inspector_frontend_impl.cc (right):

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/i...
sky/services/inspector/inspector_frontend_impl.cc:20: client()->OnDisconnect();
On 2014/11/11 16:12:50, Aaron Boodman wrote:
> The client will find out when this end of the pipe is closed. You don't
> typically need a separate message for this.

I believe this represents that the Web Inspector has disconnected from the
inspector backend, not that other side of this pip should stop talking to the
backend. :)

Chrome Inspector -> InspectorFrontendServer -> Various Agents (implemented in
JS).

This message is the frontend server telling the agents the inspector
disconnected... I think.

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

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/s...
sky/services/inspector/server.cc:29: delegate_->SetImpl(this);
On 2014/11/11 16:12:50, Aaron Boodman wrote:
> If two InspectorServer interfaces are requested, the first one will leak.
> 
> In general, I think it should be mojo style to support any number of
> connections. Providers shouldn't make assumptions about the number of
> connections they will receive.

Done.

https://codereview.chromium.org/710043004/diff/40001/sky/services/inspector/s...
sky/services/inspector/server.cc:44: delete this;
On 2014/11/11 16:12:50, Aaron Boodman wrote:
> If you are just going to delete yourself on error, then you may as well use
> BindToPipe rather than WeakBindToPipe.

I can't, unfortunately, since then I can't hae the server delete the impl while
the pipe is still open.  crbug.com/431911

[eseidel, 2014-11-11 18:23:16]

On 2014/11/11 16:12:50, Aaron Boodman wrote:
>
https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
> sky/services/inspector/inspector_frontend_impl.h:21: virtual void
> Register(InspectorFrontendImpl*) = 0;
> You can avoid the Register() function by just having the factory do the
> registration on creation. One less boilerplate.

How do I tell the server on destruction of the impl to forget about it without
having an unregister?  Use a weakptr to the impls?

>
https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
> File sky/services/inspector/server.cc (right):
> 
>
https://codereview.chromium.org/710043004/diff/100001/sky/services/inspector/...
> sky/services/inspector/server.cc:18: class ServerImpl : public
> mojo::InterfaceImpl<InspectorServer> {
> On 2014/11/11 02:53:57, abarth wrote:
> > Should we move this into server.h and create another file for the
application
> > delegate?
> 
> As I said in other class, I think the opposite.

I've deleted the other class to match your preference. :)  I agree it's nice to
move the impls next to each other especially if we're going to delete them.

[eseidel, 2014-11-11 18:28:29]

Thank you very much Aaron for taking the time to review.  I really appreciate
it!

[eseidel, 2014-11-11 18:29:25]

Committed patchset #7 (id:120001) manually as
efa9b171d1b3d2b7b6643106055c33f33096edce (presubmit successful).