Skip to content
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

Remove CoClass usages from System.Windows.Forms #4971

Merged
merged 10 commits into from
Jun 4, 2021

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented May 25, 2021

This helps make WinForms application more ILLink/NativeAOT-friendly

Fixes #4940

Microsoft Reviewers: Open in CodeFlow

This helps make WinForms application more ILLink/NativeAOT-friendly
@kant2002 kant2002 requested a review from a team as a code owner May 25, 2021 13:06
@ghost ghost assigned kant2002 May 25, 2021
@RussKie RussKie requested a review from JeremyKuhne May 26, 2021 00:00
@RussKie
Copy link
Member

RussKie commented May 26, 2021

@JeremyKuhne @AaronRobinsonMSFT may I get you to review this one?

@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label May 26, 2021
Copy link
Contributor

@weltkante weltkante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few comments with suggestions, note that these apply to all three locations but I only annotated the first occurence since the others are identical.

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I've no real issues with this change as it just makes the veneer over COM class activation explicit. I don't fully get why this needed. However, I'm not plugged into all the NativeAOT work so I could see a case for this change being cheaper than supporting how .NET has traditionally activated a COM class.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
@kant2002
Copy link
Contributor Author

@AaronRobinsonMSFT I suspect that new RCW should be treated in different way by compiler and only for Windows, which is additional complexity. Second option is that RhNewObject would be aware about CoClass, so penalty for new objects would be applied across all object creation. Again, this is what I see from where I'm sitting. This is about only change which is required for WinForms. There exists CoClass in the Accessibility, but that's about it what is needed for making file dialogs works in NativeAOT.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
Copy link
Contributor

@weltkante weltkante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with the open discussion about removing the IIDs and calling CoCreateInstance with IID_IUnknown instead which already has a constant.

Also it may be worth moving the CLSID constants into the interop files, after all these are constants from the win32 API. Not sure where they would go though @RussKie

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
@JeremyKuhne
Copy link
Member

Also it may be worth moving the CLSID constants into the interop files, after all these are constants from the win32 API. Not sure where they would go though @RussKie

I'd say under the root in Interop. Following the existing pattern of Interop.CLSID.cs with CLSID being a static class with FileSaveDialog, etc. as statics in it.

Note that there is cost to having these take strings (I don't think it gets optimized away). It might be worth changing these to Guid(uint a, ushort b, ushort c, byte d, byte e, byte f, byte g, byte h, byte i, byte j, byte k), at least long term.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 26, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 27, 2021
weltkante
weltkante previously approved these changes May 27, 2021
@kant2002
Copy link
Contributor Author

@JeremyKuhne can you take a look?

@RussKie
Copy link
Member

RussKie commented Jun 1, 2021

@kant2002 Jeremy is currently OOF, will be back next week.

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Jun 3, 2021
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@RussKie RussKie merged commit 5c93ab0 into dotnet:main Jun 4, 2021
@ghost ghost added this to the 6.0 Preview6 milestone Jun 4, 2021
@RussKie
Copy link
Member

RussKie commented Jun 4, 2021

Thank you all.

@kant2002 kant2002 deleted the kant/remove-coclass branch June 4, 2021 04:42
@kant2002 kant2002 mentioned this pull request Jun 8, 2021
11 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace FileSaveDialogRCW and FileOpenDialogRCW with manual creation of COM object
5 participants