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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ private void ApplyDefaultWebViewSettings()

// Desktop applications don't normally want to enable things like "alt-left to go back"
// or "ctrl+f to find". Developers should explicitly opt into allowing these.
// TODO: Create a way of opting back in.
// Issues #30511 and #30624 track making an option to control this.
_webview.AcceleratorKeyPressed += (sender, eventArgs) =>
{
if (eventArgs.VirtualKey != 0x49) // Allow ctrl+shift+i to open dev tools, at least for now
Expand Down
2 changes: 0 additions & 2 deletions src/Components/WebView/WebView/src/IpcReceiver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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!

throw new InvalidOperationException("Cannot receive IPC messages when no page is attached");
}

Expand Down
27 changes: 0 additions & 27 deletions src/Components/WebView/WebView/src/IpcSender.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,8 @@
using Microsoft.AspNetCore.Components.RenderTree;
using Microsoft.JSInterop;

// Sync vs Async APIs for this.
// This mainly depends on the underlying support for the browser. Assuming that there is no synchronous API
// communication is safer, since it's not guaranteed.
// In that scenario, some APIs need to expose the async nature of the communication. That happens when some
// component like the renderer needs to know the results of the operation. For example when updating the UI
// since more code needs to execute afterwards.
// In other cases like when we try to attach a component to the document, we don't necessarily need to do that
// 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.


// Handles comunication between the component abstractions (Renderer, NavigationManager, JSInterop, etc.)
// and the underlying transport channel
internal class IpcSender
Expand All @@ -47,39 +33,26 @@ public void ApplyRenderBatch(long batchId, RenderBatch renderBatch)
DispatchMessageWithErrorHandling(message);
}

// This is called by the navigation manager and needs to be forwarded to the WebView
// It might trigger the WebView to change the location of the URL and cause a LocationUpdated event.
public void Navigate(string uri, bool forceLoad)
{
DispatchMessageWithErrorHandling(IpcCommon.Serialize(IpcCommon.OutgoingMessageType.Navigate, uri, forceLoad));
}

// TODO: Make these APIs async if we want the renderer to be able to deal with errors.
// Called from Renderer to attach a new component ID to a given selector.
public void AttachToDocument(int componentId, string selector)
{
DispatchMessageWithErrorHandling(IpcCommon.Serialize(IpcCommon.OutgoingMessageType.AttachToDocument, componentId, selector));
}

// Called from the WebView to detach a root component from the document.
public void DetachFromDocument(int componentId)
{
DispatchMessageWithErrorHandling(IpcCommon.Serialize(IpcCommon.OutgoingMessageType.DetachFromDocument, componentId));
}

// Interop calls emitted by the JSRuntime
public void BeginInvokeJS(long taskId, string identifier, string argsJson, JSCallResultType resultType, long targetInstanceId)
{
DispatchMessageWithErrorHandling(IpcCommon.Serialize(IpcCommon.OutgoingMessageType.BeginInvokeJS, taskId, identifier, argsJson, resultType, targetInstanceId));
}

// TODO: We need to think about this, the invocation result contains the triplet [callId, successOrError, resultOrError]
// serialized as JSON with the options provided by the JSRuntime. The host can't operate on the "unserialized"
// data since it needs to deal with DotNetObjectReferences and JSObjectReference which the host doesn't have any visibility
// over how to serialize or deserialize.
// The strongest limitation we can find on a platform is that we might only be able to communicate with the host via "strings" (post-message)
// and in that situation we can define a separator within the string like (callId,success,resultOrError) that the
// side running in the browser can parse for processing.
public void EndInvokeDotNet(string callId, bool success, string invocationResultOrError)
{
DispatchMessageWithErrorHandling(IpcCommon.Serialize(IpcCommon.OutgoingMessageType.EndInvokeDotNet, callId, success, invocationResultOrError));
Expand Down
6 changes: 3 additions & 3 deletions src/Components/WebView/WebView/src/WebViewManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ protected bool TryGetResponseContent(string uri, bool allowFallbackOnHostPage, o

internal async Task AttachToPageAsync(string baseUrl, string startUrl)
{
// If there was some previous attached page, dispose all its resources. TODO: Are we happy
// with this pattern? The alternative would be requiring the platform author to notify us
// when the webview is navigating away so we could dispose more eagerly then.
// If there was some previous attached page, dispose all its resources. We're not eagerly disposing
// page contexts when the user navigates away, because we don't get notified about that. We could
// change this if any important reason emerges.
_currentPageContext?.Dispose();

var serviceScope = _provider.CreateScope();
Expand Down