-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API Review: Interop between COM and WinRT #4599
Conversation
|
||
// Get the COM ICoreWebView2 object from the COM interop interface. | ||
winrt::com_ptr<ICoreWebView2> coreWebView2Com; | ||
winrt::check_hresult(interop->GetComICoreWebView2(coreWebView2Com.put())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is one direction "Create" and the other "Get"?
Answering my own question: .NET CoreWebView2 and WinRT CoreWebView2 are both wrappers around the underlying COM ICoreWebView2, so going from CoreWebView2 to ICoreWebView2 is a "Get" (there is always an inner ICoreWebView2). But going from ICoreWebView2 to CoreWebView2 requires that a wrapper be manufactured, hence the Create.
(I was surprised that they are wrappers. I though the COM ICoreWebView2 directly implemented the WinRT methods too - you just had to QI for them.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering my own question: ...
You are correct. Any concerns?
(I was surprised that they are wrappers. I though the COM ICoreWebView2 directly implemented the WinRT methods too - you just had to QI for them.)
WinRT is a wrapper because we previously supported Win7 where various WinRT things wouldn't be available. A year or two ago we've dropped support for Win7+8 but have no justification to go back and change the implementation of our WinRT support.
/// Creates a CoreWebView2 WinRT object that wraps an existing COM ICoreWebView2 object. | ||
/// This allows interacting with the WebView2 control using WinRT, | ||
/// even if the control was originally created using COM. | ||
HRESULT CreateFromComICoreWebView2([in] ICoreWebView2* coreWebView2Com, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you Create twice from the same ICoreWebView2? Do you get two CoreWebView2 objects that both wrap the same ICoreWebView2? So changes to one are visible to the other? (And events are raised on both?)
Is the way to detect this to call GetComICoreWebView2 on both CoreWebView2's and see if you get the same ICoreWebView2 back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Returning the same CoreWebView2 seems like the better way to go. We can keep a weak ref to the created CoreWebView2 and return that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning the same CoreWebView2 contradicts the "create" prefix, which implies that you get a new object each time. Maybe we can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with living with that. Or let me know if you have a better name in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Get
for that reason and don't see a downside.
You could argue that that obscures the fact that it's a somewhat expensive operation (there's allocations going on). But that's true of many Get() methods, and implying perf is the reason why those methods aren't exposed as a property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a real preference for either so Get is fine if folks prefer that.
I named it CreateFromComICoreWebView2 because that's the name we went with for the .NET version of this API. (This PR is an update of an existing API spec doc for the .NET CoreWebView2.CreateFromComICoreWebView2 method that we had previously reviewed and shipped.) In theory I like naming it Create for consistency, but practically its pretty unlikely that someone will be using both the .NET and WinRT versions of this API so I don't think it would cause much confusion and its not a big blocker for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching .Net (Create
) sounds OK to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll leave as Create then. Thanks!
winrt::com_ptr<ICoreWebView2> coreWebView2Com) | ||
{ | ||
auto factory = winrt::get_activation_factory< | ||
winrt::Microsoft::Web::WebView2::Core::CoreWebView2>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since CoreWebView2 has no constructors or static methods, can I use the activation factory like this? C++/WinRT doesn't generate something for me to implement. Do I need to specify an empty static interface for CoreWebView2 in the MIDL3? Or I can move this to the activation factory of the CoreWebView2Environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, I totally overlooked that wrinkle. You can do this in code-behind, no need to pollute the IDL.
Implement a factory manually:
namespace winrt::Microsoft::Web::WebView2::Core::factory_implementation
{
struct CorewebView2 : implements<IActivationFactory, ICoreWebView2Interop>
{
[[noreturn]] IInspectable ActivateInstance() const { throw hresult_not_implemented(); }
// ICoreWebView2Interop methods go here
};
}
and pick it off in your DllGetActivationFactory before delegating to the C++/WinRT generated one.
HRESULT WINAPI DllGetActivationFactory(HSTRING classId, IActivationFactory ** factory) noexcept try
{
auto const& name = (winrt::hstring const&)classId;
if (name == L"Microsoft.Web.WebView2.CoreCoreWebView2") {
*factory = winrt::detach_abi(winrt::make<winrt::Microsoft::Web::WebView2::Core::factory_implementation::CoreWebView2>());
return S_OK;
} else {
return WINRT_GetActivationFactory(classId, (void**)factory);
}
catch (...) { return to_hresult(): }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense, thanks!
/// Creates a CoreWebView2 WinRT object that wraps an existing COM ICoreWebView2 object. | ||
/// This allows interacting with the WebView2 control using WinRT, | ||
/// even if the control was originally created using COM. | ||
HRESULT CreateFromComICoreWebView2([in] ICoreWebView2* coreWebView2Com, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Get
for that reason and don't see a downside.
You could argue that that obscures the fact that it's a somewhat expensive operation (there's allocations going on). But that's true of many Get() methods, and implying perf is the reason why those methods aren't exposed as a property.
To round out the more general scenario of interoperating between libraries written in different | ||
languages, we add the ability to convert a CoreWebView2 to and from .NET and COM, as well as | ||
to and from WinRT and COM. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing a premise: are there features of COM ICoreWebView2 that aren't exposed by WinRT CoreWebView2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some yes but also its so that you can mix code (esp libraries) written in COM / WinRT / .NET
Updates the existing API spec covering .NET and COM interop to also include generic WinRT and COM interop.