Introduce internal::ServiceRegistry to prepare for ServiceProvider split.

Introduce internal::ServiceRegistry to prepare for ServiceProvider split.

This is to get ready to split ServiceProvider into Shell and Application. It makes the existing Application no longer subclass internal::ServiceConnectorBase::Owner. Instead it puts that functionality in the new class internal::ServiceRegistry and Application uses that by composition.

The validation in this cl is meant to be consistent with what's currently in the tree. I'll redo the cl that adds the ability to add a service with a specific validator after the split is done.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276859

[DaveMoore, 2014-06-12 15:30:17]

Merge problems

[darin (slow to review), 2014-06-12 16:11:52]

This is a great approach. Just some minor issues:

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application.h (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application.h:74: virtual bool
IsConnectionValid(const mojo::String& service_name,
nit: some documentation for this method would be helpful.

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application.h:95: return
service_registry_.service_provider();
nit: indentation

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application.cc (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application.cc:16: this,
nit: 4 spaces indent from "service_registry_"?

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/service_registry.cc (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.cc:61: if
(!application_->IsConnectionValid(service_name, requestor_url)) {
I see... I think this method should be renamed. It is about asking the
application if it allows the incoming connection. I think it would help to use a
word like "Allow" or "Accept" to make it more clear what is expected.
"AllowIncomingConnection"?

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/service_registry.h (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.h:18: class ServiceRegistry :
public ServiceProvider {
since the ServiceRegistry is a ServiceProvider and has a ServiceProvider, it
might help to add a modifier to the service provider member? should we call that
a "remote service provider" or "peer service provider"?

[DaveMoore, 2014-06-12 16:28:02]

Address review concerns

[DaveMoore, 2014-06-12 16:28:14]

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application.h (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application.h:74: virtual bool
IsConnectionValid(const mojo::String& service_name,
On 2014/06/12 16:11:51, darin wrote:
> nit: some documentation for this method would be helpful.

Done.

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application.h:95: return
service_registry_.service_provider();
On 2014/06/12 16:11:51, darin wrote:
> nit: indentation

Done.

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application.cc (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application.cc:16: this,
On 2014/06/12 16:11:51, darin wrote:
> nit: 4 spaces indent from "service_registry_"?

Done.

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/service_registry.cc (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.cc:61: if
(!application_->IsConnectionValid(service_name, requestor_url)) {
On 2014/06/12 16:11:51, darin wrote:
> I see... I think this method should be renamed. It is about asking the
> application if it allows the incoming connection. I think it would help to use
a
> word like "Allow" or "Accept" to make it more clear what is expected.
> "AllowIncomingConnection"?

Done.

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/service_registry.h (right):

https://codereview.chromium.org/337533002/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.h:18: class ServiceRegistry :
public ServiceProvider {
On 2014/06/12 16:11:51, darin wrote:
> since the ServiceRegistry is a ServiceProvider and has a ServiceProvider, it
> might help to add a modifier to the service provider member? should we call
that
> a "remote service provider" or "peer service provider"?

Done.

[darin (slow to review), 2014-06-12 16:32:22]

LGTM

[DaveMoore, 2014-06-12 16:32:58]

Address review concerns

[DaveMoore, 2014-06-12 16:34:31]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:36:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davemoore@chromium.org/337533002/70001

[viettrungluu, 2014-06-12 18:01:01]

https://codereview.chromium.org/337533002/diff/70001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/service_registry.cc (right):

https://codereview.chromium.org/337533002/diff/70001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.cc:47: if
(name_to_service_connector_.empty())
[I realize that this was just moved from another location, but I just want to
record this thought.]

I don't think RemoveServiceConnector() should have this side effect. Someone
could conceivably do remove then add in succession (which wouldn't work with
this). If they want to kill the registry, then they can do so explicitly.

https://codereview.chromium.org/337533002/diff/70001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/service_registry.cc:71: return
service_connector->ConnectToService(
[Ditto.]

Nit: Extraneous "return".

[commit-bot: I haz the power, 2014-06-13 00:07:38]

Change committed as 276859