Core - Support Chrome command execution.#5257
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Chrome command execution capabilities to CefSharp. It introduces a new ChromeCommandIds enum, command ID mapping infrastructure that converts command names to numeric IDs, and extends IBrowserHost with methods to query whether a command can execute and to execute Chrome commands with specified window dispositions. The implementation enforces UI-thread execution where required and provides both synchronous and asynchronous extension helpers for convenience. ChangesChrome Command Execution and ID Mapping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp`:
- Around line 683-688: The wrapper ExecuteChromeCommand currently calls
_browserHost->ExecuteChromeCommand(...) without enforcing CEF UI-thread; add the
same UI-thread guard used by other wrappers by calling
ThrowIfExecutedOnNonCefUiThread() (or, if you prefer to marshal, post the call
to CefPostTask(TID_UI, ...) so the underlying _browserHost->ExecuteChromeCommand
runs on the CEF UI thread) before invoking _browserHost->ExecuteChromeCommand in
CefBrowserHostWrapper::ExecuteChromeCommand, keeping the existing
ThrowIfDisposed() check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a6996d1-0d1d-442b-bd43-8066b8a60931
📒 Files selected for processing (4)
CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cppCefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.hCefSharp/IBrowserHost.csCefSharp/WebBrowserExtensions.cs
| void CefBrowserHostWrapper::ExecuteChromeCommand(int commandId, WindowOpenDisposition disposition) | ||
| { | ||
| ThrowIfDisposed(); | ||
|
|
||
| _browserHost->ExecuteChromeCommand(commandId, (cef_window_open_disposition_t)disposition); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does CEF CefBrowserHost::ExecuteChromeCommand require calling on CEF UI thread?
💡 Result:
Yes, the CefBrowserHost::ExecuteChromeCommand method must be called on the CEF UI thread [1]. According to the official Chromium Embedded Framework (CEF) documentation, this method specifically requires execution on the UI thread [1]. While many methods in the CefBrowserHost class may be called on any thread in the browser process unless otherwise indicated, ExecuteChromeCommand is explicitly documented with this thread-affinity requirement [1]. If your code is executing on a different thread, you must marshal the task to the UI thread, typically by using CefPostTask with the TID_UI thread identifier [2][3].
Citations:
- 1: https://cef-builds.spotifycdn.com/docs/136.0/classCefBrowserHost.html
- 2: https://github.com/chromiumembedded/cef/blob/master/include/cef_task.h
- 3: https://www.magpcss.org/ceforum/viewtopic.php?f=6&t=18856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp"
# Show the relevant sections around ExecuteChromeCommand / CanExecuteChromeCommand
sed -n '630,720p' "$file" | nl -ba | sed -n '1,120p'
# Show the TryCloseBrowser mentioned in the comment for comparison (line ~158)
sed -n '120,220p' "$file" | nl -ba | sed -n '1,160p'
# Search for the helper used for non-UI thread checks
rg -n "ThrowIfExecutedOnNonCefUiThread|ExecutedOnNonCefUiThread|NonCefUiThread" "$file" || trueRepository: cefsharp/CefSharp
Length of output: 104
🌐 Web query:
CefBrowserHost::ExecuteChromeCommand must be called on the UI thread CEF docs
💡 Result:
Yes, CefBrowserHost::ExecuteChromeCommand must be called on the UI thread [1][2]. Official CEF documentation and implementation source code consistently specify that this method is intended for use with the Chrome runtime and has a strict UI thread requirement [1][2][3]. If you need to call this method from a different thread, you must use CEF task posting mechanisms to dispatch the call to the UI thread [3]. For example, in C++ you can use the following approach: if (!CefCurrentlyOn(TID_UI)) { CefPostTask(TID_UI, base::BindOnce(&CefBrowserHost::ExecuteChromeCommand, host, command_id, disposition)); return; } host->ExecuteChromeCommand(command_id, disposition); This pattern ensures that the command is executed safely within the context of the UI thread [3]. Failing to adhere to this requirement may result in threading violations or undefined behavior [4][3].
Citations:
- 1: https://cef-builds.spotifycdn.com/docs/136.0/classCefBrowserHost.html
- 2: https://github.com/chromiumembedded/cef/blob/master/include/cef_browser.h
- 3: chromiumembedded/cef@53ef570
- 4: https://github.com/chromiumembedded/cef/blob/45a17430/libcef/browser/browser_host_base.cc
🌐 Web query:
CefSharp CefBrowserHostWrapper ThrowIfExecutedOnNonCefUiThread TryCloseBrowser line 158
💡 Result:
In CefSharp, the method CefBrowserHostWrapper::TryCloseBrowser calls ThrowIfExecutedOnNonCefUiThread to enforce that the operation is performed on the CEF UI thread [1]. This restriction exists because many Chromium Embedded Framework (CEF) browser host operations are thread-sensitive and must be executed on the thread managed by CEF [2][3]. The specific reference to "line 158" in your query likely pertains to a version-specific implementation of the CefBrowserHostWrapper.cpp source file [1]. As codebase lines shift with updates, the exact location of this check may vary across different versions of CefSharp, but the underlying requirement remains consistent: 1. Thread Requirement: The IBrowserHost.TryCloseBrowser method must be called on the CEF UI thread [3]. 2. Enforcement: The wrapper class uses ThrowIfExecutedOnNonCefUiThread to verify the calling thread context [1]. If the method is called from a different thread (e.g., your application's main UI thread, if they differ), an exception will be thrown [1][4]. If you are encountering an exception originating from this check, you must ensure your call to TryCloseBrowser is dispatched to the CEF UI thread [4]. In WinForms or WPF, you can use the appropriate dispatcher or invoke mechanism provided by the browser control to ensure the code executes within the correct context [4].
Citations:
- 1: https://github.com/cefsharp/CefSharp/blob/master/CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp
- 2: https://github.com/cefsharp/CefSharp/blob/master/CefSharp/IBrowserHost.cs
- 3: https://cefsharp.github.io/api/100.0.x/html/M_CefSharp_IBrowserHost_TryCloseBrowser.htm
- 4: http://cefsharp.github.io/api/79.1.x/html/Events_T_CefSharp_Wpf_ChromiumWebBrowser.htm
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp"
# Print ExecuteChromeCommand + surrounding code with line numbers (no `nl`)
sed -n '630,705p' "$file" | awk '{printf "%6d\t%s\n", NR+629, $0}'
# Print the earlier area to find TryCloseBrowser / thread checks
sed -n '120,220p' "$file" | awk '{printf "%6d\t%s\n", NR+119, $0}'
# Locate thread-check helpers used in this file
rg -n "ThrowIfExecutedOnNonCefUiThread|ExecutedOnNonCefUiThread|CefCurrentlyOn|TID_UI|On(Cef|NonCef).*UiThread" "$file" || true
# Locate CanExecuteChromeCommand region for context
sed -n '600,670p' "$file" | awk '{printf "%6d\t%s\n", NR+599, $0}'Repository: cefsharp/CefSharp
Length of output: 8606
🌐 Web query:
site:cef-builds.spotifycdn.com CefBrowserHost ExecuteChromeCommand must be called on the UI thread
💡 Result:
Yes, the CefBrowserHost::ExecuteChromeCommand method must be called on the browser process UI thread [1][2][3]. This requirement is explicitly documented in the official Chromium Embedded Framework (CEF) API reference for the CefBrowserHost class [1][2][3]. If you are not currently on the UI thread, you must marshal the call to the UI thread using CefPostTask [4]. For example: CefPostTask(TID_UI, base::BindOnce([](CefRefPtr host) { host->ExecuteChromeCommand(IDC_YOUR_COMMAND, WINDOW_OPEN_DISPOSITION_CURRENT_TAB); }, browser->GetHost)); Always ensure that you are using version-safe mappings for command IDs, typically via the cef_id_for_command_id_name function, when dealing with Chrome style commands [3].
Citations:
- 1: https://cef-builds.spotifycdn.com/docs/132.0/classCefBrowserHost.html
- 2: https://cef-builds.spotifycdn.com/docs/125.0/classCefBrowserHost.html
- 3: https://cef-builds.spotifycdn.com/docs/136.0/classCefBrowserHost.html
- 4: https://cef-builds.spotifycdn.com/docs/112.3/cef__task_8h.html
Enforce CEF UI-thread for CefBrowserHostWrapper::ExecuteChromeCommand
ExecuteChromeCommand(int, WindowOpenDisposition) (CefBrowserHostWrapper.cpp:683-688) calls _browserHost->ExecuteChromeCommand(...) without a CEF UI-thread guard, while related wrapper APIs enforce it (e.g., CanExecuteChromeCommand throws unless CefCurrentlyOn(TID_UI), and TryCloseBrowser uses ThrowIfExecutedOnNonCefUiThread()). CEF’s API documentation requires CefBrowserHost::ExecuteChromeCommand to be executed on the CEF UI thread, so this wrapper should similarly check/marshal to TID_UI before invoking the underlying call. [minor]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp` around lines 683 -
688, The wrapper ExecuteChromeCommand currently calls
_browserHost->ExecuteChromeCommand(...) without enforcing CEF UI-thread; add the
same UI-thread guard used by other wrappers by calling
ThrowIfExecutedOnNonCefUiThread() (or, if you prefer to marshal, post the call
to CefPostTask(TID_UI, ...) so the underlying _browserHost->ExecuteChromeCommand
runs on the CEF UI thread) before invoking _browserHost->ExecuteChromeCommand in
CefBrowserHostWrapper::ExecuteChromeCommand, keeping the existing
ThrowIfDisposed() check.
|
✅ Build CefSharp 148.0.90-CI5505 completed (commit dbe20d8de9 by @SLT-World) |
|
❌ Build CefSharp 148.0.90-CI5511 failed (commit 76ca6993ec by @SLT-World) |
SLT-World
force-pushed
the
chrome-commands
branch
from
108b074 to
b9b077a
Compare
SLT-World
force-pushed
the
chrome-commands
branch
from
b9b077a to
fc4d1d5
Compare
|
✅ Build CefSharp 148.0.90-CI5513 completed (commit 6a98a9e8f1 by @SLT-World) |
amaitland
left a comment
amaitland
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Few comments inline, happy to discuss
| return false; | ||
| } | ||
|
|
||
| void CefBrowserHostWrapper::ExecuteChromeCommand(int commandId, WindowOpenDisposition disposition) |
There was a problem hiding this comment.
Typically we'd create an enum from the command Id's to make it more user friendly, obviously that has some downsides, needs to be kept up to date, though I wonder how often commands change.
I think it's more user friendly than exposing CEF's string to int conversion method.
There was a problem hiding this comment.
I've actually considered manually mapping the command IDs to an enum initially before discovering cef_id_for_command_id_name.
However, looking at the Chromium github mirror, these IDs are updated and changed almost every week or two.
https://github.com/chromium/chromium/commits/main/chrome/app/chrome_command_ids.h
Sometimes quite drastically, as seen in this commit:
chromium/chromium@8d255c6
Using an enum would likely require updates with every CefSharp release just to keep up, which is probably why CEF included the cef_id_for_command_id_name.
An enum is definitely more user-friendly, so I've implemented ChromeCommandIds enum as requested, but it did take quite a considerable amount of time.
Given the sheer number of commands, the time required to map them, and their frequent changes in Chromium, I do not recommend going forward with this enum.
It can be used by
(int)ChromeCommandIds.IDC_RELOAD
|
Alright, I've removed the private method, moved I've also implemented |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CefSharp.Core/WebBrowserExtensionsEx.cs (1)
293-298: ⚡ Quick winConsider adding exception handling in the marshaled delegate to prevent hanging tasks.
If
host.CanExecuteChromeCommand(commandId)throws (e.g., the host is disposed between the null check on line 282 and when this delegate executes on the CEF UI thread), the exception will be unhandled andtcswill never complete, causing the returned task to hang indefinitely.🛡️ Proposed fix to propagate exceptions
Cef.UIThreadTaskFactory.StartNew(delegate { - var availability = host.CanExecuteChromeCommand(commandId); - - tcs.TrySetResult(availability); + try + { + var availability = host.CanExecuteChromeCommand(commandId); + tcs.TrySetResult(availability); + } + catch (Exception ex) + { + tcs.TrySetException(ex); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CefSharp.Core/WebBrowserExtensionsEx.cs` around lines 293 - 298, The marshaled delegate passed to Cef.UIThreadTaskFactory.StartNew can throw (host.CanExecuteChromeCommand) and currently leaves tcs unresolved; wrap the delegate body in a try/catch, call tcs.TrySetResult(availability) on success and in the catch call tcs.TrySetException(ex) (or tcs.TrySetCanceled if you detect disposal) so the returned Task never hangs; update the delegate around the host.CanExecuteChromeCommand(commandId) call and tcs usage to ensure exceptions are propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CefSharp.Core/WebBrowserExtensionsEx.cs`:
- Around line 293-298: The marshaled delegate passed to
Cef.UIThreadTaskFactory.StartNew can throw (host.CanExecuteChromeCommand) and
currently leaves tcs unresolved; wrap the delegate body in a try/catch, call
tcs.TrySetResult(availability) on success and in the catch call
tcs.TrySetException(ex) (or tcs.TrySetCanceled if you detect disposal) so the
returned Task never hangs; update the delegate around the
host.CanExecuteChromeCommand(commandId) call and tcs usage to ensure exceptions
are propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70f8b090-9714-476c-a8a4-416b0007f765
📒 Files selected for processing (9)
CefSharp.Core.Runtime.RefAssembly/CefSharp.Core.Runtime.netcore.csCefSharp.Core.Runtime/Cef.hCefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cppCefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.hCefSharp.Core/Cef.csCefSharp.Core/WebBrowserExtensionsEx.csCefSharp/Enums/ChromeCommandIds.csCefSharp/IBrowserHost.csCefSharp/WebBrowserExtensions.cs
💤 Files with no reviewable changes (4)
- CefSharp/IBrowserHost.cs
- CefSharp/WebBrowserExtensions.cs
- CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.cpp
- CefSharp.Core.Runtime/Internals/CefBrowserHostWrapper.h
|
✅ Build CefSharp 148.0.90-CI5515 completed (commit 86270a5ad2 by @SLT-World) |
3 participants
Fixes: -
Summary:
CanExecuteChromeCommandandExecuteChromeCommandfrom CEF to CefSharp. However, they only function in Chrome runtime style.chromiumembedded/cef@53ef570
https://cef-builds.spotifycdn.com/docs/148.0/classCefBrowserHost.html#a95d2a68db5a1b3dafa5874d92fa9ff2f
Changes:
IBrowserHost.cs,WebBrowserExtensions.cs,CefBrowserHostWrapper.handCefBrowserHostWrapper.cpp.CanExecuteChromeCommand&ExecuteChromeCommand. With an additionalCanExecuteChromeCommandAsyncfor non-CEF UI thread execution.CanExecuteChromeCommandAsync&ExecuteChromeCommandtoWebBrowserExtensionssoChromiumWebBrowser.ExecuteChromeCommand(-----)can be directly used withoutGetBrowserHost().CefIdMappers.cs,CefIdMappers.handCefIdMappers.cpp.CefIdForCommandIdNameas a command ID mapping utility ("IDC_FULLSCREEN"into34030) from https://github.com/chromiumembedded/cef/blob/master/include/cef_id_mappers.hHow Has This Been Tested?
Operating System: Windows 11
Environment: Visual Studio 2022
Changes function after adding an temporary button (not included in pull request):
within
MainWindow.xaml.csin CefSharp.Wpf.HwndHost.Example.A list of supported chrome command IDs can be seen here: https://chromium.googlesource.com/chromium/src/+/master/chrome/app/chrome_command_ids.h
After some testing, I believe this feature can be quite beneficial in the following use cases within Chrome runtime style:
IDC_SAVE_PAGE, opens a save page dialog for the current site.Previously I've only been able to accomplish this with
Browser.StartDownload(Address), which did not work on custom local schemes.IDC_TASK_MANAGER, opens Chrome's task manager. Similar to WebView2's OpenTaskManagerWindow API.IDC_RELOAD_BYPASSING_CACHE&IDC_RELOAD_CLEARING_CACHE, essentially Chrome's hard reload & empty cache without reload.IDC_QRCODE_GENERATOR, opens a QR code popup.There's still a handful of commands I have not tested, but some commands appeared to have no visible effect.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes