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

Custom WebView2Loader implementation #783

Merged
merged 73 commits into from
Jan 19, 2023

Conversation

SteffenL
Copy link
Collaborator

@SteffenL SteffenL commented Jun 25, 2022

As we do not have access to the source code of Microsoft's WebView2Loader library, it causes several annoying issues:

  • MinGW-w64/GCC can only link it dynamically while Visual C++ can also link it statically.
  • Go/cgo uses MinGW-w64/GCC which excludes the option to link it statically.
  • Developers must manually manage the WebView2 dependency and make sure to bundle and ship the correct DLLs.
  • Go/cgo tooling does not currently support a workflow that makes all of this automatic for those who use our Go bindings. Instead they are by default met with an error due the WebView2 header not being found if the environment is not set up according to our instructions (note 1 and 3).
  • It is not obvious how to set up an IDE to work on one's own project when using our Go bindings due to the WebView2 dependency.

This PR attempts to reimplement parts of the WebView2Loader in order to eliminate the need for the binaries. We still need the header (note 2 and 3). After starting a reverse engineering effort I figured that WebView2Loader.dll is not actually doing a whole lot and merely provides some convenience for finding and using the client DLL of the browser (EmbeddedBrowserWebView.dll).

I found OpenWebView2Loader which I then relied on as my RE effort became more or less redundant. One can simply drop in this open-source implementation and it works just fine. We do not need the exact behavior of the official WebView2Loader which is why I did not do this.

This work is not yet complete but it is functional and I think it is a good time to begin discussions.

This branch has instructions (e.g. URLs) changed specifically for this work in order to test the work before a merge:

https://github.com/SteffenL/webview/tree/testing/custom-webview2-loader-impl

Noteworthy Changes

  • Minimalistic reimplementation of the features we need from WebView2Loader.
  • If the official loader DLL (WebView2Loader.dll) is present at runtime then it will be explicitly linked at runtime and used instead of our custom implementation. This also eliminates the need for an import library (*.lib file). See note 4.
  • Readme has been updated to reflect that our loader implementation will be used when the official one is not present. The WebView2 SDK requirement is about the same as before.

Notes

  1. Go has code generation support but it requires users to manually run go generate before building their programs. As an experiment I wrote a tool for fetching WebView2 and setting up the Go environment. This tool can be invoked via go generate by adding //go:generate go run github.com/webview/[...] to the go source file. It works well but it is not a natural workflow for Go.
  2. Technically we can generate the header file of WebView2 by compiling the WebView2.idl file using the MIDL Compiler. That works fine but requires Visual Studio/C++ tools. It would be nice to be able to get WIDL (Wine Interface Definition Language (IDL) compiler) to generate a working header as it is part of MinGW-w64 but the generated code gave me compile errors.
  3. WebView2 SDK can be pulled in and used by Go bindings using vendoring. To reduce the scope of this PR this is not included in this PR anymore.
  4. To get this exact behavior, projects using this library should stop unnecessarily linking the loader's import library (*.lib file). Failing to do so should however produce the same result because the linker (at least in case of MSVC) sees that no exported symbols are used.

To Do

  • CreateCoreWebView2EnvironmentWithOptions
  • GetAvailableCoreWebView2BrowserVersionString
  • Browser version check.
  • Support WebView2 runtimes installed system-wide.
  • Support WebView2 runtimes installed per-user.
  • Custom browser executable folder.
  • Retry logic for WebView2 initialization.

Won't Do

  • Overrides for options via environment variables, e.g. WEBVIEW2_BROWSER_EXECUTABLE_FOLDER.
  • Support Microsoft Edge Insider (preview) release channels (Beta, Dev, Canary).
  • Non-essential WebView2Loader.dll exports.

@dandeto
Copy link
Collaborator

dandeto commented Jun 26, 2022

WebView2Loader.lib is the static library. MinGW can't link that? That's a bummer. Many projects ship with dlls without a fuss, but I understand the desire to ditch them, of course. It's still possible to compile with a static lib using the windows compiler, but not for Go. It looks like supporting Go and other bindings is the main issue here, correct? They will need to depend upon a dynamic library without the option of a static one?

Replacing the WebView2Loader with the opensource alternative is something I will look into a little more. I think most people will want the official one just for the sake of it's status and stability, but I could see those who are affected by the MinGW linking deficiency wanting this option to be available.

@SteffenL
Copy link
Collaborator Author

SteffenL commented Jun 26, 2022

The naming style of the lib files is a bit unusual:

  • WebView2Loader.dll.lib - shared.
  • WebView2LoaderStatic.lib - static.

Unfortunately, the static one is not usable by MinGW-w64/GCC. Static/Dynamic linking should always be an option in my opinion but users of Go and MinGW-w64 users do no thave that option.

Much of the issue has to do with Go/cgo since it uses MinGW-w64/GCC and I believe it breaks the normal worklow of a Go developer if it does not by default link statically. It is not limited to Go though since C/C++ users have the option to use MinGW-w64 instead of Visual C++ unlike before. Other bindings would also be able to benefit from this. I personally build the library mainly using MinGW-w64 now in order to have a consistent workflow across platforms.

Instead of fully replacing WebView2Loader with the open-source alternative I would rather reimplement only the minimum amount of features needed to ensure that we can maintain stability and good quality code. It also uses WRL which is is something we eliminated recently to be more compatible with MinGW-w64 and possibly old versions of Windows, so I do not think we should reintroduce that.

For example I do not think that we need to be able to select at runtime which release channel to use of the browser engine, or to have the option to use an embedded browser or installed browser.

I think that the loader could be considered as a middleman that can be eliminated without any major consequences.

The only real downside I can think of is this:
What if we ever need to update the reimplementation of the loader in the future? I do not really think we need to because the browser functionality is not in the loader itself. The only thing I think that we may have to update in the future is the WebView2 header in order to use new browser features. I think that updating the header is unlikely to be necessary to the point that it affects the loader if it is at all possible.

But what if we really need to update the loader implementation for whatever reason? In that case we need to either do a bit of reverse engineering again or rely on someone else's reimplementation e.g. OpenWebView2Loader. It is not a given that anyone will provide an open-source alternative for any future version of the loader. If it ever turns out to look that grim some time in the future then we can still fall back to the official loader.

One idea is that we can at runtime check if the official loader is present and then use that instead of the reimplementation. Otherwise we just use our reimplementation.

With all this said I would argue that:

  • Most end-users would not care as long as it works, but they are not the target users we are solving the problem for.
  • Many if not most Go developers using our library and Go bindings do not want the pain that comes with dealing with cgo and managing dependencies (DLLs) coming from the core library with no obvious way to do it correctly. Even with instructions it is more painful than not having to deal with it at all. They normally do not have to know about these details so I believe that it is quite unexpected from their point of view.
  • Some Go developers likely think that this is important and want options, but I would like to hear their arguments for it.
  • Since C/C++ developers have to concern themselves with details all the time, they are much more likely to also be concerned about this. I do however think that most of them would appreciate one less thing to be concerned with. Is it really that important? Maybe it is to some of them.

The most important things here is that we need to do more to make people want to use the webview library, and not give them reasons to go somewhere else or just fork without contributing back to the project.

We definitely need a good default behavior, backward-compatibility and seamless fallback. I also do not think that the loader implementation is of great importance. In any case I think that the benefits of having the option available outweights the negative sides.

@SteffenL
Copy link
Collaborator Author

SteffenL commented Jun 26, 2022

Commit 0f256cf prefers WebView2Loader.dll if present. I tested mismatching versions: WebView2.h version 1.0.1150.38; WebView2Loader.dll version 1.0.1245.22. This is probably safe but I think we might need a version check for the other way around in addition to checking the browser version.

@SteffenL
Copy link
Collaborator Author

Regarding the embedded WebView2 header, alternatives I can imagine for that right now is to either split the Go bindings from the main Git repository (preferable in my opinion), or if possible move the WebView2 header into a Git submodule if that is something Go/cgo can work with.

I am open to different ideas.

@justjosias
Copy link
Member

justjosias commented Jun 26, 2022

For historical reasons, I don't think we can split the Go bindings. Do you know of a way we can do that without breaking existing dependents?

@SteffenL
Copy link
Collaborator Author

SteffenL commented Jun 26, 2022

I imagine that existing users of this repo should be locked to a specific version/commit so it would only affect new dependents or those who try to upgrade.

I have not tested this but I imagine we could redirect those who continue using this repo, e.g. something like this:

replace github.com/webview/webview v1.2.3 => github.com/webview/webview-go v1.2.3

I will experiment a bit with this idea.

@SteffenL
Copy link
Collaborator Author

SteffenL commented Jun 28, 2022

I have not gotten my idea above with replace to work so I have given up on that.

After quite a journey with trial and error, I believe that I have figured out an acceptable way to split the Go bindings from the main repository.

Since Go pins versions (Git tags or commits) of dependencies then I believe that we have the option to remove the bindings from the main repository. Google caches the Go module so that it should keep working for current dependents even if we were to delete the whole repository (not tested). They just cannot upgrade to a new version.

We can also either strip down the Go module in the main repository or keep it as-is but issue a compile warning or error via cgo using #warning or #error since we are using cgo anyway:

//#warning The webview Go bindings have moved to https://github.com/webview/webview-go - please see the project readme.
import "C"

This way people should still be able to "upgrade" and get this message after doing so for as long as they are using the old Go module.

MS WebView2 can be moved into a separate Git repository that we have to maintain. If we use my custom implementation of the loader then we do not need to host the lib/DLL files.

All of this will be tied together using so-called vendoring. Since we specify Go 1.13 as the minimum version in our go.mod file then we and dependents must add the flag -mod=vendor to the go build command. That flag became default since Go 1.14 so if we bump the minimum Go version we require to 1.14 then vendoring will apply automatically and will be more convenient for everyone who can use Go 1.14 and up.

What vendoring does is copy Go packages from dependencies into a vendor directory in the project directory and the mentioned flag makes Go consider the modules there when building instead of the local module cache (which will have unpredictable file paths).

In order to get all the non-Go sources we need into the vendor directory then those files must be in a directory that has Go sources. We can add dummy Go packages in each directory that we need to have in the vendor directory. That's just a one-line *.go file. This is an approach other people have taken. There is a long discussion at golang/go#26366 closed with no official solution. Unfortunately this means that Go will pollute the main repository.

This way we are able to specify predictable include paths to the C/C++ compiler such as the following:

#cgo CFLAGS: -Ivendor/github.com/webview/webview -I../../../github.com/webview/webview
#cgo CXXFLAGS: -Ivendor/github.com/webview/webview -Ivendor/github.com/webview/mswebview2 -I../../../github.com/webview/webview-nogo -I../../../github.com/webview/mswebview2

-Ivendor/... would be for the current project to find its dependencies, and -I../... would be for dependencies to find their dependencies.

Dependents must be made aware of vendoring and run go mod vendor at least once before go build.

One downside with this approach is that there is no way to control which dependencies end up in the vendor directory. Instead everything ends up there.

@SteffenL
Copy link
Collaborator Author

This PR has now been configured for vendoring. The purpose of this PR is not splitting the Go bindings so I have left that for another time.

I have a complete example at the following locations:

You can try it easily like this:

git clone https://github.com/SteffenL/webview-go-example
cd webview-go-example
go mod vendor
go build
# Go <1.14: go build -mod=vendor
webview-go-example.exe

@SteffenL SteffenL force-pushed the up/custom-webview2-loader-impl branch from 824e8b4 to a754134 Compare June 28, 2022 08:09
webview.h Outdated Show resolved Hide resolved
@dandeto
Copy link
Collaborator

dandeto commented Jan 10, 2023

The only downside I see to this is the added complexity to the single-header application, but all of the upsides are worth it.

The benefits of static-linking are obvious, and the fact that Go benefits so much from it is additionally compelling. I think getting rid of WebView2Loader.dll is great, even though Webview2.h is still required. It's one less thing we need to worry about. I'm glad the code you wrote is backwards-compatible so that if our implementation ever breaks in the future, users can still link WebView2Loader.dll.

Will the versioning code in #766 conflict with the versioning code found in this PR?

2 separate discussions worth having:
Splitting Go bindings
Potentially refactoring the single header library into maybe 2 or more headers

I support merging this. Do you have any additional thoughts @justjosias?

@SteffenL
Copy link
Collaborator Author

@dandeto If we split the WebView2Loader reimplementation into a separate file, I wonder if we might have some issues with the header inclusion order since webview.h should probably not depend on the new file.

Since #889 introduced WEBVIEW_MSWEBVIEW2_EXPLICIT_LINK, it might make sense to introduce something like WEBVIEW_MSWEBVIEW2_BUILTIN_IMPL as well to conditionally enable the built-in implementation of WebView2Loader.

@SteffenL
Copy link
Collaborator Author

The latest work disables the built-in WebView2 loader implementation by default because WEBVIEW_MSWEBVIEW2_EXPLICIT_LINK was introduced recently.

The following compile-time options can be used to change how the library integrates the WebView2 loader:

  • WEBVIEW_MSWEBVIEW2_EXPLICIT_LINK - The library supports linking WebView2Loader.dll explicitly, avoiding the need for import libraries (*.lib).
  • WEBVIEW_MSWEBVIEW2_BUILTIN_IMPL - Enables the built-in implementation of the WebView2 loader. Implies WEBVIEW_MSWEBVIEW2_EXPLICIT_LINK to maximize compatibility.

I would personally like the built-in implementation to be enabled by default, with the official one taking priority. Maybe we can invert the options so that those who do not want this can explicitly disable the features rather than explicitly enable them. This is up for discussion.

@dandeto
Copy link
Collaborator

dandeto commented Jan 18, 2023

@SteffenL I agree with you - I think it's best to have the built-in implementation enabled by default so that we get all of the benefits by default. I think most users aren't going to need to use the official loader.

@SteffenL
Copy link
Collaborator Author

All right. I think we can merge this first and then take care of the "inverted" options so that the built-in implementation is enabled by default. That will be the most beneficial approach for sure. Thank you for the review!

@SteffenL
Copy link
Collaborator Author

SteffenL commented Jan 18, 2023

We could also just keep it simple, remove the compile-time options and always compile the built-in implementation with explicit linking of the official one taking priority.

Existing users will not notice any difference and there are plans to introduce error codes at some point so that a missing loader DLL does not cause silent failure.

What do you think, @dandeto?

@SteffenL SteffenL force-pushed the up/custom-webview2-loader-impl branch from 3a4ab37 to 132a2f4 Compare January 19, 2023 02:00
@SteffenL SteffenL force-pushed the up/custom-webview2-loader-impl branch from af8de6e to e9d9c76 Compare January 19, 2023 03:10
@SteffenL
Copy link
Collaborator Author

The options were kept in the latest commit but defaults were changed:

  • Enable built-in WebView2Loader implementation by default.
  • Link WebView2Loader.dll explicitly by default only if the built-in implementation is enabled.
*_BUILTIN_IMPL *_EXPLICIT_LINK Effect
(undefined) (undefined) Built-in impl ON, explicit link ON.
1 (undefined) Same as above.
(undefined) 1 Same as above.
0 0 Built-in impl OFF, explicit link OFF.
0 1 Built-in impl OFF, explicit link ON.
1 0 Error - needs *_EXPLICIT_LINK=1.
1 1 Built-in impl ON, explicit link ON.

@SteffenL SteffenL force-pushed the up/custom-webview2-loader-impl branch from 4ea91f8 to e656571 Compare January 19, 2023 03:31
@SteffenL SteffenL force-pushed the up/custom-webview2-loader-impl branch from e656571 to c23f394 Compare January 19, 2023 03:37
@SteffenL
Copy link
Collaborator Author

Given that I adressed @dandeto's comment about making the built-in loader the new default (as originally intended) after the PR was approved while also leaving room to disable the options, I am going to merge this so that we can move on. Feel free to bring any new concerns to my attention so that I can address those later.

@SteffenL SteffenL merged commit 14f8e24 into webview:master Jan 19, 2023
@SteffenL SteffenL deleted the up/custom-webview2-loader-impl branch January 19, 2023 22:27
@SteffenL SteffenL added this to the v0.10.0 milestone Sep 1, 2023
@SteffenL SteffenL added the enhancement New feature, enhancement of an existing feature or a request label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, enhancement of an existing feature or a request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants