Remove [Client=] annotation from ServiceProvider

Remove [Client=] annotation from ServiceProvider

This removes the symmetrical nature of ServiceProvider and consistently
passes and uses a ServiceProvider + ServiceProvider& pair in places that
wish to bidirectionally expose services, such as the view manager.

The
view manager library now deals with InterfaceRequest<ServiceProvider>
and ServiceProviderPtr objects (i.e. c++ wrappers for handles) instead
of a concrete implementation of ServiceProvider to make it easier for
callers.

A number of places that were assuming a particular
ServiceProvider would always exist are updated to reflect the nullability
of the parameters in mojom and places that do not wish to ever look up
or provide services now pass nullptr instead of doomed pipe handles.

The JS application startup classes are reworked a bit to accomodate
exposing services on the third ConnectToApplication/AcceptConnection
parameter.

BUG=449432
R=abarth@chromium.org, sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3b67967fef6ceb2fd56569f5dbe5b937d2339b05

[jamesr, 2015-01-21 02:02:21]

jamesr@chromium.org changed reviewers:
+ abarth@chromium.org, hansmuller@chromium.org, sky@chromium.org

[jamesr, 2015-01-21 02:02:22]

Hans - please review the mojo/public/.../js changes

Adam - please review the sky/ changes

Scott - please review the view manager client lib changes and the rest (since
the rest is mostly updates for the new view manager interface)

AFAICT everything works now except for the following sky tests:

Regressions: Unexpected text-only failures (7)
  custom-elements/document-register-reentrant-null-constructor.sky [ Failure ]
  custom-elements/document-register-reentrant-throwing-constructor.sky [ Failure
]
  custom-elements/lifecycle-created-createElement-recursion.sky [ Failure ]
  framework/observe.sky [ Failure ]
  inspector/dom-mutation-modify-text-node-and-append.sky [ Failure ]
  parser/script.sky [ Failure ]
  services/network.sky [ Failure ]

which need more investigation but I believe are related to oddness with the way
sky embeds iframes and expects to connect to the internals thingamabob.  I'll
keep investigating tomorrow but I don't expect the fix to this to impact most of
this patch.

I strongly suspect the other language bindings need updates as well, but they
don't have tests in the 'mojob test' suite so we're just gonna have to figure
those out as we go.

[sky, 2015-01-21 16:59:00]

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/cpp/view.h (right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/view.h:128:
InterfaceRequest<ServiceProvider> services,
This is subtle. It's worth documenting the difference between services and
exposed_services.

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:127: //
|services| encapsulates services offered by the embedder to the embedded
The names services and exposed_services are confusing. Additionally you don't
use the names consistently; ViewManagerClientImpl uses parent_services and
parent_exposed_services. Is there better names we can use? Some ideas:

parent_to_client_services and client_to_parent_services: long, but better
conveys the directionality.
embedder_to_embedded_services and embedded_to_embedder_services: longer, and I
think 'embedder' and 'embedded' are so similar that it's easy to confuse the
two.

WDYT?

My vote is for parent_client_services and client_to_parent_services.

No matter what you go with, please use it consistently.

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:135:
ServiceProvider&? services,
We recently made service_provider required. Could you make the pair required?

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
File mojo/services/window_manager/public/interfaces/window_manager.mojom
(right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/public/interfaces/window_manager.mojom:13: // View.
See ViewMangerService::Embed() for details on |services| and |exposed_services|.
nit: > 80

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/public/interfaces/window_manager.mojom:14:
Embed(string url, ServiceProvider&? services, ServiceProvider?
exposed_services);
Again, can we make these not optional?

[jamesr, 2015-01-21 18:51:40]

With Eric's help I figured out what was breaking the sky tests (turns out
connecting to the TestHarness interface signals the start of a test, so it's
important to keep that early in DocumentView startup).  These tests now pass.

PTAL

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/cpp/view.h (right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/cpp/view.h:128:
InterfaceRequest<ServiceProvider> services,
On 2015/01/21 16:59:00, sky wrote:
> This is subtle. It's worth documenting the difference between services and
> exposed_services.

It's the same as in application.mojom and the documentation is in
view_manager.mojom, same as the other view interfaces.

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
File mojo/services/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:127: //
|services| encapsulates services offered by the embedder to the embedded
On 2015/01/21 16:59:00, sky wrote:
> No matter what you go with, please use it consistently.

Done.

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:127: //
|services| encapsulates services offered by the embedder to the embedded
On 2015/01/21 16:59:00, sky wrote:
> The names services and exposed_services are confusing. Additionally you don't
> use the names consistently; 

These are the same that //mojo/public/interfaces/application/application.mojom
uses, which had a lot of debate already.  This is the same thing and I expect
everyone working on mojo will need to be fairly familiar with the concepts and
names.  Do you think there's an inherent difference between this usage and that
one, or do you think the discussion on the application names needs to be
reopened?

> ViewManagerClientImpl uses parent_services and
> parent_exposed_services. Is there better names we can use? Some ideas:
> 
> parent_to_client_services and client_to_parent_services: long, but better
> conveys the directionality.
> embedder_to_embedded_services and embedded_to_embedder_services: longer, and I
> think 'embedder' and 'embedded' are so similar that it's easy to confuse the
> two.
> 
> WDYT?
> 
> My vote is for parent_client_services and client_to_parent_services.
> 
> No matter what you go with, please use it consistently.

https://codereview.chromium.org/858103002/diff/1/mojo/services/view_manager/p...
mojo/services/view_manager/public/interfaces/view_manager.mojom:135:
ServiceProvider&? services,
On 2015/01/21 16:59:00, sky wrote:
> We recently made service_provider required.

That was a mistake

> Could you make the pair required?

No.  In many cases (most cases, in the current codebase) there are no services
to expose in one or both directions.  In this case, allocating a message pipe
and passing a handle is overhead with no benefit.  If you have nothing to
provide or have nothing to request, you can leave these null.  The one-parameter
View::Embed() function is an example of needing neither.

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
File mojo/services/window_manager/public/interfaces/window_manager.mojom
(right):

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/public/interfaces/window_manager.mojom:13: // View.
See ViewMangerService::Embed() for details on |services| and |exposed_services|.
On 2015/01/21 16:59:00, sky wrote:
> nit: > 80

Done.

https://codereview.chromium.org/858103002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/public/interfaces/window_manager.mojom:14:
Embed(string url, ServiceProvider&? services, ServiceProvider?
exposed_services);
On 2015/01/21 16:59:00, sky wrote:
> Again, can we make these not optional?

No

[sky, 2015-01-21 22:41:05]

LGTM

[hansmuller1, 2015-01-22 00:46:38]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-01-22 00:46:38]

The JS changes LGTM. I think The JS ServiceProvider class should be revised or
renamed (ServiceExchange?) given the fact that SPs are now unidirectional. That
change can wait.

[abarth-chromium, 2015-01-22 00:48:24]

abarth@chromium.org changed reviewers:
- hansmuller@google.com

[abarth-chromium, 2015-01-22 00:48:25]

LGTM

You might need to change the code in HTMLIFrameElement.cpp that I just landed.

[jamesr, 2015-01-22 02:36:07]

Committed patchset #3 (id:40001) manually as
3b67967fef6ceb2fd56569f5dbe5b937d2339b05 (presubmit successful).