implement navigation callbacks #942
Conversation
|
Nice work! I'm glad someone with Mac expertise swooped in. Could you please de-conflict webview.h and retest for an easier merge? Sorry it took so long to receive any comments on this. |
There was a problem hiding this comment.
Sorry it has taken so long to review this, and thanks @dandeto for getting started.
I'll pitch in because there is quite a bit to comment on here. I understand this is partially based on #676 but I'll comment on this PR independently.
Aside from the inline comments:
- When do we get notified of navigation changes? Before, after, both?
- Do we get notified for the navigation as one unit, or do we get notified for little every request in between?
- Is the behavior consistent across all of the supported platforms?
- Is there a reason why the feature is missing from the C interface?
- Since a navigation handler is always added internally, how much could this impact performance when users don't need to be notified?
- It would be great with some tests.
We will take your word on Mac, because I don't think anyone here has a Mac.
I can test this when we're more confident in merging the work.
| class_addProtocol(cls, objc_getProtocol("WKNavigationDelegate")); | ||
| class_addMethod(cls, "webView:didFinishNavigation:"_sel, | ||
| (IMP)(+[](id delegate, SEL sel, id webview, id navigation) { | ||
| auto w = get_associated_webview(delegate); |
There was a problem hiding this comment.
We need to be careful here with memory management given the potential frequency of the execution of this code. What does the memory footprint look like?
There was a problem hiding this comment.
good question, not too familiar with the apple profiling frameworks.
There was a problem hiding this comment.
Something like this should give an idea about the memory consumption:
- Record the app's memory usage after a single navigation.
- Repeat navigation a bunch of times, whether with a local server (e.g.
python3 -m http.server --bind 127.0.0.1 8000) or any other way that triggers the code to execute, and watch memory consumption. Not sure if it's enough to callnavigate()or if we need to automate mouse/keyboard taps. - After the loop, see whether memory consumption goes down after a few seconds. It's pretty lazy.
- If memory consumption goes down to normal levels then it should be fine.
If this looks OK then I would just skip profiling because we probably still have some other memory-related issues on macOS that aren't too severe.
| w->m_navigateCallback(str, w->m_navigateCallbackArg); | ||
| }), | ||
| "v@:@"); | ||
| objc_registerClassPair(cls); |
There was a problem hiding this comment.
A fix is not required for this PR since the fixes in #1005 aren't merged yet, but I'll just leave a note: Will crash when attempting to register the same class a second time.
akallabeth
force-pushed
the
navigation_callbacks
branch
3 times, most recently
from
d8d136b
to
1330997
Compare
|
since the documentation at
https://learn.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/icorewebview2?view=webview2-1.0.1901.177#get_source
does not mention if the string could be allocated when the return is not
SUCCEEDED I prefer this style as there is no way a call could leak.
…On 30.08.23 10:09, Steffen André Langnes wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In webview.h
<#942 (comment)>:
> @@ -1771,6 +1826,19 @@ class webview2_com_handler
}
return S_OK;
}
+ HRESULT STDMETHODCALLTYPE Invoke(
+ ICoreWebView2 *sender, ICoreWebView2NavigationCompletedEventArgs *args) {
+ PWSTR uri = nullptr;
+ auto hr = sender->get_Source(&uri);
+ if (SUCCEEDED(hr)) {
+ auto curi =
+ std::wstring_convert<std::codecvt_utf8<wchar_t>>().to_bytes(uri);
+ if (navigateCallback)
+ navigateCallback(curi, navigateCallbackArg);
+ }
+ CoTaskMemFree(uri);
Since the if-block is already there, wouldn't it be cleaner to move
the call inside the if-block?
—
Reply to this email directly, view it on GitHub
<#942 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2PW7VU3HM5UY6GLQU7ZPLXX3YLFANCNFSM6AAAAAAZOWMK2I>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
inspired by webview#676 provide a callback in case of navigation completed with a custom argument provided at registration time.
akallabeth
force-pushed
the
navigation_callbacks
branch
from
1330997
to
c5245a4
Compare
inspired by #676
provide a callback in case of navigation completed with a custom argument provided at registration time.
(linux/windows/mac implemented)