Mojo: Implement support for |Foo&| mojom syntax

Mojo: Implement support for |Foo&| mojom syntax

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

[darin (slow to review), 2014-05-14 06:59:53]

[This is not a formal review request.]

Here is initial support for our-interfaces using "request" as the term:
InterfaceRequest<Interface> supports is_pending() and Complete() methods. Take a
look at the design and how it changes code. Let me know what you think of the
names, etc.

[DaveMoore, 2014-05-14 18:05:56]

There's something that seems really wrong about

Bar::GetFoo(InterfaceRequest<Foo> foo);

FooPtr foo;
bar->GetFoo(GetPtr(&foo))

GetPtr() called on a Ptr, passed to something that takes an InterfaceRequest<>
is surprising.

As a client I would expect to be able to write either:

bar->GetFoo(&foo);

or 

bar->GetFoo(MakeRequest(foo));

or

bar->GetFoo(foo->request());

another option might be:

FooRequest foo_request;
bar->GetFoo(&foo_request);
foo = foo_request.foo();

That might read better if Request->Promise, not sure.

Is there anything we could do to get bar->GetFoo(&foo)?

[darin (slow to review), 2014-05-14 20:03:28]

Yeah... do you have any comments on the other parts of the API? What about
the is_pending and Complete methods? Look at the implementation of
CreateGLES2Context, which really takes advantage of all of these methods.

-Darin


On Wed, May 14, 2014 at 11:05 AM, <davemoore@chromium.org> wrote:

> There's something that seems really wrong about
>
> Bar::GetFoo(InterfaceRequest<Foo> foo);
>
> FooPtr foo;
> bar->GetFoo(GetPtr(&foo))
>
> GetPtr() called on a Ptr, passed to something that takes an
> InterfaceRequest<>
> is surprising.
>
> As a client I would expect to be able to write either:
>
> bar->GetFoo(&foo);
>
> or
>
> bar->GetFoo(MakeRequest(foo));
>
> or
>
> bar->GetFoo(foo->request());
>
> another option might be:
>
> FooRequest foo_request;
> bar->GetFoo(&foo_request);
> foo = foo_request.foo();
>
> That might read better if Request->Promise, not sure.
>
> Is there anything we could do to get bar->GetFoo(&foo)?
>
> https://codereview.chromium.org/272323003/
>

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

[darin (slow to review), 2014-05-17 04:28:59]

OK, I decided on the following API:

Client-side:

  BarPtr bar;
  foo->CreateBar(Get(&bar));

Server-side:

  void FooImpl::CreateBar(InterfaceRequest<Bar> bar) {
    BindToRequest(new BarImpl(), &bar);
  }

I'm a fan of the short and sweet "Get" for the client side. We won't even need
the mojo:: bit thanks to C++'s argument-dependent lookup feature. Also, "Get"
feels like a request, and Get has the immediate side-effect of actually getting
you an object you can start calling methods on. Hopefully, this will be
something people become familiar enough with to not find it too surprising.

[DaveMoore, 2014-05-19 23:07:58]

On 2014/05/17 04:28:59, darin wrote:
> OK, I decided on the following API:
> 
> Client-side:
> 
>   BarPtr bar;
>   foo->CreateBar(Get(&bar));
> 
> Server-side:
> 
>   void FooImpl::CreateBar(InterfaceRequest<Bar> bar) {
>     BindToRequest(new BarImpl(), &bar);
>   }
> 
> I'm a fan of the short and sweet "Get" for the client side. We won't even need
> the mojo:: bit thanks to C++'s argument-dependent lookup feature. Also, "Get"
> feels like a request, and Get has the immediate side-effect of actually
getting
> you an object you can start calling methods on. Hopefully, this will be
> something people become familiar enough with to not find it too surprising.

Get() seems too vague to me. Get what?
Maybe GetRequest() instead? That seems more consistent with the arg type of
CreateBar().

[darin (slow to review), 2014-05-19 23:15:58]

Get(...) the thing inside the parenthesis. It reads like a sentence.

-Darin


On Mon, May 19, 2014 at 4:07 PM, <davemoore@chromium.org> wrote:

> On 2014/05/17 04:28:59, darin wrote:
>
>> OK, I decided on the following API:
>>
>
>  Client-side:
>>
>
>     BarPtr bar;
>>    foo->CreateBar(Get(&bar));
>>
>
>  Server-side:
>>
>
>     void FooImpl::CreateBar(InterfaceRequest<Bar> bar) {
>>      BindToRequest(new BarImpl(), &bar);
>>    }
>>
>
>  I'm a fan of the short and sweet "Get" for the client side. We won't even
>> need
>> the mojo:: bit thanks to C++'s argument-dependent lookup feature. Also,
>> "Get"
>> feels like a request, and Get has the immediate side-effect of actually
>>
> getting
>
>> you an object you can start calling methods on. Hopefully, this will be
>> something people become familiar enough with to not find it too
>> surprising.
>>
>
> Get() seems too vague to me. Get what?
> Maybe GetRequest() instead? That seems more consistent with the arg type of
> CreateBar().
>
> https://codereview.chromium.org/272323003/
>

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

[viettrungluu, 2014-05-22 23:26:55]

I'm okay with Get(), so LGTM. I don't think GetRequest() actually makes anything
clearer. It might be one of those things we just have to get used to.

[darin (slow to review), 2014-05-22 23:36:02]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-22 23:37:24]

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

[commit-bot: I haz the power, 2014-05-23 01:15:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8376)

[commit-bot: I haz the power, 2014-05-23 01:19:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 01:19:33]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[darin (slow to review), 2014-05-23 03:37:57]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-23 03:38:32]

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

[commit-bot: I haz the power, 2014-05-23 07:05:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 07:29:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 07:29:29]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[darin (slow to review), 2014-05-24 16:45:17]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-24 23:19:48]

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

[commit-bot: I haz the power, 2014-05-25 01:55:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-25 02:00:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-25 02:00:52]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[darin (slow to review), 2014-05-29 22:00:08]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-29 22:01:55]

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

[commit-bot: I haz the power, 2014-05-30 02:44:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 05:22:31]

Change committed as 273717