Custom schema, onBeforeBrowse, expose "callEvent" to cef#198
Open
CrosRoad95 wants to merge 1 commit into
Open
Conversation
WalkthroughThis PR implements a CEF scheme handler factory feature with URL navigation interception. The changes add custom scheme registration, request handling through LifeSpanHandler, URL parsing and validation, and public APIs for Manager and View to control navigation behavior via callbacks. ChangesScheme Handler Factory Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/gui/cef/app.h (1)
36-50:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
Apprender-process handler contract in CEF (OnContextCreated+GetRenderProcessHandler)
code/framework/src/gui/cef/app.cppdefinesvoid App::OnContextCreated(CefRefPtr<CefBrowser>, CefRefPtr<CefFrame>, CefRefPtr<CefV8Context>), butcode/framework/src/gui/cef/app.h’sclass Appdoes not declareOnContextCreated(...)(compile break unless declared elsewhere).code/framework/src/gui/cef/app.halso lacks aGetRenderProcessHandler()override; ensure the render-process handler is actually returned/registered fromApp.Suggested fix
class App final : public CefApp , public CefBrowserProcessHandler , public CefRenderProcessHandler , public CefSchemeHandlerFactory { @@ CefRefPtr<CefBrowserProcessHandler> GetBrowserProcessHandler() override { return this; } + CefRefPtr<CefRenderProcessHandler> GetRenderProcessHandler() override { + return this; + } @@ void OnBeforeCommandLineProcessing(const CefString &processType, CefRefPtr<CefCommandLine> commandLine) override; void OnContextInitialized() override; + void OnContextCreated(CefRefPtr<CefBrowser> browser, CefRefPtr<CefFrame> frame, CefRefPtr<CefV8Context> context) override;🤖 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 `@code/framework/src/gui/cef/app.h` around lines 36 - 50, App is missing the render-process handler contract: declare the OnContextCreated method signature in class App to match the definition in app.cpp (e.g., void OnContextCreated(CefRefPtr<CefBrowser> browser, CefRefPtr<CefFrame> frame, CefRefPtr<CefV8Context> context) override) and add the GetRenderProcessHandler() override returning this (CefRefPtr<CefRenderProcessHandler> GetRenderProcessHandler() override { return this; }) so the CefRenderProcessHandler implementation is properly exposed; update the class App declaration to include both declarations so the definitions in app.cpp compile and the render handler is registered.
🤖 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 `@code/framework/src/gui/cef/app.cpp`:
- Around line 25-27: The code reads from and mutates the unordered_map _handlers
concurrently (in Create() and RegisterSchemeHandlerFactory()), causing a data
race; protect _handlers with a mutex (prefer std::shared_mutex) and acquire a
shared/read lock when accessing in Create() (and the similar read at lines
~55-57) and an exclusive/write lock when modifying it in
RegisterSchemeHandlerFactory() so all reads/writes are synchronized and
undefined behavior is avoided.
In `@code/framework/src/gui/manager.cpp`:
- Around line 254-257: Manager::RegisterSchemeHandlerFactory currently
dereferences _cefApp without checking _cefInitialized and ignores the bool
return from CefRegisterSchemeHandlerFactory; update the method to first check
_cefInitialized and that _cefApp != nullptr (and return or report an error if
not initialized), use the provided
Framework::GUI::CEF::SchemaHandlerFactoryCallback to call
_cefApp->RegisterSchemeHandlerFactory only when safe, call
CefRegisterSchemeHandlerFactory and capture its bool result, and handle a false
return by logging or returning an error indicating registration failed or was
duplicate (include the schema/domain in the message).
In `@code/framework/src/gui/view.h`:
- Around line 189-191: SetOnBeforeBrowseCallback currently dereferences
_lifeSpanHandler without a null check; either ensure callers cannot call this
before initialization or add a defensive guard: in SetOnBeforeBrowseCallback
(the inline method) check _lifeSpanHandler for null and handle it (e.g.,
assert/throw/log and return) or lazily create/initialize the handler before
calling _lifeSpanHandler->SetOnBeforeBrowseCallback(std::move(cb)); update the
method to reference _lifeSpanHandler and the class initialization path for
_lifeSpanHandler to make intent clear.
---
Outside diff comments:
In `@code/framework/src/gui/cef/app.h`:
- Around line 36-50: App is missing the render-process handler contract: declare
the OnContextCreated method signature in class App to match the definition in
app.cpp (e.g., void OnContextCreated(CefRefPtr<CefBrowser> browser,
CefRefPtr<CefFrame> frame, CefRefPtr<CefV8Context> context) override) and add
the GetRenderProcessHandler() override returning this
(CefRefPtr<CefRenderProcessHandler> GetRenderProcessHandler() override { return
this; }) so the CefRenderProcessHandler implementation is properly exposed;
update the class App declaration to include both declarations so the definitions
in app.cpp compile and the render handler is registered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e5e5f2f-6ba9-4203-9328-cd68fe7b36eb
📒 Files selected for processing (8)
code/framework/src/gui/cef/app.cppcode/framework/src/gui/cef/app.hcode/framework/src/gui/cef/client.hcode/framework/src/gui/cef/life_span_handler.cppcode/framework/src/gui/cef/life_span_handler.hcode/framework/src/gui/manager.cppcode/framework/src/gui/manager.hcode/framework/src/gui/view.h
Comment on lines
+25
to
+27
| auto it = _handlers.find({scheme, domain}); | ||
| if (it == _handlers.end()) | ||
| return nullptr; |
There was a problem hiding this comment.
Protect _handlers with synchronization (read/write race).
Create() reads _handlers while RegisterSchemeHandlerFactory() mutates it with no lock. If registration can happen after CEF starts handling requests, this is a data race on std::unordered_map (undefined behavior).
Suggested fix
// app.h
+#include <shared_mutex>
...
private:
+ mutable std::shared_mutex _handlersMutex;
std::unordered_map<SchemeDomainKey, Framework::GUI::CEF::SchemaHandlerFactoryCallback, KeyHash> _handlers;
// app.cpp
- auto it = _handlers.find({scheme, domain});
+ std::shared_lock lock(_handlersMutex);
+ auto it = _handlers.find({scheme, domain});
if (it == _handlers.end())
return nullptr;
- _handlers[{scheme, domain}] = callback;
+ {
+ std::unique_lock lock(_handlersMutex);
+ _handlers[{scheme, domain}] = std::move(callback);
+ }Also applies to: 55-57
🤖 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 `@code/framework/src/gui/cef/app.cpp` around lines 25 - 27, The code reads from
and mutates the unordered_map _handlers concurrently (in Create() and
RegisterSchemeHandlerFactory()), causing a data race; protect _handlers with a
mutex (prefer std::shared_mutex) and acquire a shared/read lock when accessing
in Create() (and the similar read at lines ~55-57) and an exclusive/write lock
when modifying it in RegisterSchemeHandlerFactory() so all reads/writes are
synchronized and undefined behavior is avoided.
Comment on lines
+254
to
+257
| void Manager::RegisterSchemeHandlerFactory(const std::string &schema, const std::string &domain, Framework::GUI::CEF::SchemaHandlerFactoryCallback callback) { | ||
| _cefApp->RegisterSchemeHandlerFactory(schema, domain, callback); | ||
| CefRegisterSchemeHandlerFactory(schema, domain, _cefApp); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant region around the flagged lines
FILE="code/framework/src/gui/manager.cpp"
# Show method and nearby members/flags
echo "=== Context around RegisterSchemeHandlerFactory ==="
nl -ba "$FILE" | sed -n '220,320p'
echo
echo "=== Search for CEF init guards / flags in this file ==="
rg -n "_cefInitialized|_cefApp|Init\(|RegisterSchemeHandlerFactory" "$FILE"
echo
echo "=== Find definitions/usage of _cefApp and CEF init state ==="
rg -n "CefRegisterSchemeHandlerFactory" "$FILE"
rg -n "RegisterSchemeHandlerFactory\(" "$FILE"
echo
echo "=== Find other call sites for RegisterSchemeHandlerFactory in repo ==="
rg -n "RegisterSchemeHandlerFactory\s*\(" -S code | head -n 50
echo
echo "=== Check how _cefInitialized is set / what _cefInitialized means ==="
rg -n "_cefInitialized\s*=" -S code/framework/src/gui/manager.cppRepository: MafiaHub/Framework
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="code/framework/src/gui/manager.cpp"
echo "=== Verify file exists ==="
ls -la "$FILE" || true
echo
echo "=== Context around RegisterSchemeHandlerFactory (lines 240-290) ==="
awk 'NR>=240 && NR<=290 {printf "%d:%s\n", NR, $0}' "$FILE" || true
echo
echo "=== Search for init guard / members in this file ==="
rg -n "_cefInitialized|_cefApp|Init\(|RegisterSchemeHandlerFactory" "$FILE" || true
echo
echo "=== Find usages of CEF registration return value ==="
rg -n "CefRegisterSchemeHandlerFactory" "$FILE" || true
echo
echo "=== Search for other call sites in repo ==="
rg -n "RegisterSchemeHandlerFactory\s*\(" -S code | head -n 50 || true
echo
echo "=== Where is _cefInitialized defined/set in this file ==="
rg -n "_cefInitialized" "$FILE" || trueRepository: MafiaHub/Framework
Length of output: 3392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Manager ctor/dtor and Init logic
FILE="code/framework/src/gui/manager.cpp"
echo "=== Manager.cpp: lines 1-120 ==="
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "=== Manager.cpp: lines 120-220 ==="
awk 'NR>=120 && NR<=220 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "=== Inspect Manager header for members (_cefApp, _cefInitialized) ==="
HFILE="code/framework/src/gui/manager.h"
nl -ba "$HFILE" | sed -n '1,140p'
echo
echo "=== Inspect CEF app wrapper header ==="
AHD="code/framework/src/gui/cef/app.h"
nl -ba "$AHD" | sed -n '1,140p'
echo
echo "=== Inspect CEF app wrapper impl around RegisterSchemeHandlerFactory ==="
ACI="code/framework/src/gui/cef/app.cpp"
awk 'NR>=1 && NR<=120 {printf "%d:%s\n", NR, $0}' "$ACI"
echo
echo "=== Inspect CEF app wrapper impl further (around line 55 from rg output) ==="
awk 'NR>=40 && NR<=110 {printf "%d:%s\n", NR, $0}' "$ACI"Repository: MafiaHub/Framework
Length of output: 7782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MFILE="code/framework/src/gui/manager.cpp"
HFILE="code/framework/src/gui/manager.h"
AHD="code/framework/src/gui/cef/app.h"
ACI="code/framework/src/gui/cef/app.cpp"
echo "=== Search for IsInitialized() ==="
rg -n "IsInitialized\s*\(" "$MFILE" "$HFILE"
echo
echo "=== Show Manager header member declarations (look for _cefApp/_cefInitialized) ==="
awk 'NR>=1 && NR<=140 {print NR ":" $0}' "$HFILE" | rg -n "_cefApp|_cefInitialized|_initialized|IsInitialized" || true
echo
echo "=== Show Manager header around those member declarations (wider context) ==="
rg -n "_cefApp|_cefInitialized|_initialized" "$HFILE" || true
LINE=$(rg -n "_cefApp|_cefInitialized|_initialized" "$HFILE" | head -n1 | cut -d: -f1)
START=$((LINE-20)); END=$((LINE+60))
awk -v s=$START -v e=$END 'NR>=s && NR<=e {print NR ":" $0}' "$HFILE" || true
echo
echo "=== Inspect CEF app wrapper declarations ==="
awk 'NR>=1 && NR<=140 {print NR ":" $0}' "$AHD" | rg -n "RegisterSchemeHandlerFactory|class App|_cefApp|CefRegisterSchemeHandlerFactory" || true
echo
echo "=== Inspect CEF app wrapper implementation around RegisterSchemeHandlerFactory ==="
rg -n "RegisterSchemeHandlerFactory" "$ACI"
MATCH_LINE=$(rg -n "RegisterSchemeHandlerFactory" "$ACI" | head -n1 | cut -d: -f1)
START=$((MATCH_LINE-20)); END=$((MATCH_LINE+60))
awk -v s=$START -v e=$END 'NR>=s && NR<=e {print NR ":" $0}' "$ACI"Repository: MafiaHub/Framework
Length of output: 4601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for declaration/signature of CefRegisterSchemeHandlerFactory in repo headers (if vendored)
rg -n "CefRegisterSchemeHandlerFactory\s*\(" code/framework/src/gui -S || true
rg -n "CefRegisterSchemeHandlerFactory\s*\(" code -S || true
# Try to find CEF headers under include/ or third_party if present
fd -i "cef*.h" code | head -n 50
# Search likely header files for the signature (heuristic)
for f in $(fd -i "cef*.h" code 2>/dev/null | head -n 200); do
if rg -n "CefRegisterSchemeHandlerFactory" "$f" >/dev/null 2>&1; then
echo "=== Found in $f ==="
rg -n "CefRegisterSchemeHandlerFactory" "$f" | head -n 20
fi
doneRepository: MafiaHub/Framework
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call sites for Manager::RegisterSchemeHandlerFactory
rg -n "RegisterSchemeHandlerFactory\s*\(" code -S
# Narrow to likely member call syntax
rg -n "\.RegisterSchemeHandlerFactory\s*\(" code -S || true
rg -n "Manager::RegisterSchemeHandlerFactory\s*\(" code -S || true
# Also search for IsInitialized checks near potential calls
rg -n "IsInitialized\s*\(|_cefInitialized\s*|GUI_CEF_INIT_FAILED" code -S | head -n 200Repository: MafiaHub/Framework
Length of output: 4640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
HFILE="code/framework/src/gui/cef/app.h"
echo "=== CEF App class declaration (search for CefSchemeHandlerFactory / inheritance) ==="
rg -n "class App|CefSchemeHandlerFactory|SchemeHandlerFactory|RegisterSchemeHandlerFactory" "$HFILE"
echo
echo "=== Show app.h around class App definition ==="
LINE=$(rg -n "class App" "$HFILE" | head -n1 | cut -d: -f1)
START=$((LINE-40)); END=$((LINE+120))
awk -v s=$START -v e=$END 'NR>=s && NR<=e {print NR ":" $0}' "$HFILE"Repository: MafiaHub/Framework
Length of output: 2858
🌐 Web query:
CefRegisterSchemeHandlerFactory return type bool CefSchemeHandlerFactory registration
💡 Result:
The CefRegisterSchemeHandlerFactory function in the Chromium Embedded Framework (CEF) has a return type of bool [1][2]. It returns true if the scheme handler factory is successfully registered, and false if an error occurs [3][1]. This function is used to register a scheme handler factory with the global request context [1][4]. Key implementation details include: - Function Signature: In C++, it is defined as: bool CefRegisterSchemeHandlerFactory(const CefString& scheme_name, const CefString& domain_name, CefRefPtr factory) [1][2]. - Behavior: - An empty domain_name value for a standard scheme causes the factory to match all domain names [1][2]. - For non-standard schemes, the domain_name parameter is ignored [1][2]. - If the scheme_name is a custom scheme, you must also implement the CefApp::OnRegisterCustomSchemes method in all processes [1][2]. - The function may be called multiple times to change or remove a previously registered factory [1][2]. - It can be called on any thread in the browser process [1][2]. In CefSharp, the equivalent method RequestContext.RegisterSchemeHandlerFactory also returns a boolean value, indicating success or failure [5][6].
Citations:
- 1: https://cef-builds.spotifycdn.com/docs/145.0/cef__scheme_8h.html
- 2: https://github.com/chromiumembedded/cef/blob/master/include/cef_scheme.h
- 3: https://docs.rs/cef-dll-sys/latest/cef_dll_sys/fn.cef_register_scheme_handler_factory.html
- 4: https://cef-builds.spotifycdn.com/docs/105.3/cef__scheme_8h.html
- 5: https://cefsharp.github.io/api/97.1.x/html/M_CefSharp_RequestContext_RegisterSchemeHandlerFactory.htm
- 6: https://cefsharp.github.io/api/113.1.x/html/M_CefSharp_RequestContext_RegisterSchemeHandlerFactory.htm
Guard CEF scheme handler registration and handle failures explicitly.
Manager::RegisterSchemeHandlerFactorydereferences_cefAppwithout checking_cefInitialized;_cefAppis only created inInit(), so calling this before successful CEF init can dereference null.CefRegisterSchemeHandlerFactory(...)returnsbooland its result is ignored, hiding registration failure/duplicate registration issues.
Suggested fix
void Manager::RegisterSchemeHandlerFactory(const std::string &schema, const std::string &domain, Framework::GUI::CEF::SchemaHandlerFactoryCallback callback) {
+ if (!_cefInitialized || !_cefApp) {
+ Framework::Logging::GetLogger("Web")->error("Cannot register scheme handler factory before CEF initialization");
+ return;
+ }
_cefApp->RegisterSchemeHandlerFactory(schema, domain, callback);
- CefRegisterSchemeHandlerFactory(schema, domain, _cefApp);
+ if (!CefRegisterSchemeHandlerFactory(schema, domain, _cefApp)) {
+ Framework::Logging::GetLogger("Web")->error("CefRegisterSchemeHandlerFactory failed for schema='{}' domain='{}'", schema, domain);
+ }
}🤖 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 `@code/framework/src/gui/manager.cpp` around lines 254 - 257,
Manager::RegisterSchemeHandlerFactory currently dereferences _cefApp without
checking _cefInitialized and ignores the bool return from
CefRegisterSchemeHandlerFactory; update the method to first check
_cefInitialized and that _cefApp != nullptr (and return or report an error if
not initialized), use the provided
Framework::GUI::CEF::SchemaHandlerFactoryCallback to call
_cefApp->RegisterSchemeHandlerFactory only when safe, call
CefRegisterSchemeHandlerFactory and capture its bool result, and handle a false
return by logging or returning an error indicating registration failed or was
duplicate (include the schema/domain in the message).
Comment on lines
+189
to
+191
| inline void SetOnBeforeBrowseCallback(CEF::OnBeforeBrowseCallback cb) { | ||
| _lifeSpanHandler->SetOnBeforeBrowseCallback(std::move(cb)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where _lifeSpanHandler is initialized relative to SetOnBeforeBrowseCallback call sites.
rg -n "View::View\\(|_lifeSpanHandler\\s*=|SetOnBeforeBrowseCallback\\(" code/framework/src/gui -g '*.{h,hpp,hxx,cpp,cc,cxx}'Repository: MafiaHub/Framework
Length of output: 604
Guard _lifeSpanHandler (or document Init-before-use) in SetOnBeforeBrowseCallback.
SetOnBeforeBrowseCallback dereferences _lifeSpanHandler directly (_lifeSpanHandler->SetOnBeforeBrowseCallback(...)) without a null check; ensure _lifeSpanHandler is always initialized before this setter can be called (or add a defensive guard).
🤖 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 `@code/framework/src/gui/view.h` around lines 189 - 191,
SetOnBeforeBrowseCallback currently dereferences _lifeSpanHandler without a null
check; either ensure callers cannot call this before initialization or add a
defensive guard: in SetOnBeforeBrowseCallback (the inline method) check
_lifeSpanHandler for null and handle it (e.g., assert/throw/log and return) or
lazily create/initialize the handler before calling
_lifeSpanHandler->SetOnBeforeBrowseCallback(std::move(cb)); update the method to
reference _lifeSpanHandler and the class initialization path for
_lifeSpanHandler to make intent clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
callEventfunction enabling bidirectional communication between native code and scripts.