Skip to content

Core - Automatically merge enable-features/disable-features command-line arguments with existing values#5245

Merged
amaitland merged 5 commits into
cefsharp:masterfrom
SLT-World:master
May 18, 2026
Merged

Core - Automatically merge enable-features/disable-features command-line arguments with existing values#5245
amaitland merged 5 commits into
cefsharp:masterfrom
SLT-World:master

Conversation

@SLT-World
Copy link
Copy Markdown
Contributor

@SLT-World SLT-World commented May 9, 2026

Core - Automatically merge enable-features/disable-features command-line arguments with existing values.

Fixes: #5244

Summary:

  • I have added the ability to automatically merge enable-features/disable-features command-line arguments with existing values.
  • If CefSharpSettings.MergeFeaturesCommandLineArgs is false, any user-supplied list will remove and replace existing list.
  • The values were somehow being duplicated twice so a Contains check was added to mitigate that issue.

Changes: [specify the structures changed]

  • I have modified CefSharpApp.h and CefSharpSettings.cs.
    • Added MergeFeaturesCommandLineArgs to CefSharpSettings, defaults to true.
    • Modified logic to handle the merging of enable-features and disable-features lists in CefSharpApp.h.

How Has This Been Tested?
Operating System: Windows 11
Environment: Visual Studio 2022
Changes visibly function after appending and modifying

settings.CefCommandLineArgs.Add("disable-features", "GlobalMediaControls");
CefSharpSettings.MergeFeaturesCommandLineArgs = true;

within App.xaml.cs in CefSharp.Wpf.HwndHost.Example.

Screenshots (if appropriate):
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

Summary by CodeRabbit

  • New Features
    • Added a MergeFeaturesCommandLineArgs setting (enabled by default) to control how enable-features/disable-features args are applied: when enabled incoming feature lists are merged with existing values instead of overwriting them.
    • Merging now deduplicates entries and preserves first-seen ordering, providing more predictable combined feature-flag behavior.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 9, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a boolean setting to control merging of enable-features/disable-features CLI args, updates command-line processing to either merge or replace those switches based on the setting, and adds a utility to merge comma-separated feature lists while preserving order and deduplicating.

Changes

Feature Command-Line Argument Merge Control

Layer / File(s) Summary
Configuration Property
CefSharp/CefSharpSettings.cs
Introduces MergeFeaturesCommandLineArgs public static property that controls merge behavior for feature command-line arguments.
Default Initialization
CefSharp/CefSharpSettings.cs
Static constructor sets MergeFeaturesCommandLineArgs to true by default.
OnBeforeCommandLineProcessing (merge enabled)
CefSharp.Core.Runtime/Internals/CefSharpApp.h
When merging is enabled, reads existing enable-features/disable-features; if empty uses incoming value, otherwise calls CommandLineArgsMerger::MergeFeatures and replaces the switch with the merged value.
OnBeforeCommandLineProcessing (merge disabled)
CefSharp.Core.Runtime/Internals/CefSharpApp.h
When merging is disabled, removes any existing enable-features/disable-features switch and sets the switch to the incoming value.
CommandLineArgsMerger
CefSharp/Internals/CommandLineArgsMerger.cs
Adds MergeFeatures(existing, incoming) that splits comma-separated lists, trims tokens, preserves first-seen order, deduplicates across inputs, and returns a comma-joined merged string.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant CefSharpApp
  participant CefSharpSettings
  participant CommandLine
  participant CommandLineArgsMerger
  Client->>CefSharpApp: supply enable-features / disable-features arg
  CefSharpApp->>CefSharpSettings: read MergeFeaturesCommandLineArgs
  alt Merge enabled
    CefSharpApp->>CommandLine: read existing switch value
    CommandLine-->>CefSharpApp: existingValue
    CefSharpApp->>CommandLineArgsMerger: MergeFeatures(existingValue, incomingValue)
    CommandLineArgsMerger-->>CefSharpApp: mergedValue
    CefSharpApp->>CommandLine: set updated switch = mergedValue
  else Merge disabled
    CefSharpApp->>CommandLine: remove existing switch
    CefSharpApp->>CommandLine: set switch = incomingValue
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop through flags both new and old,
Trimming commas, keeping hold.
Merge when asked, or replace with care,
No dupes remain — order’s fair.
Chromium wakes with features whole.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: automatically merging enable-features/disable-features command-line arguments with existing values.
Description check ✅ Passed The PR description covers all major template sections: it references the issue, provides summary, lists changes, describes testing on Windows 11, includes a screenshot, marks the change type, and completes relevant checklist items.
Linked Issues check ✅ Passed The PR addresses issue #5244 by implementing automatic merging of enable-features/disable-features arguments when MergeFeaturesCommandLineArgs is true, preventing crashes with Chrome runtime style.
Out of Scope Changes check ✅ Passed All changes are directly related to issue #5244: new MergeFeaturesCommandLineArgs setting, CommandLineArgsMerger utility, and updated CefSharpApp.h merging logic. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/CefSharpApp.h`:
- Around line 212-228: The substring check using currentValue->Contains causes
incorrect duplicate detection; in the block gated by
CefSharpSettings::MergeFeaturesCommandLineArgs (around
commandLine->GetSwitchValue/name handling), replace the Contains-based logic
with proper comma-separated token parsing: split the existingValue (via
StringUtils::ToClr(existingValue)) and the incoming kvp->Value by commas, trim
whitespace, build a case-sensitive set/list of existing tokens, iterate the
incoming tokens and add only those not already present, then reconstruct the
merged comma-separated string and call commandLine->RemoveSwitch(name);
commandLine->AppendSwitchWithValue(name, StringUtils::ToNative(mergedValue));
this ensures exact-feature matching and correctly handles multi-feature inputs
like "Feature1,Feature2".
🪄 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: cdc755c2-d284-4ddc-b997-e038c8368ec6

📥 Commits

Reviewing files that changed from the base of the PR and between fd87988 and eea20a1.

📒 Files selected for processing (2)
  • CefSharp.Core.Runtime/Internals/CefSharpApp.h
  • CefSharp/CefSharpSettings.cs

Comment thread CefSharp.Core.Runtime/Internals/CefSharpApp.h
@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@amaitland
Copy link
Copy Markdown
Member

Thanks for the PR. We should be able to create a c# method that handles the actual merging logic, then we can write some unit tests. Not sure why it would be called twice, will need to run with debugger attached to see if there's anything obvious, might be an unexpected CEF/Chromium behaviour.

@SLT-World
Copy link
Copy Markdown
Contributor Author

SLT-World commented May 16, 2026

Sorry, I missed out on a duplicated commandLine->GetSwitchValue(name) earlier so I had to do an commit amend.

I've relocated the merging logic to a C# method CommandLineArgsMerger.MergeFeatures at CefSharp.Core.Runtime\Internals\ and made the appropriate changes to CefSharpApp.h.
But I'm not entirely sure where I would add a unit test, there's quite a number of files in CefSharp.Test.

Would these changes be what you meant?

@AppVeyorBot
Copy link
Copy Markdown

@amaitland
Copy link
Copy Markdown
Member

Sorry, I missed out on a duplicated commandLine->GetSwitchValue(name) earlier so I had to do an commit amend.

No prob 👍

But I'm not entirely sure where I would add a unit test, there's quite a number of files in CefSharp.Test.

Probably in https://github.com/cefsharp/CefSharp/tree/master/CefSharp.Test/Framework folder/namespace

Would these changes be what you meant?

Exactly 😄

@SLT-World
Copy link
Copy Markdown
Contributor Author

Probably in https://github.com/cefsharp/CefSharp/tree/master/CefSharp.Test/Framework folder/namespace

Alright, I've added CommandLineArgsMergerTests in CefSharp.Test/Framework, with 4 tests (merging, de-duplication, whitespace trimming and null handling).

Would this suffice?

@AppVeyorBot
Copy link
Copy Markdown

Comment thread CefSharp/Internals/CommandLineArgsMerger.cs Outdated
Comment thread CefSharp/Internals/CommandLineArgsMerger.cs Outdated
@SLT-World
Copy link
Copy Markdown
Contributor Author

Done, switched to using a HashSet and extracted the duplicated code into a private method.
I also added an OrderBy to ensure ordering for the output since HashSet is unordered collection.

@AppVeyorBot
Copy link
Copy Markdown

@amaitland
Copy link
Copy Markdown
Member

Done, switched to using a HashSet and extracted the duplicated code into a private method.

Excellent, thank you!

I also added an OrderBy to ensure ordering for the output since HashSet is unordered collection.

I'd be surprised if it mattered, guess alphabetical couldn't hurt.

@amaitland amaitland merged commit b5fef3b into cefsharp:master May 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants