-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] Implement third-party Permissions module #16414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from 3 commits
b595dfc
eaa3d5c
71a7489
1fbf4c6
9a26d1e
dbe8e0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// <copyright file="CoreModule.cs" company="Selenium Committers"> | ||
// Licensed to the Software Freedom Conservancy (SFC) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The SFC licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
// </copyright> | ||
|
||
using OpenQA.Selenium.BiDi.Communication.Json; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace OpenQA.Selenium.BiDi; | ||
|
||
public abstract class CoreModule : Module | ||
{ | ||
internal new BiDiJsonSerializerContext JsonContext => (BiDiJsonSerializerContext)base.JsonContext; | ||
|
||
protected override JsonSerializerContext Initialize(JsonSerializerOptions options) | ||
{ | ||
return new BiDiJsonSerializerContext(options); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,29 +18,28 @@ | |
// </copyright> | ||
|
||
using OpenQA.Selenium.BiDi.Communication; | ||
using OpenQA.Selenium.BiDi.Communication.Json; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace OpenQA.Selenium.BiDi; | ||
|
||
public abstract class Module | ||
{ | ||
protected Broker Broker { get; private set; } | ||
|
||
internal BiDiJsonSerializerContext JsonContext { get; private set; } | ||
internal JsonSerializerContext JsonContext { get; private set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not ready to generalize it in this PR. Still not clear how to move on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an example in this PR’s description, of how to move forward with these changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I though making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Observed it is theoretical changes. Let be practicable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, except for the shared JSON context introduced in #16402 It is not theoretical changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I came to the following:
Then changes looks simple. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel introducing new
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All right, |
||
|
||
protected virtual void Initialize(JsonSerializerOptions options) { } | ||
protected abstract JsonSerializerContext Initialize(JsonSerializerOptions options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should choose between |
||
|
||
internal static TModule Create<TModule>(BiDi bidi, Broker broker, JsonSerializerOptions jsonOptions, BiDiJsonSerializerContext context) | ||
public static TModule Create<TModule>(BiDi bidi, JsonSerializerOptions jsonOptions, JsonSerializerContext? cachedContext = null) | ||
where TModule : Module, new() | ||
{ | ||
TModule module = new() | ||
{ | ||
Broker = broker, | ||
JsonContext = context | ||
Broker = bidi.Broker, | ||
}; | ||
|
||
module.Initialize(jsonOptions); | ||
module.JsonContext = cachedContext ?? module.Initialize(jsonOptions); | ||
|
||
return module; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why
CoreModule
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me remind: we made
Module
classctor
parameterless, especially for external modules.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have 2 requirements:
For that reason, they need to have some shared logic.