Remove [Client=] annotation from ServiceProvider

mojo/services/view_manager/public/interfaces/view_manager.mojom

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 module mojo;
 
 import "geometry/public/interfaces/geometry.mojom";
 import "input_events/public/interfaces/input_events.mojom";
 import "mojo/public/interfaces/application/service_provider.mojom";
 import "native_viewport/public/interfaces/native_viewport.mojom";
@@ skipping 106 matching lines @@
   // OnViewDeleted().
   //
   // The embedder can detect when the embedded app disconnects by way of
   // OnEmbeddedAppDisconnected().
   //
   // When a connection embeds an app the connection no longer has priviledges
   // to access or see any of the children of the view. If the view had existing
   // children the children are removed. The one exception is the root
   // connection.
   //
-  // |service_provider| encapsulates services offered by the embedder to the
-  // embedded app alongside this Embed() call. It also provides a means for
-  // the embedder to connect to services symmetrically exposed by the embedded
-  // app. Note that if a different app is subsequently embedded at |view_id|
-  // the |service_provider|'s connection to its client in the embedded app and
-  // any services it provided are not broken and continue to be valid.
+  // |services| encapsulates services offered by the embedder to the embedded

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

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.

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

Done.

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

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?

+  // app alongside this Embed() call. |exposed_services| provides a means for
+  // the embedder to connect to services exposed by the embedded app. Note that
+  // if a different app is subsequently embedded at |view_id| the
+  // ServiceProvider connections to its client in the embedded app and any
+  // services it provided are not broken and continue to be valid.
   Embed(string url,
         uint32 view_id,
-        ServiceProvider& service_provider) => (bool success);
+        ServiceProvider&? services,

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

We recently made service_provider required. Could you make the pair required?

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

That was a mistake
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.

+        ServiceProvider? exposed_services) => (bool success);
 };
 
 // Changes to views are not sent to the connection that originated the
 // change. For example, if connection 1 changes the bounds of a view by calling
 // SetBounds(), connection 1 does not receive OnViewBoundsChanged().
 [Client=ViewManagerService]
 interface ViewManagerClient {
   // Invoked when the client application has been embedded at |root|.
   // See Embed() on ViewManagerService for more details. |window_manager_pipe|
   // is a pipe to the WindowManager.
   OnEmbed(uint16 connection_id,
           string embedder_url,
           ViewData root,
-          ServiceProvider& parent_service_provider,
+          ServiceProvider&? parent_services,
+          ServiceProvider? parent_exposed_services,
           handle<message_pipe> window_manager_pipe);
 
   // Invoked when the application embedded at |view| is disconnected.
   OnEmbeddedAppDisconnected(uint32 view);
 
   // Invoked when a view's bounds have changed.
   OnViewBoundsChanged(uint32 view,
                       mojo.Rect old_bounds,
                       mojo.Rect new_bounds);
 
@@ skipping 31 matching lines @@
   // NOTE: This is not invoked if OnViewVisibilityChanged() is invoked.
   OnViewDrawnStateChanged(uint32 view, bool drawn);
 
   // Invoked when a view property is changed. If this change is a removal,
   // |new_data| is null.
   OnViewSharedPropertyChanged(uint32 view, string name, array<uint8>? new_data);
 
   // Invoked when an event is targeted at the specified view.
   OnViewInputEvent(uint32 view, mojo.Event event) => ();
 };