-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add bindings and expose Navigation Starting and Navigation Completed #53
base: master
Are you sure you want to change the base?
Conversation
Rework navigation completed callback signature. Bind and handle navigation starting and navigation completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing. Apologies for the delays in getting a review, I have been very busy.
Unfortunately I have a lot of review comments on this one because it touches on a lot of tricky things and API design decisions that may not be outwardly apparent. I'm happy to try to help, although if you would like I can also just do the work too.
|
||
func (w *webview) navigationCompletedCallback(httpStatusCode int32, isSuccess bool, navigationId uint64, webErrorStatus int32) { | ||
w.m.Lock() | ||
var f, ok = w.bindings[NavigationCompletedBindingName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This binding mechanism is meant for the JavaScript-Go binding system. Instead, just use normal typed function pointers inside the webview struct, like is done in the chromium struct.
These handlers (navigationCompletedCallback
, navigationStartingCallback
) would be the ideal place to put the logic to grab the event args and throw them into a Go struct for the callback.
@@ -5,6 +5,7 @@ type _ICoreWebView2NavigationCompletedEventArgsVtbl struct { | |||
GetIsSuccess ComProc | |||
GetWebErrorStatus ComProc | |||
GetNavigationId ComProc | |||
GetHttpStatusCode ComProc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unsafe; GetHttpStatusCode
is only available on the extended ICoreWebView2NavigationCompletedEventArgs2
class. You need to define a new COM wrapper for ICoreWebView2NavigationCompletedEventArgs2
instead.
Take a look at how it is done for ICoreWebView2_3
: https://github.com/jchv/go-webview2/blob/2269a8d58f7e49372b823d9171948b3ae1993539/pkg/edge/ICoreWebView2_3.go
Notice the function GetICoreWebView2_3
which allows you to get a ICoreWebView2_3
from a ICoreWebView2
. The same thing is needed for ICoreWebView2NavigationCompletedEventArgs2
.
It should wind up looking something like this:
package edge
...
type _ICoreWebView2NavigationCompletedEventArgs2Vtbl struct {
_ICoreWebView2NavigationCompletedEventArgsVtbl
GetHttpStatusCode ComProc
}
type ICoreWebView2NavigationCompletedEventArgs2 struct {
vtbl *_ICoreWebView2NavigationCompletedEventArgs2Vtbl
}
func (i *ICoreWebView2NavigationCompletedEventArgs) GetICoreWebView2NavigationCompletedEventArgs2() *ICoreWebView2NavigationCompletedEventArgs2 {
var result *ICoreWebView2NavigationCompletedEventArgs2
iidICoreWebView2NavigationCompletedEventArgs2 := NewGUID("{FDF8B738-EE1E-4DB2-A329-8D7D7B74D792}")
_, _, _ = i.vtbl.QueryInterface.Call(
uintptr(unsafe.Pointer(i)),
uintptr(unsafe.Pointer(iidICoreWebView2NavigationCompletedEventArgs2)),
uintptr(unsafe.Pointer(&result)))
return result
}
@@ -331,8 +339,83 @@ func boolToInt(input bool) int { | |||
|
|||
func (e *Chromium) NavigationCompleted(sender *ICoreWebView2, args *ICoreWebView2NavigationCompletedEventArgs) uintptr { | |||
if e.NavigationCompletedCallback != nil { | |||
e.NavigationCompletedCallback(sender, args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Chromium
abstraction should continue passing the raw ICoreWebView2NavigationCompletedEventArgs
object down. Some users may already depend on this functionality.
This logic could be moved to webview.go
instead. Although webview.go
mostly avoids having actual Edge-related code in it, I do not see it as a huge problem to add it in this case.
} | ||
|
||
func (e *Chromium) NavigationStarting(sender *ICoreWebView2, args *ICoreWebView2NavigationStartingEventArgs) uintptr { | ||
if e.NavigationStartingCallback != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make the Chromium
interfaces uniform, as above, this should pass the raw ICoreWebView2NavigationStartingEvent
rather than fetching the information directly.
fValue.Call(fArgs) | ||
} | ||
|
||
func (w *webview) NavigationCompleted(f func(httpStatusCode int32, isSuccess bool, navigationId uint64, webErrorStatus int32)) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new API for this, please use the WebViewOptions
function to add this kind of event handler. The function pointer passed in from NewWithOptions
can be saved to the webview
struct. This is preferred especially because the go-webview2 Webview
interface is frozen to remain compatible with webview/webview
.
var _, _, _ = args.vtbl.GetIsSuccess.Call( | ||
uintptr(unsafe.Pointer(args)), | ||
uintptr(unsafe.Pointer(&isSuccess)), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is not strictly necessary, a way this code could be improved is by adding helpers into the ICoreWebView2NavigationCompletedEventArgs
struct. For example,
func (a *ICoreWebView2NavigationCompletedEventArgs) IsSuccess() bool {
var isSuccess uint
var _, _, _ = args.vtbl.GetIsSuccess.Call(
uintptr(unsafe.Pointer(args)),
uintptr(unsafe.Pointer(&isSuccess)),
)
return isSuccess == 1
}
(These should also error-check, but unfortunately that's already missing in a lot of places, so it's not really a big deal.)
return firstValue.Bool() | ||
} | ||
|
||
func (w *webview) NavigationStarting(f func(additionalAllowedFrameAncestors string, isRedirected bool, isUserInitiated bool, navigationId uint64, uri string) bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of the callbacks have rather long strings of arguments. I think it would be easier and more future-proof to make structs to hold these arguments, like:
type NavigationStartingEventArgs struct {
AdditionalAllowedFrameAncestors string
IsRedirected bool
IsUserInitiated bool
NavigationID uint64
URI string
}
Then the closure type could just be
func(args NavigationStartingEventArgs) bool
Ditto for NavigationCompleted.
// f must be a function | ||
// f must return true, to proceed with the navigation | ||
// f must return false, to cancel the navigation | ||
NavigationStarting(f func(additionalAllowedFrameAncestors string, isRedirected bool, isUserInitiated bool, navigationId uint64, uri string) bool) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned elsewhere, the WebView
interface is frozen to preserve compatibility with webview/webview
. The best way to add new functionality like this right now is to allow the user to pass a handler into the options struct of NewWithOptions
.
@@ -0,0 +1,21 @@ | |||
package edge | |||
|
|||
type _ICoreWebView2NavigationStartingEventArgsVtbl struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two problems here:
- Like
ICoreWebView2NavigationCompletedEventArgs
, there is a corresponding extended event args object forICoreWebView2NavigationStartingEventArgs
called, predictably,ICoreWebView2NavigationStartingEventArgs2
. As I outlined withGetHttpStatusCode
, it is not safe to include these directly without callingQueryInterface
first. The GUID forICoreWebView2NavigationStartingEventArgs2
is9086BE93-91AA-472D-A7E0-579F2BA006AD
. - This Vtable is out of order, which will lead to the wrong functions being called.
The Vtable order for ICoreWebView2NavigationStartingEventArgs
is:
[propget] HRESULT Uri([out, retval] LPWSTR* uri);
[propget] HRESULT IsUserInitiated([out, retval] BOOL* isUserInitiated);
[propget] HRESULT IsRedirected([out, retval] BOOL* isRedirected);
[propget] HRESULT RequestHeaders([out, retval] ICoreWebView2HttpRequestHeaders** requestHeaders);
[propget] HRESULT Cancel([out, retval] BOOL* cancel);
[propput] HRESULT Cancel([in] BOOL cancel);
[propget] HRESULT NavigationId([out, retval] UINT64* navigationId);
(Note that your names are fine. The propget
/propput
is syntax sugar in COM.)
And ICoreWebView2NavigationStartingEventArgs2
:
// (... everything from the original ICoreWebView2NavigationStartingEventArgs)
[propget] HRESULT AdditionalAllowedFrameAncestors([out, retval] LPWSTR* value);
[propput] HRESULT AdditionalAllowedFrameAncestors([in] LPCWSTR value);
(Note that there is a put from AdditionalAllowedFrameAncestors
as well as the get you already have.)
If you are wondering where this information comes from, it comes from the WebView2 NuGet package. You can download that here:
https://www.nuget.org/packages/Microsoft.Web.WebView2
Just click "Download Package". The corresponding nupkg
file is a ZIP archive, and once you unzip it you will see WebView2.idl
which contains the full COM IDL for all WebView2-related classes.
MSDN alone is not good enough, as MSDN displays the functions in a different order from their VTable ordering. We need these functions to be in VTable order for our memory map to align.
Add bindings for Navigation Starting and Navigation Completed which would also resolve the issue #50 .
TL;DR Changes :
I'm still new to Go and webview in general, so if I made mistakes or did something wrong, just let me know, I'll do my best to fix it.