feat(macos): inline xwidget web views via native WKWebView - #297
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It adds a substantial new macOS-only native-FFI backend (unsafe NSView/WKWebView handling, main-thread and coordinate-flip assumptions, ledger changes) that is not exercised by the primary CI, so it warrants human review beyond the one dead-field issue found.
Pull request overview
This PR implements an inline browser for macOS xwidgets using the system WKWebView as a native NSView overlay positioned over the GPU surface (since WKWebView cannot render offscreen), rather than the Linux WPE/dma-buf texture path. It ports GNU Emacs' xwidget.c/nsxwidget.m placement algorithm function-for-function, adds two missing C subrs, and flips xwidget-internal to be present on macOS via a build.rs probe. It closes the macOS half of #22.
Changes:
- New
backend/wkwebviewmodule:WkWebViewHost(view lifecycle, per-frame sync/hide) andWkWebView/Placement(Emacs-ported clip/flip geometry), wired into the render thread afterpresent. - Two new subrs (
xwidget-webkit-execute-script,xwidget-webkit-estimated-load-progress) plus theWebKitExecuteScriptasset command and host plumbing;FUNcallback signals as unsupported and load progress is advisory (0.0/1.0). neomacs_have_wkwebviewbuild probe makesxwidget-internalprovided on macOS; feature/coupled-vars/subr-surface ledgers made platform-aware (Linux unchanged); new deps and docs.
File summaries
| File | Description |
|---|---|
backend/wkwebview/mod.rs |
New WkWebViewHost: attach, create/load/execute/resize/destroy, per-frame sync_frame with touch/hide and one-view-per-model guard. |
backend/wkwebview/view.rs |
New Placement geometry (clip/flip) and WkWebView AppKit view pair; contains the unread model_* fields flagged below. |
backend/wkwebview/view_test.rs |
12 geometry tests pinning the ported Emacs arithmetic and the bottom-up reclip regression. |
render_thread/render_pass.rs |
sync_inline_web_views sweep invoked after present for the primary frame. |
render_thread/state.rs |
Adds wkwebview_host field, initialized from WkWebViewHost::new(). |
render_thread/asset_commands.rs |
Handles WebKit create/load/execute/resize/destroy on macOS. |
thread_comm.rs |
New AssetCommand::WebKitExecuteScript variant. |
xwidget.rs |
New subrs + load_progress runtime state; goto-uri sets progress to 1.0. |
display_host.rs / neomacs-bin/src/main.rs |
New execute_webkit_xwidget_script host method + command dispatch. |
c_features.rs / provide_coupled_vars.rs (+ tests) |
xwidget-internal now DetectedAtBuildTime; ledgers made platform-aware. |
gnu_subr_surface_test.rs |
Registers the two new subrs as GNU-declared-in-this-branch. |
build.rs |
detect_wkwebview emits neomacs_have_wkwebview on macOS. |
builtins/mod.rs |
Registers the two new subrs. |
Cargo.toml / Cargo.lock / docs/building.md |
objc2/WebKit deps and macOS inline-browser documentation. |
Review details
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@eval-exec I have checked MathJax is rendering flawlessly with a very heavy-math LaTeX (inline and display) markdown document: 
Preview sync is working just as it was for Vanilla Emacs (I move in the markdown document, and preview moves along). My own tests pass with this changes. E.g. |
|
@eval-exec Mermaid diagrams are rendered OK, and the zoom (in/out/fit) and pan features work as well. The following screenshot also exercises the styling (light theme now): 
The following screenshot shows the zoom controls (already zoomed in): 
|
|
@eval-exec Generated report here: https://claude.ai/code/artifact/a85a4e48-acff-4d94-b2d8-c22c8a5ee652 |
Thank you for the pr, this link is broken: |
Sorry, I forgot to change the sharing permissions 😅 |
The link works now. |
|
The native Before merging, I think three concrete correctness issues should be fixed:
I am comfortable handling the following as tracked follow-ups rather than blocking this PR:
Please document that the initial macOS With the three correctness fixes above, I support merging the native WKWebView implementation and improving the remaining behavior incrementally. |
Review catch (PR eval-exec#297). `Placement::inner_origin' positioned the WKWebView inside the clip view using `host_flipped' -- the *host's* orientation applied to a frame expressed in the *clip's* coordinate system. The clip was a stock NSView, which is bottom-left, always; the host is winit's content view, which is winit's business. GNU settles this by making the views match the arithmetic rather than the other way round. src/nsxwidget.m defines two flipped NSView subclasses -- XwWindow at :483 and XvWindow at :553, both `isFlipped { return YES; }' -- and EmacsView is flipped too (nsterm.m:8540). With all three flipped, src/xwidget.c:2993-2995 writes the inner origin unconditionally: nsxwidget_resize_view (xv, clip_right - clip_left, clip_bottom - clip_top); nsxwidget_move_widget_in_view (xv, -clip_left, -clip_top); So: XwidgetClipView, the port's first objc2 subclass, overriding isFlipped. `inner_origin' loses its parameter and becomes Emacs' formula unchanged. The reviewer's numbers, for the record: height 100, clip_top 20, visible height 80. The correct inner Y in a flipped clip is -20. The old bottom-up branch answered `visible_height + clip_top - height' = 0, pinning the widget's top edge where its bottom belongs. Whether that fired depended on what winit's content view reports for isFlipped, which nothing here had measured -- so `attach' now logs it. `ns_origin' keeps `host_flipped', because that frame really is in the host's space. Its gate is extracted into `needs_reposition' so the rule is testable without AppKit, and it grows a third case found while mapping this: the bottom-up origin also folds in the host's *own* height, which no placement diff can see, so a window resized under a widget that did not itself move left the view at a stale origin. `WkWebView' now remembers the host geometry it last placed against. Verified: the two new geometry rules are red under the old code -- old gate answers false where the test asserts true, old inner_origin(false) answers 0 where it must answer -20. 791 display-runtime tests pass.
Review catch (PR eval-exec#297). `WkWebViewHost' buffered a create that arrived before the native host view did -- but only the create, and only its size: pending: HashMap<u32, (f64, f64)>, `attach' replayed that map and nothing else. Every other entry point missed `views.get(&id)' and dropped the command with a warn. So a `make-xwidget' followed by `xwidget-webkit-goto-uri', both evaluated before the primary frame was realized, built a blank WKWebView that never navigated -- while Lisp recorded the URI and set load progress to 1.0, so `xwidget-webkit-uri' said otherwise. A resize in the same window was worse: it was dropped *and* the view was then built at the stale create size. This is reachable rather than theoretical. The primary frame starts `FrameLifecycle::Pending', and the render thread drains its command channel unconditionally, independent of frame presentation, so anything sent in that interval is gone before a view could exist for it. Two halves: - `pending' now holds the commands, not the sizes: one arrival-ordered FIFO for every id, drained through the ordinary methods by `attach'. One queue rather than one per id, because ordering matters across ids as well as within one, and a single vector gets that for free. `destroy' drops an id's queued work. At capacity the newest is dropped, not the oldest, so what survives is a coherent prefix rather than a set of orphaned loads whose create was evicted. - Every WebKit command now tries `attach', not just `WebKitCreate'. A window that becomes available *between* a create and the load behind it was previously picked up only at the next present, by which time the load had already been drained and discarded. The queue is its own type with no AppKit in it, so it is unit-testable off the main thread -- which the host itself is not, `WkWebView::new' being a WKWebView constructor. Eight tests, red by construction: the old structure had nowhere to put a URL.
Review catch (PR eval-exec#297). `AssetCommand::WebKitExecuteScript' ran the script on macOS and, on every other platform, did this: #[cfg(not(target_os = "macos"))] let _ = script; with a `debug!' above it announcing that the script had been executed. So `xwidget-webkit-execute-script' was a false implementation on Linux: silently nothing, cheerfully logged. The tree already had the missing half, orphaned. `WebKitExecuteJavaScript' carried an identical payload and a real WPE dispatch, and had no producer anywhere -- left behind by f812db8, the emacs-c bridge removal. One command that worked only on macOS, one that worked only on WPE, and no way for either to be reached from both. Consolidated onto `WebKitExecuteScript', in the shape of the neighbouring `WebKitLoadUri' arm that already compiles under both cfgs: WKWebView on macOS, `webkit_web_view_evaluate_javascript' on WPE, and -- for a build with neither backend -- a warning that says the script was dropped instead of a debug line claiming it ran. `WebKitExecuteJavaScript' is deleted; its only other reference was one construct-and-destructure test, repointed. This is a fix for Linux, not a tidy-up. `lisp/xwidget.el' routes scrolling, zoom and element navigation through this subr, and `lisp/net/shr.el' uses it to install the video element for inline video. All of them were no-ops on a `--features wpe-webkit' build. Note for CI: `wpe-webkit' is not a default feature and no workflow builds with it, so the new WPE arm is not compile-checked upstream. It is a copy of an arm that is.
Review ask (PR eval-exec#297): document that the initial macOS `xwidget-internal' support is partial, and do not describe it as complete GNU Emacs xwidget compatibility. Two places in the tree overclaimed. `c_features.rs' said the browse-url path "is complete"; `provide_coupled_vars.rs' repeated it. What is true is narrower: `xwidget-webkit-browse-url' works end to end on the primary frame, and that is enough for the flag to be worth setting, because `xwidget.el' does not require the feature -- the flag decides whether a configuration reaches for the layer at all. Both now enumerate the gaps and point at the tracking issue. `docs/building.md' gains the user-facing version of the same list, so someone meets these before filing a bug: - primary frame only; a second top-level frame gets no view - load progress is dispatched, not measured -- no WKNavigationDelegate or KVO, so a script run immediately after opening a page can precede the page - no script result callbacks; `xwidget-webkit-execute-script' signals on FUN, which is why `xwidget-webkit-get-selection' and `xwidget-webkit-insert-string' do not work - keyboard focus is not handed off in either direction; mouse works Tracked in issue 300. Two findings that are independent of this branch were split out rather than left to vanish with the PR description: an oversized xwidget vanishing where GNU crops it (issue 301), and the WPE draw path in content.rs discarding clip_rect where its sibling in layer_media.rs honours it (issue 302). Linux is untouched by all of it: `xwidget-internal' still answers NotBuilt there and ledger 190's missing subrs are still open.
257256f to
f4a7f6e
Compare
|
Thank you — all three were real, and none of them was found by any test I had written. Rebased onto 1. Clip-view orientation — Fixed structurally rather than arithmetically, as you suggested: Rewriting its gate turned up a third input nothing was watching: the bottom-up origin also folds in the host's own height, which no placement diff can see, so a window resized under an unmoved widget left a stale origin. The rule is now 2. Commands lost before attachment —
3. One caveat you should know about: 4. Documentation. You were right that it overclaimed. Issues opened:
Verification. 791 display-runtime tests, fmt and clippy clean on new code, both ledgers passing. The single remaining Linux is untouched throughout: |
|
Thank you! Is this PR ready to merge? |
|
Re-review of the latest commits: the three blockers from my earlier review are fixed.
I found one remaining before-merge issue: P2: pending-queue overflow can still recreate a blank or missing web view
neomacs/neomacs-display-runtime/src/backend/wkwebview/pending.rs Lines 67 to 90 in f4a7f6e The current capacity test queues only independent neomacs/neomacs-display-runtime/src/backend/wkwebview/pending_test.rs Lines 123 to 139 in f4a7f6e My preferred fix is to make overflow atomic per xwidget: if any command for an ID is dropped, remove that ID's already-queued commands and reject subsequent commands for it until One non-blocking design follow-up: the command taxonomy and routing are repeated across All platform cargo checks currently pass. The workflow-lint failure is from a pre-existing ShellCheck finding in |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large macOS-only native AppKit backend with intricate coordinate geometry that the Linux CI does not compile or exercise, so final human review on macOS is warranted.
Review details
- Files reviewed: 24/25 changed files
- Comments generated: 1
- Review effort level: Balanced
| pub(crate) fn builtin_xwidget_webkit_execute_script( | ||
| eval: &mut Context, | ||
| args: Vec<Value>, | ||
| ) -> EvalResult { | ||
| // GNU takes (XWIDGET SCRIPT &optional FUN) and feeds the script's return | ||
| // value to FUN. Delivering a result needs a channel from the render | ||
| // thread back to the Lisp thread, which does not exist yet, so FUN is | ||
| // accepted and reported as unsupported rather than silently ignored -- | ||
| // a caller that passes it would otherwise wait forever for a call that | ||
| // never comes. | ||
| expect_args_range("xwidget-webkit-execute-script", &args, 2, 3)?; | ||
| let value = expect_live_webkit_xwidget(args[0])?; | ||
| let script = expect_string(args[1])?; | ||
| let id = value.as_xwidget().unwrap().xwidget_id; | ||
| if args.get(2).is_some_and(|fun| !fun.is_nil()) { | ||
| return Err(signal( | ||
| "error", | ||
| vec![Value::string( | ||
| "xwidget-webkit-execute-script: the FUN callback is not supported \ | ||
| in this build; the script still runs when FUN is omitted", | ||
| )], | ||
| )); | ||
| } | ||
| if let Some(host) = eval.display_host.as_ref() { | ||
| host.execute_webkit_xwidget_script(id, script) | ||
| .map_err(|err| signal("error", vec![Value::string(err)]))?; | ||
| } | ||
| Ok(Value::NIL) | ||
| } | ||
|
|
||
| pub(crate) fn builtin_xwidget_webkit_estimated_load_progress( | ||
| _eval: &mut Context, | ||
| args: Vec<Value>, | ||
| ) -> EvalResult { | ||
| expect_args_range("xwidget-webkit-estimated-load-progress", &args, 1, 1)?; | ||
| let value = expect_live_webkit_xwidget(args[0])?; | ||
| let id = value.as_xwidget().unwrap().xwidget_id; | ||
| Ok(Value::make_float(_eval.xwidgets.webkit_load_progress(id))) | ||
| } |
WKWebView has no offscreen render path — WebKit2's process architecture rules it out, as GNU Emacs records in the header comment of src/nsxwidget.m — so macOS cannot take the dma-buf texture route that backend/wpe uses on Linux. It gets a native NSView subtree layered over the GPU surface instead, which is what GNU Emacs has shipped for years. The placement algorithm is ported from GNU Emacs function for function: produce_xwidget_glyph (xdisp.c) -> GlyphType::Xwidget, already here x_draw_xwidget_glyph_string:2841-49 -> Placement::new xwidget.c:2856 / :2961 -> Placement::moved_from/reclipped_from xwidget.c:2888, 2995, 2996 -> WkWebView::apply xwidget_end_redisplay (xwidget.c) -> WkWebViewHost::sync_frame nsxwidget_hide_view (nsxwidget.m) -> WkWebView::hide Most of the scaffolding already existed: the layout engine reserves the slot, and FrameGlyph::Xwidget already carries a resolved rect and a clip_rect. What was missing is the per-frame sweep that hides views the frame no longer references, the two dirty checks, and the Cocoa backend. The sweep runs from render_frame_window_impl after the present, which is where Emacs calls xwidget_end_redisplay from dispnew.c:4626. It has to run on every presented frame, including the retained-static fast path that skips the glyph pipeline; hooking it into the renderer's draw walk would strand views on exactly those frames. Views are hidden by default unless a frame vouched for them, so scrolling away, switching buffers and deleting a window all work without those paths knowing web views exist. Two places where the port could not be literal: - Emacs' views are flipped (nsterm.m:8540, nsxwidget.m:554) so it positions them top-down. The winit content view carries no such guarantee, so geometry is computed top-down as Emacs computes it and converted per host in Placement::ns_origin/inner_origin. A bottom-up origin depends on the visible height, so unlike Emacs we must rewrite it on a pure reclip too — a window resized shorter under a widget whose top has not moved. Covered by a regression test. - Emacs gets clipping from view nesting; we set clipsToBounds explicitly rather than rely on a platform default that has changed across macOS releases. Scope: placement only. Mouse events reach the view through the responder chain by virtue of it being a real subview, but keyboard focus handoff is not addressed here. Verified: cargo build -p neomacs links WebKit.framework; 777 lib tests pass, 12 of them new geometry tests; clippy and rustfmt clean.
…ogress The macOS WKWebView backend landed in the previous commit but the feature flag still said NotBuilt, so configuration that gates on `(featurep 'xwidget-internal)' skipped every xwidget module -- which is the correct thing for such a config to do, and the reason the flag has to move rather than the config. Two subrs stood between the flag and the truth, both on the `xwidget-webkit-browse-url' path: - xwidget-webkit-execute-script, over WKWebView's evaluateJavaScript:completionHandler:. GNU's optional FUN receives the script's return value, which needs a result channel from the render thread back to the Lisp thread that does not exist here. FUN therefore signals rather than being silently dropped: a caller that passed it would otherwise wait forever for a call that never comes. - xwidget-webkit-estimated-load-progress. Not measured, and documented as such: the web view lives on the render thread and nothing sends InputEvent::WebKitLoadFinished, so this is 0.0 before a navigation and 1.0 once one is dispatched. A stuck 0.0 renders a permanent "[0%]" in xwidget-webkit-mode's header line, which is the worse lie. The remaining names that `lisp/xwidget.el' reaches for turned out to be Lisp functions defined in that file (xwidget-insert, xwidget-put, xwidget-window-inside-pixel-width), not missing C subrs. xwidget-internal is now decided by a build.rs probe, following the neomacs_have_lcms2 pattern -- DetectedAtBuildTime requires citing a probe and `cfg!(target_os)' is not one. Linux keeps its old answer: its WPE path renders through dma-buf and ledger 190's missing subrs are still open there. `xwidget.el' does not require the feature (its `(require 'xwidget-internal)' is commented out at line 32), so the flag is advisory to configuration, which is exactly what reads it. Two ledger tests become platform-aware, both unchanged on Linux: the derived-feature order, and the coupled-vars absence probe -- whose own docstring says firing is the designed signal that a build gained a feature. XWIDGET_LAYER is rewritten because on macOS those three variables are now bound *because* the feature is present, matching GNU, rather than as a policy exception. Verified end to end against a real configuration and a 168KB Obsidian document with 170 math blocks: cap predicate t, zero skipped modules, markdown-xwidget preview renders in a live WKWebView with MathJax typeset (equations 1, 2, 3a/3b), and injected JavaScript both scrolls the page and restyles it. Not fixed: with no load-finished event, a script run immediately after enabling the preview is a no-op because the page has not loaded yet; scroll-sync runs on an idle timer and is unaffected. Pre-existing and untouched: c_features_test::the_features_this_build_ really_has_still_answer_t asserts (featurep 'inotify) and fails on any macOS build. Confirmed by stashing this change and re-running.
Review catch: `model_width` / `model_height` were only ever written -- in `new` and in `resize` -- and never read. `apply` sizes the inner web view from `placement.width` / `placement.height`, and `resize` writes the frame from its own arguments, so the fields carried no information. Removing them rather than reading from them is deliberate. `Placement` already carries the widget box precisely so the clip and the inner web-view frame derive from one value; a second copy on the struct could only drift out of step with it. The two agree by construction anyway: `parse_display_xwidget_layout` builds the glyph's width/height straight from the live xwidget object, and `xwidget-resize` mutates exactly those fields, so a resize reaches the placement on the next frame. No behaviour change.
Review catch (PR eval-exec#297). `Placement::inner_origin' positioned the WKWebView inside the clip view using `host_flipped' -- the *host's* orientation applied to a frame expressed in the *clip's* coordinate system. The clip was a stock NSView, which is bottom-left, always; the host is winit's content view, which is winit's business. GNU settles this by making the views match the arithmetic rather than the other way round. src/nsxwidget.m defines two flipped NSView subclasses -- XwWindow at :483 and XvWindow at :553, both `isFlipped { return YES; }' -- and EmacsView is flipped too (nsterm.m:8540). With all three flipped, src/xwidget.c:2993-2995 writes the inner origin unconditionally: nsxwidget_resize_view (xv, clip_right - clip_left, clip_bottom - clip_top); nsxwidget_move_widget_in_view (xv, -clip_left, -clip_top); So: XwidgetClipView, the port's first objc2 subclass, overriding isFlipped. `inner_origin' loses its parameter and becomes Emacs' formula unchanged. The reviewer's numbers, for the record: height 100, clip_top 20, visible height 80. The correct inner Y in a flipped clip is -20. The old bottom-up branch answered `visible_height + clip_top - height' = 0, pinning the widget's top edge where its bottom belongs. Whether that fired depended on what winit's content view reports for isFlipped, which nothing here had measured -- so `attach' now logs it. `ns_origin' keeps `host_flipped', because that frame really is in the host's space. Its gate is extracted into `needs_reposition' so the rule is testable without AppKit, and it grows a third case found while mapping this: the bottom-up origin also folds in the host's *own* height, which no placement diff can see, so a window resized under a widget that did not itself move left the view at a stale origin. `WkWebView' now remembers the host geometry it last placed against. Verified: the two new geometry rules are red under the old code -- old gate answers false where the test asserts true, old inner_origin(false) answers 0 where it must answer -20. 791 display-runtime tests pass.
Review catch (PR eval-exec#297). `WkWebViewHost' buffered a create that arrived before the native host view did -- but only the create, and only its size: pending: HashMap<u32, (f64, f64)>, `attach' replayed that map and nothing else. Every other entry point missed `views.get(&id)' and dropped the command with a warn. So a `make-xwidget' followed by `xwidget-webkit-goto-uri', both evaluated before the primary frame was realized, built a blank WKWebView that never navigated -- while Lisp recorded the URI and set load progress to 1.0, so `xwidget-webkit-uri' said otherwise. A resize in the same window was worse: it was dropped *and* the view was then built at the stale create size. This is reachable rather than theoretical. The primary frame starts `FrameLifecycle::Pending', and the render thread drains its command channel unconditionally, independent of frame presentation, so anything sent in that interval is gone before a view could exist for it. Two halves: - `pending' now holds the commands, not the sizes: one arrival-ordered FIFO for every id, drained through the ordinary methods by `attach'. One queue rather than one per id, because ordering matters across ids as well as within one, and a single vector gets that for free. `destroy' drops an id's queued work. At capacity the newest is dropped, not the oldest, so what survives is a coherent prefix rather than a set of orphaned loads whose create was evicted. - Every WebKit command now tries `attach', not just `WebKitCreate'. A window that becomes available *between* a create and the load behind it was previously picked up only at the next present, by which time the load had already been drained and discarded. The queue is its own type with no AppKit in it, so it is unit-testable off the main thread -- which the host itself is not, `WkWebView::new' being a WKWebView constructor. Eight tests, red by construction: the old structure had nowhere to put a URL.
Review catch (PR eval-exec#297). `AssetCommand::WebKitExecuteScript' ran the script on macOS and, on every other platform, did this: #[cfg(not(target_os = "macos"))] let _ = script; with a `debug!' above it announcing that the script had been executed. So `xwidget-webkit-execute-script' was a false implementation on Linux: silently nothing, cheerfully logged. The tree already had the missing half, orphaned. `WebKitExecuteJavaScript' carried an identical payload and a real WPE dispatch, and had no producer anywhere -- left behind by f812db8, the emacs-c bridge removal. One command that worked only on macOS, one that worked only on WPE, and no way for either to be reached from both. Consolidated onto `WebKitExecuteScript', in the shape of the neighbouring `WebKitLoadUri' arm that already compiles under both cfgs: WKWebView on macOS, `webkit_web_view_evaluate_javascript' on WPE, and -- for a build with neither backend -- a warning that says the script was dropped instead of a debug line claiming it ran. `WebKitExecuteJavaScript' is deleted; its only other reference was one construct-and-destructure test, repointed. This is a fix for Linux, not a tidy-up. `lisp/xwidget.el' routes scrolling, zoom and element navigation through this subr, and `lisp/net/shr.el' uses it to install the video element for inline video. All of them were no-ops on a `--features wpe-webkit' build. Note for CI: `wpe-webkit' is not a default feature and no workflow builds with it, so the new WPE arm is not compile-checked upstream. It is a copy of an arm that is.
Review ask (PR eval-exec#297): document that the initial macOS `xwidget-internal' support is partial, and do not describe it as complete GNU Emacs xwidget compatibility. Two places in the tree overclaimed. `c_features.rs' said the browse-url path "is complete"; `provide_coupled_vars.rs' repeated it. What is true is narrower: `xwidget-webkit-browse-url' works end to end on the primary frame, and that is enough for the flag to be worth setting, because `xwidget.el' does not require the feature -- the flag decides whether a configuration reaches for the layer at all. Both now enumerate the gaps and point at the tracking issue. `docs/building.md' gains the user-facing version of the same list, so someone meets these before filing a bug: - primary frame only; a second top-level frame gets no view - load progress is dispatched, not measured -- no WKNavigationDelegate or KVO, so a script run immediately after opening a page can precede the page - no script result callbacks; `xwidget-webkit-execute-script' signals on FUN, which is why `xwidget-webkit-get-selection' and `xwidget-webkit-insert-string' do not work - keyboard focus is not handed off in either direction; mouse works Tracked in issue 300. Two findings that are independent of this branch were split out rather than left to vanish with the PR description: an oversized xwidget vanishing where GNU crops it (issue 301), and the WPE draw path in content.rs discarding clip_rect where its sibling in layer_media.rs honours it (issue 302). Linux is untouched by all of it: `xwidget-internal' still answers NotBuilt there and ledger 190's missing subrs are still open.
Re-review catch (PR eval-exec#297, P2). The queue's overflow rule -- drop the newest command, keep the front -- was described as leaving "a coherent prefix". It is coherent per command and not per xwidget, and the boundary is where it fails: 1. With 255 entries queued, a new view's Create is accepted as entry 256 and the LoadUri behind it is dropped. On attach that replays into a blank WKWebView -- the exact failure this queue was added to prevent. 2. Once a command has been dropped, a later `forget' can bring the queue under capacity, after which commands for the dropped id are accepted again: a Create(300) that was refused followed by a LoadUri(300) that was not, leaving an orphan. The capacity test queued only independent creates, so it saw neither. Overflow is now atomic per id, as the reviewer preferred: when a command for an id is refused, that id's already-queued commands are removed and the id is rejected until Destroy. `forget' (Destroy) lifts the rejection so a fresh lifecycle under the same id starts clean. The invariant the struct now states is "every id in the queue is whole or absent". Three regression tests, the first two red before this change: a load that overflows takes its create with it; an overflowed id stays rejected after room frees up; destroying it lifts the rejection.
Review follow-up (PR eval-exec#297, non-blocking): the WebKit command shape was spelled three times -- `PendingCommand', the `replay' match in the host, and five `#[cfg(target_os = "macos")]' blocks in asset_commands.rs each doing attach-then-call -- plus four copies of "if there is no host yet, queue it and return" inside the host's per-command methods. Adding a command meant touching every one of them, and nothing checked that you had. Now `backend/wkwebview/command.rs' owns the taxonomy: `WebKitViewCommand', with the one conversion from `AssetCommand'. The host has one entry point, `dispatch', which either defers (Destroy is applied to the queue itself, so a killed xwidget is never built) or hands to `apply_live', whose `match' is the single exhaustive one -- a new variant fails to compile there. `attach' replays through `dispatch'. asset_commands.rs intercepts once, at the top of `handle_asset', and the per-arm macOS blocks are gone; the WPE arms are untouched. The conversion borrows rather than consumes so the same `AssetCommand' still reaches the WPE arms on a build with both backends; the payload is an id and at most one short string. Two conversion tests; the eleven queue tests pass unchanged under the rename. 796 display-runtime tests.
Review thread on PR eval-exec#297: the two subrs added for macOS had distinctive, cheaply testable behaviour and no coverage, while their siblings are pinned through the recording display host in xwidget_test.rs. - `xwidget-webkit-estimated-load-progress' answers 0.0 after `make-xwidget' and 1.0 after `xwidget-webkit-goto-uri': dispatched, not measured. A future measured implementation now has to change this test on purpose. - `xwidget-webkit-execute-script' signals on a non-nil FUN -- there is no result channel back to Lisp, and a callback that never fires is worse than an error -- and without FUN hands exactly one script to the host. The recording host gains an `ExecuteScript' event for this. Both are load-bearing: with the progress bump removed and the FUN check disabled, each fails.
f4a7f6e to
03faba4
Compare
|
Thanks — both P2 cases were real, and the second is the one I would not have found: I had reasoned about the overflow rule per command when the unit that matters is the xwidget. Rebased onto P2 — overflow is now atomic per xwidget: Regression tests for exactly the two boundaries you named, red before the fix and green after:
Design follow-up — one taxonomy, one dispatch: Copilot's thread — Verification. 796 display-runtime tests; fmt, clippy on new code, and Thanks also for running the |
|
Re-review of the three new commits at The requested overflow-boundary tests now pass, and the centralized dispatch is a good direction. However, I found two before-merge lifecycle/state issues. P2: Destroy can replay the xwidget it is supposed to cancelEvery native WebKit command now calls neomacs/neomacs-display-runtime/src/render_thread/asset_commands.rs Lines 38 to 53 in 03faba4 If the primary window becomes available immediately before a pending xwidget's neomacs/neomacs-display-runtime/src/backend/wkwebview/mod.rs Lines 89 to 109 in 03faba4 Only after that replay does the current
where neither create nor navigation is replayed. P2: overflow state remains unboundedThe command vector is capped at 256 because a host may never arrive, but every subsequent unique xwidget ID is inserted into the uncapped neomacs/neomacs-display-runtime/src/backend/wkwebview/pending.rs Lines 37 to 74 in 03faba4 After 256 queued creates, a stream of fresh IDs grows Additional follow-ups
Verification at this head:
I would hold merge for the Destroy replay race and bounded overflow-state fixes. |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large, entirely macOS-only native WKWebView backend plus feature-flag and ledger changes that the Linux-based CI does not compile or exercise, so final human review on macOS is warranted despite the strong test coverage of the platform-agnostic logic.
Review details
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ys bounded Re-review catch (PR eval-exec#297, round three), two P2s in the pending queue. 1. Every WebKit command called `attach' before `dispatch'. If the window became available just before a pending xwidget's Destroy, attach bound the host and replayed that xwidget's queued Create and LoadUri first -- a view briefly built, possibly already fetching, then torn down. That contradicted `dispatch''s own claim that a killed pending xwidget is never built. 2. The command queue was capped at 256, but every id refused at overflow went into an uncapped `rejected' set with one warning each. A stream of fresh ids after saturation grew memory and the log without bound. And a subtler door the first test wrote for this found: evictions free slots, the queue refills, and refill-then-evict adds one refused id per xwidget -- unbounded again. Both are decided in a new `Lifecycle' type that owns no AppKit at all. It takes a command plus "is a window available" and returns Actions -- Bind, Apply(command) -- which the host executes. Destroy never yields Bind: it forgets the id's queued work and, if already bound, applies to the live view. Overflow is a latch: once the queue has been full nothing more is queued until a window arrives, refusals are tracked only for ids that had something queued, and the warning fires once. Refusal now lives at lifecycle level, so it survives binding: a refused id cannot get a fresh Create post-attach until its Destroy. Because the decisions are pure, the reviewer's sequences are unit tests: pending Create -> pending LoadUri -> window becomes available -> Destroy yields no Bind and no replay; a flood of 10*CAPACITY fresh ids with interleaved evictions and destroys keeps retained state under CAPACITY. The first version of the latch failed that second test -- it allowed refills -- which is why the latch is total. Also from the review: `from_asset' now consumes the AssetCommand instead of cloning its strings (scripts and data: URLs can be large), returning a command this backend lacks for the WPE arms; and the conversion test names `WebKitReload' as what it is -- a WebKit command the native backend does not implement, not a non-WebKit one. Not done here, filed in issue 300: a nested AssetCommand::WebKit(_) so the conversion itself is compiler-checked; today a new WebKit* variant can compile without reaching this backend. 799 display-runtime tests.
|
Re-review of commit The previous blockers are fixed:
I found one new before-merge integration bug. P1: a failed native bind permanently corrupts lifecycle state
neomacs/neomacs-display-runtime/src/backend/wkwebview/lifecycle.rs Lines 78 to 94 in b376919 Only afterward does neomacs/neomacs-display-runtime/src/backend/wkwebview/mod.rs Lines 81 to 128 in b376919 If either handle operation fails,
The lifecycle tests cannot expose this because Please make binding transactional. A clean shape would be:
Please add a failed-bind → retry regression test. Ideally the interface should carry an explicit bind result rather than reducing “can retain an AppKit host” to P2: rejected commands can still flood debug logsEvery later command for a rejected ID emits another neomacs/neomacs-display-runtime/src/backend/wkwebview/lifecycle.rs Lines 119 to 122 in b376919 The flood test proves memory remains bounded, but with debug logging enabled a single rejected ID can still generate unbounded log volume. Please remove the per-command message or latch it behind the single saturation warning, and cover the emission behavior in the flood test. I would hold merge until native binding is transactional. |
|
Both P2s fixed in Destroy never binds. The decision of whether a command warrants binding to a window now lives in a new Because the policy is pure, your sequence is a unit test verbatim — Overflow state is bounded. Saturation is now a latch, as you suggested: once the queue has been full, nothing more is queued until a window arrives, refusals are tracked only for ids that actually had something queued, and the warning fires once. Refusal also moved up to lifecycle level so it survives binding — your first follow-up: a refused id cannot get a fresh The bounded test ( Also from the review: Verification. 799 display-runtime tests; fmt, clippy on new code, |
|
Correction to my previous review: after checking the exact pinned winit 0.30.13 macOS implementation, I overclassified the transactional-binding concern. On macOS, winit's https://github.com/rust-windowing/winit/blob/v0.30.13/src/platform_impl/macos/window.rs#L49-L60 The AppKit handle is then constructed from winit's retained view, whose pointer is asserted non-null: This PR already gates neomacs/neomacs-display-runtime/src/backend/wkwebview/mod.rs Lines 49 to 62 in b376919 Therefore, for the actual winit Making the lifecycle/native bind transition transactional would still be a cleaner and more generally robust interface—especially because Likewise, the per-rejected-command I withdraw my “hold merge” recommendation from the previous comment. From my review, this PR is mergeable once the remaining required CI checks succeed. Sorry for the severity overcall. |
There was a problem hiding this comment.
Thank you @tag-und-nacht , I think this PR can be merged now.
Will merge after neomacs v0.0.16 released: https://github.com/eval-exec/neomacs/actions/runs/33319477117
Adds an inline browser on macOS, using the system
WKWebViewinstead of WPE.This is partial support, not complete GNU Emacs xwidget compatibility.
xwidget-webkit-browse-urlworks end to end on the primary frame. What is not there —secondary frames, real navigation events and measured load progress, script result
callbacks, keyboard focus — is enumerated in
docs/building.mdand tracked in #300.It advances the macOS half of #22 rather than closing it.
Rebased onto
main(57 commits, no conflicts) and updated for review — seeReview response at the end.
Why not WPE
I tried the WPE route first and got further than expected, so recording where it actually stops:
libwpebuilds on aarch64-darwin. Stock nixpkgs fails onEGL/eglplatform.h, but nixpkgsangle(ANGLE over Metal, prebuilt) plus a hand-writtenegl.pcshim builds libwpe 1.16.3.ENABLE_WPE_LEGACY_API=OFF, headless-only,USE_GBM=OFF,USE_LIBDRM=OFF,USE_SYSTEM_SYSPROF_CAPTURE=NO. No Linux-onlyFATAL_ERRORfires; the full feature summary prints. The first Apple-specific stop is an install rule: "no FRAMEWORK DESTINATION for shared library FRAMEWORK target WebKit".wpebackend-fdodoes not build — it needs wayland-server/wayland-egl, andwaylanditself fails on darwin.None of that is the real blocker. The frame path is.
neomacs-display-runtimeexports frames witheglExportDMABUFImageMESA— a Mesa extension ANGLE does not implement — into wgpu via VulkanVK_EXT_external_memory_dma_buf. macOS has no dma-buf, wgpu runs Metal, and WPEPlatform's buffer types are DMABuf/SHM/Android with no IOSurface.ANGLE does implement an IOSurface path (
IOSurfaceSurfaceMtl,EGL_ANGLE_iosurface_client_buffer), but it is unusable here becauseWKWebViewwill not render into it. GNU Emacs records why in the header comment ofsrc/nsxwidget.m:So macOS gets what GNU Emacs gets: a native
NSViewsubtree positioned over the GPU surface, not a texture composited into it.What's here
Two commits.
1.
backend/wkwebview— the native overlay and its placement.The algorithm is GNU Emacs', ported function for function:
produce_xwidget_glyph(xdisp.c)GlyphType::Xwidget— already existedx_draw_xwidget_glyph_stringclip block (xwidget.c:2841-2849)Placement::newxwidget.c:2856/:2961Placement::moved_from/reclipped_fromnsxwidget_move_view/resize_view/move_widget_in_viewWkWebView::applyxwidget_end_redisplay(xwidget.c:4135)WkWebViewHost::sync_framensxwidget_hide_view(nsxwidget.m:607)WkWebView::hideMost of the scaffolding was already in the tree:
GlyphType::Xwidget,FrameGlyph::Xwidgetwith its resolved rect andclip_rect, and theclipped_media_rect()intersection. What was missing was the per-frame sweep, the two dirty checks, and the Cocoa backend itself.Like Emacs, a view is hidden by default unless the frame vouched for it, so scrolling away, switching buffers and deleting a window all work without any of those paths knowing inline web views exist.
2. Two subrs and the feature flag.
A working overlay changes nothing for a configuration that gates on
(featurep 'xwidget-internal)— which is the correct thing for such a configuration to do. Auditing whatlisp/xwidget.elactually reaches for on thexwidget-webkit-browse-urlpath showed most apparent gaps (xwidget-insert,xwidget-put,xwidget-window-inside-pixel-width) are Lisp functions in that same file. Only two were real C subrs:xwidget-webkit-execute-scriptoverevaluateJavaScript:completionHandler:. GNU's optionalFUNreceives the script's return value, which needs a result channel from the render thread back to the Lisp thread that does not exist here.FUNtherefore signals rather than being silently dropped — a caller that passed it would otherwise wait forever for a call that never comes.xwidget-webkit-estimated-load-progress, which is not measured and says so in its comment. Nothing sendsInputEvent::WebKitLoadFinished(the variant exists; nothing produces or consumes it), so it is 0.0 before a navigation and 1.0 once one is dispatched. A stuck 0.0 renders a permanent[0%]inxwidget-webkit-mode's header line, which seemed the worse lie. Tracked for real plumbing in [Tracking] macOS xwidget-internal: remaining gaps after #297 #300.xwidget-internalis now decided by abuild.rsprobe,neomacs_have_wkwebview, following the existingneomacs_have_lcms2pattern —DetectedAtBuildTimerequires citing a probe andcfg!(target_os)is not one. Linux keeps its old answer; its WPE path still renders through dma-buf and ledger 190's missing subrs are still open there. This is safe becausexwidget.eldoes not require the feature — its(require 'xwidget-internal)is commented out at line 32 — so the flag is advisory to configuration, which is exactly what reads it.Two ledger tests become platform-aware, both unchanged on Linux: the derived-feature order, and the coupled-vars absence probe, whose own docstring says that firing is the designed signal that a build gained a feature.
XWIDGET_LAYER's justification is rewritten because on macOS those three variables are now bound because the feature is present, matching GNU, rather than as a policy exception.Two things I got wrong first
Recording these because the reasons are more useful than the fix.
Placement does not belong in the renderer's draw walk. My first plan put it in
content.rs, next to the existing WPE quad building. That would strand views:render_frame_window_implhas a retained-static fast path that blits a cached scene and skips the glyph pipeline entirely on compositor-only frames, so placement hooked there would never run on those frames and the sweep would hide live views. It runs inrender_thread/render_pass.rsright afterrenderer.queue().present(output)— the true analogue of Emacs callingxwidget_end_redisplayfromdispnew.c:4626once redisplay has settled, not from paint code. Running it after the present also minimises the overlay's lag behind the composited frame.The coordinate flip hides a bug that Emacs cannot have. Emacs makes its xwidget views flipped (
nsterm.m:8540,nsxwidget.m:554) and positions them top-down. The winit content view carries no such guarantee, so geometry is computed top-down exactly as Emacs computes it and converted per host. That conversion means the clip origin depends on the visible height:So a pure reclip — a window resized shorter under a widget whose top has not moved — changes the origin while
moved_fromreports false. Emacs gates the reposition on movement alone and is right to, because flipped coordinates decouple the two. Here it must also run on a reclip:Pinned by
a_pure_reclip_moves_the_bottom_up_origin.Verification
Tested against a real user configuration, unmodified, on a 168 KB Obsidian document with 170 math blocks:
my/cap-xwidgets-panswerst, zero modules skipped — no config edit needed, which was the point of moving the flag rather than weakening the predicate.markdown-xwidgetpreview renders in a liveWKWebView; MathJax typeset (equations 1, 2, 3a/3b) and Mermaid diagrams both confirmed to match vanilla Emacs.scrollToplusfilter: invert(1)both scrolled the preview and flipped its palette.FUNrefusal path signals rather than hanging.neomacs-display-runtimetests pass (12 new geometry tests); feature and coupled-vars ledgers pass; clippy and rustfmt clean on new code.cargo build --releaselinks/System/Library/Frameworks/WebKit.framework.One pre-existing failure, untouched:
c_features_test::the_features_this_build_really_has_still_answer_tasserts(featurep 'inotify)and fails on any macOS build, with or without this change. I confirmed that by stashing the change and re-running, and left it alone rather than widen this PR.Known limitations
Inherited from the overlay technique, all of which GNU Emacs has shipped for years:
xwidget.c:2820-2836is ported: the first placement wins and it warns once.Not addressed here: keyboard focus handoff between neomacs and the web view. Mouse input works, since the view is a real subview in the responder chain.
Two things for maintainers
Both now filed, so they do not vanish when this merges:
xdisp.c:32703) so the row still displays. A layout-engine difference, not a backend one; it accounted for every "blank preview" during bring-up.FrameGlyph::Xwidgetinto a quad, and only one honours the clip.layer_media.rs:414crops and adjusts the texture coordinates;content.rs:1800destructuresclip_rectaway with..and pushes a full-size quad. Not forced, because the macOS route never enterscontent.rs.Review response
Rebased onto
main— 57 commits,git merge-treeand the rebase itself both clean. Fourcommits added on top rather than squashed, so the fixes read as a diff against what was
reviewed.
1. Clip-view orientation —
5ef37c7ecCorrect, and worse than latent.
attachnow logshost.isFlipped(), and the measuredanswer on this build is
true— so the old code took thehost_flippedbranch andwrote
-clip_top, the right number for a flipped clip, into a clip that was a stockNSView. A widget scrolled under the window's top edge was misplaced inside its own clipby exactly
height - visible_height, on every macOS build.Fixed the way GNU does it, rather than by patching the formula:
XwidgetClipView, aflipped
NSViewsubclass — the port's firstdefine_class!— matchingXvWindow(
nsxwidget.m:553).inner_originloses its parameter and becomes(-clip_left, -clip_top),xwidget.c:2995unchanged.ns_originkeepshost_flipped, since that frame really is inthe host's space.
Its gate is extracted into
needs_repositionso the rule is testable without AppKit, andit grew a third case found while rewriting it: the bottom-up origin also folds in the
host's own height, which no placement diff can see, so a window resize under an
unmoved widget left a stale origin.
WkWebViewnow remembers the host geometry it placedagainst. That path is latent while winit's view stays flipped; it is implemented and
tested because that is winit's answer to change, not ours.
Red-before confirmed on both new rules: the old gate answers
falsewhere the test assertstrue, and the oldinner_origin(false)answers0where your worked example requires-20.2. Commands lost before attachment —
d150626cbCorrect, and there was a second half. Only
WebKitCreateever calledattach, so even awindow that became available between a create and the load behind it was picked up only at
the next present — by which time
poll_commands, which drains unconditionally andindependently of frame presentation, had already discarded the load.
pendingnow holds the commands rather than the create's size: one arrival-ordered FIFO forall ids, drained by
attachthrough the ordinary methods. One queue rather than one per id,because ordering matters across ids too.
destroydrops an id's queued work; at capacity thenewest is dropped so what survives is a coherent prefix rather than loads whose create was
evicted. And every WebKit arm now tries
attach.The queue is its own type with no AppKit in it (
backend/wkwebview/pending.rs), so it isunit-testable off the main thread — eight tests, red by construction, since the old structure
had nowhere to put a URL.
3.
execute-scripton Linux —d78bd3ffaCorrect, and the tree already contained the missing half.
WebKitExecuteJavaScriptcarriedan identical payload and a real WPE dispatch with no producer anywhere, orphaned by
f812db8d1. So there was one command only macOS could reach and one only WPE could serve.Consolidated onto
WebKitExecuteScriptin the shape of the neighbouringWebKitLoadUriarm;WebKitExecuteJavaScriptdeleted. A build with neither backend now warns that the script wasdropped instead of logging that it ran.
One caveat worth stating:
wpe-webkitis not a default feature and no CI workflow buildswith it, so the new WPE arm is not compile-checked upstream. It is a copy of an arm that is,
and it is being verified on a real x86_64-linux machine — where
xwidget-webkit-scroll-*should go from no-op to working, which is the concrete gain this change makes for Linux.
4. Documentation
c_features.rsandprovide_coupled_vars.rsboth said the browse-url path "is complete".Corrected: what is true is that it works on the primary frame, which is the question a
configuration gating on
(featurep 'xwidget-internal)is asking. Both now enumerate the gaps.docs/building.mdcarries the user-facing version.Issues opened: #300 (tracking, your six follow-ups), #301 and #302 (the two
maintainer notes above).
Verification
cargo fmt --checkand clippy clean on new code.neovm-corefailure is thepre-existing macOS one,
the_features_this_build_really_has_still_answer_t, which asserts(featurep 'inotify)—left: "OK (t t t t t nil)". Unchanged by this branch.WKWebView, thenset-window-vscrollto forceclip_top > 0— the exact case in fix 1. The visible band is correct and flush with thetext-area top.
xwidget-internalstill answersNotBuilt, both ledger tests keeptheir pre-branch answers, and the only Linux-visible change is
execute-scriptstarting towork on a WPE build.