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

Support multiple webviews on a single WebContext (webkit2gtk) #359

Merged
merged 15 commits into from
Jul 29, 2021

Conversation

chippers
Copy link
Member

What kind of change does this PR introduce? (check at least one)

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

Does this PR introduce a breaking change? (check one)

  • Yes.
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
This PR uses locks to prevent an unknown concurrency bug with webkit2gtk from happening when using load_uri on multiple WebViews in a shared WebContext. The shared WebContext is needed for multi-window automation to take place with webkit2gtk.

Ideally, before release I would also like to see the following improvements:

  1. Move with_custom_protocol to WebContext on all platforms. This involves removing &Window from the handler.
    • The concept of the custom protocol should be application wide, not window wide. It is confusing that the same exact URI can cause different behavior depending on which window it was opened in.
    • All of our examples and benchmarks currently just wildcard (_) out the &Window and ignore it.
    • When using multiple windows on linux (I don't believe on Windows and IDK about macos), the Window that registered the custom protocol is passed instead of the Window that it is running on.
  2. The replacement for item 1 returns a Result so that wry::Error::DuplicateCustomProtocol(String) can be passed down.

The breaking change is that duplicate registering of custom protocols on unix will now fail. Without the improvements listed above, the solution for multi window applications is to add something like

let mut webview = /* WebviewBuilder stuff */;
let mut set_protocol = false;
if !set_protocol {
  webview = webview.with_custom_protocol("wry".into(), |_window, _uri| todo!());
  set_protocol = true;
}

@chippers chippers requested a review from a team as a code owner July 27, 2021 19:53
@chippers chippers requested a review from a team July 27, 2021 19:53
@chippers
Copy link
Member Author

I had forgotten that I messed around with more succinct os specific cfgs. let me know if I should keep or remove those changes

Copy link
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

Just took a look and I think fine to add these if there's no other way to do so.
Maybe we could report to upstream first since it's more like a webkitgtk issue.

I suppose we can remove Windows param in custom protocol first if even tauri doesn't use it.
And the config flags update could also open another PR for a quick merge.

src/webview/web_context.rs Outdated Show resolved Hide resolved
drop(protocols);

if exists {
Err(Error::DuplicateCustomProtocol(name.to_string()))
Copy link
Member

Choose a reason for hiding this comment

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

The conflict of duplocate custom protocol only happen if they share same web context.
I think this should let users to decide and we could add some warnings on the behaviour of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, in my item #2 the with_custom_protocol returns a Result like some of the other builder items so that the user can choose to ignore it instead of it popping off in the InnerWebView constructor. I think we can support custom protocols on both the WebviewBuilder and the WebContext, where using WebviewBuilder it will just swallow this result inside the webview constructor, and from WebContext it allows the user to handle it how they want

Copy link
Member Author

@chippers chippers Jul 28, 2021

Choose a reason for hiding this comment

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

I went with register_uri_scheme and try_register_uri_scheme. Both will return an Err(DuplicateCustomProtocol) when a duplicate is passed, but the non-try one will still pass it to the underlying platform (only webkit2gtk in this instance). The code in InnerWebview has been updated to keep the old behavior. I'll leave this conversation open if you think we should resolve it another way.

I decided to keep it like this because I think there should be another PR after this for adding the register URI functions to the WebContext itself, which will expose both. The current WebviewBuilder method if we keep it will continue to use the existing way

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to add both methods. But for adding URI method to WebContext should be exclusive on Linux imho.
Mac and Windows handle them differently (and it's even a hack on Window for now). They probably don't need it.

@chippers chippers requested a review from a team July 28, 2021 21:52
@chippers
Copy link
Member Author

This should be done if that last conversation resolves. I am going to go ahead and open up another PR from this branch for removing Window.

@wusyong wusyong merged commit 3f03d6b into dev Jul 29, 2021
@wusyong wusyong deleted the feat/webdriver-multiwindow branch July 29, 2021 02:15
@github-actions github-actions bot mentioned this pull request Jul 29, 2021
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.

2 participants