[dotnet] Implement third-party Permissions module #16414

RenderMichael · 2025-10-11T21:07:34Z

User description

🔗 Related Issues

New feature (non-breaking change which adds functionality and tests!)

New API surface:

var permissions = bidi.AsPermissions();
await permissions.SetPermissionAsync("geolocation", PermissionState.Denied, newPage, userContext.UserContext);

PR Type

Enhancement

Description

Enable external BiDi module creation and extension
Refactor module architecture with InternalModule base class
Expose Broker and JsonOptions for external access
Simplify module creation with public static method

Diagram Walkthrough

flowchart LR
  BiDi["BiDi Class"] -- "exposes" --> Broker["Internal Broker"]
  BiDi -- "exposes" --> JsonOptions["JSON Options"]
  Module["Base Module"] -- "extends" --> InternalModule["InternalModule"]
  InternalModule -- "used by" --> AllModules["All Built-in Modules"]
  Module -- "public creation" --> ExternalModules["External Modules"]

File Walkthrough

Relevant files

Enhancement

13 files

BiDi.cs `Expose internal properties and refactor module creation`	+26/-21
InternalModule.cs `Add new base class for internal modules`	+34/-0
Module.cs `Refactor to support external module creation`	+6/-7
BrowserModule.cs `Change inheritance to InternalModule`	+1/-1
BrowsingContextModule.cs `Change inheritance to InternalModule`	+1/-1
EmulationModule.cs `Change inheritance to InternalModule`	+1/-1
InputModule.cs `Change inheritance to InternalModule`	+1/-1
LogModule.cs `Change inheritance to InternalModule`	+1/-1
NetworkModule.cs `Change inheritance to InternalModule`	+1/-1
ScriptModule.cs `Change inheritance to InternalModule`	+1/-1
SessionModule.cs `Change inheritance to InternalModule`	+1/-1
StorageModule.cs `Change inheritance to InternalModule`	+1/-1
WebExtensionModule.cs `Change inheritance to InternalModule`	+1/-1

qodo-merge-pro · 2025-10-11T21:08:09Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟡

🎫 #15329

🟢	Enable a minimal, public mechanism in .NET to implement extendable third-party BiDi modules without relying on internal members.
	Expose necessary BiDi primitives (e.g., broker/transport and JSON serialization options/context) for external modules to function.
	Provide a straightforward way for consumers to create/use external BiDi modules (e.g., a factory/extension pattern).
	Maintain compatibility with existing built-in modules while refactoring to allow extensibility.
⚪	Include example usage feasibility similar to creating a Permissions module as an external extension.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-merge-pro · 2025-10-11T21:09:33Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate serializer context against options In the `Create` method, validate that the provided `cachedContext` is compatible with the `jsonOptions` by comparing their `Options` properties, and throw an `ArgumentException` if they do not match. dotnet/src/webdriver/BiDi/Module.cs [34-45] public static TModule Create<TModule>(BiDi bidi, JsonSerializerOptions jsonOptions, JsonSerializerContext? cachedContext = null) where TModule : Module, new() { + if (cachedContext is not null && cachedContext.Options != jsonOptions) + { + throw new ArgumentException("The provided JsonSerializerContext is not compatible with the provided JsonSerializerOptions.", nameof(cachedContext)); + } + TModule module = new() { Broker = bidi.Broker, }; module.JsonContext = cachedContext ?? module.Initialize(jsonOptions); return module; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential configuration issue in the public `Create` method and proposes a validation check to prevent runtime errors, which enhances the robustness of the API.	Low
Learned best practice	Validate constructor input URL Add argument validation to ensure `url` is not null/empty and is an absolute URI, throwing `ArgumentException`/`ArgumentNullException` with clear messages. dotnet/src/webdriver/BiDi/BiDi.cs [64-83] private BiDi(string url) { - var uri = new Uri(url); + if (url is null) throw new ArgumentNullException(nameof(url)); + if (string.IsNullOrWhiteSpace(url)) throw new ArgumentException("URL must not be empty.", nameof(url)); + if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) + throw new ArgumentException("URL must be a valid absolute URI.", nameof(url)); JsonOptions = DefaultBiDiOptions(); _jsonContext = new BiDiJsonSerializerContext(JsonOptions); Broker = new Broker(this, uri, JsonOptions); SessionModule = Module.Create<Session.SessionModule>(this, JsonOptions, _jsonContext); BrowsingContext = Module.Create<BrowsingContext.BrowsingContextModule>(this, JsonOptions, _jsonContext); Browser = Module.Create<Browser.BrowserModule>(this, JsonOptions, _jsonContext); Network = Module.Create<Network.NetworkModule>(this, JsonOptions, _jsonContext); InputModule = Module.Create<Input.InputModule>(this, JsonOptions, _jsonContext); Script = Module.Create<Script.ScriptModule>(this, JsonOptions, _jsonContext); Log = Module.Create<Log.LogModule>(this, JsonOptions, _jsonContext); Storage = Module.Create<Storage.StorageModule>(this, JsonOptions, _jsonContext); WebExtension = Module.Create<WebExtension.WebExtensionModule>(this, JsonOptions, _jsonContext); Emulation = Module.Create<Emulation.EmulationModule>(this, JsonOptions, _jsonContext); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate inputs early with precise checks to prevent null/invalid argument issues.	Low
Update

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

dotnet/src/webdriver/BiDi/Module.cs

nvborisenko

No value,. In any case thank you.

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

dotnet/src/webdriver/BiDi/Extensions/Permissions/PermissionDescriptor.cs

dotnet/src/webdriver/BiDi/Module.cs

nvborisenko · 2025-10-21T19:31:02Z

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs

        return bidi;
    }
+
+    public static PermissionsModule AsPermissions(this BiDi bidi)


This is definitely should be in another class. The reason is we are CLS compatible (I don't still understand why, but anyway), meaning some languages cannot deal with extension methods. They will use static method: WebDriverExtensions.AsPermissions(bidi). But it is not extension of WebDriver.

We should introduce good name for static class, and its namespace.

BiDiExtensions.AsPermissions(bidi)?

When Bluetooth module comes to under umbrella, then what? Should we have single static class for all them, or they define own static class in theirs namespace?

I vote for "everything in their namespace". It would be good pattern to demonstrate good practices how to develop module. Separate namespace is not a problem, IDE is still able to suggest extension method.

extension methods do not affect CLS compliance. Users can call WebDriverExtensions.AsPermissions(bidi) even on extension methods.

But I agree that extensions should be in separate files. I will make those changes.

My concern is about naming: WebDriverExtensions.AsPermissions(bidi) - it is not extension for WebDriver, it is for BiDi.

I followed the two suggestions: WebDriverExtensions -> BiDiExtensions, and permissions in its own area

From my point of view, extra namespaces within the same assembly, which do not have strong practical need, often provide only complexity without benefits. I'm for more clear type names, rather then giving short type names, which increases name conflict chance, and putting such types in different namespaces.

For example, Visual Studio 2022 IntelliSense, will not suggest you AsPermissions() extension method for bidi object unless you explicitly add using OpenQA.Selenium.BiDi.Permissions; to a .cs file before that. But I would like to see all the available "As..." methods in IDE without knowing and adding exact namespace before that.

I generally do support project folder structuring, similar as currently Selenium.WebDriver project is structured. It is also fine that we have all functionality under OpenQA.Selenium.BiDi namespace. But I would stop with sub-namespaces at this point. I don't see a strong need having 10+ sub-namespaces, one for each submodule. I am talking not only about extension classes, but all classes inside /dotnet/src/webdriver/BiDi folder.

Despite of namespace topic, the current BiDi folder is good. I'm ok with keeping methods like AsPermissions in separate extension classes inside corresponding folder ("Permissions"). If I had to choose a name for such extension class having method public static PermissionsModule AsPermissions(this BiDi bidi), I would name it PermissionsBiDiExtensions, not PermissionsExtensions, because the class's extension methods extend BiDi type. So just {Module}BiDiExtensions.

If you are not ready to apply single common OpenQA.Selenium.BiDi namespace for all types of /dotnet/src/webdriver/BiDi, it's understandable. But at least, it would be great to have {Module}BiDiExtensions in OpenQA.Selenium.BiDi namespace.

Thanks for your valuable input.

If you are not ready to apply single common OpenQA.Selenium.BiDi namespace for all types of /dotnet/src/webdriver/BiDi, it's understandable.

We will have hundreds files in one folder, with naming collision (despite that one file may contain several classes). I really like how we split it by modules, absolutely the same as specification dictates. It is low-level representation, so I think we are OK with it.

For example, Visual Studio 2022 IntelliSense, will not suggest you AsPermissions() extension method for bidi object unless you explicitly add using OpenQA.Selenium.BiDi.Permissions;

We should double check it. I already provided screenshot how VS suggests AsPermissions() extension method even if I didn't import that namespace.

I would name it PermissionsBiDiExtensions, not PermissionsExtensions, because the class's extension methods extend BiDi type. So just {Module}BiDiExtensions.

Absolutely, agree.

To sum up: now we need to decide in which namespace PermissionsBiDiExtensions should live, in root or in Permissions. I really don't have strong opinion. But if we put PermissionsBiDiExtensions into root namespace, it might be precedent to put all possible extensions for all modules in root.

Visual Studio 2022 IntelliSense, will not suggest you AsPermissions() extension method for bidi object unless you explicitly add using OpenQA.Selenium.BiDi.Permissions;

For me, modern VS and Rider suggest AsPermissions() even without the import statement. Like in the screenshot, the suggestion shows that it is in a new namespace, which signals "extension" to users.

I also agree with naming the extension provider type PermissionsBiDiExtensions. I will push those changes now.

now we need to decide in which namespace PermissionsBiDiExtensions should live, in root or in Permissions.

If we have AsPermissions() in a separate "permissions extensions" type, then it seems strange not to have that in the OpenQA.Selenium.BiDi.Permissions namespace. I don't have a strong opinion on moving AsPermissions() back to BiDiExtensions, though.

No consensus :( Let me decide based on external modules should be emphasized. I propose to finish this thread with:

all DTO types in Permissions namespace

any external module might provide extension method in {Module}BiDiExtensions static class under BiDi.{Module} namespace

…eneral BiDi extensinos

nvborisenko · 2025-10-24T21:09:41Z

I didn't verify whether models follow patterns. Since we are OK with the approach, please give me a time to focus on DTO.

nvborisenko · 2025-10-24T21:21:30Z

dotnet/src/webdriver/BiDi/Permissions/PermissionsModule.cs

+{
+    private PermissionsJsonSerializerContext JsonContext => (PermissionsJsonSerializerContext)base.JsonContext;
+
+    public async Task SetPermissionAsync(string permissionName, PermissionState state, string origin, UserContext? userContext, SetPermissionOptions? options = null)


It doesn't follow patter:

permissions.SetPermissionParameters = { descriptor: permissions.PermissionDescriptor, state: permissions.PermissionState, origin: text, ? embeddedOrigin: text, ? userContext: text, }

where descriptor, state, origin is mandatory.

It should accept PermissionDescriptor.

Syntax sugar? PermissionDescriptor might be implicitly converted from string.

nvborisenko · 2025-10-24T21:33:11Z

dotnet/src/webdriver/BiDi/Permissions/PermissionsModule.cs

+{
+    private PermissionsJsonSerializerContext JsonContext => (PermissionsJsonSerializerContext)base.JsonContext;
+
+    public async Task SetPermissionAsync(string permissionName, PermissionState state, string origin, UserContext? userContext, SetPermissionOptions? options = null)


Please avoid optional parameters. We are library, introducing one more option al parameter is risky, literally is a behavior breaking change.

Use Options instead.

nvborisenko · 2025-10-24T21:37:05Z

dotnet/src/webdriver/BiDi/Permissions/PermissionsModule.cs

+    }
+}
+
+[JsonSerializable(typeof(SetPermissionCommand), TypeInfoPropertyName = "Permissions_SetPermissionCommand")]


Suggested change

[JsonSerializable(typeof(SetPermissionCommand), TypeInfoPropertyName = "Permissions_SetPermissionCommand")]

[JsonSerializable(typeof(SetPermissionCommand))]

It is not necessary, and hope naming conflicts will never be inside module (we are moving to JsonContext per Module)

RenderMichael added 2 commits October 11, 2025 17:04

[dotnet] Enable external BiDi modules

b595dfc

Add InternalModule

eaa3d5c

RenderMichael requested a review from nvborisenko October 11, 2025 21:07

selenium-ci added the C-dotnet .NET Bindings label Oct 11, 2025

qodo-merge-pro bot added the Review effort 3/5 label Oct 11, 2025

Rename InternalModule to CoreModule

71a7489

nvborisenko reviewed Oct 11, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs Outdated Show resolved Hide resolved

nvborisenko reviewed Oct 11, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Module.cs Show resolved Hide resolved

nvborisenko requested changes Oct 11, 2025

View reviewed changes

[dotnet] Implement Permissions module

1fbf4c6

RenderMichael changed the title ~~[dotnet] Enable external bidi modules~~ [dotnet] Implement third-party Permissions module Oct 11, 2025

Rename type

9a26d1e

RenderMichael requested a review from nvborisenko October 11, 2025 22:47

Remove CoreModule

dbe8e0b

nvborisenko reviewed Oct 19, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs Show resolved Hide resolved

nvborisenko reviewed Oct 19, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Extensions/Permissions/PermissionDescriptor.cs Outdated Show resolved Hide resolved

nvborisenko reviewed Oct 19, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Module.cs Outdated Show resolved Hide resolved

RenderMichael added 5 commits October 20, 2025 17:16

Move Permissions module out of a separate Extensions folder

6449944

Remove unused usings

43dd406

Rename Initialize to CreateJsonContext

341338f

Merge branch 'trunk' into enable-external-bidi-modules

8d3b1b0

Fix format

6bec772

nvborisenko reviewed Oct 21, 2025

View reviewed changes

RenderMichael added 2 commits October 22, 2025 12:03

Rename extensions files, move Permissions into a separate file from g…

875f5e4

…eneral BiDi extensinos

Rename PermissionsExtensions to PermissionsBiDiExtensions

3c6d2f7

nvborisenko reviewed Oct 24, 2025

View reviewed changes

	[JsonSerializable(typeof(SetPermissionCommand), TypeInfoPropertyName = "Permissions_SetPermissionCommand")]
	[JsonSerializable(typeof(SetPermissionCommand))]

Uh oh!

Uh oh!

[dotnet] Implement third-party Permissions module #16414

Are you sure you want to change the base?

[dotnet] Implement third-party Permissions module #16414

Uh oh!

Conversation

RenderMichael commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-merge-pro bot commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

Uh oh!

Uh oh!

nvborisenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvborisenko commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RenderMichael commented Oct 11, 2025 •

edited

Loading

qodo-merge-pro bot commented Oct 11, 2025 •

edited

Loading

qodo-merge-pro bot commented Oct 11, 2025 •

edited

Loading