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

WebView add asynchronous RunScript variant: RunScriptAsync #2316

Merged
merged 16 commits into from Nov 16, 2021

Conversation

TcT2k
Copy link
Contributor

@TcT2k TcT2k commented Apr 7, 2021

Add RunScriptAsync() to wxWebView

My current implementation and proposal is this:

    virtual void RunScriptAsync(const wxString& javascript, void* clientData = NULL) const;

When running RunScriptAsync() a new event wxEVT_WEBVIEW_SCRIPT_RESULT is triggered on success or failure. It will contain the specified clientData via GetClientData(). IsError() will be false if the execution was successful and GetString() will contain the scripts output/return value. On error IsError() will be true and an error message is available via GetString().

I've simplified the current javascript wrapper code to a single javascript call. Which is especially useful for async operation but also reduces calls to the native API from 3 times per RunScript() call to once.

I've included a new generic wxWebView::RunScript() implementation which will use the backends wxWebView::RunScriptAsync() to enable synchronous calls. This should unifiy the different backends implementation as the native API is asynchronous anyway (except for IE, where I think async execution will not be implemented).

ToDo List:

  • Complete documentation
  • Integrate into sample
  • Add unit tests?
  • Add IsError() to wxWebViewEvent
  • Simplify JavaScript Wrapper
    • Edge
    • IE
    • WKWebView
    • webkit2
  • Implement RunScriptAsync()
    • Edge
    • WKWebView
    • webkit2

As always any ideas feedback is welcome.

I would also like to know how to test easily test the webkit1 backend.

@vadz
Copy link
Contributor

vadz commented Oct 25, 2021

It seems that it would be nice to merge this (see https://trac.wxwidgets.org/ticket/19293) and I don't think anybody cares about IE by now, so we could well leave this unimplemented for it, but we do need some documentation and showing this in the sample and testing it would be also very nice.

Would anybody have time to do this before 3.1.6?

@TcT2k
Copy link
Contributor Author

TcT2k commented Oct 26, 2021

Would anybody have time to do this before 3.1.6?

I meant coming back to this (actually use it in an application of mine).
My main problem were a few failing unit tests (regression) based on my simplification of the wrapper code. Having to do with type conversion. Hopefully I can get some work done on this in the coming week.

@TcT2k TcT2k force-pushed the webview_runscript_improvements branch 2 times, most recently from 0ba9992 to 35a8715 Compare October 29, 2021 13:31
@TcT2k TcT2k force-pushed the webview_runscript_improvements branch from 35a8715 to 47833a6 Compare November 3, 2021 10:19
@TcT2k TcT2k changed the title WIP: WebView add asynchronous RunScript variant: RunScriptAsync WebView add asynchronous RunScript variant: RunScriptAsync Nov 3, 2021
@TcT2k TcT2k marked this pull request as ready for review November 3, 2021 13:05
@TcT2k
Copy link
Contributor Author

TcT2k commented Nov 3, 2021

@vadz
This should be ready to review and merge now. I've addressed all open issues I wanted to tackle.

RunScriptAsync() is available for all backends apart from IE.
Internally they all already used async execution that was made synchronously in RunScript().
As already stated RunScript() is now a common method that calls the backends RunScriptAsync() internally.

Copy link
Contributor

@vadz vadz left a comment

Choose a reason for hiding this comment

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

Excellent, thanks a lot!

I'm fine with merging this as is (maybe after adding the possibly missing initialization), but please let me know if you want, or if I should, apply the minor style/wording comments.

I'll leave this here for at least a few days to give others time to review and maybe test it in any case.

Thanks again!

include/wx/gtk/webview_webkit.h Outdated Show resolved Hide resolved
include/wx/private/jsscriptwrapper.h Outdated Show resolved Hide resolved
include/wx/webview.h Show resolved Hide resolved
interface/wx/webview.h Show resolved Hide resolved
src/common/webview.cpp Outdated Show resolved Hide resolved
src/common/webview.cpp Outdated Show resolved Hide resolved
include/wx/webview.h Show resolved Hide resolved
src/common/webview.cpp Outdated Show resolved Hide resolved
@vadz vadz added this to the 3.1.6 milestone Nov 4, 2021
@TcT2k
Copy link
Contributor Author

TcT2k commented Nov 5, 2021

@vadz I've added your suggestions.
Some time in the future RunScript() should probably be deprecated, but no need to do this right now.

@vadz vadz added the ok to merge Was reviewed and ok'd by at least one person label Nov 5, 2021
@vadz
Copy link
Contributor

vadz commented Nov 5, 2021

Great, thanks!

It looks 100% ready to me now, but, as I said, I'll leave it here for a few more days just in case others can/want to test it. I'll merge it next week if there are no comments.

@vadz
Copy link
Contributor

vadz commented Nov 16, 2021

No comments, so merging, thanks again!

@vadz vadz merged commit 63f83c0 into wxWidgets:master Nov 16, 2021
@TcT2k TcT2k deleted the webview_runscript_improvements branch November 16, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok to merge Was reviewed and ok'd by at least one person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants