Add conversion from Handle to UntypedHandle

Add conversion from Handle to UntypedHandle.

UntypedHandle, as it is, makes chaining messages impossible. A received message is expected to contain UntypedHandle which cannot be generated from a Handle.
This CL allows any handle to be transformed into an UntypedHandle.

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

[qsr, 2014-05-20 12:44:10]

While trying to write bindings, I have plenty of issue if Handle cannot be
considered as UntypedHandle, so I removed the later class, and add all its
method to Handle, but made sure that concrete implementation will throw if we
try to transform it into something else.

[rmcilroy, 2014-05-20 16:34:58]

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java:26:
* @see HandleImpl#HandleImpl(UntypedHandleImpl)
Fix Javadoc (here and below).

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:37: *
Constructor for transforming an {@link UntypedHandleImpl} into a specific one.
ditto

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java:14:
class UntypedHandleImpl extends HandleImpl {
Do we even need this class any longer - can't we just implement the
toXXXHandle() methods in HandleImpl now?  I'm also unclear as to why it's OK to
convert an UntypedHandleImpl to any other Handle, but other Handle types will
throw an exception if you attempt to convert them.  Can we not just use <Handle>
as the opaque generic type, but always use the correct Impl object (e.g.,
MessagePipeHandle, ConsumerHandle, etc) when we create a concrete instance.

[qsr, 2014-05-20 17:07:48]

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java:26:
* @see HandleImpl#HandleImpl(UntypedHandleImpl)
On 2014/05/20 16:34:59, rmcilroy wrote:
> Fix Javadoc (here and below).

Done.

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:37: *
Constructor for transforming an {@link UntypedHandleImpl} into a specific one.
On 2014/05/20 16:34:59, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java:14:
class UntypedHandleImpl extends HandleImpl {
On 2014/05/20 16:34:59, rmcilroy wrote:
> Do we even need this class any longer - can't we just implement the
> toXXXHandle() methods in HandleImpl now?  I'm also unclear as to why it's OK
to
> convert an UntypedHandleImpl to any other Handle, but other Handle types will
> throw an exception if you attempt to convert them.  Can we not just use
<Handle>
> as the opaque generic type, but always use the correct Impl object (e.g.,
> MessagePipeHandle, ConsumerHandle, etc) when we create a concrete instance.

 The issue is that when I receive a handle from a message pipe I do not know
what type it is. That's the case where I use a UntypedHandleImpl, and the
receiver of the message must know what type it is and do the right conversion,
but it will only explode on first use if wrongly chosen. Once the conversion is
done once, you cannot change the type of the handle anymore.

About needing this class -> good idea, removed and put the code in HandleImpl
itself.

[rmcilroy, 2014-05-20 20:22:09]

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java:14:
class UntypedHandleImpl extends HandleImpl {

>  The issue is that when I receive a handle from a message pipe I do not know
> what type it is. That's the case where I use a UntypedHandleImpl, and the
> receiver of the message must know what type it is and do the right conversion,
> but it will only explode on first use if wrongly chosen. Once the conversion
is
> done once, you cannot change the type of the handle anymore.

When is the conversion done?  I don't really like adding the toXXXHandle methods
to the Handle type and would much prefer if HandleImpl was abstract and you
could only allocate a concrete implementation, however, I don't know enough
about the Bindings implementation to know whether this is possible.  Would it be
possible (even if in a subsequent CL)?

If this is really not possible, then I think it would be cleaner to only have
the convertors as static methods in the given class, rather than forcing each
concrete Handle class to implement toXXXHandle methods which should never be
called.  E.g., in MessagePipeHandler (and only MessagePipeHandler), have:
 public static MessagePipeHandler toMessagePipeHandle(HandlerImpl rawHandle)
and similar for each other class (and avoid adding any extra methods to the
Handle class).

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:26: * the core
implementation. Will be used to delegate all behavior.
nit - I realise this isn't this CL, but s/the/The

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:41:
HandleImpl(HandleImpl other) {
I think this constructor should be protected - it should only be called by
subclasses.  Also update javadoc to something like: "Constructor to enable
subclasses to transform a {@link HandleImpl} into a specific Handle type,
invalidating the passed argument in the process."

[qsr, 2014-05-21 09:45:30]

Following f2f discussion, changed this CL to add conversion from a Handle to an
UntypedHandle.

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/20001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/UntypedHandleImpl.java:14:
class UntypedHandleImpl extends HandleImpl {
On 2014/05/20 20:22:10, rmcilroy wrote:
> 
> >  The issue is that when I receive a handle from a message pipe I do not know
> > what type it is. That's the case where I use a UntypedHandleImpl, and the
> > receiver of the message must know what type it is and do the right
conversion,
> > but it will only explode on first use if wrongly chosen. Once the conversion
> is
> > done once, you cannot change the type of the handle anymore.
> 
> When is the conversion done?

 Whenever the user that receives handle from a message decide to do it. When we
have bindings, this conversion will be done by generated code, and will be
invisible to the user, but the core API have to get untyped handle and let the
user choose what to do.

> I don't really like adding the toXXXHandle methods
> to the Handle type and would much prefer if HandleImpl was abstract and you
> could only allocate a concrete implementation, however, I don't know enough
> about the Bindings implementation to know whether this is possible.  Would it
be
> possible (even if in a subsequent CL)?

 The problem is not the binding, the problem is the underlying API. I do not
have any way, given a handle I received from a message, to decide what type of
handle it is.

> 
> If this is really not possible, then I think it would be cleaner to only have
> the convertors as static methods in the given class, rather than forcing each
> concrete Handle class to implement toXXXHandle methods which should never be
> called.  E.g., in MessagePipeHandler (and only MessagePipeHandler), have:
>  public static MessagePipeHandler toMessagePipeHandle(HandlerImpl rawHandle)
> and similar for each other class (and avoid adding any extra methods to the
> Handle class).

 This is unfortunately not possible either. HandlerImpl is an implementation
detail. The public API is in mojo/public/java/src/org/chromium/mojo/system and
cannot have any static class. I can move the method in Core if you prefer, but
that would mean needing access to a Core instance everywhere, and would make
usage and testing harder

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:26: * the core
implementation. Will be used to delegate all behavior.
On 2014/05/20 20:22:10, rmcilroy wrote:
> nit - I realise this isn't this CL, but s/the/The

Done.

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:41:
HandleImpl(HandleImpl other) {
On 2014/05/20 20:22:10, rmcilroy wrote:
> I think this constructor should be protected - it should only be called by
> subclasses.  Also update javadoc to something like: "Constructor to enable
> subclasses to transform a {@link HandleImpl} into a specific Handle type,
> invalidating the passed argument in the process."

 protected in java is strictly weaker than package protected. Something
protected can be called from anything on the same package, while a package
protected method cannot be called from a subclass on another package. Changed
anyway if you think this makes this clearer.

[rmcilroy, 2014-05-21 11:31:43]

This looks much better to me, thanks.  lgtm after comments are addressed.

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:41:
HandleImpl(HandleImpl other) {
On 2014/05/21 09:45:30, qsr wrote:
> On 2014/05/20 20:22:10, rmcilroy wrote:
> > I think this constructor should be protected - it should only be called by
> > subclasses.  Also update javadoc to something like: "Constructor to enable
> > subclasses to transform a {@link HandleImpl} into a specific Handle type,
> > invalidating the passed argument in the process."
> 
>  protected in java is strictly weaker than package protected. Something
> protected can be called from anything on the same package, while a package
> protected method cannot be called from a subclass on another package. Changed
> anyway if you think this makes this clearer.

True, however since the class is package private, the constructor can't be
called outwith the package anyway, so it effectively has package-private
protection anyway, and I think the protected modifier is a good hint that this
should only be called from subclasses.

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
File
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java:25:
* @see HandleImpl#HandleImpl(HandleImpl)
Javadoc (thoughout)

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:40:
HandleImpl(HandleImpl other) {
protected

[qsr, 2014-05-21 11:40:18]

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
File
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java
(right):

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/DataPipeConsumerHandleImpl.java:25:
* @see HandleImpl#HandleImpl(HandleImpl)
On 2014/05/21 11:31:44, rmcilroy wrote:
> Javadoc (thoughout)

 The javadoc is correct there, HandleImpl constructor is taking a HandleImpl,
because it needs to be able to transform any handle into an untyped one.

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/80001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:40:
HandleImpl(HandleImpl other) {
On 2014/05/21 11:31:44, rmcilroy wrote:
> protected

Done.

[qsr, 2014-05-21 11:40:37]

viettrungluu@ -> gentle ping for OWNERS.

[bulach, 2014-05-21 12:45:32]

lgtm, one suggestion below. thanks for the extensive discussion this morning!
also, spurious . in the title, "as it is. makes chaining"

https://codereview.chromium.org/291103004/diff/100001/mojo/android/system/src...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/100001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:14: abstract
class HandleImpl implements Handle {
nit: should this be HandleBase? I think Impl are normally used on leaf-classes..

[qsr, 2014-05-21 12:56:13]

https://codereview.chromium.org/291103004/diff/100001/mojo/android/system/src...
File mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java (right):

https://codereview.chromium.org/291103004/diff/100001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/HandleImpl.java:14: abstract
class HandleImpl implements Handle {
On 2014/05/21 12:45:33, bulach wrote:
> nit: should this be HandleBase? I think Impl are normally used on
leaf-classes..

Done.

[viettrungluu, 2014-05-21 17:13:25]

lgtm

[qsr, 2014-05-21 19:48:35]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:58:22]

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

[commit-bot: I haz the power, 2014-05-22 03:12:28]

Change committed as 272067