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

Option to not use WebRequest for PictureBox #6684

Merged
merged 10 commits into from
Mar 10, 2022

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented Feb 12, 2022

Indirectly related to #1756 because if that would work, I would be able to fix #1756 using the same approach.
WebRequest bring ton of code to WinForms which is old, and not really interesting. NativeAOT suffer from this.

Inspired by dotnet/runtime#38397

By itself if applying <RuntimeHostConfigurationOption Include="System.Windows.Forms.PictureBox.UseWebRequest" Value="false" Trim="true" /> it will shave 1Mb from 20Mb of my sample application which compiled under NativeAOT. I include all controls in that application.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned kant2002 Feb 12, 2022
@ghost ghost added the draft draft PR label Feb 12, 2022
@kant2002 kant2002 closed this Feb 14, 2022
@kant2002 kant2002 reopened this Feb 14, 2022
@kant2002
Copy link
Contributor Author

If subsequently apply kant2002@7ce82f2 then application trimmed to 15Mb and that with reflection

@kant2002 kant2002 marked this pull request as ready for review February 17, 2022 15:35
@kant2002 kant2002 requested a review from a team as a code owner February 17, 2022 15:35
@ghost ghost removed the draft draft PR label Feb 17, 2022
@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Feb 18, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Feb 20, 2022
@kant2002
Copy link
Contributor Author

@RussKie do you think, this PR require some additional work?

@RussKie
Copy link
Member

RussKie commented Feb 28, 2022 via email

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.

I'd stick with putting this in the existing Task.Run and avoid introducing async.

@ghost ghost added the waiting-author-feedback The team requires more information from the author label Feb 28, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Mar 1, 2022
@dreddy-work dreddy-work requested a review from JeremyKuhne March 2, 2022 00:53
@kant2002
Copy link
Contributor Author

kant2002 commented Mar 3, 2022

@JeremyKuhne did I infer your proposal correctly?

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.

Spending some more time looking at this I think you'd be better off using the existing ReadCallBack(). Let me know if that doesn't work for you or if you have any other questions.

@ghost ghost added the waiting-author-feedback The team requires more information from the author label Mar 7, 2022
@JeremyKuhne
Copy link
Member

@kant2002 appreciate your effort here.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Mar 9, 2022
@kant2002
Copy link
Contributor Author

kant2002 commented Mar 9, 2022

@JeremyKuhne reason why I did use async/await in "new" implementation is because I think they are easier to reason about. And seems here appetite eventually move to HttpClient so async/await is would be natural choice. Since this is new code path, "some regressions" maybe acceptable for target audience.

Anyway, I update as you suggest the code. I do not have questions and this approach is fine.

@JeremyKuhne
Copy link
Member

I think they are easier to reason about.

Normally, I'd agree. As this code predates async you introduce a fair amount of cognitive overhead if you mix and match approaches. While the code could be completely rewritten to leverage new patterns I'd hesitate to do so without a strong justification.

Since this is new code path, "some regressions" maybe acceptable for target audience.

I think the default position should be "no differences in functionality". We can, of course, alter that based on data and discussion.

Again, thanks for your effort here.

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!

@dreddy-work dreddy-work merged commit 564463d into dotnet:main Mar 10, 2022
@ghost ghost added this to the 7.0 Preview3 milestone Mar 10, 2022
@RussKie RussKie added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Mar 11, 2022
@kant2002 kant2002 deleted the kant/async-picturebox branch March 24, 2022 13:27
@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PictureBox to use HttpClient instead of using WebRequest
4 participants