Skip to content

Clean up WebView comments #30742

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

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Conversation

SteveSandersonMS
Copy link
Member

As instructed in #30656, I've reviewed the // TODO comments and made sure they are adequately represented in issues. I think only one of them really needs to be tracked like that; the rest are OK as-is.

This PR removes the comments that are no longer applicable.

@SteveSandersonMS SteveSandersonMS added the feature-blazor-desktop This issue is related to Blazor Desktop label Mar 8, 2021
@SteveSandersonMS SteveSandersonMS requested a review from a team March 8, 2021 15:06
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 8, 2021
@@ -46,8 +46,6 @@ public async Task OnMessageReceivedAsync(PageContext pageContext, string message
// For any other message, you have to have a page attached already
if (pageContext == null)
{
// TODO: Should we just ignore these messages? Is there any way their delivery
// might be delayed until after a page has detached?
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there's any concern here. Nothing would go wrong unless we receives one of these messages before the first page connection, and that's impossible, because it's the page that sends them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully the exception here will keep us honest!

// since we only care about errors that might happen while attaching the component and the renderer doesn't
// necessarily need to know about those if we are terminating the component/host as a result.
// If we decide we need to expose the async nature of the communication channel, then we will need to keep track
// of all the message pairs/completions across the IPC channel.
Copy link
Member Author

Choose a reason for hiding this comment

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

These comments refer to an entirely different earlier incarnation of the IPC implementation.

namespace Microsoft.AspNetCore.Components.WebView
{
// These are all the messages .NET needs to know how to dispatch to JS
// TODO: Proper serialization, error handling, etc.
Copy link
Member Author

Choose a reason for hiding this comment

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

The serialization is already adequate. Error handling is something we're tightening up as part of implementing unit tests.

@SteveSandersonMS SteveSandersonMS merged commit 0cb41d4 into main Mar 9, 2021
@SteveSandersonMS SteveSandersonMS deleted the stevesa/webview-comments-cleanup branch March 9, 2021 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-desktop This issue is related to Blazor Desktop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants