Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR upgrades several projects to C++20, adds NOMINMAX macros, changes a GetJsRootObjectWrapper parameter from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
CefSharp.Core.Runtime/DragData.h (1)
29-37: Optional: considerconst CefRefPtr<CefDragData>&constructor parameter to eliminate the intermediate variable workaround.The constructor only reads from
dragData— it calls const member functions and copies the pointer into_wrappedDragData. Changing the parameter type would let all three call sites (Clone,Create,Image) pass the returned prvalue directly:♻️ Proposed refactor
- DragData(CefRefPtr<CefDragData> &dragData) : + DragData(const CefRefPtr<CefDragData> &dragData) :This is purely optional — the existing local-variable approach is correct and consistent with the broader pattern across the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/DragData.h` around lines 29 - 37, The DragData constructor currently takes a non-const CefRefPtr<CefDragData>& causing call sites (Clone, Create, Image) to create an intermediate local to bind; change the constructor signature to accept const CefRefPtr<CefDragData>& dragData so prvalues can be passed directly, keep assignment to _wrappedDragData and the existing reads (IsReadOnly, GetFileName, IsFile, IsFragment, IsLink) unchanged, and update any call sites if necessary to remove the temporary-local workaround.CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp (1)
114-114: Nit: redundant copy-initialization — prefer value-initialization shorthand.
= std::vector<CefString>()is equivalent to the default constructor and can be dropped:♻️ Proposed simplification
- std::vector<CefString> dictionarySuggestions = std::vector<CefString>(); + std::vector<CefString> dictionarySuggestions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp` at line 114, The variable dictionarySuggestions is being copy-initialized with an empty vector ("std::vector<CefString> dictionarySuggestions = std::vector<CefString>();") which is redundant; replace that with value-initialization such as "std::vector<CefString> dictionarySuggestions;" (or "std::vector<CefString> dictionarySuggestions{};") in CefContextMenuParamsWrapper.cpp to use the default constructor and remove the unnecessary copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxproj`:
- Around line 114-115: The project now sets
<LanguageStandard>stdcpp20</LanguageStandard> with
<CompileAs>CompileAsCpp</CompileAs>, but /clr + C++20 requires a minimum Visual
Studio toolset and the optional C++/CLI component; update the project and repo
to enforce and document that requirement: add/msbuild-conditional checks or a
build-fail message in CefSharp.BrowserSubprocess.Core.vcxproj that validates the
MSVC toolset/version (v143 toolset with VS 2022 >= 17.10 recommended) and that
the "C++/CLI support for v143 build tools" component is available, update CI
images to a supported VS version (pin to v17.10+), and add a clear note to
CONTRIBUTING/README documenting the minimum Visual Studio version and required
C++/CLI component so local builds fail early and contributors know to install
the component.
---
Duplicate comments:
In
`@CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj`:
- Around line 157-158: Update all project configurations to consistently use
C++20 by setting the LanguageStandard element to "stdcpp20" and ensure CompileAs
is set to "CompileAsCpp" wherever C++ compilation is intended; apply this same
change for the new arm64 targets and the other configuration blocks that contain
the LanguageStandard and CompileAs elements so all 6 configurations match (look
for the LanguageStandard and CompileAs XML elements in the
CefSharp.BrowserSubprocess.Core.netcore.vcxproj and mirror the shown entries).
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj`:
- Around line 181-182: The LanguageStandard and CompileAs settings in
CefSharp.Core.Runtime.netcore.vcxproj should be set to stdcpp20 and CompileAsCpp
consistently across all configurations—ensure the same change is applied for
every occurrence referenced (also mirror the edits where LanguageStandard and
CompileAs appear) and confirm the identical update was made in
CefSharp.BrowserSubprocess.Core.vcxproj while noting the Visual Studio version
caveat mentioned in the review; verify the project elements <LanguageStandard>
and <CompileAs> are uniformly updated across all six configurations.
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.vcxproj`:
- Around line 133-134: The project file has been updated to use C++20 by setting
the LanguageStandard to stdcpp20 and CompileAs to CompileAsCpp; ensure these
tags (LanguageStandard and CompileAs) are consistently updated across all
configurations by applying the same changes to the other occurrences referenced
(the blocks corresponding to the other configurations where the same tags
appear) so every configuration uses stdcpp20 and CompileAsCpp.
---
Nitpick comments:
In `@CefSharp.Core.Runtime/DragData.h`:
- Around line 29-37: The DragData constructor currently takes a non-const
CefRefPtr<CefDragData>& causing call sites (Clone, Create, Image) to create an
intermediate local to bind; change the constructor signature to accept const
CefRefPtr<CefDragData>& dragData so prvalues can be passed directly, keep
assignment to _wrappedDragData and the existing reads (IsReadOnly, GetFileName,
IsFile, IsFragment, IsLink) unchanged, and update any call sites if necessary to
remove the temporary-local workaround.
In `@CefSharp.Core.Runtime/Internals/CefContextMenuParamsWrapper.cpp`:
- Line 114: The variable dictionarySuggestions is being copy-initialized with an
empty vector ("std::vector<CefString> dictionarySuggestions =
std::vector<CefString>();") which is redundant; replace that with
value-initialization such as "std::vector<CefString> dictionarySuggestions;" (or
"std::vector<CefString> dictionarySuggestions{};") in
CefContextMenuParamsWrapper.cpp to use the default constructor and remove the
unnecessary copy.
|
✅ Build CefSharp 145.0.240-CI5418 completed (commit 82b3e9702d by @amaitland) |
amaitland
force-pushed
the
upgrade/cpp20
branch
from
8aae7e7 to
1a0b925
Compare
|
❌ Build CefSharp 145.0.240-CI5420 failed (commit 4062d3805c by @amaitland) |
- Set NOMINMAX - Change compiler options - Fix compiler errors
amaitland
force-pushed
the
upgrade/cpp20
branch
from
1a0b925 to
84c1873
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CefSharp.Core.Runtime/RequestContext.h (1)
45-49: Const-correctness change is correct; minor init-style inconsistency with sibling wrappers.All peer wrappers updated in this PR (e.g.,
CefBrowserWrapper,Browser,Frame,CefBrowserHostWrapper) use a member initializer list (: _member(param)), butRequestContextassigns in the body via_requestContext = context.get(). Both are equivalent, but for consistency you may consider aligning to the member-init pattern used elsewhere:♻️ Optional alignment to member init-list style
- RequestContext(const CefRefPtr<CefRequestContext>& context) - { - _requestContext = context.get(); - _settings = nullptr; - } + RequestContext(const CefRefPtr<CefRequestContext>& context) + : _requestContext(context), _settings(nullptr) + { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/RequestContext.h` around lines 45 - 49, The RequestContext constructor currently assigns members in the body; change it to use a member initializer list for consistency with other wrappers by initializing _requestContext with context.get() and _settings with nullptr in the constructor initializer list (i.e., update the RequestContext(const CefRefPtr<CefRequestContext>& context) constructor to initialize _requestContext and _settings via the member initializer list).CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h (1)
23-26: LGTM.The
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h(a separate class shown in the relevant snippets, not part of this PR) still takesCefRefPtr<CefBrowser>by value. Passing by value is already const-compatible so it's not broken, but if you want to unify the style across all wrapper constructors, that file is the one remaining outlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h` around lines 23 - 26, Constructor signature in CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h currently takes CefRefPtr<CefBrowser> by value; change it to take a const CefRefPtr<CefBrowser>& to match the style used elsewhere. Update the CefBrowserWrapper(const CefRefPtr<CefBrowser> &browser) declaration/definition to use a const reference, ensure the initializer list still assigns _browser(browser) and that no call sites rely on copy semantics, and rebuild to verify no warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj`:
- Around line 181-182: Update the project documentation to explicitly require
Visual Studio 2022 version 17.6 or newer: mention in README and CONTRIBUTING
that due to use of C++20 in libcef_dll_wrapper and related headers (see
CefSharp.Core.Runtime.netcore.vcxproj using
<LanguageStandard>stdcpp20</LanguageStandard> together with
<CLRSupport>NetCore</CLRSupport>), VS 2022 17.6+ is the minimum toolset because
earlier VS2022 releases silently downgrade /std:c++20 to /std:c++17 under /clr;
also add a short note about the two C++/CLI limitations (managed-template
two-phase lookup and module imports under /clr) so integrators are aware.
---
Nitpick comments:
In `@CefSharp.Core.Runtime/Internals/CefBrowserWrapper.h`:
- Around line 23-26: Constructor signature in
CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h currently takes
CefRefPtr<CefBrowser> by value; change it to take a const CefRefPtr<CefBrowser>&
to match the style used elsewhere. Update the CefBrowserWrapper(const
CefRefPtr<CefBrowser> &browser) declaration/definition to use a const reference,
ensure the initializer list still assigns _browser(browser) and that no call
sites rely on copy semantics, and rebuild to verify no warnings.
In `@CefSharp.Core.Runtime/RequestContext.h`:
- Around line 45-49: The RequestContext constructor currently assigns members in
the body; change it to use a member initializer list for consistency with other
wrappers by initializing _requestContext with context.get() and _settings with
nullptr in the constructor initializer list (i.e., update the
RequestContext(const CefRefPtr<CefRequestContext>& context) constructor to
initialize _requestContext and _settings via the member initializer list).
| <LanguageStandard>stdcpp20</LanguageStandard> | ||
| <CompileAs>CompileAsCpp</CompileAs> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure and find the file in question
git ls-files | head -20Repository: cefsharp/CefSharp
Length of output: 677
🏁 Script executed:
# Locate the specific .vcxproj file
find . -name "*CefSharp.Core.Runtime.netcore.vcxproj" -o -name "README*" -o -name "CONTRIBUTING*" | head -20Repository: cefsharp/CefSharp
Length of output: 168
🏁 Script executed:
# Look for build documentation and Visual Studio version requirements
fd -e md -e txt -i 'readme|contributing|build|install' --max-depth 3 2>/dev/null | head -20Repository: cefsharp/CefSharp
Length of output: 183
🏁 Script executed:
# Check the actual .vcxproj file to verify the changes
if [ -f "CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj" ]; then
echo "=== Lines 175-185 ===" && sed -n '175,185p' CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj
echo "=== Lines 210-220 ===" && sed -n '210,220p' CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj
echo "=== Total config segments ===" && grep -c "CompileAs>CompileAsCpp" CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj
fiRepository: cefsharp/CefSharp
Length of output: 1259
🏁 Script executed:
# Search for any existing documentation mentioning Visual Studio versions
rg -i "visual studio|vs\s*2022|version\s*17\.|minimum.*version" --max-count 10Repository: cefsharp/CefSharp
Length of output: 4592
🏁 Script executed:
# Read the CONTRIBUTING.md file to check for build requirements
cat -n CONTRIBUTING.mdRepository: cefsharp/CefSharp
Length of output: 11835
🏁 Script executed:
# Read the README.md file to check for build/system requirements
head -200 README.md | cat -nRepository: cefsharp/CefSharp
Length of output: 21328
🏁 Script executed:
# Search for any "17.6" or specific version mention in the repo
rg "17\.6|17\.5|17\.4|17\.3|17\.2|17\.1|17\.0" --max-count 20Repository: cefsharp/CefSharp
Length of output: 50373
🏁 Script executed:
# Check if there's a BUILDING or BUILD guide
fd -i 'build.*guide|build.*instructions|getting.*started' -e mdRepository: cefsharp/CefSharp
Length of output: 43
Document VS 2022 17.6+ as the minimum build toolset requirement.
The CEF M145 branch requires C++20 features in libcef_dll_wrapper and related headers. With stdcpp20 + CLRSupport>NetCore, Visual Studio 2022 version 17.6 becomes the hard minimum for these project files—earlier versions silently downgrade /std:c++20 to /std:c++17 under /clr, causing opaque compile errors when CEF M145 headers use C++20 constructs.
Update the README and CONTRIBUTING documentation to explicitly state VS 2022 17.6+ as required. Currently, the docs only mention "VC++ 2022 is required starting with version 138" without the specific minor version.
Two known C++/CLI limitations to be aware of (typically not affecting CEF usage):
- Two-phase name lookup for managed templates is unsupported.
- Module imports under
/clrare unsupported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CefSharp.Core.Runtime/CefSharp.Core.Runtime.netcore.vcxproj` around lines 181
- 182, Update the project documentation to explicitly require Visual Studio 2022
version 17.6 or newer: mention in README and CONTRIBUTING that due to use of
C++20 in libcef_dll_wrapper and related headers (see
CefSharp.Core.Runtime.netcore.vcxproj using
<LanguageStandard>stdcpp20</LanguageStandard> together with
<CLRSupport>NetCore</CLRSupport>), VS 2022 17.6+ is the minimum toolset because
earlier VS2022 releases silently downgrade /std:c++20 to /std:c++17 under /clr;
also add a short note about the two C++/CLI limitations (managed-template
two-phase lookup and module imports under /clr) so integrators are aware.
|
✅ Build CefSharp 145.0.240-CI5424 completed (commit accc0afa54 by @amaitland) |
2 participants
Summary:
CEF now requires C++20
Changes: [specify the structures changed]
How Has This Been Tested?
NA
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor