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

[webview2loader] Add full featured go implementation #1974

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

stffabi
Copy link
Collaborator

@stffabi stffabi commented Oct 14, 2022

The new go loader can be activated with the exp_gowebview2loader build tag.

@stffabi stffabi requested a review from leaanthony October 14, 2022 07:55
@leaanthony
Copy link
Member

This is incredible! Might take a while for me to get through it but I'll aim to do that this weekend!

}

func (r *environmentCreatedHandler) EnvironmentCompleted(errorCode HRESULT, createdEnvironment *ICoreWebView2Environment) HRESULT {
// The OpenWebview2Loader has some retry logic and retries once, didn't encounter any case when this would have been
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps @jchv has more insight

Copy link

Choose a reason for hiding this comment

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

Unfortunately, I do not. I also never noticed a case where it is needed. If there is a good reason for it, one would probably have to peel back more layers to find it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your reply. So I think we could leave this as it is for now and see what the feedback will be from the users.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking the time to leave a comment @jchv - appreciate it 🙏

)

// Resolve the GoInterface of the specified ComInterfacePointer
func Resolve[T IUnknown](ifceP uintptr) T {
Copy link
Member

Choose a reason for hiding this comment

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

Love this approach 🚀

@stffabi
Copy link
Collaborator Author

stffabi commented Oct 16, 2022

This is incredible! Might take a while for me to get through it but I'll aim to do that this weekend!

Thanks so much, was a great experience with all the com specific stuff 😀

@leaanthony
Copy link
Member

Never thought I'd ever see "com" and "great experience" in the same sentence 😂 Awesome stuff 👍

@stffabi
Copy link
Collaborator Author

stffabi commented Oct 16, 2022

Never thought I'd ever see "com" and "great experience" in the same sentence 😂 Awesome stuff 👍

That was more meant from a technical point of view of interacting with com and playing nicely together with the go runtime 😀

@stffabi stffabi force-pushed the feature/go-webview2loader branch from 3ae5045 to ddb7726 Compare October 16, 2022 12:23
Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

Looking really good! I've left a couple of very minor questions that are no biggies.

What do you think we need to have in the documentation for people to try it out and get feedback? Is this a flag you'd pass with the -flags parameter for wails build?

I noticed the comment on supporting all the features so we could do away with the dll. What scenarios would we still need it?

@stffabi stffabi force-pushed the feature/go-webview2loader branch 2 times, most recently from ccc859b to 599f5a4 Compare October 18, 2022 10:03
@stffabi
Copy link
Collaborator Author

stffabi commented Oct 18, 2022

Looking really good! I've left a couple of very minor questions that are no biggies.

Thanks for reviewing it ❤️

What do you think we need to have in the documentation for people to try it out and get feedback? Is this a flag you'd pass with the -flags parameter for wails build?

It's a build tag to be passed as -tags: wails build --tags exp_gowebview2loader. As for documentation I hadn't a good idea where to put it, do you have a good idea?

I noticed the comment on supporting all the features so we could do away with the dll. What scenarios would we still need it?

The following features are not supported and I think we explicitly don't want or need them:

  • Registry Overrides of Parameters: I don't think we want that someone (enduser) can just override the WebView2 options to be used by changing the registry key which maps to the binary. This would e.g. also allow to active the debugger of the webview.
  • Env Variable Overrides of Parameters: Same as above, from a security perspective I think even more dangerous. This would also allow to active the debugger by just starting the binary with a env variable. We already disable this for the native loader as well.
  • Does not incorporate GetCurrentPackageInfo to search for an installed runtime: This would allow override the runtime to be when bundled in an appx. So I think not relevant for Wails.

I think the options to be used should always be set by the Wails application and not by some user, which would allow to inspect internals of the app or change the behaviour of it.

@leaanthony
Copy link
Member

I think we could add these details to the output of wails build to invite people to trial it?

@stffabi
Copy link
Collaborator Author

stffabi commented Oct 18, 2022

Oh, yeah, that's a great idea. Will add that...

@stffabi
Copy link
Collaborator Author

stffabi commented Oct 18, 2022

Just added the information to wails build, what do you think about that?

@leaanthony
Copy link
Member

leaanthony commented Oct 19, 2022

Yeah, that's great. I'm wondering if we should open a ticket/discussion for capturing the feedback and have that link at the bottom of the CLI output. The issue/discussion should have a bit of detail there: What is the benefit of using it, gotchas, maybe a test matrix so people can report it works with wails doctor output. Thoughts?

@stffabi stffabi force-pushed the feature/go-webview2loader branch from f5d64bc to a913766 Compare October 19, 2022 09:45
@stffabi
Copy link
Collaborator Author

stffabi commented Oct 19, 2022

Yeah, that sounds great.

@stffabi stffabi force-pushed the feature/go-webview2loader branch from a913766 to 9c6c433 Compare October 20, 2022 07:22
@stffabi stffabi force-pushed the feature/go-webview2loader branch from 9c6c433 to ea81396 Compare October 21, 2022 07:25
@stffabi
Copy link
Collaborator Author

stffabi commented Oct 21, 2022

@leaanthony just added the link to the Feedback - Issue. For the content of the issue we could discuss that in slack.

So I think we are good to go with this one, what do you think?

@leaanthony leaanthony merged commit 0a20c8d into wailsapp:master Oct 21, 2022
@stffabi stffabi deleted the feature/go-webview2loader branch October 21, 2022 14:37
@stffabi
Copy link
Collaborator Author

stffabi commented Oct 21, 2022

Awesome, thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants