Add public implementation MarshalerSupport #18530
Conversation
jkotas
changed the title
|
|
||
| namespace Internal.Runtime.InteropServices.WindowsRuntime | ||
| { | ||
| public static class InterfaceMarshalerSupport |
There was a problem hiding this comment.
Do we need to have separate types for all these? I think all these support methods can be on a single type.
There was a problem hiding this comment.
Then the method name will be a little bit longer, such as InterfaceMarshaler_ConvertToManagedWithoutUnboxing, is it ok?
There was a problem hiding this comment.
The existing public APIs similar to this are called like Marshal.GetObjectForIUnknown, Marshal.GetObjectForIUnknown or Marshal.GetTypedObjectForIUnknown.
Can we follow the same naming convention here and call it GetObjectForIUnknown... ?
Also, is box/unbox the right name here? Unboxing has a specific meaning in .NET. As far as I can tell all other existing (public) APIs use wrap/unwrap in this context.
There was a problem hiding this comment.
Also, is box/unbox the right name here? Unboxing has a specific meaning in .NET.
@jkotas , This is related to WinRT unboxing. For function name, Do you prefer GetObjectForIUnknownNoWinRTUnboxing or GetObjectForIUnknownNoUnwrap?
I would prefer the 1st one.
There was a problem hiding this comment.
The use case/scenario seems to be related to WinRT, but the underlying function implementation doesn't appear to be specific to WinRT - stubhelpers.cpp in 868. I see the ObjFromComIP::IGNORE_WINRT_AND_SKIP_UNBOXING but that indicates the WinRT aspect should be ignored, so what happens if we pass in a non-WinRT object? I agree with @jkotas here and think something like GetObjectForIUnknownNoUnboxing seems to be exactly what the function does and documenting an example of usage should alleviate any confusion - "Usually used for WinRT scenarios..."
There was a problem hiding this comment.
This is only used in two places in CoreFX: once for INotifyCollectionChangedEventArgs and second time for IPropertyChangedEventArgs.
Why does the WinRT unboxing matters for these two places? Can't this just use the vanilla Marshal.GetUniqueObjectForIUnknown ?
If this really needs to be special cases for WinRT unboxing, my name preference would be GetObjectForIUnknownWithoutUnboxing.
There was a problem hiding this comment.
This is only used in two places in CoreFX: once for INotifyCollectionChangedEventArgs and second time for IPropertyChangedEventArgs.
There are two more places:
Why does the WinRT unboxing matters for these two places? Can't this just use the vanilla Marshal.GetUniqueObjectForIUnknown ?
For functional(correctness) point view, actually it doesn't matter. I suppose the reason why we use unboxing one is convenient and a little bit perf gain. Let's use Marshal.GetUniqueObjectForIUnknown instead
There was a problem hiding this comment.
There are two more places:
These are internal to CoreLib. They do not need the public API.
Let's use Marshal.GetUniqueObjectForIUnknown instead
Agree.
| @@ -0,0 +1,13 @@ | |||
| using System; | |||
| { | ||
| public static class EventArgsMarshalerSupport | ||
| { | ||
| public static IntPtr CreateNativeNCCEventArgsInstance(int action, object newItems, object oldItems, int newIndex, int oldIndex) |
There was a problem hiding this comment.
Does this really need to be in the runtime? It just creates an instance of a WinRT type...
There was a problem hiding this comment.
The problem is that WindowsRuntimeMarshal.GetActivationFactory() API takes Type as argument. For this case, type(PropertyChangedEventArgs) is a projected type. Thus in managed side, you can't use its WinRT view Type. also if you try to use this API with its managed view type, the API will throw.
maybe S.P.Corelib could expose a public implementation API (GetActivationFactory) which takes a string as argument.
There was a problem hiding this comment.
Okay. move these marshaling into S.R.WR
|
|
||
| namespace Internal.Runtime.InteropServices.WindowsRuntime | ||
| { | ||
| public static class EventArgsMarshalerSupport |
There was a problem hiding this comment.
This is defined as a public class/function as such we should probably have some minimal documentation with it. At least something that documents the what the arguments mean. That might be obvious if the function's use was at least explained.
|
|
||
| namespace Internal.Runtime.InteropServices.WindowsRuntime | ||
| { | ||
| public static class InterfaceMarshalerSupport |
There was a problem hiding this comment.
The use case/scenario seems to be related to WinRT, but the underlying function implementation doesn't appear to be specific to WinRT - stubhelpers.cpp in 868. I see the ObjFromComIP::IGNORE_WINRT_AND_SKIP_UNBOXING but that indicates the WinRT aspect should be ignored, so what happens if we pass in a non-WinRT object? I agree with @jkotas here and think something like GetObjectForIUnknownNoUnboxing seems to be exactly what the function does and documenting an example of usage should alleviate any confusion - "Usually used for WinRT scenarios..."
luqunl
force-pushed
the
MarshallerSupport
branch
from
2fe211d
to
9083db6
Compare
Expose two WinRT Marshaller in S.P.Corelib for S.R.WindowsRuntime to use.
Corefx PR:dotnet/corefx#30494