[Blazor][WebView] Abstractions #30456

javiercn · 2021-02-25T12:50:25Z

WIP This is the initial setup for the Blazor abstractions for running on a WebView. Just sharing the branch for now, details and a design write-up to come a bit later.

The goal is to provide the fundamental primitives for Blazor WebView as well as a "Headless" webview that we can use in our unit tests.

javiercn · 2021-02-25T12:51:24Z

~~@SteveSandersonMS @Eilon here's the branch with my WIP FYI, I'm not looking 4 feedback just yet, I'll ping the PR when I get this in the shape I want.~~

src/Components/WebView/Platforms/Wpf/src/BlazorWebView.cs

SteveSandersonMS · 2021-03-02T10:33:27Z

@javiercn @Eilon I had to rebase this onto main (and hence force-push) to pick up all the recent TypeScript changes before doing new work in that area. Please be sure to get the latest version of this branch locally before doing any further work on it, otherwise you'll have some trouble with pushing commits later.

javiercn · 2021-03-02T11:53:25Z

@javiercn @Eilon I had to rebase this onto main (and hence force-push) to pick up all the recent TypeScript changes before doing new work in that area. Please be sure to get the latest version of this branch locally before doing any further work on it, otherwise you'll have some trouble with pushing commits later.

Sounds good to me

javiercn · 2021-03-02T15:13:49Z

src/Components/WebView/WebView/src/Services/WebViewRenderer.cs

+        private readonly Dictionary<string, int> _componentIdBySelector = new();
+        private readonly Dispatcher _dispatcher;
+        private readonly IpcSender _ipcSender;
+        private long nextRenderBatchId = 1;


Do we need this here?

For Blazor Server I can understand, but for Blazor WebView I think we can rely on a simple ACK without having to keep track of the id

It just helps us discover more quickly if we have any bugs related to out-of-order delivery.

How can this situation arise?

We can have bugs, for example something to do with how we dispatch the messages or executions onto the dispatcher, which might allow for things to get out of order. It shouldn't happen, but there's no harm making sure.

BTW in my experience I've never seen such a bug while working on MBB and haven't seen it from customers, but perhaps it can more easily be an issue while we develop the code, so it seems like a good precaution. I cannot imagine such a bug would be easy to debug when things simply explode and behave oddly.

javiercn · 2021-03-02T15:19:47Z

src/Components/WebView/WebView/src/IpcCommon.cs

+            if (message != null && message.StartsWith(_ipcMessagePrefix, StringComparison.Ordinal))
+            {
+                var messageAfterPrefix = message.AsSpan(_ipcMessagePrefix.Length);
+                var parsed = (JsonElement[])JsonSerializer.Deserialize(messageAfterPrefix, typeof(JsonElement[]), JsonSerializerOptionsProvider.Options);


Something to think about here is that we are double-serializing JSON data when we do JS Interop

Yeah, I agree. I think we can make this interop mechanism more low-level and performant later if we want to. Hopefully it's all very much contained to this one file (and the equivalent on the .ts side) so it should be easy to reason about any changes.

javiercn · 2021-03-02T15:38:01Z

src/Components/WebView/WebView/src/IpcReceiver.cs

+        public IpcReceiver(WebViewManager manager)
+        {
+            _manager = manager;
+        }


I think we should reconsider the direction of this dependency.

The WebViewManager creates and manages the IPCReceiver, but then the receiver calls back to the manager and both reference each other.

I think we might be better of if we follow an alternative approach:

Handle initialization outside the IpcReceiver.

Instead of dispatching the "AttachToPage" we expose an "Initialize" event that the manager subscribes to from outside.

That way all interactions always go Manager -> (IPCXXX) and there is no coupling both ways.

Absolutely, sounds good to me! I agree it's a bit unexpected that it relies on an internal method at the moment. The only drawback that concerns me about using an event is that it's hard to deal with asynchrony and exceptions, but we can figure something out.

Eilon · 2021-03-02T22:19:57Z

src/Components/WebView/Platforms/WebView2/src/WebView2WebViewManager.cs

+
+namespace Microsoft.AspNetCore.Components.WebView.WebView2
+{
+    public class WebView2WebViewManager : WebViewManager


At some point we'll need to decide whether we want all this stuff to be public. It's not really an interesting part of the API surface from a public consumption perspective, yet it allows us to have common code between WPF, WinForms, and .NET MAUI.

No need to decide anything now. Keeping this in a separate assembly will help us ensure we have good layering for now.

I agree, the layering is fine for now.

Later on we should consider replacing the M.A.C.WebView.WebView2 package with use of a shared-source file for the WebView2WebViewManager abstraction. It would make no difference to functionality, but would mean we just don't have to ship an extra publicly-consumable package for what is intended as an internal abstraction.

src/Components/WebView/Platforms/Wpf/src/BlazorWebView.cs

javiercn · 2021-03-03T17:28:59Z

src/Components/WebView/WebView/src/WebViewManager.cs

+                await _currentPageContext.Renderer.AddRootComponentAsync(
+                    rootComponent.ComponentType,
+                    selector,
+                    rootComponent.Parameters);


I'm not sure about automatically re-rendering the components in this situation.

There's a chance that rootComponent.Parameters contains objects that have been mutated and given that we might go to a different host page, there is a chance that the same root components might not apply to the new page?

Do you have more of an end-to-end scenario in mind that would be a good example of this?

I would expect in most cases people won't use the root-component parameters at all, and in the cases where they do, it's so that different host pages can have different global settings, or so that you could have a single root component shared across Blazor WebAssembly/Server/WebView and pass them some info or flags that differ based on the hosting environment. In these scenarios it wouldn't be something you'd try to mutate, and if you did try and nothing happened, that seems like it would be fine.

given that we might go to a different host page

As it stands, the BlazorWebView controls only let you configure a single fixed host page per control, so the scenario doesn't apply. You can write to the HostPage property on the control, but once it's initialized, it ignores any such changes. We might want to make it throw if you try to write to that property after initialization.

javiercn · 2021-03-03T17:30:39Z

src/Components/WebView/WebView/src/WebViewManager.cs

+        protected bool TryGetResponseContent(string uri, bool isPageLoad, out int statusCode, out string statusMessage, out Stream content, out string headers)
+            => _staticContentProvider.TryGetResponseContent(uri, isPageLoad, out statusCode, out statusMessage, out content, out headers);
+
+        internal async Task AttachToPageAsync(string baseUrl, string startUrl)


Also, "spicy" thought is that we need to have a "root component" SetParameters API here to dispatch parameter updates to top-level components.

Yes, if this becomes a common scenario then I definitely agree. However the usability of mutating root component parameters from a WPF/WinForms control might not be great, and there will always be alternatives like sending commands via some DI service. I'd suggest we wait to see some specific customer demand here before implementing this.

SteveSandersonMS · 2021-03-04T12:51:45Z

@javiercn @Eilon We should decide what milestone we're trying to reach before merging this into main. There's a lot of worthy functionality in here already, so I'd say it would be good to merge soon.

What are the remaining pieces we want to have in before merging?

The main one that comes to mind for me is more tests. There's opportunity to backfill a load of unit tests for WebViewManager, plus @javiercn is looking into E2E automation.
We need to add copyright headers to all the files. There might be other minor code formatting things to sort out, such as any missing XML docs, and the PublicAPI files.
We should check with @dougbu if there are any concerns we need to address about adding Windows-specific projects to the build. Doug, this PR is going to add a WPF project and a WinForms project, both targeting $(DefaultNetCoreTargetFramework)-windows. We hopefully exclude them from building on non-Windows machines - does that look correct and adequate to you?

Anything else?

Eilon · 2021-03-04T16:54:59Z

@SteveSandersonMS spot on. BTW for WinForms I took the lazy route and used your RootComponent type. Certainly not meant to be a final API, so thanks for updating it!

Regarding merging into main: Let's discuss today and we should easily be able to assign tasks to get them done, especially the more trivial ones (headers, doc comments, etc.).

dougbu · 2021-03-04T23:21:35Z

any concerns we need to address about adding Windows-specific projects to the build

We have a few projects that can only be built in msbuild e.g. https://github.com/dotnet/aspnetcore/pull/30456/files#diff-366b25472963c6b33188d84139f6c8997298c5adc857a2624ab0d26a2d87baf0R9-R14 and a few others that are platform-specific. As long as the projects listed in https://github.com/dotnet/aspnetcore/pull/30456/files#diff-366b25472963c6b33188d84139f6c8997298c5adc857a2624ab0d26a2d87baf0 work on ARM64, you should be good to go. But, beware of other projects that may reference those ones and make sure such references are also conditional.

Should be equivalent functionality and limitations to WPF.

* Layering cleanups * Tests and bug fixes

pranavkm

#yolo

Eilon · 2021-03-05T17:55:34Z

I retroactively sign off 😀

ghost · 2021-03-05T17:55:36Z

Hi @Eilon. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Pilchie added the area-blazor Includes: Blazor, Razor Components label Feb 25, 2021

Eilon reviewed Mar 1, 2021

View reviewed changes

src/Components/WebView/Platforms/Wpf/src/BlazorWebView.cs Show resolved Hide resolved

SteveSandersonMS force-pushed the javiercn/blazor-webview branch from d7fbb6d to 01dffd1 Compare March 2, 2021 10:31

javiercn commented Mar 2, 2021

View reviewed changes

Eilon reviewed Mar 2, 2021

View reviewed changes

campersau reviewed Mar 3, 2021

View reviewed changes

src/Components/WebView/Platforms/Wpf/src/BlazorWebView.cs Show resolved Hide resolved

javiercn commented Mar 3, 2021

View reviewed changes

javiercn and others added 14 commits March 5, 2021 03:17

Initial project structure

16d9115

tmp

b7c3420

tmp

869718a

tmp

4a0705e

Second checkpoint

2ca6888

Continue cleanups

72a1f73

Cleanups

a5a53dc

Make things build

382dd9a

Move shared files to shared dir

3f74734

Minor tidying

f03f28b

Refactors based on feedback

567150a

Renames, comments, etc

b843c50

Get rid of additional projects in favor of unit tests

79ba7d3

Cleanup and setup tests

d0ce755

Eilon and others added 17 commits March 5, 2021 03:17

Add WinForms BlazorWebView and sample

b817127

Should be equivalent functionality and limitations to WPF.

Experiment with tightening the API for adding root components

0dc1770

Apply better defaults for desktop apps

0611835

Don't log a bogus 404 error in the dev tools console

e5dbd56

More build exclusions

0a5c425

* Implement Dispose pattern in WebViewManager

052dbb5

* Layering cleanups * Tests and bug fixes

Fixing another bad Web2View name

6706656

Make tests compile

7a3d593

Add a test

eff89c8

Add public API baselines, cleanup, fix build issues

f775847

Add license headers

96d27a3

Try to fix CI failure for WebView2Loader during Pack

5dbcd57

Add missing doc comments

28d9c00

Fix npmproj

ad53fd6

tmp

e127e2f

Use EmbeddedManifestFileProvider and same Blazor Server MSBuild

300a772

Add missing doc comments and make them show up as warnings if missing

7a0be28

javiercn force-pushed the javiercn/blazor-webview branch from cedd707 to 7a0be28 Compare March 5, 2021 12:14

Update project references

96ec4b4

javiercn marked this pull request as ready for review March 5, 2021 16:42

javiercn requested review from dougbu and a team as code owners March 5, 2021 16:42

pranavkm approved these changes Mar 5, 2021

View reviewed changes

javiercn merged commit c899471 into main Mar 5, 2021

javiercn deleted the javiercn/blazor-webview branch March 5, 2021 17:52

Eilon mentioned this pull request Mar 5, 2021

Remove erroneous line from Microsoft.AspNetCore.Components.Server.csproj #30710

Merged

[Blazor][WebView] Abstractions #30456

[Blazor][WebView] Abstractions #30456

Uh oh!

Conversation

javiercn commented Feb 25, 2021

Uh oh!

javiercn commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SteveSandersonMS commented Mar 2, 2021

Uh oh!

javiercn commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eilon commented Mar 4, 2021

Uh oh!

dougbu commented Mar 4, 2021

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Eilon commented Mar 5, 2021

Uh oh!

ghost commented Mar 5, 2021

Uh oh!

Uh oh!

javiercn commented Feb 25, 2021 •

edited

Loading

SteveSandersonMS Mar 2, 2021 •

edited

Loading

SteveSandersonMS Mar 4, 2021 •

edited

Loading

SteveSandersonMS commented Mar 4, 2021 •

edited

Loading