Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces WebContentsView abstraction for embedding web content across platforms (Darwin, Windows, Linux, Android, iOS) with configuration options and platform-specific implementations. Adds minor interface extensions to BrowserWindow and Window with no-op methods. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant WCV as WebContentsView<br/>(Core)
participant PImpl as Platform Impl<br/>(Android/Darwin/etc)
participant Native as Native View<br/>(WKWebView/GTK/Edge)
participant Window
Client->>WCV: NewWebContentsView(options)
WCV->>PImpl: newWebContentsViewImpl(parent)
PImpl->>Native: Create native view<br/>(configure preferences)
Native-->>PImpl: Return native pointer
PImpl-->>WCV: Return impl instance
WCV-->>Client: Return WebContentsView
Client->>WCV: SetURL(url)
WCV->>PImpl: setURL(url)
PImpl->>Native: Load URL
Client->>WCV: SetBounds(rect)
WCV->>PImpl: setBounds(rect)
PImpl->>Native: Update frame/bounds
Client->>WCV: Attach(window)
WCV->>PImpl: attach(window)
PImpl->>Native: Attach to native window
PImpl->>Window: Add view to window
Client->>WCV: ExecJS(script)
WCV->>PImpl: execJS(script)
PImpl->>Native: Execute JavaScript
Client->>WCV: Detach()
WCV->>PImpl: detach()
PImpl->>Window: Remove view from window
PImpl->>Native: Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
v3/pkg/webcontentsview/webcontentsview_darwin_test.go (1)
23-68: Test has no assertions — consider adding at least basic state-change checks.
TestWebContentsView_APISurfaceonly verifies that the API calls don't panic; there are not.Error/t.Fatalchecks confirming that e.g.SetURLactually propagated the URL tooptionsor thatSetBoundsupdated the stored bounds. Even with a mock impl, asserting on theWebContentsView's internal state after each call would give the test real signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/webcontentsview/webcontentsview_darwin_test.go` around lines 23 - 68, Add assertions to TestWebContentsView_APISurface to verify state changes after calling the API methods: after view.SetBounds assert that view.options.Bounds equals the passed Rect; after view.SetURL assert that view.options.URL equals the new URL; after view.ExecJS, Attach and Detach, either assert on a simple observable side-effect exposed by the mock implementation (e.g., mockWebContentsViewImpl recorded lastJS, attachedWindow, detached flag) or add small fields to mockWebContentsViewImpl/mockWindow to record calls and assert those fields changed; keep the test using the same WebContentsView, mockWebContentsViewImpl and mockWindow symbols so it stays headless and only validates that the methods propagate into the wrapper and mock implementation.v3/pkg/webcontentsview/webcontentsview.go (1)
62-70:nativeView()is in the interface but never exposed publiclyIf downstream consumers (e.g., platform-level embedding, custom rendering layers) need access to the native handle, there is currently no way to retrieve it through
WebContentsView. Consider adding a publicNativeView() unsafe.Pointermethod now that the interface already has the backing implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/webcontentsview/webcontentsview.go` around lines 62 - 70, The interface webContentsViewImpl already exposes nativeView(), but the public wrapper type WebContentsView lacks a corresponding exported method; add a public method func (w *WebContentsView) NativeView() unsafe.Pointer that simply delegates to the implementation (return w.impl.nativeView()) so callers can retrieve the native handle; update any constructors/receivers using WebContentsView to ensure w.impl is not nil before calling and document the exported method name NativeView to match the interface nativeView().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/pkg/webcontentsview/webcontentsview_darwin.m`:
- Around line 36-41: The prefs.defaultMonospaceFontSize field from the
WebContentsViewPreferences struct is never applied to the WebPreferences object;
update the same block that applies defaultFontSize and minimumFontSize to also
apply defaultMonospaceFontSize by checking if prefs.defaultMonospaceFontSize > 0
and calling [preferences setValue:@(prefs.defaultMonospaceFontSize)
forKey:@"defaultMonospaceFontSize"] (use the same prefs and preferences
variables so the bridged value is actually set).
- Around line 64-75: Both webContentsViewSetURL and webContentsViewExecJS crash
when passed a NULL C string; add NULL guards to avoid calling [NSString
stringWithUTF8String:] with NULL. In webContentsViewSetURL and
webContentsViewExecJS, first check that the char* (url or js) is not NULL and
that the view cast to WKWebView* is non-NULL; if the C string is NULL either
return early (no-op) or use an empty NSString fallback before constructing
NSURL/JS, and ensure evaluateJavaScript/loadRequest are only called on a
non-NULL WKWebView instance.
- Around line 10-13: The code uses private KVC keys on WKPreferences (e.g.,
[preferences setValue:@(prefs.devTools) forKey:@"developerExtrasEnabled"],
"loadsImagesAutomatically", "allowFileAccessFromFileURLs",
"allowUniversalAccessFromFileURLs") which can crash or silently break; replace
the devTools KVC with the public API by setting webView.inspectable = YES when
running on macOS 13.3+ (guard the assignment with an availability check) and
stop using undocumented KVC keys for image/file access (remove or gate them
behind supported public APIs if available); for plug-ins, replace the KVC
setValue:forKey: usage with the public property preferences.plugInsEnabled = YES
(use the WKPreferences instance named preferences and the prefs.devTools flag
where applicable).
In `@v3/pkg/webcontentsview/webcontentsview_linux.go`:
- Around line 101-103: setBounds currently always passes nil for parentFixed so
X/Y are ignored; update linuxWebContentsView and creation to support a GtkFixed
container: add a field on linuxWebContentsView to hold a C.GtkWidget*
(parentFixed), modify createWebContentsView_linux to create or use a GtkFixed
(instead of gtk_box_pack_start) when absolute positioning is needed, attach the
view to that fixed container and store its pointer, and change setBounds to pass
linuxWebContentsView.parentFixed to webContentsViewSetBounds_linux so
gtk_fixed_move is invoked and initial options.Bounds.X/Y are applied.
- Around line 38-46: The attach function webContentsViewAttach_linux currently
assumes the window's direct child is a GtkBox and simply packs the view at the
end; change it to use a GtkOverlay so the webview can be positioned/stacked
instead of appended: in webContentsViewAttach_linux, detect the window's child
(gtk_bin_get_child) and if it's not already a GtkOverlay, create a GtkOverlay,
reparent the existing child into the overlay as the main content, then add the
view as an overlay layer (showing it); if creating an overlay is impossible or
undesirable, update the function comment and public docs to explicitly state the
Linux behavior is "append to vbox" and that positioning/overlay is unsupported.
Ensure references are to webContentsViewAttach_linux, GtkWindow,
gtk_bin_get_child, GtkOverlay, and gtk_widget_show so reviewers can find the
change.
- Line 1: Add a GTK4-specific stub implementation so GTK4 Linux builds link:
create a new file with the complementary build tag (linux + cgo + gtk4,
excluding android and server) that defines newWebContentsViewImpl and any other
symbols referenced by NewWebContentsView, returning a safe stub (e.g., nil and
an error or a no-op implementation) to preserve the public API; ensure the stub
exports the same function signature as the existing GTK3-backed
newWebContentsViewImpl so callers of NewWebContentsView compile and link under
GTK4.
In `@v3/pkg/webcontentsview/webcontentsview_windows.go`:
- Around line 90-94: The detach() implementation currently only calls
w.chromium.Hide(), leaving the Chromium controller embedded so a subsequent
Attach() will call Embed() again and double-embed; update detach() to perform a
proper teardown or guard: either un-embed/release the controller (use the
go-webview2 controller close/release API on w.chromium) or at minimum set w.hwnd
= 0 and a detached flag (mirror Darwin clearing nsWindow) so subsequent Attach()
checks that state and skips re-Embed on an already-initialized controller;
change detach() to call the controller teardown/release method if available (or
set w.chromium = nil after safe teardown) and reset w.hwnd to zero to prevent
double-Embed.
- Around line 28-47: The GetSettings() call happens before the WebView2
controller exists so its err != nil path drops all WebPreferences; move the
settings application into attach() after calling
chromium.Embed()/edge.NewChromium() has initialized the CoreWebView2 controller,
so call chromium.GetSettings() inside attach() (or immediately after Embed())
and then invoke settings.PutAreDevToolsEnabled /
settings.PutAreDefaultContextMenusEnabled, settings.PutIsScriptEnabled and
chromium.PutZoomFactor based on parent.options.WebPreferences, preserving the
same conditional logic and logging/handling any error from GetSettings().
- Around line 77-88: The code incorrectly casts window.NativeWindow() directly
to w32.HWND causing a compile error and also fails to apply initial Bounds; in
attach() change the conversion to use an intermediate uintptr (i.e. cast
window.NativeWindow() to uintptr then to w32.HWND) when assigning w.hwnd
(referencing w32.HWND and window.NativeWindow()), and after calling
w.chromium.Embed(...) call w.setBounds(w.parent.options.Bounds) (or the
appropriate setBounds method) before w.chromium.Resize() and w.chromium.Show()
so the configured sub-window bounds are applied (referencing attach(),
w.chromium.Embed, w.chromium.Resize, w.chromium.Show, and
w.parent.options.Bounds).
In `@v3/pkg/webcontentsview/webcontentsview.go`:
- Around line 9-16: The WebContentsViewOptions struct exposes an HTML field that
is not read by either backend; update the code so it's clear and not silently
ignored by either removing the HTML field or marking it unimplemented: either
delete HTML from WebContentsViewOptions or add a doc comment on
WebContentsViewOptions.HTML stating it's unimplemented on Darwin/Windows and a
TODO to wire it up, and ensure any references like parent.options.HTML in the
Darwin and Windows backends are either removed or updated to consult the
implemented source when this is later implemented.
- Around line 28-50: NewWebContentsView assigns v.impl from
newWebContentsViewImpl without checking for nil, so subsequent public methods
(SetBounds, SetURL, ExecJS, Attach, Detach) will panic on nil dispatch; update
NewWebContentsView to return (*WebContentsView, error) and propagate the error
if newWebContentsViewImpl returns nil, or alternatively add nil-guards inside
each delegating method (e.g., check v.impl == nil and return early or surface an
error) so calls on a partially-constructed WebContentsView do not panic;
reference NewWebContentsView, newWebContentsViewImpl, WebContentsView.impl and
the methods SetBounds, SetURL, ExecJS, Attach, Detach when making the change.
In `@v3/pkg/webcontentsview/webpreferences.go`:
- Around line 16-47: Several WebPreferences fields (AllowRunningInsecureContent,
TextAreasAreResizable, WebGL, NavigateOnDragDrop, DefaultEncoding,
DefaultMonospaceFontSize) are declared in WebPreferences but are not applied by
any platform backends (Darwin, Linux) so they have no effect; either wire them
up in each backend (implement applying these prefs in the Darwin .m and Linux
backend code paths) or explicitly mark them as unimplemented by adding TODOs and
documentation comments on the WebPreferences struct explaining they are
unsupported on current backends (and remove the duplicated
DefaultMonospaceFontSize from the Darwin header if not used). Ensure references
to the symbols WebPreferences, AllowRunningInsecureContent,
TextAreasAreResizable, WebGL, NavigateOnDragDrop, DefaultEncoding, and
DefaultMonospaceFontSize are updated so callers/readers know these fields are
unsupported until implemented.
---
Nitpick comments:
In `@v3/pkg/webcontentsview/webcontentsview_darwin_test.go`:
- Around line 23-68: Add assertions to TestWebContentsView_APISurface to verify
state changes after calling the API methods: after view.SetBounds assert that
view.options.Bounds equals the passed Rect; after view.SetURL assert that
view.options.URL equals the new URL; after view.ExecJS, Attach and Detach,
either assert on a simple observable side-effect exposed by the mock
implementation (e.g., mockWebContentsViewImpl recorded lastJS, attachedWindow,
detached flag) or add small fields to mockWebContentsViewImpl/mockWindow to
record calls and assert those fields changed; keep the test using the same
WebContentsView, mockWebContentsViewImpl and mockWindow symbols so it stays
headless and only validates that the methods propagate into the wrapper and mock
implementation.
In `@v3/pkg/webcontentsview/webcontentsview.go`:
- Around line 62-70: The interface webContentsViewImpl already exposes
nativeView(), but the public wrapper type WebContentsView lacks a corresponding
exported method; add a public method func (w *WebContentsView) NativeView()
unsafe.Pointer that simply delegates to the implementation (return
w.impl.nativeView()) so callers can retrieve the native handle; update any
constructors/receivers using WebContentsView to ensure w.impl is not nil before
calling and document the exported method name NativeView to match the interface
nativeView().
| if (@available(macOS 10.11, *)) { | ||
| [preferences setValue:@(prefs.devTools) forKey:@"developerExtrasEnabled"]; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WKPreferences developerExtrasEnabled KVC key macOS 13 Ventura deprecated replacement
💡 Result:
On macOS Ventura, the old hack:
webView.configuration.preferences.setValue(true, forKey: "developerExtrasEnabled")was never public API (it’s KVC into WKPreferences) and is effectively obsolete/dead-end.
Public replacement (macOS 13.3+): set WKWebView.isInspectable = true (Obj‑C: inspectable = YES). This is the supported way to opt a WKWebView into Web Inspector (including for release builds). [1][2]
if `#available`(macOS 13.3, *) {
webView.isInspectable = true
}If you must support < macOS 13.3: there’s no public equivalent of developerExtrasEnabled; behavior depends on OS/SDK combination (older SDKs historically allowed inspection in debug builds), otherwise you’re back to private/unsupported workarounds. [1]
Sources: WebKit announcement of isInspectable and compatibility notes [1], WebKit Safari 16.4 tooling notes (availability + release builds) [2].
🌐 Web query:
WKPreferences private KVC keys loadsImagesAutomatically plugInsEnabled macOS documentation
💡 Result:
-
plugInsEnabledis not a “private KVC key” on macOS:WKPreferencesexposes a real Objective‑C propertyplugInsEnabled/setPlugInsEnabled:(i.e., you should set it via the property, notsetValue:forKey:). Microsoft’s binding shows it as an exported selector onWKPreferences. [1] -
loadsImagesAutomaticallyis not part of the publicWKPreferencesAPI (you won’t find it in Apple’s documentedWKPreferencessurface). People sometimes toggle it via KVC (setValue:forKey:@"loadsImagesAutomatically") because the underlying WebKit engine has an image-loading setting (seen in WebKit sources / legacy WebKit APIs), but that’s implementation detail, not a supported WebKit API contract forWKWebView. [2][3] -
Using private/undocumented keys via KVC is inherently unsupported: KVC is just a dynamic access mechanism; if the key isn’t a documented property, it can disappear or change behavior across macOS/WebKit updates, and can also crash with “not KVC-compliant” errors. Apple’s KVC docs describe KVC as indirect access to an object’s properties—if the property/key isn’t there, it’s not guaranteed. [4]
Practical takeaway:
- Use
WKPreferences.plugInsEnabledonly if you truly need it (and note that modern Safari/WebKit has largely moved away from legacy plugin models). - Don’t rely on
loadsImagesAutomaticallyvia KVC forWKWebView. If your goal is “don’t load images,” use a supported approach (e.g., content blockers / rule lists, or intercepting navigation/resource loads where feasible) rather than private preference keys.
Sources
[1] Microsoft Learn – WKPreferences.PlugInsEnabled exported selectors (plugInsEnabled / setPlugInsEnabled:) https://learn.microsoft.com/en-us/dotnet/api/webkit.wkpreferences.pluginsenabled?view=net-macos-26.2-10.0
[2] Jonathan Li blog – list of (largely private) WKPreferences KVC-accessed keys including loadsImagesAutomatically https://jonathanblog2000.blogspot.com/2016/11/
[3] WebKit source (legacy WebCore settings) – m_loadsImagesAutomatically / setLoadsImagesAutomatically https://sources.debian.org/src/webkitgtk/2.4.9-1~deb8u1/Source/WebCore/page/Settings.cpp
[4] Apple – Key-Value Coding Programming Guide (KVC accesses properties by string keys; undefined keys are not guaranteed) https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/KeyValueCoding/
🌐 Web query:
WebKit Inspector API macOS 13 Ventura WKPreferences alternatives
💡 Result:
On macOS 13 Ventura, the practical replacement for the old “WKPreferences developer extras” approach is to opt-in per web view using WKWebView.isInspectable (Objective‑C: inspectable). This is the supported API WebKit added specifically for enabling Safari Web Inspector for app WebViews. (webkit.org)
Supported alternative to WKPreferences (Web Inspector)
- Use
WKWebView.isInspectable = true(available starting macOS 13.3 / iOS 16.4 / tvOS 16.4), guarded with@available. (webkit.org)
How inspection actually shows up
- After enabling Safari’s developer features, your app/WebView appears under Safari → Develop → [Your Mac] → [Your App] (or an attached device). (webkit.org)
If you need a “WebView test harness” instead of changing your app
- Use a dedicated app that loads URLs in a
WKWebViewand exposes it to Safari’s Develop menu (e.g., “WebView Inspector”). (apps.cihat.dev)
Citations:
- 1: https://webkit.org/blog/13936/enabling-the-inspection-of-web-content-in-apps/?utm_source=openai
- 2: https://webkit.org/blog/13936/enabling-the-inspection-of-web-content-in-apps/?utm_source=openai
- 3: https://webkit.org/web-inspector/enabling-web-inspector/?utm_source=openai
- 4: https://apps.cihat.dev/webviewinspector/?utm_source=openai
Replace private KVC keys with public APIs to avoid silent failures and crashes across macOS versions.
[preferences setValue:@(prefs.devTools) forKey:@"developerExtrasEnabled"] is a private KVC hack. Replace it with the public API on macOS 13.3+: webView.inspectable = YES. If supporting earlier versions, there is no public equivalent.
Similarly, loadsImagesAutomatically, allowFileAccessFromFileURLs, and allowUniversalAccessFromFileURLs (lines 25–26, 33) are undocumented KVC keys with no guaranteed stability. Using undocumented KVC keys risks crashes (not KVC-compliant errors) or silent failures across WebKit/macOS updates.
plugInsEnabled (line 30) is a public property on WKPreferences — use preferences.plugInsEnabled = YES instead of setValue:forKey:.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_darwin.m` around lines 10 - 13, The
code uses private KVC keys on WKPreferences (e.g., [preferences
setValue:@(prefs.devTools) forKey:@"developerExtrasEnabled"],
"loadsImagesAutomatically", "allowFileAccessFromFileURLs",
"allowUniversalAccessFromFileURLs") which can crash or silently break; replace
the devTools KVC with the public API by setting webView.inspectable = YES when
running on macOS 13.3+ (guard the assignment with an availability check) and
stop using undocumented KVC keys for image/file access (remove or gate them
behind supported public APIs if available); for plug-ins, replace the KVC
setValue:forKey: usage with the public property preferences.plugInsEnabled = YES
(use the WKPreferences instance named preferences and the prefs.devTools flag
where applicable).
| if (prefs.defaultFontSize > 0) { | ||
| [preferences setValue:@(prefs.defaultFontSize) forKey:@"defaultFontSize"]; | ||
| } | ||
| if (prefs.minimumFontSize > 0) { | ||
| preferences.minimumFontSize = prefs.minimumFontSize; | ||
| } |
There was a problem hiding this comment.
defaultMonospaceFontSize is in the header struct but never applied in the .m.
WebContentsViewPreferences.defaultMonospaceFontSize is bridged from WebPreferences to the C struct but there is no [preferences setValue:@(prefs.defaultMonospaceFontSize) forKey:@"defaultMonospaceFontSize"] call here, while defaultFontSize and minimumFontSize are applied. This is an implementation gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_darwin.m` around lines 36 - 41, The
prefs.defaultMonospaceFontSize field from the WebContentsViewPreferences struct
is never applied to the WebPreferences object; update the same block that
applies defaultFontSize and minimumFontSize to also apply
defaultMonospaceFontSize by checking if prefs.defaultMonospaceFontSize > 0 and
calling [preferences setValue:@(prefs.defaultMonospaceFontSize)
forKey:@"defaultMonospaceFontSize"] (use the same prefs and preferences
variables so the bridged value is actually set).
| void webContentsViewSetURL(void* view, const char* url) { | ||
| WKWebView* webView = (WKWebView*)view; | ||
| NSString* nsURL = [NSString stringWithUTF8String:url]; | ||
| NSURLRequest* request = [NSURLRequest requestWithURL:[NSURL URLWithString:nsURL]]; | ||
| [webView loadRequest:request]; | ||
| } | ||
|
|
||
| void webContentsViewExecJS(void* view, const char* js) { | ||
| WKWebView* webView = (WKWebView*)view; | ||
| NSString* script = [NSString stringWithUTF8String:js]; | ||
| [webView evaluateJavaScript:script completionHandler:nil]; | ||
| } |
There was a problem hiding this comment.
Null-pointer crash in webContentsViewSetURL and webContentsViewExecJS.
[NSString stringWithUTF8String:url] aborts with "NSString cannot be created with a NULL C string" if url or js is NULL. Both callers (Go's setURL/execJS) pass C.CString(...) which is safe for non-empty strings, but any upstream code that passes an empty or uninitialized string could send NULL through cgo.
🛡️ Proposed NULL guards
void webContentsViewSetURL(void* view, const char* url) {
+ if (!url) return;
WKWebView* webView = (WKWebView*)view; void webContentsViewExecJS(void* view, const char* js) {
+ if (!js) return;
WKWebView* webView = (WKWebView*)view;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void webContentsViewSetURL(void* view, const char* url) { | |
| WKWebView* webView = (WKWebView*)view; | |
| NSString* nsURL = [NSString stringWithUTF8String:url]; | |
| NSURLRequest* request = [NSURLRequest requestWithURL:[NSURL URLWithString:nsURL]]; | |
| [webView loadRequest:request]; | |
| } | |
| void webContentsViewExecJS(void* view, const char* js) { | |
| WKWebView* webView = (WKWebView*)view; | |
| NSString* script = [NSString stringWithUTF8String:js]; | |
| [webView evaluateJavaScript:script completionHandler:nil]; | |
| } | |
| void webContentsViewSetURL(void* view, const char* url) { | |
| if (!url) return; | |
| WKWebView* webView = (WKWebView*)view; | |
| NSString* nsURL = [NSString stringWithUTF8String:url]; | |
| NSURLRequest* request = [NSURLRequest requestWithURL:[NSURL URLWithString:nsURL]]; | |
| [webView loadRequest:request]; | |
| } | |
| void webContentsViewExecJS(void* view, const char* js) { | |
| if (!js) return; | |
| WKWebView* webView = (WKWebView*)view; | |
| NSString* script = [NSString stringWithUTF8String:js]; | |
| [webView evaluateJavaScript:script completionHandler:nil]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_darwin.m` around lines 64 - 75, Both
webContentsViewSetURL and webContentsViewExecJS crash when passed a NULL C
string; add NULL guards to avoid calling [NSString stringWithUTF8String:] with
NULL. In webContentsViewSetURL and webContentsViewExecJS, first check that the
char* (url or js) is not NULL and that the view cast to WKWebView* is non-NULL;
if the C string is NULL either return early (no-op) or use an empty NSString
fallback before constructing NSURL/JS, and ensure evaluateJavaScript/loadRequest
are only called on a non-NULL WKWebView instance.
| @@ -0,0 +1,132 @@ | |||
| //go:build linux && cgo && !gtk4 && !android && !server | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all webcontentsview_linux files
echo "=== Searching for webcontentsview_linux files ==="
fd "webcontentsview_linux" --extension go
echo -e "\n=== Full webcontentsview directory listing ==="
find v3/pkg/webcontentsview -name "*.go" -type f | sortRepository: wailsapp/wails
Length of output: 583
🏁 Script executed:
#!/bin/bash
echo "=== webcontentsview.go (main interface) ==="
head -100 v3/pkg/webcontentsview/webcontentsview.go
echo -e "\n=== webcontentsview_linux.go (build constraint and start) ==="
head -50 v3/pkg/webcontentsview/webcontentsview_linux.go
echo -e "\n=== Searching for 'newWebContentsViewImpl' definition ==="
grep -r "newWebContentsViewImpl" v3/pkg/webcontentsview/
echo -e "\n=== Searching for webcontentsview imports in GTK4 context ==="
rg "webcontentsview" v3 -t go --max-count=20Repository: wailsapp/wails
Length of output: 5461
🏁 Script executed:
#!/bin/bash
echo "=== Search for imports of webcontentsview package ==="
rg "github.com/wailsapp/wails/v3/pkg/webcontentsview" v3 -t go --max-count=30
echo -e "\n=== Search for NewWebContentsView usage ==="
rg "NewWebContentsView" v3 -t go -B 2 -A 2 --max-count=30
echo -e "\n=== Check application package for GTK4 references and webcontentsview usage ==="
rg "webcontentsview|gtk4" v3/pkg/application -t go --max-count=30Repository: wailsapp/wails
Length of output: 2810
🏁 Script executed:
#!/bin/bash
echo "=== Check if webcontentsview is in any go.mod ==="
find v3 -name "go.mod" -exec grep -l "webcontentsview" {} \;
echo -e "\n=== Check if webcontentsview is in go.sum ==="
grep -i "webcontentsview" v3/go.sum 2>/dev/null || echo "Not found in go.sum"
echo -e "\n=== List all go.mod files ==="
find v3 -name "go.mod" -type f
echo -e "\n=== Check v3/go.mod for internal package structure ==="
head -50 v3/go.mod
echo -e "\n=== Search for 'webcontentsview' references outside the package itself ==="
rg "webcontentsview" v3 --type go -g "!v3/pkg/webcontentsview"
echo -e "\n=== Check README or docs for WebContentsView ==="
rg -i "webcontentsview|web content view" v3 --max-count=10Repository: wailsapp/wails
Length of output: 11570
🏁 Script executed:
#!/bin/bash
echo "=== Check build constraint in webcontentsview_linux.go again ==="
head -5 v3/pkg/webcontentsview/webcontentsview_linux.go
echo -e "\n=== Check what webkit version is used in webcontentsview_linux ==="
grep -A 5 "#cgo" v3/pkg/webcontentsview/webcontentsview_linux.go
echo -e "\n=== Check if GTK4 uses webkitgtk-6.0 ==="
grep "webkitgtk-6.0\|webkit2gtk" v3/pkg/application/linux_cgo_gtk4.go
echo -e "\n=== Check for any GTK4 webcontentsview stub (catch variations) ==="
find v3/pkg/webcontentsview -type f -name "*.go" -o -name "*.m" -o -name "*.c" | xargs ls -la
echo -e "\n=== Check git history for webcontentsview_linux constraint (recent context) ==="
git log --oneline -n 30 v3/pkg/webcontentsview/ 2>/dev/null || echo "Git history not available"Repository: wailsapp/wails
Length of output: 1673
GTK4 Linux builds cannot use webcontentsview — missing GTK4 stub implementation.
The build constraint !gtk4 on webcontentsview_linux.go excludes this GTK3 implementation for GTK4 Linux builds, but there is no corresponding webcontentsview_linux_gtk4.go stub. Any GTK4 code that imports webcontentsview and calls NewWebContentsView will fail to link because newWebContentsViewImpl is undefined for that build configuration. While the package is currently unused internally, this represents an incomplete public API for GTK4.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_linux.go` at line 1, Add a
GTK4-specific stub implementation so GTK4 Linux builds link: create a new file
with the complementary build tag (linux + cgo + gtk4, excluding android and
server) that defines newWebContentsViewImpl and any other symbols referenced by
NewWebContentsView, returning a safe stub (e.g., nil and an error or a no-op
implementation) to preserve the public API; ensure the stub exports the same
function signature as the existing GTK3-backed newWebContentsViewImpl so callers
of NewWebContentsView compile and link under GTK4.
| static void webContentsViewAttach_linux(void* window, void* view) { | ||
| // Attempt to add to the main container. Wails v3 usually uses a vbox. | ||
| GtkWindow *gtkWindow = GTK_WINDOW(window); | ||
| GtkWidget *child = gtk_bin_get_child(GTK_BIN(gtkWindow)); | ||
| if (child != NULL && GTK_IS_BOX(child)) { | ||
| gtk_box_pack_start(GTK_BOX(child), GTK_WIDGET(view), FALSE, FALSE, 0); | ||
| gtk_widget_show(GTK_WIDGET(view)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fragile hardcoded assumption about Wails' internal GTK window structure.
webContentsViewAttach_linux silently does nothing if the window's direct child is not a GtkBox. The comment acknowledges "Wails v3 usually uses a vbox" — but this is implementation-specific and undocumented. Even when the child is a GtkBox, gtk_box_pack_start appends the webview at the end of the box rather than overlaying it at a specific position, which contradicts the purpose of a WebContentsView.
Consider either:
- Using an overlay container (e.g.
GtkOverlay) when attaching, or - Documenting Linux's current "append to vbox" behavior as a known limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_linux.go` around lines 38 - 46, The
attach function webContentsViewAttach_linux currently assumes the window's
direct child is a GtkBox and simply packs the view at the end; change it to use
a GtkOverlay so the webview can be positioned/stacked instead of appended: in
webContentsViewAttach_linux, detect the window's child (gtk_bin_get_child) and
if it's not already a GtkOverlay, create a GtkOverlay, reparent the existing
child into the overlay as the main content, then add the view as an overlay
layer (showing it); if creating an overlay is impossible or undesirable, update
the function comment and public docs to explicitly state the Linux behavior is
"append to vbox" and that positioning/overlay is unsupported. Ensure references
are to webContentsViewAttach_linux, GtkWindow, gtk_bin_get_child, GtkOverlay,
and gtk_widget_show so reviewers can find the change.
| if window.NativeWindow() != nil { | ||
| w.hwnd = w32.HWND(window.NativeWindow()) | ||
| w.chromium.Embed(w.hwnd) | ||
|
|
||
| w.chromium.Resize() | ||
| w.chromium.Show() | ||
|
|
||
| if w.parent.options.URL != "" { | ||
| w.chromium.Navigate(w.parent.options.URL) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check w32 type definitions for HANDLE and HWND
echo "=== Checking w32 type definitions ==="
rg -n "type\s+(HANDLE|HWND)" --type=go v3/pkg/w32/ -A2Repository: wailsapp/wails
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check how other code converts unsafe.Pointer to w32.HWND
echo "=== Checking unsafe.Pointer to w32.HWND conversions ==="
rg -n "w32\.HWND\(" --type=go -B2 -A2 v3/ | head -100Repository: wailsapp/wails
Length of output: 555
🏁 Script executed:
#!/bin/bash
# Check Darwin implementation for Bounds handling
echo "=== Checking Darwin webcontentsview attach() for Bounds handling ==="
find v3 -name "*webcontentsview*darwin*" -type f | head -5Repository: wailsapp/wails
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Search for w32 package definition more broadly
echo "=== Searching for w32 package import/definition ==="
rg -n "package\s+w32|import.*w32" --type=go v3/ | head -20Repository: wailsapp/wails
Length of output: 814
🏁 Script executed:
#!/bin/bash
# Check if w32 is in vendor directory or external
echo "=== Checking for w32 in vendor or imports ==="
find v3 -type f -name "*.go" -exec grep -l "w32\." {} \; | head -10Repository: wailsapp/wails
Length of output: 726
🏁 Script executed:
#!/bin/bash
# Look at the full context of webcontentsview_windows.go
echo "=== Full webcontentsview_windows.go file ==="
wc -l v3/pkg/webcontentsview/webcontentsview_windows.goRepository: wailsapp/wails
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check the Darwin attach() for Bounds handling
echo "=== Darwin attach() implementation ==="
rg -n "func.*attach" v3/pkg/webcontentsview/webcontentsview_darwin.m -A20Repository: wailsapp/wails
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check typedef.go for HANDLE and HWND definitions
echo "=== v3/pkg/w32/typedef.go ==="
cat v3/pkg/w32/typedef.goRepository: wailsapp/wails
Length of output: 28208
🏁 Script executed:
#!/bin/bash
# Read full webcontentsview_windows.go
echo "=== Full webcontentsview_windows.go ==="
cat -n v3/pkg/webcontentsview/webcontentsview_windows.goRepository: wailsapp/wails
Length of output: 3013
🏁 Script executed:
#!/bin/bash
# Check Darwin implementation for comparison
echo "=== webcontentsview_darwin.go ==="
cat -n v3/pkg/webcontentsview/webcontentsview_darwin.goRepository: wailsapp/wails
Length of output: 3436
w32.HWND(window.NativeWindow()) is a compile error — missing intermediate uintptr cast
Per the Go specification, unsafe.Pointer can only be directly converted to the predeclared uintptr type, not to a named type whose underlying type is uintptr. Since w32.HWND → w32.HANDLE → uintptr, the correct conversion requires an intermediate step: w32.HWND(uintptr(window.NativeWindow())).
Additionally, the initial Bounds from WebContentsViewOptions are never applied during attach() — Resize() fills the entire parent window, ignoring the configured sub-window bounds. Call setBounds() with w.parent.options.Bounds after embed to match the Darwin behaviour.
Proposed fix
- w.hwnd = w32.HWND(window.NativeWindow())
+ w.hwnd = w32.HWND(uintptr(window.NativeWindow()))
w.chromium.Embed(w.hwnd)
+ if (w.parent.options.Bounds != application.Rect{}) {
+ w.setBounds(w.parent.options.Bounds)
+ }
w.chromium.Resize()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_windows.go` around lines 77 - 88, The
code incorrectly casts window.NativeWindow() directly to w32.HWND causing a
compile error and also fails to apply initial Bounds; in attach() change the
conversion to use an intermediate uintptr (i.e. cast window.NativeWindow() to
uintptr then to w32.HWND) when assigning w.hwnd (referencing w32.HWND and
window.NativeWindow()), and after calling w.chromium.Embed(...) call
w.setBounds(w.parent.options.Bounds) (or the appropriate setBounds method)
before w.chromium.Resize() and w.chromium.Show() so the configured sub-window
bounds are applied (referencing attach(), w.chromium.Embed, w.chromium.Resize,
w.chromium.Show, and w.parent.options.Bounds).
| func (w *windowsWebContentsView) detach() { | ||
| if w.chromium != nil { | ||
| w.chromium.Hide() | ||
| } | ||
| } |
There was a problem hiding this comment.
detach() only hides the WebView2 instance; re-Attach() will double-Embed()
Hide() keeps the Chromium instance embedded in the parent HWND. If Attach() is called again after Detach(), Embed() will be invoked a second time on an already-initialized controller, which is unsafe with go-webview2. Add a guard or a proper teardown path, and reset w.hwnd to zero on detach, mirroring the Darwin implementation that clears nsWindow.
🐛 Proposed fix
func (w *windowsWebContentsView) detach() {
if w.chromium != nil {
w.chromium.Hide()
+ w.hwnd = 0
}
}
func (w *windowsWebContentsView) attach(window application.Window) {
if window.NativeWindow() != nil {
+ if w.hwnd != 0 {
+ return // already attached
+ }
w.hwnd = w32.HWND(uintptr(window.NativeWindow()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview_windows.go` around lines 90 - 94, The
detach() implementation currently only calls w.chromium.Hide(), leaving the
Chromium controller embedded so a subsequent Attach() will call Embed() again
and double-embed; update detach() to perform a proper teardown or guard: either
un-embed/release the controller (use the go-webview2 controller close/release
API on w.chromium) or at minimum set w.hwnd = 0 and a detached flag (mirror
Darwin clearing nsWindow) so subsequent Attach() checks that state and skips
re-Embed on an already-initialized controller; change detach() to call the
controller teardown/release method if available (or set w.chromium = nil after
safe teardown) and reset w.hwnd to zero to prevent double-Embed.
| // WebContentsViewOptions represents the options for creating a WebContentsView. | ||
| type WebContentsViewOptions struct { | ||
| Name string | ||
| URL string | ||
| HTML string | ||
| Bounds application.Rect | ||
| WebPreferences WebPreferences | ||
| } |
There was a problem hiding this comment.
HTML field in WebContentsViewOptions is unimplemented on all platforms
Neither the Darwin nor the Windows backend reads parent.options.HTML, so setting this field silently has no effect. Either remove it until it is wired up, or add a doc comment marking it as unimplemented.
✏️ Suggested doc annotation (minimal change)
URL string
- HTML string
+ // HTML is not yet implemented and is reserved for future use.
+ HTML string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WebContentsViewOptions represents the options for creating a WebContentsView. | |
| type WebContentsViewOptions struct { | |
| Name string | |
| URL string | |
| HTML string | |
| Bounds application.Rect | |
| WebPreferences WebPreferences | |
| } | |
| // WebContentsViewOptions represents the options for creating a WebContentsView. | |
| type WebContentsViewOptions struct { | |
| Name string | |
| URL string | |
| // HTML is not yet implemented and is reserved for future use. | |
| HTML string | |
| Bounds application.Rect | |
| WebPreferences WebPreferences | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview.go` around lines 9 - 16, The
WebContentsViewOptions struct exposes an HTML field that is not read by either
backend; update the code so it's clear and not silently ignored by either
removing the HTML field or marking it unimplemented: either delete HTML from
WebContentsViewOptions or add a doc comment on WebContentsViewOptions.HTML
stating it's unimplemented on Darwin/Windows and a TODO to wire it up, and
ensure any references like parent.options.HTML in the Darwin and Windows
backends are either removed or updated to consult the implemented source when
this is later implemented.
| func NewWebContentsView(options WebContentsViewOptions) *WebContentsView { | ||
| result := &WebContentsView{ | ||
| id: uint(atomic.AddUintptr(&webContentsViewID, 1)), | ||
| options: options, | ||
| } | ||
| result.impl = newWebContentsViewImpl(result) | ||
| return result | ||
| } | ||
|
|
||
| // SetBounds sets the position and size of the WebContentsView relative to its parent. | ||
| func (v *WebContentsView) SetBounds(bounds application.Rect) { | ||
| v.impl.setBounds(bounds) | ||
| } | ||
|
|
||
| // SetURL loads the given URL into the WebContentsView. | ||
| func (v *WebContentsView) SetURL(url string) { | ||
| v.impl.setURL(url) | ||
| } | ||
|
|
||
| // ExecJS executes the given javascript in the WebContentsView. | ||
| func (v *WebContentsView) ExecJS(js string) { | ||
| v.impl.execJS(js) | ||
| } |
There was a problem hiding this comment.
No nil guard on impl — all public methods will panic if construction fails
NewWebContentsView assigns impl without checking the return value. If newWebContentsViewImpl returns nil (e.g., on a native-view allocation failure as proposed in the Darwin fix), every subsequent call to SetBounds, SetURL, ExecJS, Attach, or Detach will panic on a nil interface dispatch.
Either return (*WebContentsView, error) from the constructor, or add nil guards on each delegating method.
✏️ Minimal nil-guard approach
func (v *WebContentsView) SetBounds(bounds application.Rect) {
+ if v.impl == nil { return }
v.impl.setBounds(bounds)
}
func (v *WebContentsView) SetURL(url string) {
+ if v.impl == nil { return }
v.impl.setURL(url)
}
func (v *WebContentsView) ExecJS(js string) {
+ if v.impl == nil { return }
v.impl.execJS(js)
}
func (v *WebContentsView) Attach(window application.Window) {
+ if v.impl == nil { return }
v.impl.attach(window)
}
func (v *WebContentsView) Detach() {
+ if v.impl == nil { return }
v.impl.detach()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webcontentsview.go` around lines 28 - 50,
NewWebContentsView assigns v.impl from newWebContentsViewImpl without checking
for nil, so subsequent public methods (SetBounds, SetURL, ExecJS, Attach,
Detach) will panic on nil dispatch; update NewWebContentsView to return
(*WebContentsView, error) and propagate the error if newWebContentsViewImpl
returns nil, or alternatively add nil-guards inside each delegating method
(e.g., check v.impl == nil and return early or surface an error) so calls on a
partially-constructed WebContentsView do not panic; reference
NewWebContentsView, newWebContentsViewImpl, WebContentsView.impl and the methods
SetBounds, SetURL, ExecJS, Attach, Detach when making the change.
| // AllowRunningInsecureContent allows an https page to run http code. Default is false. | ||
| AllowRunningInsecureContent u.Bool | ||
|
|
||
| // Images enables or disables image loading. Default is true. | ||
| Images u.Bool | ||
|
|
||
| // TextAreasAreResizable controls whether text areas can be resized. Default is true. | ||
| TextAreasAreResizable u.Bool | ||
|
|
||
| // WebGL enables or disables WebGL. Default is true. | ||
| WebGL u.Bool | ||
|
|
||
| // Plugins enables or disables plugins. Default is false. | ||
| Plugins u.Bool | ||
|
|
||
| // ZoomFactor sets the default zoom factor of the page. Default is 1.0. | ||
| ZoomFactor float64 | ||
|
|
||
| // NavigateOnDragDrop controls whether dropping files triggers navigation. Default is false. | ||
| NavigateOnDragDrop u.Bool | ||
|
|
||
| // DefaultFontSize sets the default font size. Default is 16. | ||
| DefaultFontSize int | ||
|
|
||
| // DefaultMonospaceFontSize sets the default monospace font size. Default is 13. | ||
| DefaultMonospaceFontSize int | ||
|
|
||
| // MinimumFontSize sets the minimum font size. Default is 0. | ||
| MinimumFontSize int | ||
|
|
||
| // DefaultEncoding sets the default character encoding. Default is "UTF-8". | ||
| DefaultEncoding string |
There was a problem hiding this comment.
Several WebPreferences fields are declared but silently ignored by all current platform backends.
The following fields have no corresponding implementation in the Darwin, Linux, or other visible platform backends:
AllowRunningInsecureContentTextAreasAreResizableWebGLNavigateOnDragDropDefaultEncodingDefaultMonospaceFontSize(declared in the Darwin header struct but not applied in.m)
Until these are wired up, users who set them will see no effect. Consider either deferring the declaration of unimplemented fields or adding a clear doc note that they are not yet supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v3/pkg/webcontentsview/webpreferences.go` around lines 16 - 47, Several
WebPreferences fields (AllowRunningInsecureContent, TextAreasAreResizable,
WebGL, NavigateOnDragDrop, DefaultEncoding, DefaultMonospaceFontSize) are
declared in WebPreferences but are not applied by any platform backends (Darwin,
Linux) so they have no effect; either wire them up in each backend (implement
applying these prefs in the Darwin .m and Linux backend code paths) or
explicitly mark them as unimplemented by adding TODOs and documentation comments
on the WebPreferences struct explaining they are unsupported on current backends
(and remove the duplicated DefaultMonospaceFontSize from the Darwin header if
not used). Ensure references to the symbols WebPreferences,
AllowRunningInsecureContent, TextAreasAreResizable, WebGL, NavigateOnDragDrop,
DefaultEncoding, and DefaultMonospaceFontSize are updated so callers/readers
know these fields are unsupported until implemented.
Fix compile errors on Windows builds related to the webcontentsview package. - Corrected the argument type for `ResizeWithBounds` to accept `*edge.Rect`. - Added the missing `takeSnapshot()` method to satisfy the `webContentsViewImpl` interface. These changes were verified by running Windows-targeted compile checks for the `webcontentsview` and `application` packages within the `v3` directory. Co-Authored-By: Cosine CLI (codex) <agent@cosine.sh>
PandelisZ
force-pushed
the
v3-webcontentsview
branch
from
9cbb3f6 to
8b78759
Compare
1 participant
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Release Notes