Mojo: RemotePtr<S> + bindings changes for Peer attribute.

Mojo: RemotePtr<S> + bindings changes for Peer attribute.

This adds an end-to-end test of the system w/ generated code as part of the CL. The generated code can be deleted once we have a GYP rule for generating code.

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

[darin (slow to review), 2013-11-08 08:52:32]

As of patch set #1, what remains is:

 1- Add support for attributes to mojom.py and mojom_data.py
 2- Read the "Peer" attribute in mojom_cpp_generator.py

[DaveMoore, 2013-11-11 18:49:04]

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/gen...
File mojo/public/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/gen...
mojo/public/bindings/generators/mojom_cpp_generator.py:311: (method.name, ",
".join(params), method_postfix))
Since this ends up being a statically defined thing, I find it strange that it's
defined through a dynamic attribute format. Why not express it as a top level
construct. I'd imagine anything defined through a dynamic mechanism to end up as
a dynamic bag of properties that could be interrogated through a C++ api.

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
File mojo/public/bindings/sample/generated/sample_service.h (right):

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
mojo/public/bindings/sample/generated/sample_service.h:119: typedef ServiceProxy
_ProxyType;
Why bother with the "Type" part? Could it just be _Proxy, _Stub and _Peer?

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
File mojo/public/bindings/sample/sample_service.idl (right):

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
mojo/public/bindings/sample/sample_service.idl:29: [Peer=ServiceClient]
It's a bit fragile that you have to specify this pairing in both places instead
of one. It might be better to have an approach that lists pairs
[Peers=ServiceClient,Service]

I don't like that much either though. Maybe instead of putting the attributes on
both interaces we would just list it on one.

Do we expect these to pretty much always be Acks? Maybe the binding should be at
the method level.

interface Service {
  [Ack=ServiceClient::DidFrobinate]
  void Frobinate(Foo foo @0, bool baz @1, handle port @2) @0;
};

interface ServiceClient {
  // No need to declare anything here
  void DidFrobinate(int32 result @0) @0;
};

Going even farther we could drop the second interface declaration and generate
it out of the Ack declaration itself. As a consumer of the interface it might
read really well

[Client=ServiceClient]
interface Service {
  void Frobinate(Foo foo @0, bool baz @1, handle port @2) @0;
  -> void DidFrobinate(int32 result @0) @0;
}

I'm sure there's a better decl format, but I'd rather see us institutionalize
Acks instead of having a large number of our clients cons them up. We could do
something with message ids too.

https://codereview.chromium.org/66353002/diff/250001/mojo/public/tests/math_c...
File mojo/public/tests/math_calculator.idl (right):

https://codereview.chromium.org/66353002/diff/250001/mojo/public/tests/math_c...
mojo/public/tests/math_calculator.idl:10: [Peer=Calculator]
This is a counter example to the Ack syntax I mentioned before (although there
would be nothing to prevent us from allowing the same method to be used as an
ack for multiple calls).

[Ben Goodger (Google), 2013-11-11 19:10:15]

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
File mojo/public/bindings/sample/sample_service.idl (right):

https://codereview.chromium.org/66353002/diff/250001/mojo/public/bindings/sam...
mojo/public/bindings/sample/sample_service.idl:29: [Peer=ServiceClient]
I'm not sure you can guarantee it'd be only ack-type method calls though. Some
calls to Service might result in N calls to ServiceClient.

A counterpoint to that could be that in that case the N calls should be routed
to some other interface that is passed to Service at an opportune moment. This
would limit the use of *Client interfaces to pure acks and the syntax you
propose would work, just not sure how useful that'd be.

[darin (slow to review), 2013-11-11 22:13:32]

Dave and I chatted and decided on sticking with the current IDL syntax
for now. We may revisit this decision in the future. However, instead
of passing the bag of attributes to the generator, I'll make the mojom
Interface type have a peer property. The parser will be responsible for
setting this property.

[darin (slow to review), 2013-11-12 00:48:41]

On 2013/11/11 22:13:32, darin wrote:
> Dave and I chatted and decided on sticking with the current IDL syntax
> for now. We may revisit this decision in the future. However, instead
> of passing the bag of attributes to the generator, I'll make the mojom
> Interface type have a peer property. The parser will be responsible for
> setting this property.

This is ready for review again. Thanks!

[darin (slow to review), 2013-11-12 00:48:43]

On 2013/11/11 22:13:32, darin wrote:
> Dave and I chatted and decided on sticking with the current IDL syntax
> for now. We may revisit this decision in the future. However, instead
> of passing the bag of attributes to the generator, I'll make the mojom
> Interface type have a peer property. The parser will be responsible for
> setting this property.

This is ready for review. Thanks!

[DaveMoore, 2013-11-12 01:15:37]

https://codereview.chromium.org/66353002/diff/360001/mojo/public/bindings/sam...
File mojo/public/bindings/sample/generated/sample_service.h (right):

https://codereview.chromium.org/66353002/diff/360001/mojo/public/bindings/sam...
mojo/public/bindings/sample/generated/sample_service.h:119: typedef ServiceProxy
_ProxyType;
In interface_declaration the "Type" has been dropped from the type decls.

[darin (slow to review), 2013-11-12 01:33:45]

Oops, will fix. Does the rest look alright? Thanks!
On Nov 11, 2013 5:15 PM, <davemoore@chromium.org> wrote:

>
> https://codereview.chromium.org/66353002/diff/360001/mojo/
> public/bindings/sample/generated/sample_service.h
> File mojo/public/bindings/sample/generated/sample_service.h (right):
>
> https://codereview.chromium.org/66353002/diff/360001/mojo/
> public/bindings/sample/generated/sample_service.h#newcode119
> mojo/public/bindings/sample/generated/sample_service.h:119: typedef
> ServiceProxy _ProxyType;
> In interface_declaration the "Type" has been dropped from the type
> decls.
>
> https://codereview.chromium.org/66353002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2013-11-12 18:54:54]

LGTM if the Type thing is fixed

[commit-bot: I haz the power, 2013-11-12 20:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/66353002/470001

[commit-bot: I haz the power, 2013-11-12 23:40:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/66353002/470001

[commit-bot: I haz the power, 2013-11-13 00:44:50]

Change committed as 234681