Adds ScopedHandleBase::From

Adds ScopedHandleBase::From

Intended for use when converting from a ScopedMessagePipeHandle to some Interface, eg:
ScopedXHandle::From(scoped_message_pipe_handle.Pass());

BUG=none
TEST=none
R=darin@chromium.org

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

[vtl, 2014-04-03 14:58:30]

https://codereview.chromium.org/220063005/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/interface.h (right):

https://codereview.chromium.org/220063005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/interface.h:68: //
ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
drive-by nit: message_pip -> message_pipe

[darin (slow to review), 2014-04-03 15:00:21]

How about dropping the Convert prefix? mojo::ToInterface<S>? We could also
add a static From method on ScopedFooHandle.
On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:

> Reviewers: darin,
>
> Description:
> Adds mojo::ConvertToInterface
>
> It converts from a ScopedMessagePipeHandle to an interface. This way
> code that was:
> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
> becomes:
> ConvertToInterface<X>(scoped_message_pipe.Pass());
>
> BUG=none
> TEST=none
> R=darin@chromium.org
>
> Please review this at https://codereview.chromium.org/220063005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+10, -0 lines):
>   M mojo/public/cpp/bindings/interface.h
>
>
> Index: mojo/public/cpp/bindings/interface.h
> diff --git a/mojo/public/cpp/bindings/interface.h
> b/mojo/public/cpp/bindings/interface.h
> index bba8fef89fc685b0f6b46841363450f5cb321009..
> ff6bf36a4267e7669380bb2a97af1ef433fcafbc 100644
> --- a/mojo/public/cpp/bindings/interface.h
> +++ b/mojo/public/cpp/bindings/interface.h
> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>    typedef ScopedMessagePipeHandle ScopedHandle;
>  };
>
> +// Use to convert from a ScopedMessagePipeHandle to an scoped interface
> +// handle. For example,
> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
> +// where OmniboxPage names an interface in a mojom file.
> +template <typename S>
> +typename Interface<S>::ScopedHandle ConvertToInterface(
> +    ScopedMessagePipeHandle message_pipe_handle) {
> +  return Interface<S>::ScopedHandle(
> +      Interface<S>::Handle(message_pipe_handle.release().value()));
> +}
>
>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
> interfaces
>  // on either end.
>
>
>

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

[vtl, 2014-04-03 15:19:04]

On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org> wrote:

> How about dropping the Convert prefix? mojo::ToInterface<S>? We could also
> add a static From method on ScopedFooHandle.
>
I don't think this would work, since ScopedFooHandle is just a typedef of
ScopedHandleBase<FooHandle>. (You might subclass, but that seems dubious to
me, especially given what we do to emulate move semantics -- e.g., Pass()
probably wouldn't be quite right. Possibly, ScopedHandleBase<> should be
marked final.)


> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
>
>> Reviewers: darin,
>>
>> Description:
>> Adds mojo::ConvertToInterface
>>
>> It converts from a ScopedMessagePipeHandle to an interface. This way
>> code that was:
>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
>> becomes:
>> ConvertToInterface<X>(scoped_message_pipe.Pass());
>>
>> BUG=none
>> TEST=none
>> R=darin@chromium.org
>>
>> Please review this at https://codereview.chromium.org/220063005/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files (+10, -0 lines):
>>   M mojo/public/cpp/bindings/interface.h
>>
>>
>> Index: mojo/public/cpp/bindings/interface.h
>> diff --git a/mojo/public/cpp/bindings/interface.h
>> b/mojo/public/cpp/bindings/interface.h
>> index bba8fef89fc685b0f6b46841363450f5cb321009..
>> ff6bf36a4267e7669380bb2a97af1ef433fcafbc 100644
>> --- a/mojo/public/cpp/bindings/interface.h
>> +++ b/mojo/public/cpp/bindings/interface.h
>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>>    typedef ScopedMessagePipeHandle ScopedHandle;
>>  };
>>
>> +// Use to convert from a ScopedMessagePipeHandle to an scoped interface
>> +// handle. For example,
>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
>> +// where OmniboxPage names an interface in a mojom file.
>> +template <typename S>
>> +typename Interface<S>::ScopedHandle ConvertToInterface(
>> +    ScopedMessagePipeHandle message_pipe_handle) {
>> +  return Interface<S>::ScopedHandle(
>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
>> +}
>>
>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
>> interfaces
>>  // on either end.
>>
>>
>>

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-04-03 15:32:14]

Why not put From on ScopedHandleBase?
On Apr 3, 2014 8:19 AM, "Viet-Trung Luu" <vtl@google.com> wrote:

> On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> How about dropping the Convert prefix? mojo::ToInterface<S>? We could
>> also add a static From method on ScopedFooHandle.
>>
> I don't think this would work, since ScopedFooHandle is just a typedef of
> ScopedHandleBase<FooHandle>. (You might subclass, but that seems dubious to
> me, especially given what we do to emulate move semantics -- e.g., Pass()
> probably wouldn't be quite right. Possibly, ScopedHandleBase<> should be
> marked final.)
>
>
>> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
>>
>>> Reviewers: darin,
>>>
>>> Description:
>>> Adds mojo::ConvertToInterface
>>>
>>> It converts from a ScopedMessagePipeHandle to an interface. This way
>>> code that was:
>>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
>>> becomes:
>>> ConvertToInterface<X>(scoped_message_pipe.Pass());
>>>
>>> BUG=none
>>> TEST=none
>>> R=darin@chromium.org
>>>
>>> Please review this at https://codereview.chromium.org/220063005/
>>>
>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>
>>> Affected files (+10, -0 lines):
>>>   M mojo/public/cpp/bindings/interface.h
>>>
>>>
>>> Index: mojo/public/cpp/bindings/interface.h
>>> diff --git a/mojo/public/cpp/bindings/interface.h
>>> b/mojo/public/cpp/bindings/interface.h
>>> index bba8fef89fc685b0f6b46841363450f5cb321009..
>>> ff6bf36a4267e7669380bb2a97af1ef433fcafbc 100644
>>> --- a/mojo/public/cpp/bindings/interface.h
>>> +++ b/mojo/public/cpp/bindings/interface.h
>>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>>>    typedef ScopedMessagePipeHandle ScopedHandle;
>>>  };
>>>
>>> +// Use to convert from a ScopedMessagePipeHandle to an scoped interface
>>> +// handle. For example,
>>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
>>> +// where OmniboxPage names an interface in a mojom file.
>>> +template <typename S>
>>> +typename Interface<S>::ScopedHandle ConvertToInterface(
>>> +    ScopedMessagePipeHandle message_pipe_handle) {
>>> +  return Interface<S>::ScopedHandle(
>>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
>>> +}
>>>
>>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
>>> interfaces
>>>  // on either end.
>>>
>>>
>>>
>

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

[sky, 2014-04-03 15:39:57]

Woudn't that require ScopedHandleBase::From to take a ScopedMessagePipeHandle?

  -Scott

On Thu, Apr 3, 2014 at 8:32 AM, Darin Fisher <darin@chromium.org> wrote:
> Why not put From on ScopedHandleBase?
>
> On Apr 3, 2014 8:19 AM, "Viet-Trung Luu" <vtl@google.com> wrote:
>>
>> On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org> wrote:
>>>
>>> How about dropping the Convert prefix? mojo::ToInterface<S>? We could
>>> also add a static From method on ScopedFooHandle.
>>
>> I don't think this would work, since ScopedFooHandle is just a typedef of
>> ScopedHandleBase<FooHandle>. (You might subclass, but that seems dubious to
>> me, especially given what we do to emulate move semantics -- e.g., Pass()
>> probably wouldn't be quite right. Possibly, ScopedHandleBase<> should be
>> marked final.)
>>
>>>
>>> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
>>>>
>>>> Reviewers: darin,
>>>>
>>>> Description:
>>>> Adds mojo::ConvertToInterface
>>>>
>>>> It converts from a ScopedMessagePipeHandle to an interface. This way
>>>> code that was:
>>>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
>>>> becomes:
>>>> ConvertToInterface<X>(scoped_message_pipe.Pass());
>>>>
>>>> BUG=none
>>>> TEST=none
>>>> R=darin@chromium.org
>>>>
>>>> Please review this at https://codereview.chromium.org/220063005/
>>>>
>>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>>
>>>> Affected files (+10, -0 lines):
>>>>   M mojo/public/cpp/bindings/interface.h
>>>>
>>>>
>>>> Index: mojo/public/cpp/bindings/interface.h
>>>> diff --git a/mojo/public/cpp/bindings/interface.h
>>>> b/mojo/public/cpp/bindings/interface.h
>>>> index
>>>>
bba8fef89fc685b0f6b46841363450f5cb321009..ff6bf36a4267e7669380bb2a97af1ef433fcafbc
>>>> 100644
>>>> --- a/mojo/public/cpp/bindings/interface.h
>>>> +++ b/mojo/public/cpp/bindings/interface.h
>>>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>>>>    typedef ScopedMessagePipeHandle ScopedHandle;
>>>>  };
>>>>
>>>> +// Use to convert from a ScopedMessagePipeHandle to an scoped interface
>>>> +// handle. For example,
>>>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
>>>> +// where OmniboxPage names an interface in a mojom file.
>>>> +template <typename S>
>>>> +typename Interface<S>::ScopedHandle ConvertToInterface(
>>>> +    ScopedMessagePipeHandle message_pipe_handle) {
>>>> +  return Interface<S>::ScopedHandle(
>>>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
>>>> +}
>>>>
>>>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
>>>> interfaces
>>>>  // on either end.
>>>>
>>>>
>>
>

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

[sky, 2014-04-03 15:40:16]

I updated the patch (renamed to ToInterface and fixed spelling).

On Thu, Apr 3, 2014 at 8:39 AM, Scott Violet <sky@chromium.org> wrote:
> Woudn't that require ScopedHandleBase::From to take a
ScopedMessagePipeHandle?
>
>   -Scott
>
> On Thu, Apr 3, 2014 at 8:32 AM, Darin Fisher <darin@chromium.org> wrote:
>> Why not put From on ScopedHandleBase?
>>
>> On Apr 3, 2014 8:19 AM, "Viet-Trung Luu" <vtl@google.com> wrote:
>>>
>>> On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org> wrote:
>>>>
>>>> How about dropping the Convert prefix? mojo::ToInterface<S>? We could
>>>> also add a static From method on ScopedFooHandle.
>>>
>>> I don't think this would work, since ScopedFooHandle is just a typedef of
>>> ScopedHandleBase<FooHandle>. (You might subclass, but that seems dubious to
>>> me, especially given what we do to emulate move semantics -- e.g., Pass()
>>> probably wouldn't be quite right. Possibly, ScopedHandleBase<> should be
>>> marked final.)
>>>
>>>>
>>>> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
>>>>>
>>>>> Reviewers: darin,
>>>>>
>>>>> Description:
>>>>> Adds mojo::ConvertToInterface
>>>>>
>>>>> It converts from a ScopedMessagePipeHandle to an interface. This way
>>>>> code that was:
>>>>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
>>>>> becomes:
>>>>> ConvertToInterface<X>(scoped_message_pipe.Pass());
>>>>>
>>>>> BUG=none
>>>>> TEST=none
>>>>> R=darin@chromium.org
>>>>>
>>>>> Please review this at https://codereview.chromium.org/220063005/
>>>>>
>>>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>>>
>>>>> Affected files (+10, -0 lines):
>>>>>   M mojo/public/cpp/bindings/interface.h
>>>>>
>>>>>
>>>>> Index: mojo/public/cpp/bindings/interface.h
>>>>> diff --git a/mojo/public/cpp/bindings/interface.h
>>>>> b/mojo/public/cpp/bindings/interface.h
>>>>> index
>>>>>
bba8fef89fc685b0f6b46841363450f5cb321009..ff6bf36a4267e7669380bb2a97af1ef433fcafbc
>>>>> 100644
>>>>> --- a/mojo/public/cpp/bindings/interface.h
>>>>> +++ b/mojo/public/cpp/bindings/interface.h
>>>>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>>>>>    typedef ScopedMessagePipeHandle ScopedHandle;
>>>>>  };
>>>>>
>>>>> +// Use to convert from a ScopedMessagePipeHandle to an scoped interface
>>>>> +// handle. For example,
>>>>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
>>>>> +// where OmniboxPage names an interface in a mojom file.
>>>>> +template <typename S>
>>>>> +typename Interface<S>::ScopedHandle ConvertToInterface(
>>>>> +    ScopedMessagePipeHandle message_pipe_handle) {
>>>>> +  return Interface<S>::ScopedHandle(
>>>>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
>>>>> +}
>>>>>
>>>>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
>>>>> interfaces
>>>>>  // on either end.
>>>>>
>>>>>
>>>
>>

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

[vtl, 2014-04-03 15:43:45]

I think Darin means:

template <FromHandleType>
static ScopedHandleBase<HandleType> From(ScopedHandleBase<FromHandleType>
from) { ... }

I thought that I tried that and it didn't work for some reason, but I might
be wrong.


On Thu, Apr 3, 2014 at 8:39 AM, Scott Violet <sky@chromium.org> wrote:

> Woudn't that require ScopedHandleBase::From to take a
> ScopedMessagePipeHandle?
>
>   -Scott
>
> On Thu, Apr 3, 2014 at 8:32 AM, Darin Fisher <darin@chromium.org> wrote:
> > Why not put From on ScopedHandleBase?
> >
> > On Apr 3, 2014 8:19 AM, "Viet-Trung Luu" <vtl@google.com> wrote:
> >>
> >> On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org>
> wrote:
> >>>
> >>> How about dropping the Convert prefix? mojo::ToInterface<S>? We could
> >>> also add a static From method on ScopedFooHandle.
> >>
> >> I don't think this would work, since ScopedFooHandle is just a typedef
> of
> >> ScopedHandleBase<FooHandle>. (You might subclass, but that seems
> dubious to
> >> me, especially given what we do to emulate move semantics -- e.g.,
> Pass()
> >> probably wouldn't be quite right. Possibly, ScopedHandleBase<> should be
> >> marked final.)
> >>
> >>>
> >>> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
> >>>>
> >>>> Reviewers: darin,
> >>>>
> >>>> Description:
> >>>> Adds mojo::ConvertToInterface
> >>>>
> >>>> It converts from a ScopedMessagePipeHandle to an interface. This way
> >>>> code that was:
> >>>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
> >>>> becomes:
> >>>> ConvertToInterface<X>(scoped_message_pipe.Pass());
> >>>>
> >>>> BUG=none
> >>>> TEST=none
> >>>> R=darin@chromium.org
> >>>>
> >>>> Please review this at https://codereview.chromium.org/220063005/
> >>>>
> >>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >>>>
> >>>> Affected files (+10, -0 lines):
> >>>>   M mojo/public/cpp/bindings/interface.h
> >>>>
> >>>>
> >>>> Index: mojo/public/cpp/bindings/interface.h
> >>>> diff --git a/mojo/public/cpp/bindings/interface.h
> >>>> b/mojo/public/cpp/bindings/interface.h
> >>>> index
> >>>>
>
bba8fef89fc685b0f6b46841363450f5cb321009..ff6bf36a4267e7669380bb2a97af1ef433fcafbc
> >>>> 100644
> >>>> --- a/mojo/public/cpp/bindings/interface.h
> >>>> +++ b/mojo/public/cpp/bindings/interface.h
> >>>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
> >>>>    typedef ScopedMessagePipeHandle ScopedHandle;
> >>>>  };
> >>>>
> >>>> +// Use to convert from a ScopedMessagePipeHandle to an scoped
> interface
> >>>> +// handle. For example,
> >>>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
> >>>> +// where OmniboxPage names an interface in a mojom file.
> >>>> +template <typename S>
> >>>> +typename Interface<S>::ScopedHandle ConvertToInterface(
> >>>> +    ScopedMessagePipeHandle message_pipe_handle) {
> >>>> +  return Interface<S>::ScopedHandle(
> >>>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
> >>>> +}
> >>>>
> >>>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
> >>>> interfaces
> >>>>  // on either end.
> >>>>
> >>>>
> >>
> >
>

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

[vtl, 2014-04-03 15:46:38]

On Thu, Apr 3, 2014 at 8:43 AM, Viet-Trung Luu <vtl@google.com> wrote:

> I think Darin means:
>
> template <FromHandleType>
> static ScopedHandleBase<HandleType> From(ScopedHandleBase<FromHandleType>
> from) { ... }
>

Or maybe

template <FromScopedHandleType>
static ScopedHandleBase<HandleType> From(FromScopedHandleType> from) { ... }

I'd worry about the From... type being inferred, thus losing safety.


>
> I thought that I tried that and it didn't work for some reason, but I
> might be wrong.
>
>
> On Thu, Apr 3, 2014 at 8:39 AM, Scott Violet <sky@chromium.org> wrote:
>
>> Woudn't that require ScopedHandleBase::From to take a
>> ScopedMessagePipeHandle?
>>
>>   -Scott
>>
>> On Thu, Apr 3, 2014 at 8:32 AM, Darin Fisher <darin@chromium.org> wrote:
>> > Why not put From on ScopedHandleBase?
>> >
>> > On Apr 3, 2014 8:19 AM, "Viet-Trung Luu" <vtl@google.com> wrote:
>> >>
>> >> On Thu, Apr 3, 2014 at 8:00 AM, Darin Fisher <darin@chromium.org>
>> wrote:
>> >>>
>> >>> How about dropping the Convert prefix? mojo::ToInterface<S>? We could
>> >>> also add a static From method on ScopedFooHandle.
>> >>
>> >> I don't think this would work, since ScopedFooHandle is just a typedef
>> of
>> >> ScopedHandleBase<FooHandle>. (You might subclass, but that seems
>> dubious to
>> >> me, especially given what we do to emulate move semantics -- e.g.,
>> Pass()
>> >> probably wouldn't be quite right. Possibly, ScopedHandleBase<> should
>> be
>> >> marked final.)
>> >>
>> >>>
>> >>> On Apr 3, 2014 7:52 AM, <sky@chromium.org> wrote:
>> >>>>
>> >>>> Reviewers: darin,
>> >>>>
>> >>>> Description:
>> >>>> Adds mojo::ConvertToInterface
>> >>>>
>> >>>> It converts from a ScopedMessagePipeHandle to an interface. This way
>> >>>> code that was:
>> >>>> ScopedXHandle(XHandle(scoped_message_pipe.release().value()));
>> >>>> becomes:
>> >>>> ConvertToInterface<X>(scoped_message_pipe.Pass());
>> >>>>
>> >>>> BUG=none
>> >>>> TEST=none
>> >>>> R=darin@chromium.org
>> >>>>
>> >>>> Please review this at https://codereview.chromium.org/220063005/
>> >>>>
>> >>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >>>>
>> >>>> Affected files (+10, -0 lines):
>> >>>>   M mojo/public/cpp/bindings/interface.h
>> >>>>
>> >>>>
>> >>>> Index: mojo/public/cpp/bindings/interface.h
>> >>>> diff --git a/mojo/public/cpp/bindings/interface.h
>> >>>> b/mojo/public/cpp/bindings/interface.h
>> >>>> index
>> >>>>
>>
bba8fef89fc685b0f6b46841363450f5cb321009..ff6bf36a4267e7669380bb2a97af1ef433fcafbc
>> >>>> 100644
>> >>>> --- a/mojo/public/cpp/bindings/interface.h
>> >>>> +++ b/mojo/public/cpp/bindings/interface.h
>> >>>> @@ -63,6 +63,16 @@ struct Interface<mojo::NoInterface> {
>> >>>>    typedef ScopedMessagePipeHandle ScopedHandle;
>> >>>>  };
>> >>>>
>> >>>> +// Use to convert from a ScopedMessagePipeHandle to an scoped
>> interface
>> >>>> +// handle. For example,
>> >>>> +// ConvertToInterface<OmniboxPage>(message_pip_handle.Pass()),
>> >>>> +// where OmniboxPage names an interface in a mojom file.
>> >>>> +template <typename S>
>> >>>> +typename Interface<S>::ScopedHandle ConvertToInterface(
>> >>>> +    ScopedMessagePipeHandle message_pipe_handle) {
>> >>>> +  return Interface<S>::ScopedHandle(
>> >>>> +      Interface<S>::Handle(message_pipe_handle.release().value()));
>> >>>> +}
>> >>>>
>> >>>>  // InterfacePipe<S,P> is used to construct a MessagePipe with typed
>> >>>> interfaces
>> >>>>  // on either end.
>> >>>>
>> >>>>
>> >>
>> >
>>
>
>

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

[sky, 2014-04-03 19:59:06]

Update to ScopedHandleBase::From.

[viettrungluu, 2014-04-03 20:42:48]

https://codereview.chromium.org/220063005/diff/80001/mojo/public/cpp/system/c...
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/220063005/diff/80001/mojo/public/cpp/system/c...
mojo/public/cpp/system/core.h:52: template <typename Passed>
Nit: Could you call this PassedHandleType?

https://codereview.chromium.org/220063005/diff/80001/mojo/public/cpp/system/c...
mojo/public/cpp/system/core.h:54: return ScopedHandleBase<HandleType>(
Please add a comment:
// The |static_cast| here is needed to ensure type-safety (namely,
// that |HandleType| is a |PassedHandleType|).

https://codereview.chromium.org/220063005/diff/80001/mojo/public/cpp/system/c...
mojo/public/cpp/system/core.h:55:
static_cast<HandleType>(other.release().value()));
It should be static_cast<HandleType>(other.release()).value()

[sky, 2014-04-03 21:10:05]

Added some more magic to get it compiling with a cast.

[viettrungluu, 2014-04-03 21:19:08]

LGTM apart from  the backwards message, but Darin should probably look at it,
since he may have better ideas....

https://codereview.chromium.org/220063005/diff/100001/mojo/public/cpp/system/...
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/220063005/diff/100001/mojo/public/cpp/system/...
mojo/public/cpp/system/core.h:57:
PassedHandleType_is_not_a_subtype_of_HandleType);
This should be the other way around:
HandleType_is_not_a_subtype_of_PassedHandleType.

[sky, 2014-04-03 21:26:44]

https://codereview.chromium.org/220063005/diff/100001/mojo/public/cpp/system/...
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/220063005/diff/100001/mojo/public/cpp/system/...
mojo/public/cpp/system/core.h:57:
PassedHandleType_is_not_a_subtype_of_HandleType);
On 2014/04/03 21:19:08, viettrungluu wrote:
> This should be the other way around:
> HandleType_is_not_a_subtype_of_PassedHandleType.

Done.

[darin (slow to review), 2014-04-04 22:40:48]

It might be good to add a simple test. Too bad we don't have a way to test that
something fails to compile.

https://codereview.chromium.org/220063005/diff/120001/mojo/public/cpp/system/...
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/220063005/diff/120001/mojo/public/cpp/system/...
mojo/public/cpp/system/core.h:59:
static_cast<HandleType>(other.release().value()));
Are you sure about the .value() call here? Doesn't that strip the
PassedHandleType down to a MojoHandle, which any HandleType can be initialized
from? It feels like this might allow you to initialize a
ScopedHandleBase<MessagePipeHandle> from a
ScopedHandleBase<DataPipeConsumerHandle>.

I would have expected this to work out if you leave off the .value() call.

[darin (slow to review), 2014-04-04 22:41:46]

Hmm, we have base/bind_unittest.nc. That appears to be a "does not compile"
test. I wonder how that works!

[darin (slow to review), 2014-04-04 22:56:14]

https://codereview.chromium.org/220063005/diff/120001/mojo/public/cpp/system/...
File mojo/public/cpp/system/core.h (right):

https://codereview.chromium.org/220063005/diff/120001/mojo/public/cpp/system/...
mojo/public/cpp/system/core.h:59:
static_cast<HandleType>(other.release().value()));
On 2014/04/04 22:40:49, darin wrote:
> Are you sure about the .value() call here? Doesn't that strip the
> PassedHandleType down to a MojoHandle, which any HandleType can be initialized
> from? It feels like this might allow you to initialize a
> ScopedHandleBase<MessagePipeHandle> from a
> ScopedHandleBase<DataPipeConsumerHandle>.
> 
> I would have expected this to work out if you leave off the .value() call.

Nevermind. I somehow missed the compile assert :-P

[darin (slow to review), 2014-04-04 23:00:08]

OK, LGTM

[vtl, 2014-04-04 23:04:41]

It doesn't.


On Fri, Apr 4, 2014 at 3:41 PM, <darin@chromium.org> wrote:

> Hmm, we have base/bind_unittest.nc. That appears to be a "does not
> compile"
> test. I wonder how that works!
>
> https://codereview.chromium.org/220063005/
>

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

[vtl, 2014-04-04 23:06:42]

crbug.com/105388 :(

On Fri, Apr 4, 2014 at 4:04 PM, Viet-Trung Luu <vtl@google.com> wrote:

> It doesn't.
>
>
> On Fri, Apr 4, 2014 at 3:41 PM, <darin@chromium.org> wrote:
>
>> Hmm, we have base/bind_unittest.nc. That appears to be a "does not
>> compile"
>> test. I wonder how that works!
>>
>> https://codereview.chromium.org/220063005/
>>
>
>

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

[sky, 2014-04-04 23:09:30]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-04 23:09:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/220063005/120001

[sky, 2014-04-04 23:09:54]

For the record, Darin said LGTM to me in person.

[vtl, 2014-04-04 23:10:51]

On 2014/04/04 23:09:54, sky wrote:
> For the record, Darin said LGTM to me in person.

He said it up there too. :)

[commit-bot: I haz the power, 2014-04-05 02:40:34]

Change committed as 261958