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

feat(windows): Add the implementation and examples of the host object for the Windows system. #1109

Closed
wants to merge 1 commit into from

Conversation

defims
Copy link

@defims defims commented Dec 1, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Checklist

Other information

My project wvwasi needs synchronous methods, so I implemented add_host_object_to_script for Windows in this pull request.

@defims defims requested a review from a team as a code owner December 1, 2023 05:18
@defims defims mentioned this pull request Dec 1, 2023
@defims defims changed the title Add the implementation and examples of the host object for the Windows system. feat(windows): Add the implementation and examples of the host object for the Windows system. Dec 1, 2023
@wusyong
Copy link
Member

wusyong commented Dec 4, 2023

I have tried to add this feature before, but this is a pretty dangerous one IHMO. And it only supports Windows.
However, I saw many other issues are requesting this feature: #1110
So the best approach to add this is probably move this to platform specific trait.

@defims
Copy link
Author

defims commented Dec 4, 2023

I have tried to add this feature before, but this is a pretty dangerous one IHMO. And it only supports Windows. However, I saw many other issues are requesting this feature: #1110 So the best approach to add this is probably move this to platform specific trait.

@wusyong I agree, so I put the method in WebViewExtWindows, and I don't do any additional packaging, just leave the creation of hostObject to the user, and they will handle the risks on their own.

@defims defims force-pushed the feat/windows/host-object branch 2 times, most recently from 248c849 to 8040345 Compare December 7, 2023 09:04
@defims
Copy link
Author

defims commented Dec 8, 2023

@wusyong I don't know how to fix that two error checks. Would you help me fix it or just ignore it?

@wusyong
Copy link
Member

wusyong commented Dec 11, 2023

@defims Could you format the code at least? The doc CI seems broken at the moment. #1117 is the tracking issue.
I can merge this one once other CI pipelines are passed.

@defims
Copy link
Author

defims commented Dec 11, 2023

@defims Could you format the code at least? The doc CI seems broken at the moment. #1117 is the tracking issue. I can merge this one once other CI pipelines are passed.

@wusyong Sorry, I didn't notice that one of them was a formatting issue. Of course, I formatted it and submitted it. Please check it again.😊

@dklassic
Copy link
Contributor

Hey @defims thanks for contributing! Though we have enforced commit signing so I'll need you to sign your commit before we can merge it.

@defims defims force-pushed the feat/windows/host-object branch from f4e6b44 to 6adc311 Compare December 11, 2023 14:02
@defims
Copy link
Author

defims commented Dec 11, 2023

Hey @defims thanks for contributing! Though we have enforced commit signing so I'll need you to sign your commit before we can merge it.

@dklassic My pleasure. I have resubmitted it and signed the commit. Please check it again.😊

@amrbashir
Copy link
Member

Looks like this PR is only exposing APIs that can be accessed through webview.controller and I don't see any value in adding it here tbh.

@defims
Copy link
Author

defims commented Dec 12, 2023

Looks like this PR is only exposing APIs that can be accessed through webview.controller and I don't see any value in adding it here tbh.

@amrbashir Thank you for your review.

HostObject mainly provides synchronous methods. Existing methods such as synchronous XHR are already restricted to worker in mainstream browsers, mainly due to the concern of blocking the rendering process.

However, the situation is different in the webview. Webview is mainly used on the device, which is more controllable. In some scenarios, such as some synchronous libraries heavily dependent on C/C++, it is not realistic to rewrite the library completely. In this case, synchronous methods are more important than blocking the rendering process.

There are other synchronous scenarios, which are not listed here. Of course, these scenarios are rare, so I personally do not recommend using synchronous methods as the main communication method.

For this reason, I have not wrapped related methods too much. Instead, I only expose basic APIs on WebViewExtWindows. This allows us to cover a small number of applicable scenarios with the lowest maintenance cost. We also do not need to consider the burden of use too much, as users can manage the burden themselves.

WKWebView and WebKitGTK currently do not have similar APIs. The cost of simulating them with existing interfaces is too high, so they have not been added yet. If the relevant API documentation is provided, I will be happy to add them and encapsulate them into a unified interface.

@defims
Copy link
Author

defims commented Dec 12, 2023

Same goes for #1115

@defims defims closed this Dec 12, 2023
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.

feat: synchronous communication between webview and rust
4 participants